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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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?

> A few comments nevertheless, which may result in there wanting to be
> another revision.

I'll plan to send v8. There were some minor comments from Roger on the
removed snippet that I will also address, and I have another proposed
change.

>> ---
>> Candidate for backport to 4.19 (the next patch depends on this one)
> 
> With this dependency (we definitely want to backport the other patch) ...
> 
>> v6->v7:
>> * cope with multiple invocations of pci_add_device for VFs
>> * get rid of enclosing struct for single member
>> * during PF removal attempt with VFs still present:
>>     * keep PF
>>     * mark broken
>>     * don't unlink
>>     * return error
>> * during VF add:
>>     * initialize pf_pdev in declaration
>>     * remove logic catering to adding VFs without PF
> 
> ... I'd like to point out that this change has an at least theoretical
> risk of causing regressions. I therefore wonder whether that wouldn't
> better be a separate change, not to be backported.

That makes sense. I'll split it into a separate patch for the next rev.

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

>> +            struct pci_dev *vf_pdev;
> 
> const?

Yes, if it's still needed

>> +            bool already_added = false;
>> +
>> +            if ( !pf_pdev )
>> +            {
>> +                printk(XENLOG_WARNING
>> +                       "Attempted to add SR-IOV device VF %pp without PF 
>> %pp\n",
> 
> I'd omit "device" here.

OK

> (These changes alone I'd be happy to do while committing.)

I'll include the changes in v8.

>> +                       &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.
             */

>> +            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);
}