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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 09.08.2022 17:06, Rahul Singh wrote:
>> On 9 Aug 2022, at 11:02 am, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 05.08.2022 17:43, Rahul Singh wrote:
>>> --- 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;                                                              
>   
> }  

That would about double the code in the functions. Imo all it takes
is to alter the while() conditions, prefixing what is there with
"seg == -1 &&".

Actually while looking there I've noticed the get_pseg() uses in
both functions aren't quite right for the "seg == -1" case either.
I'll make a patch there, which I think shouldn't collide with yours.

Jan