Re: [PATCH v2 1/2] xen-pciback: redo VF placement in the virtual topology

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.05.2021 16:44, Jan Beulich wrote:
> On 20.05.2021 16:38, Boris Ostrovsky wrote:
>>
>> On 5/20/21 3:43 AM, Jan Beulich wrote:
>>> On 20.05.2021 02:36, Boris Ostrovsky wrote:
>>>> On 5/18/21 12:13 PM, Jan Beulich wrote:
>>>>>  
>>>>> @@ -95,22 +95,25 @@ static int __xen_pcibk_add_pci_dev(struc
>>>>>  
>>>>>   /*
>>>>>    * Keep multi-function devices together on the virtual PCI bus, except
>>>>> -  * virtual functions.
>>>>> +  * that we want to keep virtual functions at func 0 on their own. They
>>>>> +  * aren't multi-function devices and hence their presence at func 0
>>>>> +  * may cause guests to not scan the other functions.
>>>>
>>>> So your reading of the original commit is that whatever the issue it was, 
>>>> only function zero was causing the problem? In other words, you are not 
>>>> concerned that pci_scan_slot() may now look at function 1 and skip all 
>>>> higher-numbered function (assuming the problem is still there)?
>>> I'm not sure I understand the question: Whether to look at higher numbered
>>> slots is a function of slot 0's multi-function bit alone, aiui. IOW if
>>> slot 1 is being looked at in the first place, slots 2-7 should also be
>>> looked at.
>>
>>
>> Wasn't the original patch describing a problem strictly as one for 
>> single-function devices, so the multi-function bit is not set? I.e. if all 
>> VFs (which are single-function devices) are placed in the same slot then 
>> pci_scan_slot() would only look at function 0 and ignore anything 
>> higher-numbered.
>>
>>
>> My question is whether it would "only look at function 0 and ignore anything 
>> higher-numbered" or "only look at the lowest-numbered function and ignore 
>> anything higher-numbered".
> 
> The common scanning logic is to look at slot 0 first. If that's populated,
> other slots get looked at only if slot 0 has the multi-function bit set.
> If slot 0 is not populated, nothing is known about the other slots, and
> hence they need to be scanned.

In particular Linux'es next_fn() ends with

        /* dev may be NULL for non-contiguous multifunction devices */
        if (!dev || dev->multifunction)
                return (fn + 1) % 8;

        return 0;

Jan