Implement SR-IOV support for PVH

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

+
 #endif /* __XEN__ */
 
 #else /* !CONFIG_HAS_VPCI */

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

index a98ddbb32e..dd233b8b03 100644

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

 /* Passthrough handlers. */
 uint32_t cf_check vpci_hw_read8(
     const struct pci_dev *pdev, unsigned int reg, void *data);

There is no need to store ranges for each PCI device, as they are only used during the mapping/unmapping process and can be reused for each device. This also allows to avoid the need to allocate and destroy rangesets for each device.

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

---
v1->v2:
* new patch

---
 xen/common/domain.c       | 24 ++++++++++++++
 xen/drivers/vpci/header.c | 66 ++++++++++++++-------------------------
 xen/drivers/vpci/vpci.c   |  3 --
 xen/include/xen/vpci.h    |  4 ++-
 4 files changed, 51 insertions(+), 46 deletions(-)

xen/common/domain.c +0 -0

index e06174fca7..76b0163616 100644

xen/common/domain.c +6 -0

+            if ( !v->vpci.bar_mem[i] )
+                goto fail_sched;
+        }
+    }
+#endif
+
     d->vcpu[vcpu_id] = v;
     if ( vcpu_id != 0 )
     {

xen/drivers/vpci/header.c +24 -42

index 07ec991a12..cb64d9b9fc 100644
@@ -195,6 +195,7 @@ bool vpci_process_pending(struct vcpu *v)
     for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
     {
         struct vpci_bar *bar = &header->bars[i];
+        struct rangeset *mem = v->vpci.bar_mem[i];
         struct map_data data = {
             .d = v->domain,
             .map = v->vpci.cmd & PCI_COMMAND_MEMORY,

@@ -202,10 +203,10 @@ bool vpci_process_pending(struct vcpu *v)
         };
         int rc;
 
-        if ( rangeset_is_empty(bar->mem) )
+        if ( rangeset_is_empty(mem) )
             continue;
 
-        rc = rangeset_consume_ranges(bar->mem, map_range, &data);
+        rc = rangeset_consume_ranges(mem, map_range, &data);
 
         if ( rc == -ERESTART )
         {

@@ -223,8 +224,8 @@ bool vpci_process_pending(struct vcpu *v)
 
             /* Clean all the rangesets */
             for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
-                if ( !rangeset_is_empty(header->bars[i].mem) )
-                     rangeset_purge(header->bars[i].mem);
+                if ( !rangeset_is_empty(v->vpci.bar_mem[i]) )
+                     rangeset_purge(v->vpci.bar_mem[i]);
 
             v->vpci.pdev = NULL;
 

@@ -259,13 +260,14 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
     for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
     {
         struct vpci_bar *bar = &header->bars[i];
+        struct rangeset *mem = current->vpci.bar_mem[i];
         struct map_data data = { .d = d, .map = true, .bar = bar };
 
-        if ( rangeset_is_empty(bar->mem) )
+        if ( rangeset_is_empty(mem) )
             continue;
 
-        while ( (rc = rangeset_consume_ranges(bar->mem, map_range,
-                                              &data)) == -ERESTART )
+        while ( (rc = rangeset_consume_ranges(mem, map_range, &data)) ==
+                -ERESTART )
         {
             /*
              * It's safe to drop and reacquire the lock in this context

@@ -330,12 +332,13 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
     for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
     {
         struct vpci_bar *bar = &header->bars[i];
+        struct rangeset *mem = current->vpci.bar_mem[i];
         unsigned long start = PFN_DOWN(bar->addr);
         unsigned long end = PFN_DOWN(bar->addr + bar->size - 1);
         unsigned long start_guest = PFN_DOWN(bar->guest_addr);
         unsigned long end_guest = PFN_DOWN(bar->guest_addr + bar->size - 1);
 
-        if ( !bar->mem )
+        if ( !mem )
             continue;
 
         if ( !MAPPABLE_BAR(bar) ||

@@ -353,7 +356,7 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
             continue;
         }
 
-        ASSERT(rangeset_is_empty(bar->mem));
+        ASSERT(rangeset_is_empty(mem));
 
         /*
          * Make sure that the guest set address has the same page offset

@@ -368,7 +371,7 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
             return -EINVAL;
         }
 
-        rc = rangeset_add_range(bar->mem, start_guest, end_guest);
+        rc = rangeset_add_range(mem, start_guest, end_guest);
         if ( rc )
         {
             printk(XENLOG_G_WARNING "Failed to add [%lx, %lx]: %d\n",

@@ -379,12 +382,12 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
         /* Check for overlap with the already setup BAR ranges. */
         for ( j = 0; j < i; j++ )
         {
-            struct vpci_bar *prev_bar = &header->bars[j];
+            struct rangeset *prev_mem = current->vpci.bar_mem[j];
 
-            if ( rangeset_is_empty(prev_bar->mem) )
+            if ( rangeset_is_empty(prev_mem) )
                 continue;
 
-            rc = rangeset_remove_range(prev_bar->mem, start_guest, end_guest);
+            rc = rangeset_remove_range(prev_mem, start_guest, end_guest);
             if ( rc )
             {
                 gprintk(XENLOG_WARNING,

@@ -394,7 +397,7 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
             }
         }
 
-        rc = pci_sanitize_bar_memory(bar->mem);
+        rc = pci_sanitize_bar_memory(mem);
         if ( rc )
         {
             gprintk(XENLOG_WARNING,

@@ -411,14 +414,14 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
         unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
                                      vmsix_table_size(pdev->vpci, i) - 1);
 
-        for ( j = 0; j < ARRAY_SIZE(header->bars); j++ )
+        for ( j = 0; j < ARRAY_SIZE(current->vpci.bar_mem); j++ )
         {
-            const struct vpci_bar *bar = &header->bars[j];
+            struct rangeset *mem = current->vpci.bar_mem[j];
 
-            if ( rangeset_is_empty(bar->mem) )
+            if ( rangeset_is_empty(mem) )
                 continue;
 
-            rc = rangeset_remove_range(bar->mem, start, end);
+            rc = rangeset_remove_range(mem, start, end);
             if ( rc )
             {
                 gprintk(XENLOG_WARNING,

@@ -468,8 +471,9 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
                 for ( j = 0; j < ARRAY_SIZE(header->bars); j++)
                 {
                     const struct vpci_bar *bar = &header->bars[j];
+                    struct rangeset *mem = current->vpci.bar_mem[j];
 
-                    if ( !rangeset_overlaps_range(bar->mem, start, end) ||
+                    if ( !rangeset_overlaps_range(mem, start, end) ||
                          /*
                           * If only the ROM enable bit is toggled check against
                           * other BARs in the same device for overlaps, but not

@@ -480,7 +484,7 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
                           bar->type == VPCI_BAR_ROM) )
                         continue;
 
-                    rc = rangeset_remove_range(bar->mem, start, end);
+                    rc = rangeset_remove_range(mem, start, end);
                     if ( rc )
                     {
                         gprintk(XENLOG_WARNING,

@@ -733,18 +737,6 @@ static void cf_check rom_write(
     }
 }
 
-static int bar_add_rangeset(const struct pci_dev *pdev, struct vpci_bar *bar,
-                            unsigned int i)
-{
-    char str[32];
-
-    snprintf(str, sizeof(str), "%pp:BAR%u", &pdev->sbdf, i);
-
-    bar->mem = rangeset_new(pdev->domain, str, RANGESETF_no_print);
-
-    return !bar->mem ? -ENOMEM : 0;
-}
-
 static int vpci_init_capability_list(struct pci_dev *pdev)
 {
     int rc;

@@ -989,10 +981,6 @@ int vpci_init_header(struct pci_dev *pdev)
         else
             bars[i].type = VPCI_BAR_MEM32;
 
-        rc = bar_add_rangeset(pdev, &bars[i], i);
-        if ( rc )
-            goto fail;
-
         rc = pci_size_mem_bar(pdev->sbdf, reg, &addr, &size,
                               (i == num_bars - 1) ? PCI_BAR_LAST : 0);
         if ( rc < 0 )

@@ -1046,12 +1034,6 @@ int vpci_init_header(struct pci_dev *pdev)
                                4, rom);
         if ( rc )
             rom->type = VPCI_BAR_EMPTY;
-        else
-        {
-            rc = bar_add_rangeset(pdev, rom, num_bars);
-            if ( rc )
-                goto fail;
-        }
     }
     else if ( !is_hwdom )
     {

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

index f66f50c8ba..af61b521b0 100644
@@ -357,9 +357,6 @@ void vpci_deassign_device(struct pci_dev *pdev)
     }
     spin_unlock(&pdev->vpci->lock);
 
-    for ( i = 0; i < ARRAY_SIZE(pdev->vpci->header.bars); i++ )
-        rangeset_destroy(pdev->vpci->header.bars[i].mem);
-
     xfree(pdev->vpci);
     pdev->vpci = NULL;
 }

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

index dd233b8b03..fa654545e5 100644
@@ -118,7 +118,6 @@ struct vpci {
             uint64_t guest_addr;
             uint64_t size;
             uint64_t resizable_sizes;
-            struct rangeset *mem;
             enum {
                 VPCI_BAR_EMPTY,
                 VPCI_BAR_IO,

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

+#endif
     uint16_t cmd;
     bool rom_only : 1;
 };

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

index 07ec991a12..cb64d9b9fc 100644

xen/common/domain.c static int vcpu_teardown(struct vcpu *v) · line 458

Quoted 8 lines · Mykyta Poturai

xen/common/domain.c +2 -0

@@ -454,6 +454,14 @@ static int vcpu_teardown(struct vcpu *v)
  */
 static void vcpu_destroy(struct vcpu *v)
 {
+#ifdef CONFIG_HAS_VPCI
+    int i;

Nit: No plain int please when ...

xen/common/domain.c static int vcpu_teardown(struct vcpu *v) · line 459

Quoted 1 line

xen/common/domain.c +1 -0

+    for ( i = 0; i < ARRAY_SIZE(v->vpci.bar_mem); i++ )

... its values are only ever non-negative. (Applies elsewhere as well.)

This allows waiting a specified number of cycles on the vcpu. Once the wait has finished a callback is executed.

Note that this is still not used, but introduced here in order to simplify the complexity of the patches that actually make use of it.

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

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

---
v1->v2:
* new patch

---
 xen/drivers/vpci/header.c | 125 ++++++++++++++++++++++----------------
 xen/include/xen/vpci.h    |  19 ++++++
 2 files changed, 90 insertions(+), 54 deletions(-)

xen/drivers/vpci/header.c +51 -49

index cb64d9b9fc..284964f0d4 100644
@@ -175,76 +175,92 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
 
 bool vpci_process_pending(struct vcpu *v)
 {
-    const struct pci_dev *pdev = v->vpci.pdev;
-    struct vpci_header *header = NULL;
-    unsigned int i;
-
-    if ( !pdev )
-        return false;
-
-    read_lock(&v->domain->pci_lock);
-
-    if ( !pdev->vpci || (v->domain != pdev->domain) )
+    switch ( v->vpci.task )
     {
-        v->vpci.pdev = NULL;
-        read_unlock(&v->domain->pci_lock);
-        return false;
-    }
-
-    header = &pdev->vpci->header;
-    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+    case MODIFY_MEMORY:
     {
-        struct vpci_bar *bar = &header->bars[i];
-        struct rangeset *mem = v->vpci.bar_mem[i];
-        struct map_data data = {
-            .d = v->domain,
-            .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
-            .bar = bar,
-        };
-        int rc;
+        const struct pci_dev *pdev = v->vpci.memory.pdev;
+        struct vpci_header *header = NULL;
+        unsigned int i;
 
-        if ( rangeset_is_empty(mem) )
-            continue;
+        if ( !pdev )
+            break;
 
-        rc = rangeset_consume_ranges(mem, map_range, &data);
+        read_lock(&v->domain->pci_lock);
 
-        if ( rc == -ERESTART )
+        if ( !pdev->vpci || (v->domain != pdev->domain) )
         {
+            v->vpci.memory.pdev = NULL;
             read_unlock(&v->domain->pci_lock);
-            return true;
+            break;
         }
 
-        if ( rc )
+        header = &pdev->vpci->header;
+        for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
         {
-            spin_lock(&pdev->vpci->lock);
-            /* Disable memory decoding unconditionally on failure. */
-            modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
-                            false);
-            spin_unlock(&pdev->vpci->lock);
+            struct vpci_bar *bar = &header->bars[i];
+            struct rangeset *mem = v->vpci.bar_mem[i];
+            struct map_data data = {
+                .d = v->domain,
+                .map = v->vpci.memory.cmd & PCI_COMMAND_MEMORY,
+                .bar = bar,
+            };
+            int rc;
+
+            if ( rangeset_is_empty(mem) )
+                continue;
 
-            /* Clean all the rangesets */
-            for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
-                if ( !rangeset_is_empty(v->vpci.bar_mem[i]) )
-                     rangeset_purge(v->vpci.bar_mem[i]);
+            rc = rangeset_consume_ranges(mem, map_range, &data);
 
-            v->vpci.pdev = NULL;
+            if ( rc == -ERESTART )
+            {
+                read_unlock(&v->domain->pci_lock);
+                return true;
+            }
 
-            read_unlock(&v->domain->pci_lock);
+            if ( rc )
+            {
+                spin_lock(&pdev->vpci->lock);
+                /* Disable memory decoding unconditionally on failure. */
+                modify_decoding(pdev, v->vpci.memory.cmd & ~PCI_COMMAND_MEMORY,
+                                false);
+                spin_unlock(&pdev->vpci->lock);
+
+                /* Clean all the rangesets */
+                for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+                    if ( !rangeset_is_empty(v->vpci.bar_mem[i]) )
+                        rangeset_purge(v->vpci.bar_mem[i]);
+
+                v->vpci.memory.pdev = NULL;
+
+                read_unlock(&v->domain->pci_lock);
 
-            if ( !is_hardware_domain(v->domain) )
-                domain_crash(v->domain);
+                if ( !is_hardware_domain(v->domain) )
+                    domain_crash(v->domain);
 
-            return false;
+                break;
+            }
         }
-    }
-    v->vpci.pdev = NULL;
+        v->vpci.memory.pdev = NULL;
 
-    spin_lock(&pdev->vpci->lock);
-    modify_decoding(pdev, v->vpci.cmd, v->vpci.rom_only);

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

+    case WAIT:
+        if ( NOW() < v->vpci.wait.end )
+            return true;
+        v->vpci.wait.callback(v->vpci.wait.data);
+        break;
+    case NONE:
+        return false;
+    }
 
+    v->vpci.task = NONE;
     return false;
 }
 

@@ -295,9 +311,10 @@ static void defer_map(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
      * is mapped. This can lead to parallel mapping operations being
      * started for the same device if the domain is not well-behaved.
      */
-    curr->vpci.pdev = pdev;
-    curr->vpci.cmd = cmd;
-    curr->vpci.rom_only = rom_only;
+    curr->vpci.memory.pdev = pdev;
+    curr->vpci.memory.cmd = cmd;
+    curr->vpci.memory.rom_only = rom_only;
+    curr->vpci.task = MODIFY_MEMORY;
     /*
      * Raise a scheduler softirq in order to prevent the guest from resuming
      * execution with pending mapping operations, to trigger the invocation

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

index fa654545e5..47cdb54d42 100644

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

 #ifdef __XEN__
+    enum {
+        NONE,
+        MODIFY_MEMORY,
+        WAIT,
+    } task;
     struct rangeset *bar_mem[PCI_HEADER_NORMAL_NR_BARS + 1];
+    union {
+        struct {
+            /* Store state while {un}mapping of PCI BARs. */
+            const struct pci_dev *pdev;
+            uint16_t cmd;
+            bool rom_only : 1;
+        } memory;

xen/drivers/vpci/header.c +54 -50

@@ -175,76 +175,92 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
 
 bool vpci_process_pending(struct vcpu *v)
 {
-    const struct pci_dev *pdev = v->vpci.pdev;
-    struct vpci_header *header = NULL;
-    unsigned int i;
-
-    if ( !pdev )
-        return false;
-
-    read_lock(&v->domain->pci_lock);
-
-    if ( !pdev->vpci || (v->domain != pdev->domain) )
+    switch ( v->vpci.task )
     {
-        v->vpci.pdev = NULL;
-        read_unlock(&v->domain->pci_lock);
-        return false;
-    }
-
-    header = &pdev->vpci->header;
-    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+    case MODIFY_MEMORY:
     {
-        struct vpci_bar *bar = &header->bars[i];
-        struct rangeset *mem = v->vpci.bar_mem[i];
-        struct map_data data = {
-            .d = v->domain,
-            .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
-            .bar = bar,
-        };
-        int rc;
+        const struct pci_dev *pdev = v->vpci.memory.pdev;
+        struct vpci_header *header = NULL;
+        unsigned int i;
 
-        if ( rangeset_is_empty(mem) )
-            continue;
+        if ( !pdev )
+            break;
 
-        rc = rangeset_consume_ranges(mem, map_range, &data);
+        read_lock(&v->domain->pci_lock);
 
-        if ( rc == -ERESTART )
+        if ( !pdev->vpci || (v->domain != pdev->domain) )
         {
+            v->vpci.memory.pdev = NULL;
             read_unlock(&v->domain->pci_lock);
-            return true;
+            break;
         }
 
-        if ( rc )
+        header = &pdev->vpci->header;
+        for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
         {
-            spin_lock(&pdev->vpci->lock);
-            /* Disable memory decoding unconditionally on failure. */
-            modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
-                            false);
-            spin_unlock(&pdev->vpci->lock);
+            struct vpci_bar *bar = &header->bars[i];
+            struct rangeset *mem = v->vpci.bar_mem[i];
+            struct map_data data = {
+                .d = v->domain,
+                .map = v->vpci.memory.cmd & PCI_COMMAND_MEMORY,
+                .bar = bar,
+            };
+            int rc;
+
+            if ( rangeset_is_empty(mem) )
+                continue;
 
-            /* Clean all the rangesets */
-            for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
-                if ( !rangeset_is_empty(v->vpci.bar_mem[i]) )
-                     rangeset_purge(v->vpci.bar_mem[i]);
+            rc = rangeset_consume_ranges(mem, map_range, &data);
 
-            v->vpci.pdev = NULL;
+            if ( rc == -ERESTART )
+            {
+                read_unlock(&v->domain->pci_lock);
+                return true;
+            }
 
-            read_unlock(&v->domain->pci_lock);
+            if ( rc )
+            {
+                spin_lock(&pdev->vpci->lock);
+                /* Disable memory decoding unconditionally on failure. */
+                modify_decoding(pdev, v->vpci.memory.cmd & ~PCI_COMMAND_MEMORY,
+                                false);
+                spin_unlock(&pdev->vpci->lock);
+
+                /* Clean all the rangesets */
+                for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+                    if ( !rangeset_is_empty(v->vpci.bar_mem[i]) )
+                        rangeset_purge(v->vpci.bar_mem[i]);
+
+                v->vpci.memory.pdev = NULL;
+
+                read_unlock(&v->domain->pci_lock);
 
-            if ( !is_hardware_domain(v->domain) )
-                domain_crash(v->domain);
+                if ( !is_hardware_domain(v->domain) )
+                    domain_crash(v->domain);
 
-            return false;
+                break;
+            }
         }
-    }
-    v->vpci.pdev = NULL;
+        v->vpci.memory.pdev = NULL;
 
-    spin_lock(&pdev->vpci->lock);
-    modify_decoding(pdev, v->vpci.cmd, v->vpci.rom_only);
-    spin_unlock(&pdev->vpci->lock);
+        spin_lock(&pdev->vpci->lock);
+        modify_decoding(pdev, v->vpci.memory.cmd, v->vpci.memory.rom_only);
+        spin_unlock(&pdev->vpci->lock);
 

Quoted 128 lines · Mykyta Poturai

xen/drivers/vpci/header.c +54 -50

@@ -175,76 +175,92 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
 
  bool vpci_process_pending(struct vcpu *v)
  {
-    const struct pci_dev *pdev = v->vpci.pdev;
-    struct vpci_header *header = NULL;
-    unsigned int i;
-
-    if ( !pdev )
-        return false;
-
-    read_lock(&v->domain->pci_lock);
-
-    if ( !pdev->vpci || (v->domain != pdev->domain) )
+    switch ( v->vpci.task )
      {
-        v->vpci.pdev = NULL;
-        read_unlock(&v->domain->pci_lock);
-        return false;
-    }
-
-    header = &pdev->vpci->header;
-    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+    case MODIFY_MEMORY:
      {
-        struct vpci_bar *bar = &header->bars[i];
-        struct rangeset *mem = v->vpci.bar_mem[i];
-        struct map_data data = {
-            .d = v->domain,
-            .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
-            .bar = bar,
-        };
-        int rc;
+        const struct pci_dev *pdev = v->vpci.memory.pdev;
+        struct vpci_header *header = NULL;
+        unsigned int i;
 
-        if ( rangeset_is_empty(mem) )
-            continue;
+        if ( !pdev )
+            break;
 
-        rc = rangeset_consume_ranges(mem, map_range, &data);
+        read_lock(&v->domain->pci_lock);
 
-        if ( rc == -ERESTART )
+        if ( !pdev->vpci || (v->domain != pdev->domain) )
          {
+            v->vpci.memory.pdev = NULL;
              read_unlock(&v->domain->pci_lock);
-            return true;
+            break;
          }
 
-        if ( rc )
+        header = &pdev->vpci->header;
+        for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
          {
-            spin_lock(&pdev->vpci->lock);
-            /* Disable memory decoding unconditionally on failure. */
-            modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
-                            false);
-            spin_unlock(&pdev->vpci->lock);
+            struct vpci_bar *bar = &header->bars[i];
+            struct rangeset *mem = v->vpci.bar_mem[i];
+            struct map_data data = {
+                .d = v->domain,
+                .map = v->vpci.memory.cmd & PCI_COMMAND_MEMORY,
+                .bar = bar,
+            };
+            int rc;
+
+            if ( rangeset_is_empty(mem) )
+                continue;
 
-            /* Clean all the rangesets */
-            for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
-                if ( !rangeset_is_empty(v->vpci.bar_mem[i]) )
-                     rangeset_purge(v->vpci.bar_mem[i]);
+            rc = rangeset_consume_ranges(mem, map_range, &data);
 
-            v->vpci.pdev = NULL;
+            if ( rc == -ERESTART )
+            {
+                read_unlock(&v->domain->pci_lock);
+                return true;
+            }
 
-            read_unlock(&v->domain->pci_lock);
+            if ( rc )
+            {
+                spin_lock(&pdev->vpci->lock);
+                /* Disable memory decoding unconditionally on failure. */
+                modify_decoding(pdev, v->vpci.memory.cmd & ~PCI_COMMAND_MEMORY,
+                                false);
+                spin_unlock(&pdev->vpci->lock);
+
+                /* Clean all the rangesets */
+                for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+                    if ( !rangeset_is_empty(v->vpci.bar_mem[i]) )
+                        rangeset_purge(v->vpci.bar_mem[i]);
+
+                v->vpci.memory.pdev = NULL;
+
+                read_unlock(&v->domain->pci_lock);
 
-            if ( !is_hardware_domain(v->domain) )
-                domain_crash(v->domain);
+                if ( !is_hardware_domain(v->domain) )
+                    domain_crash(v->domain);
 
-            return false;
+                break;
+            }
          }
-    }
-    v->vpci.pdev = NULL;
+        v->vpci.memory.pdev = NULL;
 
-    spin_lock(&pdev->vpci->lock);
-    modify_decoding(pdev, v->vpci.cmd, v->vpci.rom_only);
-    spin_unlock(&pdev->vpci->lock);
+        spin_lock(&pdev->vpci->lock);
+        modify_decoding(pdev, v->vpci.memory.cmd, v->vpci.memory.rom_only);
+        spin_unlock(&pdev->vpci->lock);
 

xen/drivers/vpci/header.c static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd, · line 258

Quoted 10 lines

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

-    read_unlock(&v->domain->pci_lock);
+        read_unlock(&v->domain->pci_lock);
+
+        break;
+    }
+    case WAIT:
+        if ( NOW() < v->vpci.wait.end )
+            return true;
+        v->vpci.wait.callback(v->vpci.wait.data);
+        break;

As just indicated in reply to patch 6, busy waiting isn't really acceptable. This is even more so when the waiting exceeds the typical length of a scheduling timeslice.

In that other reply I said to put the vCPU to sleep, but you need to be careful there too: The domain may not expect its vCPU to not make any progress for such an extended period of time. This may need doing entirely differently: Once the command register was written, you may want to record the time after which accesses to the VF registers are permitted. Earlier accesses would simply be terminated. You may still additionally need a timer, in order to kick off BAR mapping after that time. (Yet better would be if the BAR mapping could be done during those 100ms. After all that may be a reason why this long a delay is specified: Firmware on the device may also require some time to set up the BARs accordingly.)

Jan

Quoted 144 lines · Jan Beulich

Quoted 128 lines · Mykyta Poturai

xen/drivers/vpci/header.c +54 -50

@@ -175,76 +175,92 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
 
  bool vpci_process_pending(struct vcpu *v)
  {
-    const struct pci_dev *pdev = v->vpci.pdev;
-    struct vpci_header *header = NULL;
-    unsigned int i;
-
-    if ( !pdev )
-        return false;
-
-    read_lock(&v->domain->pci_lock);
-
-    if ( !pdev->vpci || (v->domain != pdev->domain) )
+    switch ( v->vpci.task )
      {
-        v->vpci.pdev = NULL;
-        read_unlock(&v->domain->pci_lock);
-        return false;
-    }
-
-    header = &pdev->vpci->header;
-    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+    case MODIFY_MEMORY:
      {
-        struct vpci_bar *bar = &header->bars[i];
-        struct rangeset *mem = v->vpci.bar_mem[i];
-        struct map_data data = {
-            .d = v->domain,
-            .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
-            .bar = bar,
-        };
-        int rc;
+        const struct pci_dev *pdev = v->vpci.memory.pdev;
+        struct vpci_header *header = NULL;
+        unsigned int i;
 
-        if ( rangeset_is_empty(mem) )
-            continue;
+        if ( !pdev )
+            break;
 
-        rc = rangeset_consume_ranges(mem, map_range, &data);
+        read_lock(&v->domain->pci_lock);
 
-        if ( rc == -ERESTART )
+        if ( !pdev->vpci || (v->domain != pdev->domain) )
          {
+            v->vpci.memory.pdev = NULL;
              read_unlock(&v->domain->pci_lock);
-            return true;
+            break;
          }
 
-        if ( rc )
+        header = &pdev->vpci->header;
+        for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
          {
-            spin_lock(&pdev->vpci->lock);
-            /* Disable memory decoding unconditionally on failure. */
-            modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
-                            false);
-            spin_unlock(&pdev->vpci->lock);
+            struct vpci_bar *bar = &header->bars[i];
+            struct rangeset *mem = v->vpci.bar_mem[i];
+            struct map_data data = {
+                .d = v->domain,
+                .map = v->vpci.memory.cmd & PCI_COMMAND_MEMORY,
+                .bar = bar,
+            };
+            int rc;
+
+            if ( rangeset_is_empty(mem) )
+                continue;
 
-            /* Clean all the rangesets */
-            for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
-                if ( !rangeset_is_empty(v->vpci.bar_mem[i]) )
-                     rangeset_purge(v->vpci.bar_mem[i]);
+            rc = rangeset_consume_ranges(mem, map_range, &data);
 
-            v->vpci.pdev = NULL;
+            if ( rc == -ERESTART )
+            {
+                read_unlock(&v->domain->pci_lock);
+                return true;
+            }
 
-            read_unlock(&v->domain->pci_lock);
+            if ( rc )
+            {
+                spin_lock(&pdev->vpci->lock);
+                /* Disable memory decoding unconditionally on failure. */
+                modify_decoding(pdev, v->vpci.memory.cmd & ~PCI_COMMAND_MEMORY,
+                                false);
+                spin_unlock(&pdev->vpci->lock);
+
+                /* Clean all the rangesets */
+                for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+                    if ( !rangeset_is_empty(v->vpci.bar_mem[i]) )
+                        rangeset_purge(v->vpci.bar_mem[i]);
+
+                v->vpci.memory.pdev = NULL;
+
+                read_unlock(&v->domain->pci_lock);
 
-            if ( !is_hardware_domain(v->domain) )
-                domain_crash(v->domain);
+                if ( !is_hardware_domain(v->domain) )
+                    domain_crash(v->domain);
 
-            return false;
+                break;
+            }
          }
-    }
-    v->vpci.pdev = NULL;
+        v->vpci.memory.pdev = NULL;
 
-    spin_lock(&pdev->vpci->lock);
-    modify_decoding(pdev, v->vpci.cmd, v->vpci.rom_only);
-    spin_unlock(&pdev->vpci->lock);
+        spin_lock(&pdev->vpci->lock);
+        modify_decoding(pdev, v->vpci.memory.cmd, v->vpci.memory.rom_only);
+        spin_unlock(&pdev->vpci->lock);
 

xen/drivers/vpci/header.c static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd, · line 258

Quoted 10 lines

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

-    read_unlock(&v->domain->pci_lock);
+        read_unlock(&v->domain->pci_lock);
+
+        break;
+    }
+    case WAIT:
+        if ( NOW() < v->vpci.wait.end )
+            return true;
+        v->vpci.wait.callback(v->vpci.wait.data);
+        break;

As just indicated in reply to patch 6, busy waiting isn't really acceptable. This is even more so when the waiting exceeds the typical length of a scheduling timeslice.

Quoted 10 lines

In that other reply I said to put the vCPU to sleep, but you need to be careful there too: The domain may not expect its vCPU to not make any progress for such an extended period of time. This may need doing entirely differently: Once the command register was written, you may want to record the time after which accesses to the VF registers are permitted. Earlier accesses would simply be terminated. You may still additionally need a timer, in order to kick off BAR mapping after that time. (Yet better would be if the BAR mapping could be done during those 100ms. After all that may be a reason why this long a delay is specified: Firmware on the device may also require some time to set up the BARs accordingly.)

I am not sure it would work that way. If we look at how linux initialized sriov, it writes VFE and MSE bits, waits 100ms and then expects VFs to be operational. If they are not operational at that moment, then it considers the operation failed and removes all VFs. If we also wait 100ms before enabling access, the probability of a guest trying to access something before we allow it would be very high.

Quoted 156 lines · Mykyta Poturai

Quoted 144 lines · Jan Beulich

Quoted 128 lines · Mykyta Poturai

xen/drivers/vpci/header.c +54 -50

@@ -175,76 +175,92 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
 
   bool vpci_process_pending(struct vcpu *v)
   {
-    const struct pci_dev *pdev = v->vpci.pdev;
-    struct vpci_header *header = NULL;
-    unsigned int i;
-
-    if ( !pdev )
-        return false;
-
-    read_lock(&v->domain->pci_lock);
-
-    if ( !pdev->vpci || (v->domain != pdev->domain) )
+    switch ( v->vpci.task )
       {
-        v->vpci.pdev = NULL;
-        read_unlock(&v->domain->pci_lock);
-        return false;
-    }
-
-    header = &pdev->vpci->header;
-    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+    case MODIFY_MEMORY:
       {
-        struct vpci_bar *bar = &header->bars[i];
-        struct rangeset *mem = v->vpci.bar_mem[i];
-        struct map_data data = {
-            .d = v->domain,
-            .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
-            .bar = bar,
-        };
-        int rc;
+        const struct pci_dev *pdev = v->vpci.memory.pdev;
+        struct vpci_header *header = NULL;
+        unsigned int i;
 
-        if ( rangeset_is_empty(mem) )
-            continue;
+        if ( !pdev )
+            break;
 
-        rc = rangeset_consume_ranges(mem, map_range, &data);
+        read_lock(&v->domain->pci_lock);
 
-        if ( rc == -ERESTART )
+        if ( !pdev->vpci || (v->domain != pdev->domain) )
           {
+            v->vpci.memory.pdev = NULL;
               read_unlock(&v->domain->pci_lock);
-            return true;
+            break;
           }
 
-        if ( rc )
+        header = &pdev->vpci->header;
+        for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
           {
-            spin_lock(&pdev->vpci->lock);
-            /* Disable memory decoding unconditionally on failure. */
-            modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
-                            false);
-            spin_unlock(&pdev->vpci->lock);
+            struct vpci_bar *bar = &header->bars[i];
+            struct rangeset *mem = v->vpci.bar_mem[i];
+            struct map_data data = {
+                .d = v->domain,
+                .map = v->vpci.memory.cmd & PCI_COMMAND_MEMORY,
+                .bar = bar,
+            };
+            int rc;
+
+            if ( rangeset_is_empty(mem) )
+                continue;
 
-            /* Clean all the rangesets */
-            for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
-                if ( !rangeset_is_empty(v->vpci.bar_mem[i]) )
-                     rangeset_purge(v->vpci.bar_mem[i]);
+            rc = rangeset_consume_ranges(mem, map_range, &data);
 
-            v->vpci.pdev = NULL;
+            if ( rc == -ERESTART )
+            {
+                read_unlock(&v->domain->pci_lock);
+                return true;
+            }
 
-            read_unlock(&v->domain->pci_lock);
+            if ( rc )
+            {
+                spin_lock(&pdev->vpci->lock);
+                /* Disable memory decoding unconditionally on failure. */
+                modify_decoding(pdev, v->vpci.memory.cmd & ~PCI_COMMAND_MEMORY,
+                                false);
+                spin_unlock(&pdev->vpci->lock);
+
+                /* Clean all the rangesets */
+                for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+                    if ( !rangeset_is_empty(v->vpci.bar_mem[i]) )
+                        rangeset_purge(v->vpci.bar_mem[i]);
+
+                v->vpci.memory.pdev = NULL;
+
+                read_unlock(&v->domain->pci_lock);
 
-            if ( !is_hardware_domain(v->domain) )
-                domain_crash(v->domain);
+                if ( !is_hardware_domain(v->domain) )
+                    domain_crash(v->domain);
 
-            return false;
+                break;
+            }
           }
-    }
-    v->vpci.pdev = NULL;
+        v->vpci.memory.pdev = NULL;
 
-    spin_lock(&pdev->vpci->lock);
-    modify_decoding(pdev, v->vpci.cmd, v->vpci.rom_only);
-    spin_unlock(&pdev->vpci->lock);
+        spin_lock(&pdev->vpci->lock);
+        modify_decoding(pdev, v->vpci.memory.cmd, v->vpci.memory.rom_only);
+        spin_unlock(&pdev->vpci->lock);
 

xen/drivers/vpci/header.c static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd, · line 258

Quoted 10 lines

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

-    read_unlock(&v->domain->pci_lock);
+        read_unlock(&v->domain->pci_lock);
+
+        break;
+    }
+    case WAIT:
+        if ( NOW() < v->vpci.wait.end )
+            return true;
+        v->vpci.wait.callback(v->vpci.wait.data);
+        break;

As just indicated in reply to patch 6, busy waiting isn't really acceptable. This is even more so when the waiting exceeds the typical length of a scheduling timeslice.

Quoted 10 lines

In that other reply I said to put the vCPU to sleep, but you need to be careful there too: The domain may not expect its vCPU to not make any progress for such an extended period of time. This may need doing entirely differently: Once the command register was written, you may want to record the time after which accesses to the VF registers are permitted. Earlier accesses would simply be terminated. You may still additionally need a timer, in order to kick off BAR mapping after that time. (Yet better would be if the BAR mapping could be done during those 100ms. After all that may be a reason why this long a delay is specified: Firmware on the device may also require some time to set up the BARs accordingly.)

Quoted 6 lines

I am not sure it would work that way. If we look at how linux initialized sriov, it writes VFE and MSE bits, waits 100ms and then expects VFs to be operational. If they are not operational at that moment, then it considers the operation failed and removes all VFs. If we also wait 100ms before enabling access, the probability of a guest trying to access something before we allow it would be very high.

Well, not really. Our counting of the 100ms necessarily starts before Dom0's. Furthermore it may be acceptable (or even appropriate) to stall premature accesses (because they shouldn't occur in the first place), by blocking the vCPU at that point. A middle route may be possible: Terminate accesses in, say, the first 90ms, and stall the vCPU for any access past that, but before the 100ms expired.

Quoted 164 lines · Jan Beulich

Quoted 156 lines · Mykyta Poturai

Quoted 144 lines · Jan Beulich

Quoted 128 lines · Mykyta Poturai

xen/drivers/vpci/header.c +54 -50

@@ -175,76 +175,92 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
 
   bool vpci_process_pending(struct vcpu *v)
   {
-    const struct pci_dev *pdev = v->vpci.pdev;
-    struct vpci_header *header = NULL;
-    unsigned int i;
-
-    if ( !pdev )
-        return false;
-
-    read_lock(&v->domain->pci_lock);
-
-    if ( !pdev->vpci || (v->domain != pdev->domain) )
+    switch ( v->vpci.task )
       {
-        v->vpci.pdev = NULL;
-        read_unlock(&v->domain->pci_lock);
-        return false;
-    }
-
-    header = &pdev->vpci->header;
-    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+    case MODIFY_MEMORY:
       {
-        struct vpci_bar *bar = &header->bars[i];
-        struct rangeset *mem = v->vpci.bar_mem[i];
-        struct map_data data = {
-            .d = v->domain,
-            .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
-            .bar = bar,
-        };
-        int rc;
+        const struct pci_dev *pdev = v->vpci.memory.pdev;
+        struct vpci_header *header = NULL;
+        unsigned int i;
 
-        if ( rangeset_is_empty(mem) )
-            continue;
+        if ( !pdev )
+            break;
 
-        rc = rangeset_consume_ranges(mem, map_range, &data);
+        read_lock(&v->domain->pci_lock);
 
-        if ( rc == -ERESTART )
+        if ( !pdev->vpci || (v->domain != pdev->domain) )
           {
+            v->vpci.memory.pdev = NULL;
               read_unlock(&v->domain->pci_lock);
-            return true;
+            break;
           }
 
-        if ( rc )
+        header = &pdev->vpci->header;
+        for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
           {
-            spin_lock(&pdev->vpci->lock);
-            /* Disable memory decoding unconditionally on failure. */
-            modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
-                            false);
-            spin_unlock(&pdev->vpci->lock);
+            struct vpci_bar *bar = &header->bars[i];
+            struct rangeset *mem = v->vpci.bar_mem[i];
+            struct map_data data = {
+                .d = v->domain,
+                .map = v->vpci.memory.cmd & PCI_COMMAND_MEMORY,
+                .bar = bar,
+            };
+            int rc;
+
+            if ( rangeset_is_empty(mem) )
+                continue;
 
-            /* Clean all the rangesets */
-            for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
-                if ( !rangeset_is_empty(v->vpci.bar_mem[i]) )
-                     rangeset_purge(v->vpci.bar_mem[i]);
+            rc = rangeset_consume_ranges(mem, map_range, &data);
 
-            v->vpci.pdev = NULL;
+            if ( rc == -ERESTART )
+            {
+                read_unlock(&v->domain->pci_lock);
+                return true;
+            }
 
-            read_unlock(&v->domain->pci_lock);
+            if ( rc )
+            {
+                spin_lock(&pdev->vpci->lock);
+                /* Disable memory decoding unconditionally on failure. */
+                modify_decoding(pdev, v->vpci.memory.cmd & ~PCI_COMMAND_MEMORY,
+                                false);
+                spin_unlock(&pdev->vpci->lock);
+
+                /* Clean all the rangesets */
+                for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+                    if ( !rangeset_is_empty(v->vpci.bar_mem[i]) )
+                        rangeset_purge(v->vpci.bar_mem[i]);
+
+                v->vpci.memory.pdev = NULL;
+
+                read_unlock(&v->domain->pci_lock);
 
-            if ( !is_hardware_domain(v->domain) )
-                domain_crash(v->domain);
+                if ( !is_hardware_domain(v->domain) )
+                    domain_crash(v->domain);
 
-            return false;
+                break;
+            }
           }
-    }
-    v->vpci.pdev = NULL;
+        v->vpci.memory.pdev = NULL;
 
-    spin_lock(&pdev->vpci->lock);
-    modify_decoding(pdev, v->vpci.cmd, v->vpci.rom_only);
-    spin_unlock(&pdev->vpci->lock);
+        spin_lock(&pdev->vpci->lock);
+        modify_decoding(pdev, v->vpci.memory.cmd, v->vpci.memory.rom_only);
+        spin_unlock(&pdev->vpci->lock);
 

xen/drivers/vpci/header.c static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd, · line 258

Quoted 10 lines

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

-    read_unlock(&v->domain->pci_lock);
+        read_unlock(&v->domain->pci_lock);
+
+        break;
+    }
+    case WAIT:
+        if ( NOW() < v->vpci.wait.end )
+            return true;
+        v->vpci.wait.callback(v->vpci.wait.data);
+        break;

As just indicated in reply to patch 6, busy waiting isn't really acceptable. This is even more so when the waiting exceeds the typical length of a scheduling timeslice.

Quoted 10 lines

In that other reply I said to put the vCPU to sleep, but you need to be careful there too: The domain may not expect its vCPU to not make any progress for such an extended period of time. This may need doing entirely differently: Once the command register was written, you may want to record the time after which accesses to the VF registers are permitted. Earlier accesses would simply be terminated. You may still additionally need a timer, in order to kick off BAR mapping after that time. (Yet better would be if the BAR mapping could be done during those 100ms. After all that may be a reason why this long a delay is specified: Firmware on the device may also require some time to set up the BARs accordingly.)

Quoted 6 lines

I am not sure it would work that way. If we look at how linux initialized sriov, it writes VFE and MSE bits, waits 100ms and then expects VFs to be operational. If they are not operational at that moment, then it considers the operation failed and removes all VFs. If we also wait 100ms before enabling access, the probability of a guest trying to access something before we allow it would be very high.

Quoted 6 lines

Well, not really. Our counting of the 100ms necessarily starts before Dom0's. Furthermore it may be acceptable (or even appropriate) to stall premature accesses (because they shouldn't occur in the first place), by blocking the vCPU at that point. A middle route may be possible: Terminate accesses in, say, the first 90ms, and stall the vCPU for any access past that, but before the 100ms expired.

Is there any real benefit to doing all this work instead of just waiting for Dom0 to register the FVs? Implementing it the way you described would require a relatively complex state machine and two timers per sriov-capable device. And will also probably require some hacks to handle partially initialized VFs in Xen. This adds a lot of work and many possible bugs for not a lot of benefit in my opinion.

Quoted 172 lines · Mykyta Poturai

Quoted 164 lines · Jan Beulich

Quoted 156 lines · Mykyta Poturai

Quoted 144 lines · Jan Beulich

Quoted 128 lines · Mykyta Poturai

xen/drivers/vpci/header.c +54 -50

@@ -175,76 +175,92 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
 
    bool vpci_process_pending(struct vcpu *v)
    {
-    const struct pci_dev *pdev = v->vpci.pdev;
-    struct vpci_header *header = NULL;
-    unsigned int i;
-
-    if ( !pdev )
-        return false;
-
-    read_lock(&v->domain->pci_lock);
-
-    if ( !pdev->vpci || (v->domain != pdev->domain) )
+    switch ( v->vpci.task )
        {
-        v->vpci.pdev = NULL;
-        read_unlock(&v->domain->pci_lock);
-        return false;
-    }
-
-    header = &pdev->vpci->header;
-    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+    case MODIFY_MEMORY:
        {
-        struct vpci_bar *bar = &header->bars[i];
-        struct rangeset *mem = v->vpci.bar_mem[i];
-        struct map_data data = {
-            .d = v->domain,
-            .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
-            .bar = bar,
-        };
-        int rc;
+        const struct pci_dev *pdev = v->vpci.memory.pdev;
+        struct vpci_header *header = NULL;
+        unsigned int i;
 
-        if ( rangeset_is_empty(mem) )
-            continue;
+        if ( !pdev )
+            break;
 
-        rc = rangeset_consume_ranges(mem, map_range, &data);
+        read_lock(&v->domain->pci_lock);
 
-        if ( rc == -ERESTART )
+        if ( !pdev->vpci || (v->domain != pdev->domain) )
            {
+            v->vpci.memory.pdev = NULL;
                read_unlock(&v->domain->pci_lock);
-            return true;
+            break;
            }
 
-        if ( rc )
+        header = &pdev->vpci->header;
+        for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
            {
-            spin_lock(&pdev->vpci->lock);
-            /* Disable memory decoding unconditionally on failure. */
-            modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
-                            false);
-            spin_unlock(&pdev->vpci->lock);
+            struct vpci_bar *bar = &header->bars[i];
+            struct rangeset *mem = v->vpci.bar_mem[i];
+            struct map_data data = {
+                .d = v->domain,
+                .map = v->vpci.memory.cmd & PCI_COMMAND_MEMORY,
+                .bar = bar,
+            };
+            int rc;
+
+            if ( rangeset_is_empty(mem) )
+                continue;
 
-            /* Clean all the rangesets */
-            for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
-                if ( !rangeset_is_empty(v->vpci.bar_mem[i]) )
-                     rangeset_purge(v->vpci.bar_mem[i]);
+            rc = rangeset_consume_ranges(mem, map_range, &data);
 
-            v->vpci.pdev = NULL;
+            if ( rc == -ERESTART )
+            {
+                read_unlock(&v->domain->pci_lock);
+                return true;
+            }
 
-            read_unlock(&v->domain->pci_lock);
+            if ( rc )
+            {
+                spin_lock(&pdev->vpci->lock);
+                /* Disable memory decoding unconditionally on failure. */
+                modify_decoding(pdev, v->vpci.memory.cmd & ~PCI_COMMAND_MEMORY,
+                                false);
+                spin_unlock(&pdev->vpci->lock);
+
+                /* Clean all the rangesets */
+                for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+                    if ( !rangeset_is_empty(v->vpci.bar_mem[i]) )
+                        rangeset_purge(v->vpci.bar_mem[i]);
+
+                v->vpci.memory.pdev = NULL;
+
+                read_unlock(&v->domain->pci_lock);
 
-            if ( !is_hardware_domain(v->domain) )
-                domain_crash(v->domain);
+                if ( !is_hardware_domain(v->domain) )
+                    domain_crash(v->domain);
 
-            return false;
+                break;
+            }
            }
-    }
-    v->vpci.pdev = NULL;
+        v->vpci.memory.pdev = NULL;
 
-    spin_lock(&pdev->vpci->lock);
-    modify_decoding(pdev, v->vpci.cmd, v->vpci.rom_only);
-    spin_unlock(&pdev->vpci->lock);
+        spin_lock(&pdev->vpci->lock);
+        modify_decoding(pdev, v->vpci.memory.cmd, v->vpci.memory.rom_only);
+        spin_unlock(&pdev->vpci->lock);
 

xen/drivers/vpci/header.c static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd, · line 258

Quoted 10 lines

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

-    read_unlock(&v->domain->pci_lock);
+        read_unlock(&v->domain->pci_lock);
+
+        break;
+    }
+    case WAIT:
+        if ( NOW() < v->vpci.wait.end )
+            return true;
+        v->vpci.wait.callback(v->vpci.wait.data);
+        break;

As just indicated in reply to patch 6, busy waiting isn't really acceptable. This is even more so when the waiting exceeds the typical length of a scheduling timeslice.

Quoted 10 lines

In that other reply I said to put the vCPU to sleep, but you need to be careful there too: The domain may not expect its vCPU to not make any progress for such an extended period of time. This may need doing entirely differently: Once the command register was written, you may want to record the time after which accesses to the VF registers are permitted. Earlier accesses would simply be terminated. You may still additionally need a timer, in order to kick off BAR mapping after that time. (Yet better would be if the BAR mapping could be done during those 100ms. After all that may be a reason why this long a delay is specified: Firmware on the device may also require some time to set up the BARs accordingly.)

Quoted 6 lines

I am not sure it would work that way. If we look at how linux initialized sriov, it writes VFE and MSE bits, waits 100ms and then expects VFs to be operational. If they are not operational at that moment, then it considers the operation failed and removes all VFs. If we also wait 100ms before enabling access, the probability of a guest trying to access something before we allow it would be very high.

Quoted 6 lines

Well, not really. Our counting of the 100ms necessarily starts before Dom0's. Furthermore it may be acceptable (or even appropriate) to stall premature accesses (because they shouldn't occur in the first place), by blocking the vCPU at that point. A middle route may be possible: Terminate accesses in, say, the first 90ms, and stall the vCPU for any access past that, but before the 100ms expired.

Quoted 6 lines

Is there any real benefit to doing all this work instead of just waiting for Dom0 to register the FVs? Implementing it the way you described would require a relatively complex state machine and two timers per sriov-capable device. And will also probably require some hacks to handle partially initialized VFs in Xen. This adds a lot of work and many possible bugs for not a lot of benefit in my opinion.

Odd that you ask me this question. If there was no benefit, why did you do it this way?

Quoted 180 lines · Jan Beulich

Quoted 172 lines · Mykyta Poturai

Quoted 164 lines · Jan Beulich

Quoted 156 lines · Mykyta Poturai

Quoted 144 lines · Jan Beulich

Quoted 128 lines · Mykyta Poturai

xen/drivers/vpci/header.c +54 -50

@@ -175,76 +175,92 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
 
    bool vpci_process_pending(struct vcpu *v)
    {
-    const struct pci_dev *pdev = v->vpci.pdev;
-    struct vpci_header *header = NULL;
-    unsigned int i;
-
-    if ( !pdev )
-        return false;
-
-    read_lock(&v->domain->pci_lock);
-
-    if ( !pdev->vpci || (v->domain != pdev->domain) )
+    switch ( v->vpci.task )
        {
-        v->vpci.pdev = NULL;
-        read_unlock(&v->domain->pci_lock);
-        return false;
-    }
-
-    header = &pdev->vpci->header;
-    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+    case MODIFY_MEMORY:
        {
-        struct vpci_bar *bar = &header->bars[i];
-        struct rangeset *mem = v->vpci.bar_mem[i];
-        struct map_data data = {
-            .d = v->domain,
-            .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
-            .bar = bar,
-        };
-        int rc;
+        const struct pci_dev *pdev = v->vpci.memory.pdev;
+        struct vpci_header *header = NULL;
+        unsigned int i;
 
-        if ( rangeset_is_empty(mem) )
-            continue;
+        if ( !pdev )
+            break;
 
-        rc = rangeset_consume_ranges(mem, map_range, &data);
+        read_lock(&v->domain->pci_lock);
 
-        if ( rc == -ERESTART )
+        if ( !pdev->vpci || (v->domain != pdev->domain) )
            {
+            v->vpci.memory.pdev = NULL;
                read_unlock(&v->domain->pci_lock);
-            return true;
+            break;
            }
 
-        if ( rc )
+        header = &pdev->vpci->header;
+        for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
            {
-            spin_lock(&pdev->vpci->lock);
-            /* Disable memory decoding unconditionally on failure. */
-            modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
-                            false);
-            spin_unlock(&pdev->vpci->lock);
+            struct vpci_bar *bar = &header->bars[i];
+            struct rangeset *mem = v->vpci.bar_mem[i];
+            struct map_data data = {
+                .d = v->domain,
+                .map = v->vpci.memory.cmd & PCI_COMMAND_MEMORY,
+                .bar = bar,
+            };
+            int rc;
+
+            if ( rangeset_is_empty(mem) )
+                continue;
 
-            /* Clean all the rangesets */
-            for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
-                if ( !rangeset_is_empty(v->vpci.bar_mem[i]) )
-                     rangeset_purge(v->vpci.bar_mem[i]);
+            rc = rangeset_consume_ranges(mem, map_range, &data);
 
-            v->vpci.pdev = NULL;
+            if ( rc == -ERESTART )
+            {
+                read_unlock(&v->domain->pci_lock);
+                return true;
+            }
 
-            read_unlock(&v->domain->pci_lock);
+            if ( rc )
+            {
+                spin_lock(&pdev->vpci->lock);
+                /* Disable memory decoding unconditionally on failure. */
+                modify_decoding(pdev, v->vpci.memory.cmd & ~PCI_COMMAND_MEMORY,
+                                false);
+                spin_unlock(&pdev->vpci->lock);
+
+                /* Clean all the rangesets */
+                for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+                    if ( !rangeset_is_empty(v->vpci.bar_mem[i]) )
+                        rangeset_purge(v->vpci.bar_mem[i]);
+
+                v->vpci.memory.pdev = NULL;
+
+                read_unlock(&v->domain->pci_lock);
 
-            if ( !is_hardware_domain(v->domain) )
-                domain_crash(v->domain);
+                if ( !is_hardware_domain(v->domain) )
+                    domain_crash(v->domain);
 
-            return false;
+                break;
+            }
            }
-    }
-    v->vpci.pdev = NULL;
+        v->vpci.memory.pdev = NULL;
 
-    spin_lock(&pdev->vpci->lock);
-    modify_decoding(pdev, v->vpci.cmd, v->vpci.rom_only);
-    spin_unlock(&pdev->vpci->lock);
+        spin_lock(&pdev->vpci->lock);
+        modify_decoding(pdev, v->vpci.memory.cmd, v->vpci.memory.rom_only);
+        spin_unlock(&pdev->vpci->lock);
 

xen/drivers/vpci/header.c static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd, · line 258

Quoted 10 lines

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

-    read_unlock(&v->domain->pci_lock);
+        read_unlock(&v->domain->pci_lock);
+
+        break;
+    }
+    case WAIT:
+        if ( NOW() < v->vpci.wait.end )
+            return true;
+        v->vpci.wait.callback(v->vpci.wait.data);
+        break;

As just indicated in reply to patch 6, busy waiting isn't really acceptable. This is even more so when the waiting exceeds the typical length of a scheduling timeslice.

Quoted 10 lines

In that other reply I said to put the vCPU to sleep, but you need to be careful there too: The domain may not expect its vCPU to not make any progress for such an extended period of time. This may need doing entirely differently: Once the command register was written, you may want to record the time after which accesses to the VF registers are permitted. Earlier accesses would simply be terminated. You may still additionally need a timer, in order to kick off BAR mapping after that time. (Yet better would be if the BAR mapping could be done during those 100ms. After all that may be a reason why this long a delay is specified: Firmware on the device may also require some time to set up the BARs accordingly.)

Quoted 6 lines

I am not sure it would work that way. If we look at how linux initialized sriov, it writes VFE and MSE bits, waits 100ms and then expects VFs to be operational. If they are not operational at that moment, then it considers the operation failed and removes all VFs. If we also wait 100ms before enabling access, the probability of a guest trying to access something before we allow it would be very high.

Quoted 6 lines

Well, not really. Our counting of the 100ms necessarily starts before Dom0's. Furthermore it may be acceptable (or even appropriate) to stall premature accesses (because they shouldn't occur in the first place), by blocking the vCPU at that point. A middle route may be possible: Terminate accesses in, say, the first 90ms, and stall the vCPU for any access past that, but before the 100ms expired.

Quoted 6 lines

Is there any real benefit to doing all this work instead of just waiting for Dom0 to register the FVs? Implementing it the way you described would require a relatively complex state machine and two timers per sriov-capable device. And will also probably require some hacks to handle partially initialized VFs in Xen. This adds a lot of work and many possible bugs for not a lot of benefit in my opinion.

Quoted 2 lines

Odd that you ask me this question. If there was no benefit, why did you do it this way?

Roger asked for this approach in V1, and I saw that it can be done in a relatively straightforward way, so I implemented it. I didn’t exactly get what the benefits were, but I assumed that there are some and the effort is not too big to just do it if the maintainers are asking for it.

Now with the posibility of redoing everything *again* and making it much more complex I started to really think if its really needed. So I want to know your and others' opinions on registering VFs with Dom0.

before trying to remove them.
Some IOMMU platforms, such as SMMU-V3 and IPMMU-VMSA don't support removing devices. Trying to call remove_device will result in a crash. So check that platform_ops have remove_device set before calling it.

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

@@ -1545,6 +1545,12 @@ static int iommu_remove_device(struct pci_dev *pdev)
     if ( !is_iommu_enabled(pdev->domain) )
         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 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.

Allow forcing vpci_modify_bars to not defer the actual mapping changes,

which is needed to fix the sequential calls to vpci_modify_bars when enabling VFs from Dom0.

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

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

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

---
v1->v2:
* switch to VF discovery by Xen
* fix sequential vpci_modify_bars calls
* move documentation changes to a separate commit

---
 xen/drivers/vpci/Makefile |   2 +-
 xen/drivers/vpci/header.c |  17 +-
 xen/drivers/vpci/sriov.c  | 363 ++++++++++++++++++++++++++++++++++++++
 xen/include/xen/vpci.h    |  12 +-
 4 files changed, 385 insertions(+), 9 deletions(-)
 create mode 100644 xen/drivers/vpci/sriov.c

xen/drivers/vpci/Makefile +0 -0

index a7c8a30a89..fe1e57b64d 100644

xen/drivers/vpci/header.c +10 -5

                             uint16_t cmd)
 {
     struct vpci_header *header = &pdev->vpci->header;

@@ -323,7 +323,8 @@ static void defer_map(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
     raise_softirq(SCHEDULE_SOFTIRQ);
 }
 
-int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
+int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only,
+                     bool no_defer)
 {
     struct vpci_header *header = &pdev->vpci->header;
     struct pci_dev *tmp;

@@ -519,7 +520,7 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
         d = dom_xen;
     }
 
-    if ( system_state < SYS_STATE_active )
+    if ( system_state < SYS_STATE_active || no_defer )
     {
         /*
          * Mappings might be created when building Dom0 if the memory decoding

@@ -566,7 +567,7 @@ static void cf_check cmd_write(
          * memory decoding bit has not been changed, so leave everything as-is,
          * hoping the guest will realize and try again.
          */
-        vpci_modify_bars(pdev, cmd, false);
+        vpci_modify_bars(pdev, cmd, false, false);
     else
         pci_conf_write16(pdev->sbdf, reg, cmd);
 }

@@ -736,7 +737,7 @@ static void cf_check rom_write(
      */
     else if ( vpci_modify_bars(pdev,
                                new_enabled ? PCI_COMMAND_MEMORY : 0,
-                               true) )
+                               true, false) )
         /*
          * No memory has been added or removed from the p2m (because the actual
          * p2m changes are deferred in defer_map) and the ROM enable bit has

@@ -954,6 +955,9 @@ int vpci_init_header(struct pci_dev *pdev)
 
     header->guest_cmd = cmd;
 
+    if ( pdev->info.is_virtfn )
+        return vf_init_header(pdev);
+
     /* Disable memory decoding before sizing. */
     if ( !is_hwdom || (cmd & PCI_COMMAND_MEMORY) )
         pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd & ~PCI_COMMAND_MEMORY);

@@ -1062,7 +1066,8 @@ int vpci_init_header(struct pci_dev *pdev)
             goto fail;
     }
 
-    return (cmd & PCI_COMMAND_MEMORY) ? vpci_modify_bars(pdev, cmd, false) : 0;
+    return (cmd & PCI_COMMAND_MEMORY)
+                ? vpci_modify_bars(pdev, cmd, false, false) : 0;
 
  fail:
     pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);

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

new file mode 100644
index 0000000000..6f691149e9

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

+#include <xen/sched.h>
+#include <xen/vpci.h>
+#include <xsm/xsm.h>
+
+static int vf_init_bars(const 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;
+    unsigned int sriov_pos = pci_find_ext_capability(pf_pdev,
+                                                     PCI_EXT_CAP_ID_SRIOV);
+    uint16_t offset = pci_conf_read16(pf_pdev->sbdf,
+                                      sriov_pos + PCI_SRIOV_VF_OFFSET);
+    uint16_t stride = pci_conf_read16(pf_pdev->sbdf,
+                                      sriov_pos + PCI_SRIOV_VF_STRIDE);
+
+    vf_idx = vf_pdev->sbdf.sbdf - (pf_pdev->sbdf.sbdf + offset);
+    if ( vf_idx < 0 )
+        return -EINVAL;
+
+    if ( stride )
+    {
+        if ( vf_idx % stride )
+            return -EINVAL;
+        vf_idx /= stride;
+    }
+
+    /*
+     * 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++ )
+    {
+        bars[i].addr = physfn_vf_bars[i].addr + vf_idx * physfn_vf_bars[i].size;
+        bars[i].guest_addr = bars[i].addr;
+        bars[i].size = physfn_vf_bars[i].size;
+        bars[i].type = physfn_vf_bars[i].type;
+        bars[i].prefetchable = physfn_vf_bars[i].prefetchable;
+    }
+
+    return 0;
+}

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

+        rc = vpci_modify_bars(vf_pdev, cmd, false, true);
+        if ( rc )
+        {
+            gprintk(XENLOG_ERR, "failed to %s VF %pp: %d\n",

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

+static int size_vf_bars(struct pci_dev *pf_pdev, unsigned int sriov_pos)
+{
+    /*
+     * NB: a non-const pci_dev of the PF is needed in order to update
+     * vf_rlen.
+     */
+    struct vpci_bar *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);
+
+    /* Read BARs for VFs out of PF's SR-IOV extended capability. */
+    bars = pf_pdev->vpci->sriov->vf_bars;
+    /* 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.
+         */
+        pf_pdev->physfn.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 = rc > 0 ? 0 : rc;
+
+    return rc;
+}
+
+struct callback_data {
+    const struct pci_dev *pdev;
+    unsigned int pos;
+    uint32_t value;
+    bool enable : 1;
+    bool disable : 1;
+    bool map : 1;
+    bool unmap : 1;
+};

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

+    }
+
+    if ( cb->enable || cb->disable )
+    {
+        for ( i = 0; i < sriov->num_vfs; i++ )
+        {
+            const pci_sbdf_t vf_sbdf = {
+                .sbdf = pdev->sbdf.sbdf + offset + stride * i,
+            };
+

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

+            }
+            if ( cb->disable )
+                rc = pci_remove_device(vf_sbdf.seg, vf_sbdf.bus, vf_sbdf.devfn);
+
+            if ( rc && rc != -ENODEV)
+                gprintk(XENLOG_ERR, "failed to %s VF %pp: %d\n",
+                        cb->enable ? "add" : "remove", &vf_sbdf, rc);
+        }
+    }
+
+    if ( cb->map )
+    {
+        write_lock(&pdev->domain->pci_lock);
+        rc = map_vfs(pdev, PCI_COMMAND_MEMORY);
+
+        if ( rc )
+            map_vfs(pdev, 0);
+        write_unlock(&pdev->domain->pci_lock);
+    }
+
+    pci_conf_write16(pdev->sbdf, cb->pos + PCI_SRIOV_CTRL, cb->value);
+    xfree(cb);
+}
+
+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;
+    struct callback_data *cb = NULL;
+    uint16_t control = pci_conf_read16(pdev->sbdf, reg);
+    bool mem_enabled = control & PCI_SRIOV_CTRL_MSE;
+    bool new_mem_enabled = val & PCI_SRIOV_CTRL_MSE;
+    bool enabled = control & PCI_SRIOV_CTRL_VFE;
+    bool new_enabled = val & PCI_SRIOV_CTRL_VFE;

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

+
+    if ( !cb )
+    {
+        gprintk(XENLOG_ERR,
+                "%pp: Unable to allocate memory for SR-IOV enable\n",
+                pdev);
+        return;
+    }
+
+    cb->pdev = pdev;
+    cb->pos = sriov_pos;
+    cb->value = val;
+    cb->map = new_mem_enabled && !mem_enabled;
+    cb->unmap = !new_mem_enabled && mem_enabled;
+    cb->enable = new_enabled && !enabled;
+    cb->disable = !new_enabled && enabled;
+
+    current->vpci.task = WAIT;
+    current->vpci.wait.callback = control_write_cb;
+    current->vpci.wait.data = cb;
+    current->vpci.wait.end = NOW();
+
+    if ( cb->enable )
+    {
+        size_vf_bars((struct pci_dev *)pdev, sriov_pos);
+
+        /*
+         * Only update the number of active VFs when enabling, when
+         * disabling use the cached value in order to always remove the same
+         * number of VFs that were active.
+         */
+        sriov->num_vfs = pci_conf_read16(pdev->sbdf,
+                                         sriov_pos + PCI_SRIOV_NUM_VF);
+        /*
+         * NB: VFE needs to be enabled before calling pci_add_device so Xen
+         * can access the config space of VFs. FIXME casting away const-ness
+         * to modify vf_rlen
+         */
+        pci_conf_write16(pdev->sbdf, reg, control | PCI_SRIOV_CTRL_VFE);
+        /*
+         * The spec states that the software must wait at least 100ms before
+         * attempting to access VF registers when enabling virtual functions
+         * on the PF.
+         */
+
+        current->vpci.wait.end = NOW() + MILLISECS(100);
+    }
+}
+
+int vf_init_header(struct pci_dev *vf_pdev)
+{
+    const struct pci_dev *pf_pdev;
+    unsigned int sriov_pos;
+    int rc = 0;
+    uint16_t ctrl;
+
+    ASSERT(rw_is_write_locked(&vf_pdev->domain->pci_lock));
+
+    if ( !vf_pdev->info.is_virtfn )
+        return 0;
+
+    pf_pdev = vf_pdev->pf_pdev;
+    ASSERT(pf_pdev);
+
+    rc = vf_init_bars(vf_pdev);
+    if ( rc )
+        return rc;
+
+    sriov_pos = pci_find_ext_capability(pf_pdev, PCI_EXT_CAP_ID_SRIOV);
+    ctrl = pci_conf_read16(pf_pdev->sbdf, sriov_pos + PCI_SRIOV_CTRL);
+
+    if ( (pf_pdev->domain == vf_pdev->domain) && (ctrl & PCI_SRIOV_CTRL_MSE) )
+    {
+        rc = vpci_modify_bars(vf_pdev, PCI_COMMAND_MEMORY, false, false);
+        if ( rc )
+            return rc;
+    }
+
+    return rc;
+}
+
+static int cf_check init_sriov(struct pci_dev *pdev)
+{
+    unsigned int pos;
+
+    ASSERT(!pdev->info.is_virtfn);
+
+    pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_SRIOV);

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

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

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

+
+    return 0;
+}
+
+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/include/xen/vpci.h +0 -0

index 47cdb54d42..ae5f3b7274 100644

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

 /* Assign vPCI to device by adding handlers. */
 int __must_check vpci_assign_device(struct pci_dev *pdev);

@@ -146,7 +147,6 @@ struct vpci {
          * upon to know whether BARs are mapped into the guest p2m.
          */
         bool bars_mapped      : 1;
-        /* FIXME: currently there's no support for SR-IOV. */
     } header;
 
     /* MSI data. */

@@ -200,6 +200,13 @@ struct vpci {
             struct vpci_arch_msix_entry arch;
         } entries[];
     } *msix;
+
+    struct vpci_sriov {
+        /* PF only */
+        struct vpci_bar vf_bars[PCI_SRIOV_NUM_BARS];
+        uint16_t num_vfs;
+    } *sriov;
+
 #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
     /* Guest SBDF of the device. */
 #define INVALID_GUEST_SBDF ((pci_sbdf_t){ .sbdf = ~0U })

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

 #endif /* __XEN__ */
 

Quoted 10 lines · Mykyta Poturai

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

Allow forcing vpci_modify_bars to not defer the actual mapping changes,

I don't think this is suitable. We perform the p2m operations in a deferred context because they may take a long time. And since they may take a long time, the logic is interruptible: in map_range(), we perform a general_preempt_check() and return -ERESTART so that we give a chance for other pending work to complete, including the scheduler softirq. If vpci_process_pending() returns true, it will be called again and is expected to resume where it left off. The vcpu won't continue until vpci_process_pending() returns false.

Quoted 2 lines

which is needed to fix the sequential calls to vpci_modify_bars when enabling VFs from Dom0.

I'm guessing you resorted to this because you need to perform multiple mapping operations, but the vPCI deferred mapping mechanism only supports a single operation? If so, this is an issue I've been attempting to resolve for some time with the BAR-write-with-memory-decoding-enabled series [1]. In that series I'm working on introducing the ability perform multiple mapping operations. I'm almost ready to send v3 of the BAR-write-with-memory-decoding-enabled series, and I hope you don't mind that I include your patch ("vpci: Use pervcpu ranges for BAR mapping"). You may consider the possibility of basing SR-IOV on this work if suitable.

[1] https://lore.kernel.org/xen-devel/[email protected]/T/#t

Regardless, ultimately we need to find a way to return from vpci_process_pending() during the potentially long-running p2m operations. As an alternative suggestion, could you return from control_write_cb() after each call to map_vfs(), and somehow make it resume where it left off?

xen/drivers/vpci/sriov.c new file +36 -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;
+    struct callback_data *cb = NULL;
+    uint16_t control = pci_conf_read16(pdev->sbdf, reg);
+    bool mem_enabled = control & PCI_SRIOV_CTRL_MSE;
+    bool new_mem_enabled = val & PCI_SRIOV_CTRL_MSE;
+    bool enabled = control & PCI_SRIOV_CTRL_VFE;
+    bool new_enabled = val & PCI_SRIOV_CTRL_VFE;
+
+    ASSERT(!pdev->info.is_virtfn);
+
+    if ( new_enabled == enabled && new_mem_enabled == mem_enabled )
+    {
+        pci_conf_write16(pdev->sbdf, reg, val);
+        return;
+    }
+
+    cb = xzalloc(struct callback_data);
+
+    if ( !cb )
+    {
+        gprintk(XENLOG_ERR,
+                "%pp: Unable to allocate memory for SR-IOV enable\n",
+                pdev);
+        return;
+    }
+
+    cb->pdev = pdev;
+    cb->pos = sriov_pos;
+    cb->value = val;
+    cb->map = new_mem_enabled && !mem_enabled;
+    cb->unmap = !new_mem_enabled && mem_enabled;
+    cb->enable = new_enabled && !enabled;

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

+        /*
+         * Only update the number of active VFs when enabling, when
+         * disabling use the cached value in order to always remove the same
+         * number of VFs that were active.
+         */
+        sriov->num_vfs = pci_conf_read16(pdev->sbdf,
+                                         sriov_pos + PCI_SRIOV_NUM_VF);
+        /*
+         * NB: VFE needs to be enabled before calling pci_add_device so Xen
+         * can access the config space of VFs. FIXME casting away const-ness

From: Stewart Hildebrand <[email protected]>

SR-IOV virtual functions have simplified configuration space such as Vendor ID is derived from the physical function and Device ID comes from SR-IOV extended capability.
Emulate those, so we can provide VID/DID pair for guests which use PCI passthrough for SR-IOV virtual functions.

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

+    {
+        uint16_t vid = pci_conf_read16(pf_pdev->sbdf, PCI_VENDOR_ID);
+        uint16_t did = pci_conf_read16(pf_pdev->sbdf,
+                                       sriov_pos + PCI_SRIOV_VF_DID);
+        struct vpci_bar *bars = vf_pdev->vpci->header.bars;
+        unsigned int i;
+
+        rc = vpci_add_register(vf_pdev->vpci, vpci_read_val, NULL,
+                               PCI_VENDOR_ID, 2, (void *)(uintptr_t)vid);
+        if ( rc )
+            return rc;
+
+        rc = vpci_add_register(vf_pdev->vpci, vpci_read_val, NULL,
+                               PCI_DEVICE_ID, 2, (void *)(uintptr_t)did);
+        if ( rc )
+            return rc;
+
+        /* Hardcode multi-function device bit to 0 */
+        rc = vpci_add_register(vf_pdev->vpci, vpci_read_val, NULL,
+                               PCI_HEADER_TYPE, 1,
+                               (void *)PCI_HEADER_TYPE_NORMAL);
+        if ( rc )
+            return rc;
+
+        rc = vpci_add_register(vf_pdev->vpci, vpci_hw_read32, NULL,
+                               PCI_CLASS_REVISION, 4, NULL);
+        if ( rc )
+            return rc;
+
+        for ( i = 0; i < PCI_SRIOV_NUM_BARS; i++ )
+        {
+            switch ( pf_pdev->vpci->sriov->vf_bars[i].type )
+            {