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.hildebr...@amd.com>
>
> I'm okay with this, so in principle
> Reviewed-by: Jan Beulich <jbeul...@suse.com>
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);
}
