Re: [PATCH v4 4/5] vpci: add SR-IOV support for PVH Dom0

[Mykyta Poturai <Mykyta_Poturai@xxxxxxxx>]

On 10/28/26 3:49 PM, Jan Beulich wrote:
> On 15.10.2026 16:08, Mykyta Poturai wrote:

 >> --- a/xen/drivers/vpci/Makefile
 >> +++ b/xen/drivers/vpci/Makefile
 >> @@ -1,3 +1,4 @@
 >>  obj-y += cap.o
 >> +obj-y += sriov.o
 >>  obj-y += vpci.o header.o rebar.o
 >>  obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o
 >
 > When I added cap.o on a separate line, the intention was that new files would
 > follow suite, including the respecting of sorting. Imo sriov.o wants to go
 > after msi{,x}.o. Seeing how the latter are conditionally included - wouldn't
 > SR-IOV also want to have a HAS_SRIOV control?

I will fix the sorting, but I think guarding every PCI cap under a config 
option seems 
like an overkill to me. MSI/X being an exception because it depends heavily on 
platform 
support (having a working ITS on Arm at least).

>> +    ASSERT(rw_is_locked(&d->pci_lock));
>> +
>> +    for_each_pdev ( d, pdev )
>> +    {
>> +        const struct vpci_sriov *sriov = pdev->vpci ? pdev->vpci->sriov : 
>> NULL;
>> +        unsigned int i;
>> +
>> +        if ( !sriov || !sriov->vfs_ready || NOW() < sriov->vfs_ready )
>> +            continue;
>> +
>> +        for ( i = 0; i < sriov->num_vfs; i++ )
>> +            if ( vf_sbdf(pdev, i).sbdf == sbdf.sbdf )
>> +                return pdev;
>> +    }
> 
> This loop can easily have hundreds of iterations, when the number of SR-IOV
> PFs in a system is likely relatively small. I wonder whether there shouldn't
> be a separate list of "PDEVs with VFs under construction".
> 

It can take a decent number of iterations, but it only runs when there is  a VF
still waiting to be added, so it is once per VF Enable and never on a normal
vPCI access. And even a few hundred iterations of a loop like this is nothing
next to the pci_add_device() that same access performs. I don't see a reason to
introduce the complexity of another list for that.

>> +static int cf_check init_sriov(struct pci_dev *pdev)
>> +{
>> +    unsigned int pos;
>> +
>> +    if ( pdev->info.is_virtfn )
>> +        return -EINVAL;
> 
> Can this happen? I.e. doesn't this want to be an assertion, much like ...
> 
>> +    pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_SRIOV);
>> +
>> +    ASSERT(pos);
> 
> ... this one?
> 

I think it is better to leave it to have some protection for malicious/faulty
devices that have VFs report SR-IOV caps themselves. Not sure if such devices
exist but still the check costs nothing.