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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

On 2026-10-15 10:08, 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 a write handler for the SR-IOV control register, so Xen can
> discover the VFs a PF enables and map their BARs. VF BARs live in the
> PF's capability and are re-sized on every VF Enable, as the System Page
> Size register can change them. They decode only while both VF Enable
> and VF MSE are set.
>
> Xen adds the VFs itself: on VF Enable their routing IDs are recorded
> and config space accesses to them terminated for 100ms, as the spec
> permits; the first access past that adds the VF. Clearing VF Enable
> removes them again.
>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>

> +/*
> + * Marking is serialised by the hardware domain's pci_lock, which the SR-IOV
> + * control register write handler holds; unmarking only clears a bit.
Maybe add an ASSERT()?

> + */
> +int pci_vf_setup_begin(pci_sbdf_t sbdf)
> +{
> +    struct pci_seg *pseg = get_pseg(sbdf.seg);
> +
> +    if ( !pseg )
> +        return -ENODEV;
> +
> +    if ( !pseg->vf_setup_map )
> +    {
> +        unsigned int nr = PCI_BDF(-1, -1, -1) + 1;
> +
> +        pseg->vf_setup_map = xvzalloc_array(unsigned long, BITS_TO_LONGS(nr));
> +        if ( !pseg->vf_setup_map )
> +            return -ENOMEM;
> +    }
> +
> +    set_bit(sbdf.bdf, pseg->vf_setup_map);
> +
> +    return 0;
> +}
> diff --git a/xen/drivers/pci/physdev.c b/xen/drivers/pci/physdev.c
> index 5695c300f7..7c0277a7e4 100644
> --- a/xen/drivers/pci/physdev.c
> +++ b/xen/drivers/pci/physdev.c
> @@ -26,15 +26,15 @@ ret_t pci_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> arg)
>           if ( copy_from_guest(&add, arg, 1) != 0 )
>               break;
>   
> -        pdev_info.is_extfn = (add.flags & XEN_PCI_DEV_EXTFN);
> +        /*
> +         * Virtual functions are always discovered by the hypervisor, so
> +         * they don't need to be added here.
Does a PV dom0 still need this path?  vPCI won't be used in that case.

> +         */
>           if ( add.flags & XEN_PCI_DEV_VIRTFN )
> -        {
> -            pdev_info.is_virtfn = true;
> -            pdev_info.physfn.bus = add.physfn.bus;
> -            pdev_info.physfn.devfn = add.physfn.devfn;
> -        }
> -        else
> -            pdev_info.is_virtfn = false;
> +            return 0;
> +
> +        pdev_info.is_extfn = (add.flags & XEN_PCI_DEV_EXTFN);
> +        pdev_info.is_virtfn = false;
>   
>   #ifdef CONFIG_NUMA
>           if ( add.flags & XEN_PCI_DEV_PXM )
> diff --git a/xen/drivers/vpci/sriov.c b/xen/drivers/vpci/sriov.c
> new file mode 100644
> index 0000000000..4e09bdfbfd
> --- /dev/null
> +++ b/xen/drivers/vpci/sriov.c
> @@ -0,0 +1,507 @@
> +static int vf_init_bars(struct pci_dev *vf_pdev)
> +{
> +    int vf_idx;
> +    unsigned int i;
> +    const struct pci_dev *pf_pdev = vf_pdev->pf_pdev;
> +    struct vpci_bar *bars = vf_pdev->vpci->header.bars;
> +    struct vpci_bar *physfn_vf_bars = pf_pdev->vpci->sriov->vf_bars;
> +    struct vpci_sriov *sriov = pf_pdev->vpci->sriov;
> +
> +    vf_idx = vf_pdev->sbdf.sbdf - (pf_pdev->sbdf.sbdf + sriov->offset);
> +    if ( vf_idx < 0 )
> +        return -EINVAL;
> +
> +    if ( sriov->num_vfs > 1 && !sriov->stride )
> +        return -EINVAL;
> +
> +    if ( sriov->stride )
> +    {
> +        if ( vf_idx % sriov->stride )
> +            return -EINVAL;
> +        vf_idx /= sriov->stride;
> +    }
> +
> +    if ( vf_idx >= sriov->num_vfs )
> +        return -EINVAL;
> +
> +    /*
> +     * Set up BARs for this VF out of PF's VF BARs taking into account
> +     * the index of the VF.
> +     */
> +    for ( i = 0; i < PCI_SRIOV_NUM_BARS; i++ )
> +    {
> +        struct vpci_bar *pf_bar = &physfn_vf_bars[i];
Maybe just remove physfn_vf_bars since it is only used once?

> +        struct vpci_bar *bar = &bars[i];
> +
> +        /*
> +         * The upper half of a 64-bit BAR holds no address or size of its own;
> +         * the handlers reach the lower half through its type alone.
> +         */
> +        if ( pf_bar->type == VPCI_BAR_MEM64_HI )
> +        {
> +            bar->type = VPCI_BAR_MEM64_HI;
> +            continue;
> +        }
> +
> +        if ( pf_bar->type != VPCI_BAR_MEM32 &&
> +             pf_bar->type != VPCI_BAR_MEM64_LO )
> +            continue;
> +
> +        bar->addr         = pf_bar->addr + vf_idx * pf_bar->size;
AIUI, the PF has the SR-IOV BARs, which are sized like normal.  But I 
thought the pf_bar->size needs to be reduced by some factor to get the 
per-VF size.  I can't find where that is done.  Or am I wrong about this?
> +        bar->guest_addr   = is_hardware_domain(vf_pdev->domain) ? bar->addr : 
> 0;
> +        bar->size         = pf_bar->size;
> +        bar->type         = pf_bar->type;
> +        bar->prefetchable = pf_bar->prefetchable;
> +    }
> +
> +    return 0;
> +}
> +static int map_vfs(const struct pci_dev *pf_pdev, uint16_t cmd)
> +{
> +    struct pci_dev *vf_pdev;
> +    int rc;
> +
> +    ASSERT(rw_is_write_locked(&pf_pdev->domain->pci_lock));
> +
> +    list_for_each_entry(vf_pdev, &pf_pdev->vf_list, vf_list)
> +    {
> +        /* Don't try to change mapping if it is already in the correct state. 
> */
> +        if ( vf_pdev->vpci->header.bars_mapped == !!cmd )
> +            continue;
> +
> +        rc = vpci_modify_bars(vf_pdev, cmd, false);
> +        if ( rc )
> +        {
> +            gprintk(XENLOG_ERR, "failed to %s VF %pp: %d\n",
> +                    (cmd & PCI_COMMAND_MEMORY) ? "map" : "unmap",
> +                    &vf_pdev->sbdf, rc);
> +            return rc;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static void size_vf_bars(const struct pci_dev *pf_pdev, unsigned int sriov_pos,
> +                         uint64_t *vf_rlen)
> +{
> +    struct vpci_bar *bars = pf_pdev->vpci->sriov->vf_bars;
> +    unsigned int i;
> +    int rc = 0;
> +
> +    ASSERT(rw_is_write_locked(&pf_pdev->domain->pci_lock));
> +    ASSERT(!pf_pdev->info.is_virtfn);
> +    ASSERT(pf_pdev->vpci->sriov);
AIUI, because sriov was already dereferenced above, this can be dropped 
by the compiler.  At least if (!pf_pdev->vpci->sriov) can be dropped.
> +
> +    /* Set the BARs addresses and size. */
> +    for ( i = 0; i < PCI_SRIOV_NUM_BARS; i += rc )
> +    {
> +        unsigned int idx = sriov_pos + PCI_SRIOV_BAR + i * 4;
> +        uint32_t bar;
> +        uint64_t addr, size;
> +
> +        bar = pci_conf_read32(pf_pdev->sbdf, idx);
> +
> +        rc = pci_size_mem_bar(pf_pdev->sbdf, idx, &addr, &size,
> +                              PCI_BAR_VF |
> +                              ((i == PCI_SRIOV_NUM_BARS - 1) ? PCI_BAR_LAST
> +                                                             : 0));
> +
> +        /*
> +         * Update vf_rlen on the PF. According to the spec the size of
> +         * the BARs can change if the system page size register is
> +         * modified, so always update rlen when enabling VFs.
> +         */
> +        vf_rlen[i] = size;
> +
> +        if ( !size )
> +        {
> +            bars[i].type = VPCI_BAR_EMPTY;
> +            continue;
> +        }
> +
> +        bars[i].addr = addr;
> +        bars[i].guest_addr = addr;
> +        bars[i].size = size;
> +        bars[i].prefetchable = bar & PCI_BASE_ADDRESS_MEM_PREFETCH;
> +
> +        switch ( rc )
> +        {
> +        case 1:
> +            bars[i].type = VPCI_BAR_MEM32;
> +            break;
> +
> +        case 2:
> +            bars[i].type = VPCI_BAR_MEM64_LO;
> +            bars[i + 1].type = VPCI_BAR_MEM64_HI;
> +            break;
> +
> +        default:
> +            ASSERT_UNREACHABLE();
> +            rc = 1;
> +        }
> +    }
> +}

> +static int cf_check init_sriov(struct pci_dev *pdev)
> +{
> +    unsigned int pos;
> +
> +    if ( pdev->info.is_virtfn )
> +        return -EINVAL;
> +
> +    pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_SRIOV);
> +
> +    ASSERT(pos);
> +
> +    if ( xsm_resource_setup_pci(XSM_PRIV, pdev->sbdf.bdf) )
This is a little misleading.  During boot, current is Xen - d[IDLE]. 
With my XSM_HW_PRIV patches, I failed this check since Xen is not the 
hardware domain.
It seems to me the point of this check is to only expose the SR-IOV 
capability to a suitable domain.  If the PF is assigned to a domU - then 
don't show the SR-IOV capability.
SR-IOV and its BARs is more of a hardware domain responsibility, so I 
think this should just be an is_hardware_domain() check.  And I think it 
should be based on pdev->domain and not current.  On the cleanup side, 
this is already checking is_hardware_domain(pdev->domain).
> +    {
> +        printk(XENLOG_ERR
> +               "%pp: SR-IOV configuration unsupported for unpriv %pd\n",
> +               &pdev->sbdf, pdev->domain);
> +        return -EACCES;
> +    }
> +
> +    pdev->vpci->sriov = xvzalloc(struct vpci_sriov);
> +    if ( !pdev->vpci->sriov )
> +        return -ENOMEM;
> +
> +    pdev->vpci->sriov->pos = pos;
> +
> +    /*
> +     * We need to modify vf_rlen in control_write but we can't do it cleanly
> +     * from pdev because write callback only accepts const pdev. Moving vf_rlen
> +     * inside of struct vpci_sriov is also not possible because it is used
> +     * before vpci init. So pass it here as additional data to not require
> +     * dropping const in control_write.
> +     */
> +    return vpci_add_register(pdev->vpci, vpci_hw_read16, control_write,
> +                             pos + PCI_SRIOV_CTRL, 2, &pdev->physfn.vf_rlen);
> +}
> +
> +static int cf_check cleanup_sriov(const struct pci_dev *pdev, bool hide)
> +{
> +    unsigned int pos;
> +    int rc;
> +
> +    if ( !pdev->vpci->sriov )
> +        return 0;
> +
> +    ASSERT(!pdev->info.is_virtfn);
> +
> +    unmark_vfs(pdev);
> +
> +    if ( !list_empty(&pdev->vf_list) )
> +    {
> +        struct pci_dev *vf_pdev, *temp;
> +
> +        printk(XENLOG_WARNING
Thunderbird stripped it from my reply, but there is trailing space above.

> +               "Attempting to remove SR-IOV PF %pp with VFs still present, VFs will 
> be forcefully removed first\n",
> +               &pdev->sbdf);
> +
> +        map_vfs(pdev, 0);
> +        list_for_each_entry_safe_reverse(vf_pdev, temp, &pdev->vf_list, 
> vf_list)
> +        {
> +            pci_remove_device(vf_pdev->sbdf.seg, vf_pdev->sbdf.bus,
> +                              vf_pdev->sbdf.devfn);
> +        }
> +    }
> +
> +    pos = pdev->vpci->sriov->pos;
> +    if ( !hide )
> +    {
> +        XVFREE(pdev->vpci->sriov);
> +        return 0;
> +    }
> +
> +    rc = vpci_remove_registers(pdev->vpci, pos + PCI_SRIOV_CTRL, 2);
> +    if ( rc )
> +    {
> +        printk(XENLOG_ERR "%pd %pp: fail to remove SRIOV handlers rc=%d\n",
> +                pdev->domain, &pdev->sbdf, rc);
> +        ASSERT_UNREACHABLE();
> +        return rc;
> +    }
> +    XVFREE(pdev->vpci->sriov);
> +
> +    /*
> +     * Unprivileged domains have a deny by default register access policy, no
> +     * need to add any further handlers for them.
> +     */
> +    if ( !is_hardware_domain(pdev->domain) )
This is the counterpart to the earlier XSM check.

> +        return 0;
> +
> +    rc = vpci_add_register(pdev->vpci, vpci_hw_read16, NULL,
> +                           pos + PCI_SRIOV_CTRL, 2, NULL);
> +    if ( rc )
> +        printk(XENLOG_ERR "%pd %pp: fail to add SRIOV ctrl handler rc=%d\n",
> +               pdev->domain, &pdev->sbdf, rc);
> +
> +    return rc;
> +}
> +
> +REGISTER_VPCI_EXTCAP(SRIOV, init_sriov, cleanup_sriov);
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 9a422f9ea0..767fe5c6c9 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -486,6 +487,11 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, 
> unsigned int size)
>           return data;
>       }
>   
> +    /* Ahead of any lock being taken, as adding a device acquires them. */
> +    if ( unlikely(is_hardware_domain(d) && pci_vf_setup_pending(sbdf)) &&
unlikely() here breaks the tests build.  Add
    #define unlikely
to tools/tests/vpci/emul.h

> +         !vpci_sriov_add_vf(sbdf) )
> +        return data & (0xffffffffU >> (32 - 8 * size));
> +
>       /*
>        * Find the PCI dev matching the address, which for hwdom also requires
>        * consulting DomXEN.  Passthrough everything that's not trapped.
> @@ -607,6 +613,11 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, 
> unsigned int size,
>           return;
>       }
>   
> +    /* Ahead of any lock being taken, as adding a device acquires them. */
> +    if ( unlikely(is_hardware_domain(d) && pci_vf_setup_pending(sbdf)) &&
> +         !vpci_sriov_add_vf(sbdf) )
> +        return;
> +
>       /*
>        * Find the PCI dev matching the address, which for hwdom also requires
>        * consulting DomXEN.  Passthrough everything that's not trapped.
> @@ -680,6 +691,9 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, unsigned 
> int size,
>           /* Tailing gap, write the remaining. */
>           vpci_write_hw(sbdf, reg + data_offset, size - data_offset,
>                         data >> (data_offset * 8));
> +
> +    /* Only once every lock has been released, as removal acquires them. */
> +    vpci_sriov_drop_vfs();
Hooking every vpci_write() for this seems a little unfortunate.  Could 
it be hooked into the vpci softirq?
>   }
>   
>   /* Helper function to check an access size and alignment on vpci space. */
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index 26aa120583..eead9487d0 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -145,6 +145,22 @@ struct vpci {
>           unsigned int nbars:3;
>       } rebar;
>   
> +    struct vpci_sriov {
> +        /* PF only */
> +        struct vpci_bar vf_bars[PCI_SRIOV_NUM_BARS];
> +        /*
> +         * When the VFs which are enabled but not added to Xen yet are
> +         * guaranteed to respond to config space accesses.  Zero if there are
> +         * none.
> +         */
> +        s_time_t vfs_ready;
> +        /* Routing ID offset of the first VF, and stride between VFs. */
> +        uint16_t offset;
> +        uint16_t stride;
> +        uint16_t num_vfs;
> +        unsigned int pos;
unsigned int before uint16_t packs the structure a little better.

Thanks,
Jason