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

[Mykyta Poturai <Mykyta_Poturai@xxxxxxxx>]

On 7/28/25 14:33, Roger Pau Monné wrote:
> On Fri, Jul 25, 2025 at 02:24:33PM +0000, Mykyta Poturai wrote:
>> From: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
>>
>> This code is expected to only be used by privileged domains,
>> unprivileged domains should not get access to the SR-IOV capability.
>>
>> Implement RW handlers for PCI_SRIOV_CTRL register to dynamically
>> map/unmap VF BARS. Recalculate BAR sizes before mapping VFs to account
>> for possible changes in the system page size register.
>>
>> Relies on dom0 to enable SR-IOV and PHYSDEVOP to inform Xen about
>> addition/removal of VFs.
> 
> Why I'm not opposed to allowing registration of devices using
> PHYSDEVOP, can't Xen detect the addition of the VFs and add them
> itself?
> 
> When I worked on this long time ago, the version of the series that I
> posted had registration of the VFs done by Xen also:
> 
> https://lore.kernel.org/xen-devel/20180717094830.54806-12-roger.pau@xxxxxxxxxx/
> 
>>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
>> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
>> ---
>>   CHANGELOG.md              |   3 +-
>>   SUPPORT.md                |   2 -
>>   xen/drivers/vpci/Makefile |   2 +-
>>   xen/drivers/vpci/header.c |   3 +
>>   xen/drivers/vpci/sriov.c  | 235 ++++++++++++++++++++++++++++++++++++++
>>   xen/drivers/vpci/vpci.c   |   1 +
>>   xen/include/xen/vpci.h    |   7 +-
>>   7 files changed, 247 insertions(+), 6 deletions(-)
>>   create mode 100644 xen/drivers/vpci/sriov.c
>>
>> diff --git a/CHANGELOG.md b/CHANGELOG.md
>> index 5f31ca08fe..7b0e8beb76 100644
>> --- a/CHANGELOG.md
>> +++ b/CHANGELOG.md
>> @@ -23,8 +23,7 @@ The format is based on [Keep a 
>> Changelog](https://keepachangelog.com/en/1.0.0/
>>    - On x86:
>>      - Option to attempt to fixup p2m page-faults on PVH dom0.
>>      - Resizable BARs is supported for PVH dom0.
>> -   - Support PCI passthrough for HVM domUs when dom0 is PVH (note SR-IOV
>> -     capability usage is not yet supported on PVH dom0).
>> +   - Support PCI passthrough for HVM domUs when dom0 is PVH.
> 
> Don't you need to move this out of the x86 specific section?
> 
> According to the cover letter you are testing on an ARM board, so this
> probably needs to be put in a non-arch part of CHANGELOG?
> 
>>      - Smoke tests for the FreeBSD Xen builds in Cirrus CI.
>>   
>>    - On Arm:
>> diff --git a/SUPPORT.md b/SUPPORT.md
>> index 6a82a92189..830b598714 100644
>> --- a/SUPPORT.md
>> +++ b/SUPPORT.md
>> @@ -170,8 +170,6 @@ unexpected behavior or issues on some hardware.
>>   
>>   At least the following features are missing on a PVH dom0:
>>   
>> -  * PCI SR-IOV.
>> -
>>     * Native NMI forwarding (nmi=dom0 command line option).
>>   
>>     * MCE handling.
>> diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile
>> index a7c8a30a89..fe1e57b64d 100644
>> --- a/xen/drivers/vpci/Makefile
>> +++ b/xen/drivers/vpci/Makefile
>> @@ -1,2 +1,2 @@
>> -obj-y += vpci.o header.o rebar.o
>> +obj-y += vpci.o header.o rebar.o sriov.o
>>   obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o
>> diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
>> index f947f652cd..0a840c6dcc 100644
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -839,6 +839,9 @@ static int cf_check init_header(struct pci_dev *pdev)
>>   
>>       ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
>>   
>> +    if ( pdev->info.is_virtfn )
>> +        return 0;
>> +
>>       switch ( pci_conf_read8(pdev->sbdf, PCI_HEADER_TYPE) & 0x7f )
>>       {
>>       case PCI_HEADER_TYPE_NORMAL:
>> diff --git a/xen/drivers/vpci/sriov.c b/xen/drivers/vpci/sriov.c
>> new file mode 100644
>> index 0000000000..640430e3e9
>> --- /dev/null
>> +++ b/xen/drivers/vpci/sriov.c
>> @@ -0,0 +1,235 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Handlers for accesses to the SR-IOV capability structure.
>> + *
>> + * Copyright (C) 2018 Citrix Systems R&D
> 
> If there's a Citrix copyright header here, shouldn't there be a
> matching Signed-off-by from someone at Citrix (I think that's likely
> me)?
> 
> Otherwise if there's no content authored by a Citrix employee the
> copyright notice must be removed.  We need to be careful with
> copyright and attribution.
> 
> And in any case the date should be updated.
> 

Can I add your SOB or is it better to remove the copyright? Looking at 
the patches you provided, I think this ones were definitely based on 
them, but there are also a lot of changes since then.

-- 
Mykyta