On 11/25/24 11:08, Jan Beulich wrote:
> On 15.11.2024 17:09, 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 remove the associated VFs before
>> removing the PF. If removal 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 calls to pci_get_pdev() and pci_add_device() down to avoid
>> dropping and re-acquiring the pcidevs_lock().
>>
>> Check !pdev->pf_pdev before adding the VF to the list to guard against
>> adding it multiple times.
>>
>> Signed-off-by: Stewart Hildebrand <[email protected]>
>
> Reviewed-by: Jan Beulich <[email protected]>
Thanks!
>> @@ -698,12 +691,48 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>> if ( info )
>> {
>> pdev->info = *info;
>> - /*
>> - * VF's 'is_extfn' field is used to indicate whether its PF is an
>> - * 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));
>> +
>> + if ( !pf_pdev )
>> + {
>> + ret = pci_add_device(seg, info->physfn.bus,
>> info->physfn.devfn,
>> + NULL, node);
>> + if ( ret )
>> + {
>> + printk(XENLOG_WARNING
>> + "Failed to add SR-IOV device PF %pp for VF
>> %pp\n",
>> + &PCI_SBDF(seg, info->physfn.bus,
>> info->physfn.devfn),
>> + &pdev->sbdf);
>> + free_pdev(pseg, pdev);
>> + goto out;
>> + }
>> + pf_pdev = pci_get_pdev(NULL, PCI_SBDF(seg, info->physfn.bus,
>> + info->physfn.devfn));
>> + if ( !pf_pdev )
>> + {
>> + printk(XENLOG_ERR
>> + "Inconsistent PCI state: failed to find newly
>> added PF %pp for VF %pp\n",
>> + &PCI_SBDF(seg, info->physfn.bus,
>> info->physfn.devfn),
>> + &pdev->sbdf);
>> + ASSERT_UNREACHABLE();
>> + free_pdev(pseg, pdev);
>> + ret = -EILSEQ;
>> + goto out;
>> + }
>> + }
>> +
>> + if ( !pdev->pf_pdev )
>> + {
>> + /* VF inherits its 'is_extfn' from PF */
>> + pdev->info.is_extfn = pf_pdev->info.is_extfn;
>> + list_add(&pdev->vf_list, &pf_pdev->vf_list);
>> + pdev->pf_pdev = pf_pdev;
>
> As a general pattern, I think filling fields before adding to lists is
> preferable.
> For now it doesn't really matter here (lock held here and while removing), yet
> still: Thoughts towards flipping the last two lines above, perhaps while
> committing?
I'm OK with that, feel free to do so.
