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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

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 <stewart.hildebrand@xxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

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.