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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Jan,

> On 9 Aug 2022, at 11:02 am, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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.

Ack. 
> 
>> --- 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).

If I understand correctly then fixed code should be like this:                  
                      
   
—snip— 
….                                                                  
    if ( !pseg )                                                                
    {                                                                           
        if ( seg == -1 )                                                        
            radix_tree_gang_lookup(&pci_segments, (void **)&pseg, 0, 1);        
        if ( !pseg )                                                            
            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) );                        
        return NULL;                                                            
    }                                                                           
                                                                                
    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;                                                                
}  


> 
> 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?

Sorry for the confusion all the patches are independent of each other.
Maybe this is because I send them via a single git send-mail command.
I will fix that in the next version. 

Regards,
Rahul