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 4:13 pm, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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 &&".

I agree with you this will avoid duplication of the code.

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

Ok. I will test the patch once you sent it..

Regards,
Rahul