Re: [PATCH v7 1/2] xen/pci: introduce PF<->VF links

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.11.2024 19:50, Stewart Hildebrand wrote:
> On 11/14/24 05:34, Jan Beulich wrote:
>> On 12.11.2024 21:53, Stewart Hildebrand wrote:
>>> Add links between a VF's struct pci_dev and its associated PF struct
>>> pci_dev.
>>>
>>> The hardware domain is expected to add a PF first before adding
>>> associated VFs. Similarly, the hardware domain is expected to remove the
>>> associated VFs before removing the PF. If adding/removing happens out of
>>> order, print a warning and return an error. This means that VFs can only
>>> exist with an associated PF.
>>>
>>> Additionally, if the hardware domain attempts to remove a PF with VFs
>>> still present, mark the PF and VFs broken, because Linux Dom0 has been
>>> observed to not respect the error returned.
>>>
>>> Move the call to pci_get_pdev() down to avoid dropping and re-acquiring
>>> the pcidevs_lock(). Drop the call to pci_add_device() as it is invalid
>>> to add a VF without an existing PF.
>>>
>>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
>>
>> I'm okay with this, so in principle
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Thanks, I very much appreciate it! However, is it appropriate for me to
> pick up this tag considering the requested/proposed changes?

In general if in doubt, leave it out. Here, since you're meaning to
make further changes, it certainly wants leaving out.

>>> @@ -703,7 +696,38 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>           * extended function.
>>>           */
>>>          if ( pdev->info.is_virtfn )
>>> -            pdev->info.is_extfn = pf_is_extfn;
>>> +        {
>>> +            struct pci_dev *pf_pdev = pci_get_pdev(NULL,
>>> +                                                   PCI_SBDF(seg,
>>> +                                                           
>>> info->physfn.bus,
>>> +                                                           
>>> info->physfn.devfn));
> 
> BTW, since I'm spinning another rev anyway, are there any opinions on
> the indentation here?

Well, yes. Andrew's preferred (or so I think) way of laying this out
would (imo) certainly be better here:

            struct pci_dev *pf_pdev =
                pci_get_pdev(NULL,
                             PCI_SBDF(seg, info->physfn.bus,
                                      info->physfn.devfn));

(with less line wrapping if stuff fits within 80 chars, which I didn't
specifically check).

>>> +                       &pdev->sbdf,
>>> +                       &PCI_SBDF(seg, info->physfn.bus, 
>>> info->physfn.devfn));
>>> +                free_pdev(pseg, pdev);
>>> +                ret = -ENODEV;
>>> +                goto out;
>>> +            }
>>> +
>>> +            pdev->info.is_extfn = pf_pdev->info.is_extfn;
>>
>> There's a comment related to this, partly visible in patch context above.
>> That comment imo needs moving here. And correcting while moving (it's
>> inverted imo, or at least worded ambiguously).
> 
> I'll move it. As far as wording goes, I suggest:
> 
>             /*
>              * PF's 'is_extfn' field indicates whether the VF is an extended
>              * function.
>              */

Or maybe "VF inherits its 'is_extfn' from PF"?

>>> +            pdev->pf_pdev = pf_pdev;
>>> +            list_for_each_entry(vf_pdev, &pf_pdev->vf_list, vf_list)
>>> +            {
>>> +                if ( vf_pdev == pdev )
>>> +                {
>>> +                    already_added = true;
>>> +                    break;
>>> +                }
>>> +            }
>>> +            if ( !already_added )
>>> +                list_add(&pdev->vf_list, &pf_pdev->vf_list);
>>> +        }
>>>      }
>>
>> Personally, as I have a dislike for excess variables, I'd have gotten away
>> without "already_added". Instead of setting it to true, vf_pdev could be
>> set to NULL. Others may, however, consider this "obfuscation" or alike.
> 
> This relies on vf_pdev being set to non-NULL when the list is empty and
> after the last iteration if the list doesn't contain the element. I had
> to look up the definitions of list_for_each_entry, INIT_LIST_HEAD, and
> list_{add,del,entry} to verify that vf_pdev would be non-NULL in those
> cases.
> 
> Perhaps a better approach would be to introduce list_add_unique() in
> Xen's list library? Then we can also get rid of the vf_pdev variable.
> 
> static inline bool list_contains(struct list_head *entry,
>                                  struct list_head *head)
> {
>    struct list_head *ptr;
> 
>    list_for_each(ptr, head)
>    {
>        if ( ptr == entry )
>            return true;
>    }
> 
>    return false;
> }
> 
> static inline void list_add_unique(struct list_head *new,
>                                    struct list_head *head)
> {
>     if ( !list_contains(new, head) )
>         list_add(new, head);
> }

I'm uncertain of this kind of an addition. For long lists one would need to
be careful with whether to actually use list_contains(). It being a simple
library function would make this easy to overlook.

Jan