Implement SR-IOV support for PVH

From: Stewart Hildebrand <[email protected]>

Rename and export vpci_modify_bars, vpci_guest_mem_bar_{read,write}

Signed-off-by Stewart Hildebrand <[email protected]>

Signed-off-by Mykyta Poturai <[email protected]>

Reviewed-by Teddy Astie <[email protected]>

From: Mykyta Poturai <[email protected]>

In the BAR (un)mapping paths latch the rangeset of the BAR being processed into a local variable, instead of dereferencing struct vpci_bar on every use. Preparatory work for moving the rangesets out of struct vpci_bar.

No functional change.

Signed-off-by Mykyta Poturai <[email protected]>

Signed-off-by Stewart Hildebrand <[email protected]>

---
v3->v4:
* new patch, split out of "vpci: Use pervcpu ranges for BAR mapping" so that the non-functional churn is separate from the functional change

From: Stewart Hildebrand <[email protected]>

Introduce vPCI BAR mapping task queue. Store information needed to map/unmap BARs in struct vpci_map_task. Allow queueing of BAR map/unmap operations in a list, thus making it possible to perform multiple p2m operations associated with single PCI config space register access.

The rangesets describing the ranges to be (un)mapped move from struct vpci_bar into the task. They are only live for the duration of the operation, and a single per-device set cannot describe more than one

queued operation. Allocate them along with the task and free them once the task has been processed.

This is preparatory work for further changes that need to perform multiple unmap/map operations before returning to guest.

At the moment, only a single operation will be queued. However, when multiple operations are queued, there is a check in modify_bars() to skip BARs already in the requested state that will no longer be accurate. Remove this check in preparation of upcoming changes.

Amends: 622bdd962822 ("vpci/header: handle p2m range sets per BAR")
Message-ID: <[email protected]>

Signed-off-by Stewart Hildebrand <[email protected]>

Signed-off-by Mykyta Poturai <[email protected]>

xen/drivers/vpci/header.c +18 -0

+        if ( !task->bars[i].mem )
+        {
+            destroy_map_task(task);
+            return NULL;
+        }
+
+        task->bars[i].addr = pdev->vpci->header.bars[i].addr;
+        task->bars[i].guest_addr = pdev->vpci->header.bars[i].guest_addr;
+    }
+
+    task->cmd = cmd;
+    task->rom_only = rom_only;
+    task->pdev = pdev;
+
+    return task;
+}
+
+static void defer_map(struct vpci_map_task *task)
  {
      struct vcpu *curr = current;
 
 

@@ -312,11 +388,17 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
      struct pci_dev *tmp;

xen/drivers/vpci/header.c +3 -0

+    if ( !task )
+        return -ENOMEM;
+
      /*
       * Create a rangeset per BAR that represents the current device memory
       * region and compare it against all the currently active BAR memory

xen/drivers/vpci/header.c +3 -1

+                       : (bar->type == VPCI_BAR_ROM && !header->rom_enabled)) )
              continue;
 
          if ( !pci_check_bar(pdev, _mfn(start), _mfn(end)) )

@@ -152,14 +153,24 @@ struct vpci {
  #endif
  };
 
-struct vpci_vcpu {
+#ifdef __XEN__
+struct vpci_map_task {
      /* Per-vcpu structure to store state while {un}mapping of PCI BARs. */

xen/include/xen/vpci.h +4 -3

@@ -19,6 +19,8 @@
  */
 #define VPCI_MAX_VIRT_DEV       (PCI_SLOT(~0) + 1)
 
+void vpci_vcpu_init(struct vcpu *v);
+
 /* Assign vPCI to device by adding handlers. */
 int __must_check vpci_assign_device(struct pci_dev *pdev);
 

@@ -54,7 +56,6 @@ struct vpci {
             uint64_t guest_addr;
             uint64_t size;
             uint64_t resizable_sizes;
-            struct rangeset *mem;
             enum {
                 VPCI_BAR_EMPTY,
                 VPCI_BAR_IO,

@@ -152,14 +153,24 @@ struct vpci {
 #endif
 };
 
-struct vpci_vcpu {
+#ifdef __XEN__
+struct vpci_map_task {
     /* Per-vcpu structure to store state while {un}mapping of PCI BARs. */
-    const struct pci_dev *pdev;

xen/include/xen/vpci.h +4 -1

+struct vpci_vcpu {
+    struct list_head task_queue;
 };
 
-#ifdef __XEN__
 void vpci_dump_msi(void);
 
 /* Arch-specific vPCI MSI helpers. */

@@ -204,6 +215,8 @@ bool vpci_ecam_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len,
 #else /* !CONFIG_HAS_VPCI */
 struct vpci_vcpu {};
 
+static inline void vpci_vcpu_init(struct vcpu *v) { }
+
 static inline int vpci_reinit_ext_capabilities(struct pci_dev *pdev)
 {
     return 0;

From: Stewart Hildebrand <[email protected]>

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 <[email protected]>

Signed-off-by Roger Pau Monné <[email protected]>

Signed-off-by Mykyta Poturai <[email protected]>

+/*
+ * Marking is serialised by the hardware domain's pci_lock, which the SR-IOV
+ * control register write handler holds; unmarking only clears a bit.

+ */

xen/drivers/passthrough/pci.c +20 -0

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

xen/drivers/pci/physdev.c +0 -0

index 5695c300f7..7c0277a7e4 100644

xen/drivers/pci/physdev.c +5 -7

+         */
          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 )

xen/drivers/vpci/sriov.c new file +23 -0

new file mode 100644
index 0000000000..4e09bdfbfd
@@ -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;
+    }
+

xen/drivers/vpci/sriov.c new file +7 -0

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

xen/drivers/vpci/sriov.c new file +34 -0

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

xen/drivers/vpci/sriov.c new file +52 -0

+
+    /* 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)
+{

xen/drivers/vpci/sriov.c new file +31 -0

+    {
+        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;

xen/drivers/vpci/sriov.c new file +23 -0

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

xen/drivers/vpci/sriov.c new file +22 -0

+        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:
+ */

xen/drivers/vpci/vpci.c +0 -0

index 9a422f9ea0..767fe5c6c9 100644

xen/drivers/vpci/vpci.c +8 -0

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

xen/drivers/vpci/vpci.c +0 -0

  }
 
  /* Helper function to check an access size and alignment on vpci space. */

xen/include/xen/vpci.h +4 -0

index 26aa120583..eead9487d0 100644
@@ -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];
+        /*

Quoted 13 lines · Mykyta Poturai

From: Stewart Hildebrand <[email protected]>

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

xen/drivers/vpci/sriov.c line 67

Quoted 7 lines

+        }

xen/drivers/vpci/sriov.c new file +6 -0

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

Quoted 7 lines · Mykyta Poturai

xen/drivers/vpci/vpci.c +0 -0

@@ -486,6 +487,11 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size)
          return data;
      }
 

xen/drivers/vpci/vpci.c uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size) · line 491

Quoted 2 lines

xen/drivers/vpci/vpci.c +2 -0

+    /* 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

Quoted 7 lines

xen/include/xen/vpci.h +0 -0

@@ -145,6 +145,22 @@ struct vpci {
          unsigned int nbars:3;
      } rebar;
 

Quoted 4 lines

xen/include/xen/vpci.h +4 -0

+    struct vpci_sriov {
+        /* PF only */
+        struct vpci_bar vf_bars[PCI_SRIOV_NUM_BARS];
+        /*

xen/include/xen/vpci.h struct vpci { · line 161

Quoted 10 lines

xen/include/xen/vpci.h +10 -0

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

xen/drivers/passthrough/pci.c +3 -0

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

+ sriov->vfs_ready = 0;

xen/drivers/vpci/Makefile +4 -0

+    ASSERT(rw_is_locked(&d->pci_lock));
+
+    for_each_pdev ( d, pdev )
+    {

xen/drivers/vpci/Makefile +22 -0

+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(). */

xen/drivers/vpci/Makefile +6 -0

+    pdev->vpci->sriov = xvzalloc(struct vpci_sriov);
+    if ( !pdev->vpci->sriov )
+        return -ENOMEM;
+
+    pdev->vpci->sriov->pos = pos;
+

xen/drivers/vpci/Makefile +9 -0

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

xen/drivers/vpci/Makefile +21 -0

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

xen/drivers/vpci/Makefile +9 -0

@@ -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;

CHANGELOG.md The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) · line 19

Quoted 7 lines · Mykyta Poturai

CHANGELOG.md +1 -0

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

Quoted 23 lines

xen/drivers/passthrough/pci.c +3 -0

@@ -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;
  }
 

xen/drivers/passthrough/pci.c const unsigned long *pci_get_ro_map(u16 seg) · line 148

Quoted 4 lines

xen/drivers/passthrough/pci.c +4 -0

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

Quoted 5 lines · Jan Beulich

Quoted 4 lines · Mykyta Poturai

xen/drivers/passthrough/pci.c +0 -0

@@ -139,6 +142,47 @@ const unsigned long *pci_get_ro_map(u16 seg)
      return pseg ? pseg->ro_map : NULL;
  }
 

Quoted 8 lines

xen/drivers/passthrough/pci.c const unsigned long *pci_get_ro_map(u16 seg) · line 148

Quoted 4 lines

xen/drivers/passthrough/pci.c +4 -0

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

It isn't. The test runs ahead of any lock because adding the VF takes them.

We could put it under the hardware domain's pci_lock, which is what control_write() holds for marking and unmarking, but it would have to be dropped before vpci_sriov_add_vf() anyway: pci_add_device() takes pcidevs_lock, and the order is pcidevs_lock then pci_lock. So the test and the add can't be made atomic with the locks we have. Is the lock worth taking just for the bit test?

The only thing I can see actually misfiring is vf_setup_map: xvzalloc_array() zeroes it and the pointer store publishes it with nothing in between, so a reader can theoretically see the pointer ahead of the zeroing and test an uninitialised bit. Should we reallocate it in alloc_pseg(), or just add a memory barrier?

Quoted 14 lines · Mykyta Poturai

Quoted 5 lines · Jan Beulich

Quoted 4 lines · Mykyta Poturai

xen/drivers/passthrough/pci.c +0 -0

@@ -139,6 +142,47 @@ const unsigned long *pci_get_ro_map(u16 seg)
       return pseg ? pseg->ro_map : NULL;
   }
 

Quoted 8 lines

xen/drivers/passthrough/pci.c const unsigned long *pci_get_ro_map(u16 seg) · line 148

Quoted 4 lines

xen/drivers/passthrough/pci.c +4 -0

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

Quoted 8 lines

It isn't. The test runs ahead of any lock because adding the VF takes them.

We could put it under the hardware domain's pci_lock, which is what control_write() holds for marking and unmarking, but it would have to be dropped before vpci_sriov_add_vf() anyway: pci_add_device() takes pcidevs_lock, and the order is pcidevs_lock then pci_lock. So the test and the add can't be made atomic with the locks we have. Is the lock worth taking just for the bit test?

No, such locking would make sense only when held from before the test until after the result was actually acted upon (in the case here: the bit was cleared by pci_vf_setup_end()).

pcidevs_lock vs d->pci_lock wouldn't really be an issue here, though: The former is recursive, so you could acquire ahead of acquiring d->pci_lock. The "inner" acquire in pci_add_device() would then be little more than a no-op. (Of course proper commentary would need adding if such an approach was taken.)

Quoted 4 lines

The only thing I can see actually misfiring is vf_setup_map: xvzalloc_array() zeroes it and the pointer store publishes it with nothing in between, so a reader can theoretically see the pointer ahead of the zeroing and test an uninitialised bit. Should we reallocate it in alloc_pseg(), or just add a memory barrier?

Hmm, is this really the only issue? What if two racing pci_vf_setup_pending() both yield true? Then we'd end up with two competing vpci_sriov_add_vf().

Jan

xen/drivers/vpci/Makefile line 4

Quoted 7 lines · Mykyta Poturai

xen/drivers/vpci/Makefile +1 -0

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

Quoted 10 lines

xen/drivers/vpci/Makefile +10 -0

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

xen/drivers/vpci/Makefile

Quoted 4 lines

xen/drivers/vpci/Makefile +4 -0

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

xen/drivers/vpci/Makefile

Quoted 6 lines

xen/drivers/vpci/Makefile +6 -0

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

xen/drivers/vpci/Makefile

Quoted 3 lines

xen/drivers/vpci/Makefile +3 -0

+    pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_SRIOV);
+
+    ASSERT(pos);

... this one?

From: Stewart Hildebrand <[email protected]>

Emulate guest BAR register values based on PF BAR values for VFs. This allows creating a guest view of the normal BAR registers and emulates the size and properties as it is done during PCI device enumeration by the guest.

Expose VID/DID and class/revision to the guest.

Note, that VFs ROM BAR is read-only and is all zeros, but VF may provide access to the PFs ROM via emulation and is not implemented.

Note this only removes one of the gaps on the way to vPCI DomU support; VFs are not usable in DomUs until the rest of it is upstream. The DomU testing behind this patch used out-of-tree vPCI DomU patches.

Signed-off-by Stewart Hildebrand <[email protected]>

Signed-off-by Mykyta Poturai <[email protected]>

xen/drivers/vpci/sriov.c +0 -0

@@ -79,19 +79,80 @@ int vpci_vf_init_header(struct pci_dev *vf_pdev)
     const struct pci_dev *pf_pdev;

xen/drivers/vpci/sriov.c +7 -2

-    ctrl = pci_conf_read16(pf_pdev->sbdf,
-                           pf_pdev->vpci->sriov->pos + PCI_SRIOV_CTRL);
+    ctrl = pci_conf_read16(pf_pdev->sbdf, sriov_pos + PCI_SRIOV_CTRL);
+
+    if ( IS_ENABLED(CONFIG_HAS_VPCI_GUEST_SUPPORT) &&
+         pf_pdev->domain != vf_pdev->domain )
+    {
+        struct vpci_bar *bars = vf_pdev->vpci->header.bars;
+        /*

xen/drivers/vpci/sriov.c +8 -0

+        for ( unsigned int i = 0; i < PCI_SRIOV_NUM_BARS; i++ )
+        {
+            switch ( pf_pdev->vpci->sriov->vf_bars[i].type )
+            {
+            case VPCI_BAR_MEM32:
+            case VPCI_BAR_MEM64_LO:
+            case VPCI_BAR_MEM64_HI:
+                rc = vpci_add_register(vf_pdev->vpci, vpci_guest_mem_bar_read,