Re: [PATCH] [RFC] xen/vpci: modify pci_get_pdev_by_domain() & pci_get_pdev()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.08.2022 17:43, Rahul Singh wrote:
> pci_get_pdev_by_domain() and pci_get_pdev() functions find the pdev in
> the pseg list. If pdev is not in the pseg list, the functions will try
> to find the pdev in the next segment. It is not right to find the pdev
> in the next segment as this will result in the corruption of another
> device in a different segment with the same BDF.
> 
> An issue that was observed when implementing the PCI passthrough on ARM.
> When we deassign the device from domU guest, the device is assigned
> to dom_io and not to dom0, but the tool stack that runs in dom0 will try
> to configure the device from dom0. vpci will find the device based on
> conversion of GPA to SBDF and will try to find the device in dom0, but
> because device is assigned to dom_io, pci_get_pdev_by_domain() will
> return pdev with same BDF from next segment.
> 
> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>

This wants a Fixes: tag.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -593,13 +593,10 @@ struct pci_dev *pci_get_pdev(int seg, int bus, int 
> devfn)
>              return NULL;
>      }
>  
> -    do {
> -        list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> -            if ( (pdev->bus == bus || bus == -1) &&
> -                 (pdev->devfn == devfn || devfn == -1) )
> -                return pdev;
> -    } while ( radix_tree_gang_lookup(&pci_segments, (void **)&pseg,
> -                                     pseg->nr + 1, 1) );
> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> +        if ( (pdev->bus == bus || bus == -1) &&
> +             (pdev->devfn == devfn || devfn == -1) )
> +            return pdev;
>  
>      return NULL;
>  }
> @@ -642,14 +639,11 @@ struct pci_dev *pci_get_pdev_by_domain(const struct 
> domain *d, int seg,
>              return NULL;
>      }
>  
> -    do {
> -        list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> -            if ( (pdev->bus == bus || bus == -1) &&
> -                 (pdev->devfn == devfn || devfn == -1) &&
> -                 (pdev->domain == d) )
> -                return pdev;
> -    } while ( radix_tree_gang_lookup(&pci_segments, (void **)&pseg,
> -                                     pseg->nr + 1, 1) );
> +    list_for_each_entry ( pdev, &pseg->alldevs_list, alldevs_list )
> +        if ( (pdev->bus == bus || bus == -1) &&
> +             (pdev->devfn == devfn || devfn == -1) &&
> +             (pdev->domain == d) )
> +            return pdev;
>  
>      return NULL;
>  }

Indeed present behavior is wrong - thanks for spotting. However in
both cases you're moving us from one wrongness to another: The
lookup of further segments _is_ necessary when the incoming "seg"
is -1 (and apparently when this logic was introduced that was the
only case considered).

As an aside - my mail UI shows me unexpected threading between
this patch and two subsequent ones. If they were actually meant
to be a series, can they please be marked n/3?

Jan