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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.10.2026 16:08, Mykyta Poturai wrote:
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -16,6 +16,7 @@ The format is based on [Keep a 
> Changelog](https://keepachangelog.com/en/1.0.0/)
>       afterwards.
>  
>  ### Added
> + - PCI SR-IOV is supported for PVH dom0.

Isn't this x86-only right now?

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -32,6 +32,7 @@
>  #include <xen/softirq.h>
>  #include <xen/tasklet.h>
>  #include <xen/vpci.h>
> +#include <xen/xvmalloc.h>
>  #include <xen/msi.h>
>  #include <xsm/xsm.h>
>  #include "ats.h"
> @@ -40,6 +41,8 @@ struct pci_seg {
>      struct list_head alldevs_list;
>      u16 nr;
>      unsigned long *ro_map;
> +    /* BDFs of VFs which have been enabled but which Xen has not added yet. 
> */
> +    unsigned long *vf_setup_map;
>      /* bus2bridge_lock protects bus2bridge array */
>      spinlock_t bus2bridge_lock;
>  #define MAX_BUSES 256
> @@ -139,6 +142,47 @@ const unsigned long *pci_get_ro_map(u16 seg)
>      return pseg ? pseg->ro_map : NULL;
>  }
>  
> +/*
> + * Marking is serialised by the hardware domain's pci_lock, which the SR-IOV
> + * control register write handler holds; unmarking only clears a bit.
> + */

What about pci_vf_setup_pending()? How's that (to be) serialized again marking
and unmarking?

> --- 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?

> +/*
> + * Accesses to a marked BDF are terminated until 100ms after VF Enable; the
> + * first one past that adds the VF, which control_write() cannot do itself.
> + */
> +static pci_sbdf_t vf_sbdf(const struct pci_dev *pf_pdev, unsigned int idx)
> +{
> +    const struct vpci_sriov *sriov = pf_pdev->vpci->sriov;
> +    pci_sbdf_t sbdf = pf_pdev->sbdf;
> +
> +    sbdf.bdf += sriov->offset + sriov->stride * idx;
> +
> +    return sbdf;
> +}

The comment ahead of the function doesn't appear to be related to it. Does
it belong elsewhere?

> +static void mark_vfs(const struct pci_dev *pf_pdev)
> +{
> +    struct vpci_sriov *sriov = pf_pdev->vpci->sriov;
> +    unsigned int i;
> +
> +    /* VF Offset and VF Stride are only valid while VF Enable is set. */
> +    sriov->offset = pci_conf_read16(pf_pdev->sbdf,
> +                                    sriov->pos + PCI_SRIOV_VF_OFFSET);
> +    sriov->stride = pci_conf_read16(pf_pdev->sbdf,
> +                                    sriov->pos + PCI_SRIOV_VF_STRIDE);

What if we end up reading 0 here, yet ->num_vfs is greater than 1? Or put
differently, ...

> +    sriov->vfs_ready = NOW() + MILLISECS(100);
> +
> +    for ( i = 0; i < sriov->num_vfs; i++ )
> +    {
> +        pci_sbdf_t sbdf = vf_sbdf(pf_pdev, i);
> +        int rc = pci_vf_setup_begin(sbdf);
> +
> +        if ( rc )
> +            gprintk(XENLOG_WARNING, "%pp: cannot set up VF %pp: %d\n",
> +                    &pf_pdev->sbdf, &sbdf, rc);

... wouldn't pci_vf_setup_begin() better indicate to its caller when the
BDF was already marked prior to the call? Also, if you didn't end up
setting the bit here, shouldn't ...

> +    }
> +}
> +
> +static void unmark_vfs(const struct pci_dev *pf_pdev)
> +{
> +    struct vpci_sriov *sriov = pf_pdev->vpci->sriov;
> +    unsigned int i;
> +
> +    if ( !sriov->vfs_ready )
> +        return;
> +
> +    for ( i = 0; i < sriov->num_vfs; i++ )
> +        pci_vf_setup_end(vf_sbdf(pf_pdev, i));

... its clearing better also be avoided?

> +    sriov->vfs_ready = 0;

Might this better be done ahead of the loop?

> +}
> +
> +/* Find the PF which has enabled @sbdf, if that VF is ready to be added. */
> +static const struct pci_dev *ready_pf(const struct domain *d, pci_sbdf_t 
> sbdf)
> +{
> +    struct pci_dev *pdev;

You return pointer-to-const, and nothing in the function alters *pdev.

> +    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".

> +void vpci_sriov_drop_vfs(void)
> +{
> +    struct vcpu *curr = current;
> +    pci_sbdf_t pf_sbdf = curr->vpci.drop_vfs_pf;
> +    struct pci_dev *pf, *vf, *tmp;
> +
> +    if ( likely(pf_sbdf.sbdf == ~0U) )
> +        return;
> +
> +    curr->vpci.drop_vfs_pf.sbdf = ~0U;

The call site has a comment that the call here happens after all locks
were dropped. To prevent two calls here racing, the check-and-update
here needs to be atomic imo.

> +    /*
> +     * The unmapping queued by control_write() still references the VF pdevs,
> +     * so it has to complete before they are freed.
> +     */
> +    while ( vpci_process_pending(curr) )
> +        process_pending_softirqs();

This isn't very nice, yet I also don't have any better suggestion.

> +    pcidevs_lock();
> +    pf = pci_get_pdev(NULL, pf_sbdf);

Why is this a global lookup?

> +static void cf_check control_write(const struct pci_dev *pdev, unsigned int 
> reg,
> +                                   uint32_t val, void *data)
> +{
> +    unsigned int sriov_pos = reg - PCI_SRIOV_CTRL;
> +    struct vpci_sriov *sriov = pdev->vpci->sriov;
> +    uint16_t control = pci_conf_read16(pdev->sbdf, reg);
> +    bool enabled = control & PCI_SRIOV_CTRL_VFE;
> +    bool new_enabled = val & PCI_SRIOV_CTRL_VFE;
> +    /* VF BARs decode only while both VF Enable and VF MSE are set. */
> +    bool decoding = enabled && (control & PCI_SRIOV_CTRL_MSE);
> +    bool new_decoding = new_enabled && (val & PCI_SRIOV_CTRL_MSE);
> +    int rc;
> +
> +    ASSERT(!pdev->info.is_virtfn);
> +
> +    if ( new_enabled == enabled && new_decoding == decoding )
> +    {
> +        pci_conf_write16(pdev->sbdf, reg, val);
> +        return;
> +    }
> +
> +    /* Unmap while the VF pdevs are still around for vpci_modify_bars(). */
> +    if ( decoding && !new_decoding )
> +        map_vfs(pdev, 0);
> +
> +    if ( enabled && !new_enabled )
> +    {
> +        unmark_vfs(pdev);
> +
> +        /* The VFs cease to exist below, so Xen has to remove them. */
> +        if ( !list_empty(&pdev->vf_list) )
> +            current->vpci.drop_vfs_pf = pdev->sbdf;

Minimally an assertion may be wanted here, to check that drop_vfs_pf isn't
already in use.

> +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?

> +    if ( xsm_resource_setup_pci(XSM_PRIV, pdev->sbdf.bdf) )
> +    {
> +        printk(XENLOG_ERR
> +               "%pp: SR-IOV configuration unsupported for unpriv %pd\n",
> +               &pdev->sbdf, pdev->domain);
> +        return -EACCES;
> +    }

In addition to Jason's remark (which may require Daniel's input): Why "unpriv"
in the log message? That's fine when dummy or SILO are in use, but that may
be entirely off under Flask.

> +    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);
> +}

This kind of comment is at risk of going stale without anyone noticing. It's
anyway written more like something belonging in a commit message.

> +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 
> +               "Attempting to remove SR-IOV PF %pp with VFs still present, 
> VFs will be forcefully removed first\n",
> +               &pdev->sbdf);

What would be lost if "with VFs still present" was dropped from the log message?

> +        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);
> +        }

Nit: No need for figure braces here.

> +    }
> +
> +    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) )
> +        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);

I'm inclined to suggest "re-add", so it's clear from just seeing the message in
a log that this isn't about init of the device.

> @@ -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)) &&
> +         !vpci_sriov_add_vf(sbdf) )
> +        return data & (0xffffffffU >> (32 - 8 * size));

Is there a reason you can't simply "return data" here, like is done in the
first conditional of the function?

> @@ -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;
> +    } *sriov;
> +
>  #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>      /* Guest SBDF of the device. */
>  #define INVALID_GUEST_SBDF ((pci_sbdf_t){ .sbdf = ~0U })

While for guests we have control over the range of valid SBDF values, ...

> @@ -169,6 +185,12 @@ struct vpci_map_task {
>  
>  struct vpci_vcpu {
>      struct list_head task_queue;
> +    /*
> +     * PF whose VFs have been disabled by the config space write in progress,
> +     * and which Xen has to remove once the PCI locks have been dropped.  All
> +     * ones when there is none.
> +     */
> +    pci_sbdf_t drop_vfs_pf;

... I don't see what would guarantee an all-ones pci_sbdf_t to not represent
a real device.

Jan