Implement SR-IOV support for PVH

From: Stewart Hildebrand <[email protected]>

Export functions required for SR-IOV support.

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

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.

Move the rangesets from struct vpci_bar to struct vpci_vcpu and perform (de-)allocation with vcpu (de-)allocation. Introduce RANGESET_DESTROY() macro to free a rangeset and set the pointer to NULL.

Amends: 622bdd962822 ("vpci/header: handle p2m range sets per BAR")

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

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

---
v2->v3:
* synced with BAR write with memory decoding enabled series[1]
* add Amends tag
* remove unused variable i due to rebasing over 998060dd9101 ("vPCI:
move vpci_init_capabilities() to a separate file")
* enclose entire struct vpci_vcpu inside #ifdef __XEN__
* s/bar_mem/mem/
* use ARRAY_SIZE
* put init/destroy in functions
* only allocate for domains with vPCI and idle domain
* replace 'if ( !mem ) continue;' with ASSERT

v1->v2
* new patch

[1]: https://patchew.org/Xen/[email protected]/

---
 xen/common/domain.c        |  5 +++
 xen/drivers/vpci/header.c  | 67 ++++++++++++++------------------------
 xen/drivers/vpci/vpci.c    | 34 ++++++++++++++++---
 xen/include/xen/rangeset.h |  7 ++++
 xen/include/xen/vpci.h     | 10 ++++--
 5 files changed, 73 insertions(+), 50 deletions(-)

xen/common/domain.c +5 -0

index bb9e210c28..5ef7db8f09 100644
@@ -455,6 +455,8 @@ static int vcpu_teardown(struct vcpu *v)
  */
 static void vcpu_destroy(struct vcpu *v)
 {
+    vpci_vcpu_destroy(v);
+
     free_vcpu_struct(v);
 }
 

@@ -512,6 +514,9 @@ struct vcpu *vcpu_create(struct domain *d, unsigned int vcpu_id)
     if ( arch_vcpu_create(v) != 0 )
         goto fail_sched;
 
+    if ( vpci_vcpu_init(v) )
+        goto fail_sched;
+
     d->vcpu[vcpu_id] = v;
     if ( vcpu_id != 0 )
     {

xen/drivers/vpci/header.c +17 -15

index 5d5ba5c016..5bfb541b6a 100644
@@ -196,6 +196,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.mem[i];
         struct map_data data = {
             .d = v->domain,
             .map = v->vpci.cmd & PCI_COMMAND_MEMORY,

@@ -203,10 +204,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 )
         {

@@ -224,8 +225,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.mem[i]) )
+                     rangeset_purge(v->vpci.mem[i]);
 
             v->vpci.pdev = NULL;
 

@@ -260,13 +261,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.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

@@ -331,13 +333,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.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 )
-            continue;
+        ASSERT(mem);
 
         if ( !MAPPABLE_BAR(bar) ||
              (rom_only ? bar->type != VPCI_BAR_ROM

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

@@ -369,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",

@@ -380,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.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,

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

xen/drivers/vpci/header.c +6 -27

         {
-            const struct vpci_bar *bar = &header->bars[j];
+            struct rangeset *mem = current->vpci.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,

@@ -469,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.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

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

@@ -734,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;
-}
-
 int vpci_init_header(struct pci_dev *pdev)
 {
     uint16_t cmd;

@@ -856,10 +847,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 )

@@ -913,12 +900,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 +29 -5

index 0ac9ec8b04..d069ca6d9c 100644
@@ -24,6 +24,35 @@
 
 #ifdef __XEN__
 
+void vpci_vcpu_destroy(struct vcpu *v)
+{
+    if ( !has_vpci(v->domain) && !is_idle_domain(v->domain) )
+        return;
+
+    for ( unsigned int i = 0; i < ARRAY_SIZE(v->vpci.mem); i++ )
+        RANGESET_DESTROY(v->vpci.mem[i]);
+}
+
+int vpci_vcpu_init(struct vcpu *v)
+{
+    unsigned int i;
+
+    if ( !has_vpci(v->domain) && !is_idle_domain(v->domain) )
+        return 0;
+
+    for ( i = 0; i < ARRAY_SIZE(v->vpci.mem); i++ )
+    {
+        char str[32];
+
+        snprintf(str, sizeof(str), "%pv:BAR%u", v, i);
+        v->vpci.mem[i] = rangeset_new(v->domain, str, RANGESETF_no_print);
+        if ( !v->vpci.mem[i] )
+            return -ENOMEM;
+    }
+
+    return 0;
+}
+
 #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
 static int assign_virtual_sbdf(struct pci_dev *pdev)
 {

@@ -89,8 +118,6 @@ struct vpci_register *vpci_get_register(const struct vpci *vpci,
 
 void vpci_deassign_device(struct pci_dev *pdev)
 {
-    unsigned int i;
-
     ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
 
     if ( !has_vpci(pdev->domain) || !pdev->vpci )

@@ -116,9 +143,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/rangeset.h +0 -0

index 817505badf..f01e00ec92 100644

Quoted 4 lines · Mykyta Poturai

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

@@ -412,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);
 

xen/drivers/vpci/header.c int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only) · line 417

Quoted 2 lines

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

-        for ( j = 0; j < ARRAY_SIZE(header->bars); j++ )
+        for ( j = 0; j < ARRAY_SIZE(current->vpci.mem); j++ )

You make a non-trivial use of current in vpci_modify_bars(), maybe you should consider introducing a local variable for it:

struct *vcpu curr = current;

current expands to a call to get_cpu_info(9, which is better to avoid doing repeatedly, specially in the context above which is used as a loop upper bound.

Quoted 5 lines · Roger Pau Monné

Quoted 4 lines · Mykyta Poturai

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

@@ -412,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);
 

Quoted 7 lines

xen/drivers/vpci/header.c int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only) · line 417

Quoted 2 lines

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

-        for ( j = 0; j < ARRAY_SIZE(header->bars); j++ )
+        for ( j = 0; j < ARRAY_SIZE(current->vpci.mem); j++ )

You make a non-trivial use of current in vpci_modify_bars(), maybe you should consider introducing a local variable for it:

struct *vcpu curr = current;

Without any intention to negate this, ...

Quoted 3 lines

current expands to a call to get_cpu_info(9, which is better to avoid doing repeatedly, specially in the context above which is used as a loop upper bound.

... I'd like to point out that "current" isn't evaluated when used by ARRAY_SIZE() (resolving to two uses of sizeof()).

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

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.

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

Message-ID: <[email protected]>

---
v2->v3:
* new patch in this series, borrowed from [1]

[1]: https://patchew.org/Xen/[email protected]/[email protected]/

---
 xen/common/domain.c        |   5 +-
 xen/drivers/vpci/header.c  | 227 ++++++++++++++++++++++++++-----------
 xen/drivers/vpci/vpci.c    |  28 +----
 xen/include/xen/rangeset.h |   7 --
 xen/include/xen/vpci.h     |  21 ++--
 5 files changed, 179 insertions(+), 109 deletions(-)

xen/common/domain.c +1 -4

index 5ef7db8f09..b1931be987 100644
@@ -455,8 +455,6 @@ static int vcpu_teardown(struct vcpu *v)
  */
 static void vcpu_destroy(struct vcpu *v)
 {
-    vpci_vcpu_destroy(v);
-
     free_vcpu_struct(v);
 }
 

@@ -514,8 +512,7 @@ struct vcpu *vcpu_create(struct domain *d, unsigned int vcpu_id)
     if ( arch_vcpu_create(v) != 0 )
         goto fail_sched;
 
-    if ( vpci_vcpu_init(v) )
-        goto fail_sched;
+    vpci_vcpu_init(v);
 
     d->vcpu[vcpu_id] = v;
     if ( vcpu_id != 0 )

xen/drivers/vpci/header.c +162 -65

index 5bfb541b6a..451cdd3a6f 100644
@@ -23,6 +23,7 @@
 #include <xen/lib.h>
 #include <xen/sched.h>
 #include <xen/softirq.h>
+#include <xen/xvmalloc.h>
 
 #include <xsm/xsm.h>
 

@@ -35,7 +36,7 @@
 
 struct map_data {
     struct domain *d;
-    const struct vpci_bar *bar;
+    const struct vpci_bar_map *bar;
     bool map;
 };
 

@@ -174,32 +175,20 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
         ASSERT_UNREACHABLE();
 }
 
-bool vpci_process_pending(struct vcpu *v)
+static int vpci_process_map_task(const struct pci_dev *pdev,
+                                 struct vpci_map_task *task)
 {
-    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) )
-    {
-        v->vpci.pdev = NULL;
-        read_unlock(&v->domain->pci_lock);
-        return false;
-    }
+    ASSERT(rw_is_locked(&pdev->domain->pci_lock));
 
-    header = &pdev->vpci->header;
-    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+    for ( i = 0; i < ARRAY_SIZE(task->bars); i++ )
     {
-        struct vpci_bar *bar = &header->bars[i];
-        struct rangeset *mem = v->vpci.mem[i];
+        struct vpci_bar_map *bar = &task->bars[i];
+        struct rangeset *mem = bar->mem;
         struct map_data data = {
-            .d = v->domain,
-            .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
+            .d = pdev->domain,
+            .map = task->cmd & PCI_COMMAND_MEMORY,
             .bar = bar,
         };
         int rc;

@@ -210,58 +199,116 @@ bool vpci_process_pending(struct vcpu *v)
         rc = rangeset_consume_ranges(mem, map_range, &data);
 
         if ( rc == -ERESTART )
-        {
-            read_unlock(&v->domain->pci_lock);
-            return true;
-        }
+            return rc;
 
         if ( rc )
         {
             spin_lock(&pdev->vpci->lock);
             /* Disable memory decoding unconditionally on failure. */
-            modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
-                            false);
+            modify_decoding(pdev, task->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.mem[i]) )
-                     rangeset_purge(v->vpci.mem[i]);
+            if ( !is_hardware_domain(pdev->domain) )
+                domain_crash(pdev->domain);
+
+            return rc;
+        }
+    }
+
+    spin_lock(&pdev->vpci->lock);
+    modify_decoding(pdev, task->cmd, task->rom_only);
+    spin_unlock(&pdev->vpci->lock);
+
+    return 0;
+}
+
+static void destroy_map_task(struct vpci_map_task *task)
+{
+    unsigned int i;
+
+    if ( !task )
+    {
+        ASSERT_UNREACHABLE();
+        return;
+    }
+
+    for ( i = 0; i < ARRAY_SIZE(task->bars); i++ )
+        rangeset_destroy(task->bars[i].mem);
+
+    xvfree(task);
+}
+
+static void clear_map_queue(struct vcpu *v)
+{
+    struct vpci_map_task *task;
+
+    while ( (task = list_first_entry_or_null(&v->vpci.task_queue,
+                                             struct vpci_map_task,
+                                             next)) != NULL )
+    {
+        list_del(&task->next);
+        destroy_map_task(task);
+    }
+}
+
+bool vpci_process_pending(struct vcpu *v)
+{
+    const struct pci_dev *pdev = v->vpci.pdev;
+    struct vpci_map_task *task;
 
-            v->vpci.pdev = NULL;
+    if ( !pdev )
+        return false;
 
+    read_lock(&v->domain->pci_lock);
+
+    if ( !pdev->vpci || (v->domain != pdev->domain) )
+    {
+        clear_map_queue(v);
+        v->vpci.pdev = NULL;
+        read_unlock(&v->domain->pci_lock);
+        return false;
+    }
+
+    while ( (task = list_first_entry_or_null(&v->vpci.task_queue,
+                                             struct vpci_map_task,
+                                             next)) != NULL )
+    {
+        int rc = vpci_process_map_task(pdev, task);
+
+        if ( rc == -ERESTART )
+        {
             read_unlock(&v->domain->pci_lock);
+            return true;
+        }
 
-            if ( !is_hardware_domain(v->domain) )
-                domain_crash(v->domain);
+        list_del(&task->next);
+        destroy_map_task(task);
 
-            return false;
+        if ( rc )
+        {
+            clear_map_queue(v);
+            break;
         }
     }
     v->vpci.pdev = NULL;
 
-    spin_lock(&pdev->vpci->lock);
-    modify_decoding(pdev, v->vpci.cmd, v->vpci.rom_only);
-    spin_unlock(&pdev->vpci->lock);
-
     read_unlock(&v->domain->pci_lock);
 
     return false;
 }
 
 static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
-                            uint16_t cmd)
+                            struct vpci_map_task *task)
 {
-    struct vpci_header *header = &pdev->vpci->header;
     int rc = 0;
     unsigned int i;
 
     ASSERT(rw_is_write_locked(&d->pci_lock));
 
-    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+    for ( i = 0; i < ARRAY_SIZE(task->bars); i++ )
     {
-        struct vpci_bar *bar = &header->bars[i];
-        struct rangeset *mem = current->vpci.mem[i];
+        struct vpci_bar_map *bar = &task->bars[i];
+        struct rangeset *mem = bar->mem;
         struct map_data data = { .d = d, .map = true, .bar = bar };
 
         if ( rangeset_is_empty(mem) )

@@ -281,15 +328,52 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
         }
     }
     if ( !rc )
-        modify_decoding(pdev, cmd, false);
+        modify_decoding(pdev, task->cmd, false);
 
     return rc;
 }
 
-static void defer_map(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
+static struct vpci_map_task *alloc_map_task(const struct pci_dev *pdev,
+                                            uint16_t cmd, bool rom_only)
+{
+    struct vpci_map_task *task;
+    unsigned int i;
+
+    task = xvzalloc(struct vpci_map_task);
+
+    if ( !task )
+        return NULL;
+
+    for ( i = 0; i < ARRAY_SIZE(task->bars); i++ )
+    {
+        if ( !MAPPABLE_BAR(&pdev->vpci->header.bars[i]) )
+            continue;
+
+        task->bars[i].mem = rangeset_new(pdev->domain, NULL,
+                                         RANGESETF_no_print);
+
+        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;
+
+    return task;
+}
+
+static void defer_map(const struct pci_dev *pdev, struct vpci_map_task *task)
 {
     struct vcpu *curr = current;
 
+    ASSERT(!curr->vpci.pdev || curr->vpci.pdev == pdev);
+
     /*
      * FIXME: when deferring the {un}map the state of the device should not
      * be trusted. For example the enable bit is toggled after the device

@@ -297,8 +381,8 @@ static void defer_map(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
      * 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;
+    list_add_tail(&task->next, &curr->vpci.task_queue);
+
     /*
      * Raise a scheduler softirq in order to prevent the guest from resuming
      * execution with pending mapping operations, to trigger the invocation

@@ -313,11 +397,17 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
     struct pci_dev *tmp;
     const struct domain *d;
     const struct vpci_msix *msix = pdev->vpci->msix;
+    struct vpci_map_task *task;
     unsigned int i, j;
     int rc;
 
     ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
 
+    task = alloc_map_task(pdev, cmd, rom_only);
+
+    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

@@ -333,19 +423,18 @@ 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.mem[i];
+        struct rangeset *mem = task->bars[i].mem;
         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);
 
-        ASSERT(mem);
+        if ( !mem )
+            continue;
 
         if ( !MAPPABLE_BAR(bar) ||
              (rom_only ? bar->type != VPCI_BAR_ROM
-                       : (bar->type == VPCI_BAR_ROM && !header->rom_enabled)) ||
-             /* Skip BARs already in the requested state. */
-             bar->enabled == !!(cmd & PCI_COMMAND_MEMORY) )
+                       : (bar->type == VPCI_BAR_ROM && !header->rom_enabled)) )
             continue;
 
         if ( !pci_check_bar(pdev, _mfn(start), _mfn(end)) )

@@ -368,7 +457,8 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
             gprintk(XENLOG_G_WARNING,
                     "%pp: can't map BAR%u - offset mismatch: %#lx vs %#lx\n",
                     &pdev->sbdf, i, bar->guest_addr, bar->addr);
-            return -EINVAL;
+            rc = -EINVAL;
+            goto fail;
         }
 
         rc = rangeset_add_range(mem, start_guest, end_guest);

@@ -376,13 +466,13 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
         {
             printk(XENLOG_G_WARNING "Failed to add [%lx, %lx]: %d\n",
                    start_guest, end_guest, rc);
-            return rc;
+            goto fail;
         }
 
         /* Check for overlap with the already setup BAR ranges. */
         for ( j = 0; j < i; j++ )
         {
-            struct rangeset *prev_mem = current->vpci.mem[j];
+            struct rangeset *prev_mem = task->bars[j].mem;
 
             if ( rangeset_is_empty(prev_mem) )
                 continue;

@@ -393,7 +483,7 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
                 gprintk(XENLOG_WARNING,
                        "%pp: failed to remove overlapping range [%lx, %lx]: %d\n",
                         &pdev->sbdf, start_guest, end_guest, rc);
-                return rc;
+                goto fail;
             }
         }
 

@@ -403,7 +493,7 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
             gprintk(XENLOG_WARNING,
                     "%pp: failed to sanitize BAR#%u memory: %d\n",
                     &pdev->sbdf, i, rc);
-            return rc;
+            goto fail;
         }
     }
 

@@ -414,9 +504,9 @@ 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(current->vpci.mem); j++ )
+        for ( j = 0; j < ARRAY_SIZE(task->bars); j++ )
         {
-            struct rangeset *mem = current->vpci.mem[j];
+            struct rangeset *mem = task->bars[j].mem;
 
             if ( rangeset_is_empty(mem) )
                 continue;

@@ -427,7 +517,7 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
                 gprintk(XENLOG_WARNING,
                        "%pp: failed to remove MSIX table [%lx, %lx]: %d\n",
                         &pdev->sbdf, start, end, rc);
-                return rc;
+                goto fail;
             }
         }
     }

@@ -471,7 +561,7 @@ 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.mem[j];
+                    struct rangeset *mem = task->bars[j].mem;
 
                     if ( !rangeset_overlaps_range(mem, start, end) ||
                          /*

@@ -490,7 +580,7 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
                         gprintk(XENLOG_WARNING,
                                 "%pp: failed to remove [%lx, %lx]: %d\n",
                                 &pdev->sbdf, start, end, rc);
-                        return rc;
+                        goto fail;
                     }
                 }
             }

@@ -513,12 +603,19 @@ int vpci_modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
          * will always be to establish mappings and process all the BARs.
          */
         ASSERT((cmd & PCI_COMMAND_MEMORY) && !rom_only);
-        return apply_map(pdev->domain, pdev, cmd);
+        rc = apply_map(pdev->domain, pdev, task);
+        destroy_map_task(task);
+        return rc;
     }
 
-    defer_map(pdev, cmd, rom_only);
+    defer_map(pdev, task);
 
     return 0;
+
+ fail:
+    destroy_map_task(task);
+
+    return rc;
 }
 
 static void cf_check cmd_write(

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

index d069ca6d9c..ce9fb5b357 100644
@@ -24,33 +24,9 @@
 
 #ifdef __XEN__
 
-void vpci_vcpu_destroy(struct vcpu *v)
+void vpci_vcpu_init(struct vcpu *v)
 {
-    if ( !has_vpci(v->domain) && !is_idle_domain(v->domain) )
-        return;
-
-    for ( unsigned int i = 0; i < ARRAY_SIZE(v->vpci.mem); i++ )
-        RANGESET_DESTROY(v->vpci.mem[i]);
-}
-
-int vpci_vcpu_init(struct vcpu *v)
-{
-    unsigned int i;
-
-    if ( !has_vpci(v->domain) && !is_idle_domain(v->domain) )
-        return 0;
-
-    for ( i = 0; i < ARRAY_SIZE(v->vpci.mem); i++ )
-    {
-        char str[32];
-
-        snprintf(str, sizeof(str), "%pv:BAR%u", v, i);
-        v->vpci.mem[i] = rangeset_new(v->domain, str, RANGESETF_no_print);
-        if ( !v->vpci.mem[i] )
-            return -ENOMEM;
-    }
-
-    return 0;
+    INIT_LIST_HEAD(&v->vpci.task_queue);
 }
 
 #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT

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

index f01e00ec92..817505badf 100644

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

-
 /*
  * Set a limit on the number of ranges that may exist in set @r.
  * NOTE: This must be called while @r is empty.

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

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

@@ -155,14 +154,23 @@ struct vpci {
 };
 
 #ifdef __XEN__
-struct vpci_vcpu {

Quoted 7 lines · Mykyta Poturai

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

index b55bacbe6e..e34f7abe6d 100644
@@ -155,14 +154,23 @@ struct vpci {
 };
 

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

Quoted 12 lines

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

 #ifdef __XEN__
-struct vpci_vcpu {
+struct vpci_map_task {
     /* Per-vcpu structure to store state while {un}mapping of PCI BARs. */
-    const struct pci_dev *pdev;
-    struct rangeset *mem[ARRAY_SIZE(((struct vpci_header *)NULL)->bars)];
+    struct list_head next;
+    struct vpci_bar_map {
+        uint64_t addr;
+        uint64_t guest_addr;
+        struct rangeset *mem;
+    } bars[ARRAY_SIZE(((struct vpci_header *)NULL)->bars)];

I'm a bit puzzled (possibly missing something), but why are you keeping this vpci_bar_map array here? AFAICT map tasks are allocated on-demand after this change (by using {alloc,destroy}_map_task()).

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. Also force VFs to always use emulated reads for command register, this is needed to prevent some drivers accidentally unmapping BARs. Discovery of VFs is done by Dom0, which must register them with Xen.

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

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

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

---
v2->v3:
* rework adding VFs with multiple mapping operations support
* use private.h
* style fixes
* fixup cleanup_sriov
* emulate command register read for VFs

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

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. Also force VFs to always use emulated reads for command register, this is needed to prevent some drivers accidentally unmapping BARs.

Discovery of VFs is
done by Dom0, which must register them with Xen.

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

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

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

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

@@ -0,0 +1,314 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Handlers for accesses to the SR-IOV capability structure.
+ *
+ * Copyright (C) 2026 Citrix Systems R&D
+ */
+
+#include <xen/sched.h>
+#include <xen/vpci.h>
+#include <xsm/xsm.h>
+#include "private.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;

xen/drivers/vpci/sriov.c new file +10 -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)
+    {
+        rc = vpci_modify_bars(vf_pdev, cmd, false);

xen/drivers/vpci/sriov.c new file +12 -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 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;
+    int rc;
+

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. Also force VFs to always use emulated reads for command register, this is needed to prevent some drivers accidentally unmapping BARs.

This apparently refers to the change to vpci_init_header(). Writes are already intercepted. How would a read lead to accidental BAR unmap? Even for writes I don't see how a VF driver could accidentally unmap BARs, as the memory decode bit there is hardwired to 0.

Quoted 2 lines

Discovery of VFs is
done by Dom0, which must register them with Xen.

If we intercept control register writes, why would we still require Dom0 to report the VFs that appear?

Aren't you effectively busy-waiting for these 100ms, by simply

from vpci_process_pending() until the time has passed? This imo is a

want to set a timer and put the vCPU to sleep, to wake it up again

timer has expired. That'll then eliminate the need for the

Question is whether we need to actually go this far (right away). I

don't mean to hand PFs to DomU-s. As long as we keep them in the hardware domain, can't we trust it to set things up correctly, just like we

a number of other aspects?

Jan

Quoted 14 lines

xen/drivers/vpci/sriov.c new file +14 -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)
+    {
+        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",

xen/drivers/vpci/sriov.c line 73

Quoted 6 lines

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

+                    &vf_pdev->sbdf, rc);
+            return rc;
+        }
+
+        vf_pdev->vpci->header.guest_cmd &= ~PCI_COMMAND_MEMORY;
+        vf_pdev->vpci->header.guest_cmd |= (cmd & PCI_COMMAND_MEMORY);

As mentioned elsewhere as well, this bit is supposed to be 0 for VFs.

xen/drivers/vpci/sriov.c line 216

Quoted 6 lines

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

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

Doesn't VF enable also need to be set for the BARs to be mapped?

Quoted 17 lines · Jan Beulich

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. Also force VFs to always use emulated reads for command register, this is needed to prevent some drivers accidentally unmapping BARs.

This apparently refers to the change to vpci_init_header(). Writes are already intercepted. How would a read lead to accidental BAR unmap? Even for writes I don't see how a VF driver could accidentally unmap BARs, as the memory decode bit there is hardwired to 0.

Quoted 6 lines

Quoted 2 lines

Discovery of VFs is
done by Dom0, which must register them with Xen.

If we intercept control register writes, why would we still require Dom0 to report the VFs that appear?

Sorry, I don't understand this question. You specifically requested this to be done this way in V2. Quoting your reply from V2 below.

Quoted 1 line

Aren't you effectively busy-waiting for these 100ms, by simply

returning "true"

Quoted 1 line

from vpci_process_pending() until the time has passed? This imo is a

no-go. You

Quoted 1 line

want to set a timer and put the vCPU to sleep, to wake it up again

when the

Quoted 1 line

timer has expired. That'll then eliminate the need for the

not-so-nice patch 4.

Quoted 1 line

Question is whether we need to actually go this far (right away). I

expect you

Quoted 2 lines

don't mean to hand PFs to DomU-s. As long as we keep them in the hardware domain, can't we trust it to set things up correctly, just like we

trust it in

Quoted 1 line

a number of other aspects?

Quoted 14 lines

Quoted 14 lines

xen/drivers/vpci/sriov.c new file +14 -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)
+    {
+        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",

Quoted 8 lines

xen/drivers/vpci/sriov.c line 73

Quoted 6 lines

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

+                    &vf_pdev->sbdf, rc);
+            return rc;
+        }
+
+        vf_pdev->vpci->header.guest_cmd &= ~PCI_COMMAND_MEMORY;
+        vf_pdev->vpci->header.guest_cmd |= (cmd & PCI_COMMAND_MEMORY);

As mentioned elsewhere as well, this bit is supposed to be 0 for VFs.

There are some devices that expose VFs with the same VID/DID as in the PF, causing Linux to use normal driver for them and threat them like normal devices. At some point, those normal drivers try to do a read-modify-update of the command register and end up writing 0 to PCI_COMMAND_MEMORY, causing cmd_write to unmap the BARS of that device. I am not sure, maybe it would be better to just ignore cmd writes for VFs?

Quoted 7 lines

Quoted 6 lines

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

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

Quoted 1 line

Doesn't VF enable also need to be set for the BARs to be mapped?

I don't think so. Enabling memory space logically maps very well to mapping memory to the guest. I don’t see any benefit of also requiring VFE bit here.

Quoted 34 lines · Mykyta Poturai

Quoted 17 lines · Jan Beulich

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. Also force VFs to always use emulated reads for command register, this is needed to prevent some drivers accidentally unmapping BARs.

This apparently refers to the change to vpci_init_header(). Writes are already intercepted. How would a read lead to accidental BAR unmap? Even for writes I don't see how a VF driver could accidentally unmap BARs, as the memory decode bit there is hardwired to 0.

Quoted 6 lines

Quoted 2 lines

Discovery of VFs is
done by Dom0, which must register them with Xen.

If we intercept control register writes, why would we still require Dom0 to report the VFs that appear?

Sorry, I don't understand this question. You specifically requested this to be done this way in V2. Quoting your reply from V2 below.

Quoted 1 line

Aren't you effectively busy-waiting for these 100ms, by simply

returning "true"

Quoted 1 line

from vpci_process_pending() until the time has passed? This imo is a

no-go. You

Quoted 1 line

want to set a timer and put the vCPU to sleep, to wake it up again

when the

Quoted 9 lines

Quoted 1 line

timer has expired. That'll then eliminate the need for the

not-so-nice patch 4.

Quoted 1 line

Question is whether we need to actually go this far (right away). I

expect you

Quoted 2 lines

don't mean to hand PFs to DomU-s. As long as we keep them in the hardware domain, can't we trust it to set things up correctly, just like we

trust it in

Quoted 1 line

a number of other aspects?

How's any of this related to the question I raised here, or your reply thereto? If we intercept PCI_SRIOV_CTRL, we know when VFs are created. Why still demand Dom0 to report them then?

Quoted 23 lines

Quoted 14 lines

Quoted 14 lines

xen/drivers/vpci/sriov.c new file +14 -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)
+    {
+        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",

Quoted 8 lines

xen/drivers/vpci/sriov.c line 73

Quoted 6 lines

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

+                    &vf_pdev->sbdf, rc);
+            return rc;
+        }
+
+        vf_pdev->vpci->header.guest_cmd &= ~PCI_COMMAND_MEMORY;
+        vf_pdev->vpci->header.guest_cmd |= (cmd & PCI_COMMAND_MEMORY);

As mentioned elsewhere as well, this bit is supposed to be 0 for VFs.

Quoted 6 lines

There are some devices that expose VFs with the same VID/DID as in the PF, causing Linux to use normal driver for them and threat them like normal devices. At some point, those normal drivers try to do a read-modify-update of the command register and end up writing 0 to PCI_COMMAND_MEMORY, causing cmd_write to unmap the BARS of that device. I am not sure, maybe it would be better to just ignore cmd writes for VFs?

No. We should treat r/o bits as r/o (which for this bit implies it not controlling BAR mapping).

Quoted 9 lines

Quoted 8 lines

xen/drivers/vpci/sriov.c line 216

Quoted 6 lines

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

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

Doesn't VF enable also need to be set for the BARs to be mapped?

Quoted 3 lines

I don't think so. Enabling memory space logically maps very well to mapping memory to the guest. I don’t see any benefit of also requiring VFE bit here.

Iirc the spec is quite explicit in this regard.

Jan

Quoted 45 lines · Jan Beulich

Quoted 34 lines · Mykyta Poturai

Quoted 17 lines · Jan Beulich

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. Also force VFs to always use emulated reads for command register, this is needed to prevent some drivers accidentally unmapping BARs.

This apparently refers to the change to vpci_init_header(). Writes are already intercepted. How would a read lead to accidental BAR unmap? Even for writes I don't see how a VF driver could accidentally unmap BARs, as the memory decode bit there is hardwired to 0.

Quoted 6 lines

Quoted 2 lines

Discovery of VFs is
done by Dom0, which must register them with Xen.

If we intercept control register writes, why would we still require Dom0 to report the VFs that appear?

Sorry, I don't understand this question. You specifically requested this to be done this way in V2. Quoting your reply from V2 below.

Quoted 1 line

Aren't you effectively busy-waiting for these 100ms, by simply

returning "true"

Quoted 1 line

from vpci_process_pending() until the time has passed? This imo is a

no-go. You

Quoted 1 line

want to set a timer and put the vCPU to sleep, to wake it up again

when the

Quoted 9 lines

Quoted 1 line

timer has expired. That'll then eliminate the need for the

not-so-nice patch 4.

Quoted 1 line

Question is whether we need to actually go this far (right away). I

expect you

Quoted 2 lines

don't mean to hand PFs to DomU-s. As long as we keep them in the hardware domain, can't we trust it to set things up correctly, just like we

trust it in

Quoted 1 line

a number of other aspects?

Quoted 4 lines

How's any of this related to the question I raised here, or your reply thereto? If we intercept PCI_SRIOV_CTRL, we know when VFs are created. Why still demand Dom0 to report them then?

The spec states that VFs can take up to 100ms after the VF_ENABLE bit is set to become alive. We discussed in the V2 that it is not acceptable to do a required 100ms wait in Xen while blocking a domain. And not doing that blocking would require some mechanism to only allow a domain to run for precisely 99(or more?)ms. You yourself suggested that we can trust the hardware domain with registering VFs if we already trust it with other PCI-related stuff. Did you change your mind, or am I completely misunderstanding this question?

Quoted 51 lines · Mykyta Poturai

Quoted 45 lines · Jan Beulich

Quoted 34 lines · Mykyta Poturai

Quoted 17 lines · Jan Beulich

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. Also force VFs to always use emulated reads for command register, this is needed to prevent some drivers accidentally unmapping BARs.

This apparently refers to the change to vpci_init_header(). Writes are already intercepted. How would a read lead to accidental BAR unmap? Even for writes I don't see how a VF driver could accidentally unmap BARs, as the memory decode bit there is hardwired to 0.

Quoted 6 lines

Quoted 2 lines

Discovery of VFs is
done by Dom0, which must register them with Xen.

If we intercept control register writes, why would we still require Dom0 to report the VFs that appear?

Sorry, I don't understand this question. You specifically requested this to be done this way in V2. Quoting your reply from V2 below.

Quoted 1 line

Aren't you effectively busy-waiting for these 100ms, by simply

returning "true"

Quoted 1 line

from vpci_process_pending() until the time has passed? This imo is a

no-go. You

Quoted 1 line

want to set a timer and put the vCPU to sleep, to wake it up again

when the

Quoted 9 lines

Quoted 1 line

timer has expired. That'll then eliminate the need for the

not-so-nice patch 4.

Quoted 1 line

Question is whether we need to actually go this far (right away). I

expect you

Quoted 2 lines

don't mean to hand PFs to DomU-s. As long as we keep them in the hardware domain, can't we trust it to set things up correctly, just like we

trust it in

Quoted 1 line

a number of other aspects?

Quoted 4 lines

How's any of this related to the question I raised here, or your reply thereto? If we intercept PCI_SRIOV_CTRL, we know when VFs are created. Why still demand Dom0 to report them then?

Quoted 8 lines

The spec states that VFs can take up to 100ms after the VF_ENABLE bit is set to become alive. We discussed in the V2 that it is not acceptable to do a required 100ms wait in Xen while blocking a domain. And not doing that blocking would require some mechanism to only allow a domain to run for precisely 99(or more?)ms. You yourself suggested that we can trust the hardware domain with registering VFs if we already trust it with other PCI-related stuff. Did you change your mind, or am I completely misunderstanding this question?

No, I still think that we can trust hwdom enough. Nevertheless we should aim at being independent of it where possible. And I seem to recall that I had also outlined an approach how to avoid spin-waiting for 100ms in the hypervisor.

Quoted 61 lines · Jan Beulich

Quoted 59 lines · Mykyta Poturai

Quoted 45 lines · Jan Beulich

Quoted 34 lines · Mykyta Poturai

Quoted 17 lines · Jan Beulich

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. Also force VFs to always use emulated reads for command register, this is needed to prevent some drivers accidentally unmapping BARs.

This apparently refers to the change to vpci_init_header(). Writes are already intercepted. How would a read lead to accidental BAR unmap? Even for writes I don't see how a VF driver could accidentally unmap BARs, as the memory decode bit there is hardwired to 0.

Quoted 6 lines

Quoted 2 lines

Discovery of VFs is
done by Dom0, which must register them with Xen.

If we intercept control register writes, why would we still require Dom0 to report the VFs that appear?

Sorry, I don't understand this question. You specifically requested this to be done this way in V2. Quoting your reply from V2 below.

Quoted 1 line

Aren't you effectively busy-waiting for these 100ms, by simply

returning "true"

Quoted 1 line

from vpci_process_pending() until the time has passed? This imo is a

no-go. You

Quoted 1 line

want to set a timer and put the vCPU to sleep, to wake it up again

when the

Quoted 9 lines

Quoted 1 line

timer has expired. That'll then eliminate the need for the

not-so-nice patch 4.

Quoted 1 line

Question is whether we need to actually go this far (right away). I

expect you

Quoted 2 lines

don't mean to hand PFs to DomU-s. As long as we keep them in the hardware domain, can't we trust it to set things up correctly, just like we

trust it in

Quoted 1 line

a number of other aspects?

Quoted 4 lines

How's any of this related to the question I raised here, or your reply thereto? If we intercept PCI_SRIOV_CTRL, we know when VFs are created. Why still demand Dom0 to report them then?

The spec states that VFs can take up to 100ms after the VF_ENABLE bit is set to become alive. We discussed in the V2 that it is not acceptable to do a required 100ms wait in Xen while blocking a domain. And not doing that blocking would require some mechanism to only allow a domain to run for precisely 99(or more?)ms. You yourself suggested that we can trust the hardware domain with registering VFs if we already trust it with other PCI-related stuff. Did you change your mind, or am I completely misunderstanding this question?

Quoted 4 lines

No, I still think that we can trust hwdom enough. Nevertheless we should aim at being independent of it where possible. And I seem to recall that I had also outlined an approach how to avoid spin-waiting for 100ms in the hypervisor.

I want to clarify: you are telling that Xen should not wait for hwdom to report VFs and instead create them by itself. Is this correct?

Quoted 61 lines · Jan Beulich

Quoted 59 lines · Mykyta Poturai

Quoted 45 lines · Jan Beulich

Quoted 34 lines · Mykyta Poturai

Quoted 17 lines · Jan Beulich

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. Also force VFs to always use emulated reads for command register, this is needed to prevent some drivers accidentally unmapping BARs.

This apparently refers to the change to vpci_init_header(). Writes are already intercepted. How would a read lead to accidental BAR unmap? Even for writes I don't see how a VF driver could accidentally unmap BARs, as the memory decode bit there is hardwired to 0.

Quoted 6 lines

Quoted 2 lines

Discovery of VFs is
done by Dom0, which must register them with Xen.

If we intercept control register writes, why would we still require Dom0 to report the VFs that appear?

Sorry, I don't understand this question. You specifically requested this to be done this way in V2. Quoting your reply from V2 below.

Quoted 1 line

Aren't you effectively busy-waiting for these 100ms, by simply

returning "true"

Quoted 1 line

from vpci_process_pending() until the time has passed? This imo is a

no-go. You

Quoted 1 line

want to set a timer and put the vCPU to sleep, to wake it up again

when the

Quoted 9 lines

Quoted 1 line

timer has expired. That'll then eliminate the need for the

not-so-nice patch 4.

Quoted 1 line

Question is whether we need to actually go this far (right away). I

expect you

Quoted 2 lines

don't mean to hand PFs to DomU-s. As long as we keep them in the hardware domain, can't we trust it to set things up correctly, just like we

trust it in

Quoted 1 line

a number of other aspects?

Quoted 4 lines

How's any of this related to the question I raised here, or your reply thereto? If we intercept PCI_SRIOV_CTRL, we know when VFs are created. Why still demand Dom0 to report them then?

The spec states that VFs can take up to 100ms after the VF_ENABLE bit is set to become alive. We discussed in the V2 that it is not acceptable to do a required 100ms wait in Xen while blocking a domain. And not doing that blocking would require some mechanism to only allow a domain to run for precisely 99(or more?)ms. You yourself suggested that we can trust the hardware domain with registering VFs if we already trust it with other PCI-related stuff. Did you change your mind, or am I completely misunderstanding this question?

Quoted 4 lines

No, I still think that we can trust hwdom enough. Nevertheless we should aim at being independent of it where possible. And I seem to recall that I had also outlined an approach how to avoid spin-waiting for 100ms in the hypervisor.

I want to clarify: you are telling that Xen should not wait for hwdom to report VFs and instead create them by itself. Is this correct?

Quoted 67 lines · Volodymyr Babchuk

Quoted 65 lines · Jan Beulich

Quoted 51 lines · Mykyta Poturai

Quoted 45 lines · Jan Beulich

Quoted 34 lines · Mykyta Poturai

Quoted 17 lines · Jan Beulich

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. Also force VFs to always use emulated reads for command register, this is needed to prevent some drivers accidentally unmapping BARs.

This apparently refers to the change to vpci_init_header(). Writes are already intercepted. How would a read lead to accidental BAR unmap? Even for writes I don't see how a VF driver could accidentally unmap BARs, as the memory decode bit there is hardwired to 0.

Quoted 6 lines

Quoted 2 lines

Discovery of VFs is
done by Dom0, which must register them with Xen.

If we intercept control register writes, why would we still require Dom0 to report the VFs that appear?

Sorry, I don't understand this question. You specifically requested this to be done this way in V2. Quoting your reply from V2 below.

Quoted 1 line

Aren't you effectively busy-waiting for these 100ms, by simply

returning "true"

Quoted 1 line

from vpci_process_pending() until the time has passed? This imo is a

no-go. You

Quoted 1 line

want to set a timer and put the vCPU to sleep, to wake it up again

when the

Quoted 9 lines

Quoted 1 line

timer has expired. That'll then eliminate the need for the

not-so-nice patch 4.

Quoted 1 line

Question is whether we need to actually go this far (right away). I

expect you

Quoted 2 lines

don't mean to hand PFs to DomU-s. As long as we keep them in the hardware domain, can't we trust it to set things up correctly, just like we

trust it in

Quoted 1 line

a number of other aspects?

Quoted 4 lines

How's any of this related to the question I raised here, or your reply thereto? If we intercept PCI_SRIOV_CTRL, we know when VFs are created. Why still demand Dom0 to report them then?

Quoted 8 lines

The spec states that VFs can take up to 100ms after the VF_ENABLE bit is set to become alive. We discussed in the V2 that it is not acceptable to do a required 100ms wait in Xen while blocking a domain. And not doing that blocking would require some mechanism to only allow a domain to run for precisely 99(or more?)ms. You yourself suggested that we can trust the hardware domain with registering VFs if we already trust it with other PCI-related stuff. Did you change your mind, or am I completely misunderstanding this question?

No, I still think that we can trust hwdom enough. Nevertheless we should aim at being independent of it where possible. And I seem to recall that I had also outlined an approach how to avoid spin-waiting for 100ms in the hypervisor.

Quoted 2 lines

I want to clarify: you are telling that Xen should not wait for hwdom to report VFs and instead create them by itself. Is this correct?

If that's technically possible, yes.

Hi Jan,

Quoted 68 lines · Jan Beulich

Quoted 67 lines · Volodymyr Babchuk

Quoted 65 lines · Jan Beulich

Quoted 51 lines · Mykyta Poturai

Quoted 45 lines · Jan Beulich

Quoted 34 lines · Mykyta Poturai

Quoted 17 lines · Jan Beulich

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. Also force VFs to always use emulated reads for command register, this is needed to prevent some drivers accidentally unmapping BARs.

This apparently refers to the change to vpci_init_header(). Writes are already intercepted. How would a read lead to accidental BAR unmap? Even for writes I don't see how a VF driver could accidentally unmap BARs, as the memory decode bit there is hardwired to 0.

Quoted 6 lines

Quoted 2 lines

Discovery of VFs is
done by Dom0, which must register them with Xen.

If we intercept control register writes, why would we still require Dom0 to report the VFs that appear?

Sorry, I don't understand this question. You specifically requested this to be done this way in V2. Quoting your reply from V2 below.

Quoted 1 line

Aren't you effectively busy-waiting for these 100ms, by simply

returning "true"

Quoted 1 line

from vpci_process_pending() until the time has passed? This imo is a

no-go. You

Quoted 1 line

want to set a timer and put the vCPU to sleep, to wake it up again

when the

Quoted 9 lines

Quoted 1 line

timer has expired. That'll then eliminate the need for the

not-so-nice patch 4.

Quoted 1 line

Question is whether we need to actually go this far (right away). I

expect you

Quoted 2 lines

don't mean to hand PFs to DomU-s. As long as we keep them in the hardware domain, can't we trust it to set things up correctly, just like we

trust it in

Quoted 1 line

a number of other aspects?

Quoted 4 lines

How's any of this related to the question I raised here, or your reply thereto? If we intercept PCI_SRIOV_CTRL, we know when VFs are created. Why still demand Dom0 to report them then?

Quoted 8 lines

The spec states that VFs can take up to 100ms after the VF_ENABLE bit is set to become alive. We discussed in the V2 that it is not acceptable to do a required 100ms wait in Xen while blocking a domain. And not doing that blocking would require some mechanism to only allow a domain to run for precisely 99(or more?)ms. You yourself suggested that we can trust the hardware domain with registering VFs if we already trust it with other PCI-related stuff. Did you change your mind, or am I completely misunderstanding this question?

No, I still think that we can trust hwdom enough. Nevertheless we should aim at being independent of it where possible. And I seem to recall that I had also outlined an approach how to avoid spin-waiting for 100ms in the hypervisor.

Quoted 4 lines

Quoted 2 lines

I want to clarify: you are telling that Xen should not wait for hwdom to report VFs and instead create them by itself. Is this correct?

If that's technically possible, yes.

Okay, so let's clear this. If I remember correct, you discussed this with Mykyta in the previous version and suggested to put the vCPU to sleep for 100ms.

I don't think that this is a good idea, because guest kernel will not be happy about that.

Quoted 72 lines · Volodymyr Babchuk

Hi Jan,

Quoted 68 lines · Jan Beulich

Quoted 67 lines · Volodymyr Babchuk

Quoted 65 lines · Jan Beulich

Quoted 51 lines · Mykyta Poturai

Quoted 45 lines · Jan Beulich

Quoted 34 lines · Mykyta Poturai

Quoted 17 lines · Jan Beulich

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. Also force VFs to always use emulated reads for command register, this is needed to prevent some drivers accidentally unmapping BARs.

This apparently refers to the change to vpci_init_header(). Writes are already intercepted. How would a read lead to accidental BAR unmap? Even for writes I don't see how a VF driver could accidentally unmap BARs, as the memory decode bit there is hardwired to 0.

Quoted 6 lines

Quoted 2 lines

Discovery of VFs is
done by Dom0, which must register them with Xen.

If we intercept control register writes, why would we still require Dom0 to report the VFs that appear?

Sorry, I don't understand this question. You specifically requested this to be done this way in V2. Quoting your reply from V2 below.

Quoted 1 line

Aren't you effectively busy-waiting for these 100ms, by simply

returning "true"

Quoted 1 line

from vpci_process_pending() until the time has passed? This imo is a

no-go. You

Quoted 1 line

want to set a timer and put the vCPU to sleep, to wake it up again

when the

Quoted 9 lines

Quoted 1 line

timer has expired. That'll then eliminate the need for the

not-so-nice patch 4.

Quoted 1 line

Question is whether we need to actually go this far (right away). I

expect you

Quoted 2 lines

don't mean to hand PFs to DomU-s. As long as we keep them in the hardware domain, can't we trust it to set things up correctly, just like we

trust it in

Quoted 1 line

a number of other aspects?

Quoted 4 lines

How's any of this related to the question I raised here, or your reply thereto? If we intercept PCI_SRIOV_CTRL, we know when VFs are created. Why still demand Dom0 to report them then?

Quoted 8 lines

The spec states that VFs can take up to 100ms after the VF_ENABLE bit is set to become alive. We discussed in the V2 that it is not acceptable to do a required 100ms wait in Xen while blocking a domain. And not doing that blocking would require some mechanism to only allow a domain to run for precisely 99(or more?)ms. You yourself suggested that we can trust the hardware domain with registering VFs if we already trust it with other PCI-related stuff. Did you change your mind, or am I completely misunderstanding this question?

No, I still think that we can trust hwdom enough. Nevertheless we should aim at being independent of it where possible. And I seem to recall that I had also outlined an approach how to avoid spin-waiting for 100ms in the hypervisor.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

I want to clarify: you are telling that Xen should not wait for hwdom to report VFs and instead create them by itself. Is this correct?

If that's technically possible, yes.

Okay, so let's clear this. If I remember correct, you discussed this with Mykyta in the previous version and suggested to put the vCPU to sleep for 100ms.

I don't think I did (except perhaps from a very abstract perspective), precisely because of ...

Quoted 2 lines

I don't think that this is a good idea, because guest kernel will not be happy about that.

... this. Instead iirc I suggested to refuse (short-circuit) handling VF register accesses for the next 100ms.

Jan

Quoted 85 lines · Jan Beulich

Quoted 72 lines · Volodymyr Babchuk

Hi Jan,

Quoted 68 lines · Jan Beulich

Quoted 67 lines · Volodymyr Babchuk

Quoted 65 lines · Jan Beulich

Quoted 51 lines · Mykyta Poturai

Quoted 45 lines · Jan Beulich

Quoted 34 lines · Mykyta Poturai

Quoted 17 lines · Jan Beulich

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. Also force VFs to always use emulated reads for command register, this is needed to prevent some drivers accidentally unmapping BARs.

This apparently refers to the change to vpci_init_header(). Writes are already intercepted. How would a read lead to accidental BAR unmap? Even for writes I don't see how a VF driver could accidentally unmap BARs, as the memory decode bit there is hardwired to 0.

Quoted 6 lines

Quoted 2 lines

Discovery of VFs is
done by Dom0, which must register them with Xen.

If we intercept control register writes, why would we still require Dom0 to report the VFs that appear?

Sorry, I don't understand this question. You specifically requested this to be done this way in V2. Quoting your reply from V2 below.

Quoted 1 line

Aren't you effectively busy-waiting for these 100ms, by simply

returning "true"

Quoted 1 line

from vpci_process_pending() until the time has passed? This imo is a

no-go. You

Quoted 1 line

want to set a timer and put the vCPU to sleep, to wake it up again

when the

Quoted 9 lines

Quoted 1 line

timer has expired. That'll then eliminate the need for the

not-so-nice patch 4.

Quoted 1 line

Question is whether we need to actually go this far (right away). I

expect you

Quoted 2 lines

don't mean to hand PFs to DomU-s. As long as we keep them in the hardware domain, can't we trust it to set things up correctly, just like we

trust it in

Quoted 1 line

a number of other aspects?

Quoted 4 lines

How's any of this related to the question I raised here, or your reply thereto? If we intercept PCI_SRIOV_CTRL, we know when VFs are created. Why still demand Dom0 to report them then?

Quoted 8 lines

The spec states that VFs can take up to 100ms after the VF_ENABLE bit is set to become alive. We discussed in the V2 that it is not acceptable to do a required 100ms wait in Xen while blocking a domain. And not doing that blocking would require some mechanism to only allow a domain to run for precisely 99(or more?)ms. You yourself suggested that we can trust the hardware domain with registering VFs if we already trust it with other PCI-related stuff. Did you change your mind, or am I completely misunderstanding this question?

No, I still think that we can trust hwdom enough. Nevertheless we should aim at being independent of it where possible. And I seem to recall that I had also outlined an approach how to avoid spin-waiting for 100ms in the hypervisor.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

I want to clarify: you are telling that Xen should not wait for hwdom to report VFs and instead create them by itself. Is this correct?

If that's technically possible, yes.

Okay, so let's clear this. If I remember correct, you discussed this with Mykyta in the previous version and suggested to put the vCPU to sleep for 100ms.

I don't think I did (except perhaps from a very abstract perspective), precisely because of ...

Quoted 7 lines

Quoted 2 lines

I don't think that this is a good idea, because guest kernel will not be happy about that.

... this. Instead iirc I suggested to refuse (short-circuit) handling VF register accesses for the next 100ms.

Jan

Do you have any suggestions on how to ensure that we accurately catch the window where 100ms have already passed, but guests haven’t tried to read anything yet, to flip this back? As I mentioned in the previous version, Linux, for example, doesn’t attempt to re-read anything if the first read failed after 100ms. So it appears to me that this approach would be prone to racing with the guest for getting to the VF first. One approach I can think of is to somehow swap the register handlers back in-flight during the first read by the guest if 100ms have already passed. However, this would still depend on Dom0 for registering VFs, but in a more convoluted way. We also can’t add the VFs before 100ms have passed and add timing checks to all register handlers, because pci_add_device and everything below it expects the device to be functional at the moment of addition.

Maybe you see some other way to avoid these problems that I am missing?

Quoted 112 lines · Mykyta Poturai

Quoted 85 lines · Jan Beulich

Quoted 72 lines · Volodymyr Babchuk

Hi Jan,

Quoted 68 lines · Jan Beulich

Quoted 67 lines · Volodymyr Babchuk

Quoted 65 lines · Jan Beulich

Quoted 51 lines · Mykyta Poturai

Quoted 45 lines · Jan Beulich

Quoted 34 lines · Mykyta Poturai

Quoted 17 lines · Jan Beulich

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. Also force VFs to always use emulated reads for command register, this is needed to prevent some drivers accidentally unmapping BARs.

This apparently refers to the change to vpci_init_header(). Writes are already intercepted. How would a read lead to accidental BAR unmap? Even for writes I don't see how a VF driver could accidentally unmap BARs, as the memory decode bit there is hardwired to 0.

Quoted 6 lines

Quoted 2 lines

Discovery of VFs is
done by Dom0, which must register them with Xen.

If we intercept control register writes, why would we still require Dom0 to report the VFs that appear?

Sorry, I don't understand this question. You specifically requested this to be done this way in V2. Quoting your reply from V2 below.

Quoted 1 line

Aren't you effectively busy-waiting for these 100ms, by simply

returning "true"

Quoted 1 line

from vpci_process_pending() until the time has passed? This imo is a

no-go. You

Quoted 1 line

want to set a timer and put the vCPU to sleep, to wake it up again

when the

Quoted 9 lines

Quoted 1 line

timer has expired. That'll then eliminate the need for the

not-so-nice patch 4.

Quoted 1 line

Question is whether we need to actually go this far (right away). I

expect you

Quoted 2 lines

don't mean to hand PFs to DomU-s. As long as we keep them in the hardware domain, can't we trust it to set things up correctly, just like we

trust it in

Quoted 1 line

a number of other aspects?

Quoted 4 lines

How's any of this related to the question I raised here, or your reply thereto? If we intercept PCI_SRIOV_CTRL, we know when VFs are created. Why still demand Dom0 to report them then?

Quoted 8 lines

The spec states that VFs can take up to 100ms after the VF_ENABLE bit is set to become alive. We discussed in the V2 that it is not acceptable to do a required 100ms wait in Xen while blocking a domain. And not doing that blocking would require some mechanism to only allow a domain to run for precisely 99(or more?)ms. You yourself suggested that we can trust the hardware domain with registering VFs if we already trust it with other PCI-related stuff. Did you change your mind, or am I completely misunderstanding this question?

No, I still think that we can trust hwdom enough. Nevertheless we should aim at being independent of it where possible. And I seem to recall that I had also outlined an approach how to avoid spin-waiting for 100ms in the hypervisor.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

I want to clarify: you are telling that Xen should not wait for hwdom to report VFs and instead create them by itself. Is this correct?

If that's technically possible, yes.

Okay, so let's clear this. If I remember correct, you discussed this with Mykyta in the previous version and suggested to put the vCPU to sleep for 100ms.

I don't think I did (except perhaps from a very abstract perspective), precisely because of ...

Quoted 7 lines

Quoted 2 lines

I don't think that this is a good idea, because guest kernel will not be happy about that.

... this. Instead iirc I suggested to refuse (short-circuit) handling VF register accesses for the next 100ms.

Jan

Do you have any suggestions on how to ensure that we accurately catch the window where 100ms have already passed, but guests haven’t tried to read anything yet, to flip this back? As I mentioned in the previous version, Linux, for example, doesn’t attempt to re-read anything if the first read failed after 100ms. So it appears to me that this approach would be prone to racing with the guest for getting to the VF first. One approach I can think of is to somehow swap the register handlers back in-flight during the first read by the guest if 100ms have already passed. However, this would still depend on Dom0 for registering VFs, but in a more convoluted way. We also can’t add the VFs before 100ms have passed and add timing checks to all register handlers, because pci_add_device and everything below it expects the device to be functional at the moment of addition.

Quoted 1 line

Maybe you see some other way to avoid these problems that I am missing?

We could maybe do some middle ground here, kind of similar to what Linux does. The overall idea would be to put on hold any accesses to the device(s) PCI config space for 100ms, that would include the PF and any VFs. At the point when VF enable is set Xen already knows the position of the VFs in the PCI config space.

Any PCI config space access attempts to the PF or VFs during that 100ms window would cause the guest vCPU to be put on hold, and the access would only be retried once the 100ms window has passed and Xen has registered the VFs with vPCI. This approach needs extra logic to put vPCI accesses on hold, similar to what Xen does when mapping a BAR into the p2m, and a timer to defer the adding of the Vfs and the unlocking of the affected PCI config space region.

That would be a middle ground IMO, as the guest vCPUs could be running freely, unless accesses to the affected PCI config space was attempted before the 100ms window, at which point they would be blocked waiting for the timeout to expire. A well-behaved domain shouldn't try to access the PCI config space either ahead the 100ms window expiring.

Hi Roger,

Quoted 129 lines · Roger Pau Monné

Quoted 112 lines · Mykyta Poturai

Quoted 85 lines · Jan Beulich

Quoted 72 lines · Volodymyr Babchuk

Hi Jan,

Quoted 68 lines · Jan Beulich

Quoted 67 lines · Volodymyr Babchuk

Quoted 65 lines · Jan Beulich

Quoted 51 lines · Mykyta Poturai

Quoted 45 lines · Jan Beulich

Quoted 34 lines · Mykyta Poturai

Quoted 17 lines · Jan Beulich

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. Also force VFs to
always use emulated reads for command register, this is needed to
prevent some drivers accidentally unmapping BARs.

This apparently refers to the change to vpci_init_header(). Writes are already intercepted. How would a read lead to accidental BAR unmap? Even for writes I don't see how a VF driver could accidentally unmap BARs, as the memory decode bit there is hardwired to 0.

Quoted 6 lines

Quoted 2 lines

Discovery of VFs is
done by Dom0, which must register them with Xen.

If we intercept control register writes, why would we still require Dom0 to report the VFs that appear?

Sorry, I don't understand this question. You specifically requested this to be done this way in V2. Quoting your reply from V2 below.

Quoted 1 line

Aren't you effectively busy-waiting for these 100ms, by simply

returning "true"

Quoted 1 line

from vpci_process_pending() until the time has passed? This imo is a

no-go. You

Quoted 1 line

want to set a timer and put the vCPU to sleep, to wake it up again

when the

Quoted 9 lines

Quoted 1 line

timer has expired. That'll then eliminate the need for the

not-so-nice patch 4.

Quoted 1 line

Question is whether we need to actually go this far (right away). I

expect you

Quoted 2 lines

don't mean to hand PFs to DomU-s. As long as we keep them in the hardware domain, can't we trust it to set things up correctly, just like we

trust it in

Quoted 1 line

a number of other aspects?

Quoted 4 lines

How's any of this related to the question I raised here, or your reply thereto? If we intercept PCI_SRIOV_CTRL, we know when VFs are created. Why still demand Dom0 to report them then?

Quoted 8 lines

The spec states that VFs can take up to 100ms after the VF_ENABLE bit is set to become alive. We discussed in the V2 that it is not acceptable to do a required 100ms wait in Xen while blocking a domain. And not doing that blocking would require some mechanism to only allow a domain to run for precisely 99(or more?)ms. You yourself suggested that we can trust the hardware domain with registering VFs if we already trust it with other PCI-related stuff. Did you change your mind, or am I completely misunderstanding this question?

No, I still think that we can trust hwdom enough. Nevertheless we should aim at being independent of it where possible. And I seem to recall that I had also outlined an approach how to avoid spin-waiting for 100ms in the hypervisor.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

I want to clarify: you are telling that Xen should not wait for hwdom to report VFs and instead create them by itself. Is this correct?

If that's technically possible, yes.

Okay, so let's clear this. If I remember correct, you discussed this with Mykyta in the previous version and suggested to put the vCPU to sleep for 100ms.

I don't think I did (except perhaps from a very abstract perspective), precisely because of ...

Quoted 7 lines

Quoted 2 lines

I don't think that this is a good idea, because guest kernel will not be happy about that.

... this. Instead iirc I suggested to refuse (short-circuit) handling VF register accesses for the next 100ms.

Jan

Do you have any suggestions on how to ensure that we accurately catch the window where 100ms have already passed, but guests haven’t tried to read anything yet, to flip this back? As I mentioned in the previous version, Linux, for example, doesn’t attempt to re-read anything if the first read failed after 100ms. So it appears to me that this approach would be prone to racing with the guest for getting to the VF first. One approach I can think of is to somehow swap the register handlers back in-flight during the first read by the guest if 100ms have already passed. However, this would still depend on Dom0 for registering VFs, but in a more convoluted way. We also can’t add the VFs before 100ms have passed and add timing checks to all register handlers, because pci_add_device and everything below it expects the device to be functional at the moment of addition.

Quoted 1 line

Maybe you see some other way to avoid these problems that I am missing?

We could maybe do some middle ground here, kind of similar to what Linux does. The overall idea would be to put on hold any accesses to the device(s) PCI config space for 100ms, that would include the PF and any VFs. At the point when VF enable is set Xen already knows the position of the VFs in the PCI config space.

Any PCI config space access attempts to the PF or VFs during that 100ms window would cause the guest vCPU to be put on hold, and the access would only be retried once the 100ms window has passed and Xen has registered the VFs with vPCI. This approach needs extra logic to put vPCI accesses on hold, similar to what Xen does when mapping a BAR into the p2m, and a timer to defer the adding of the Vfs and the unlocking of the affected PCI config space region.

Quoted 5 lines

That would be a middle ground IMO, as the guest vCPUs could be running freely, unless accesses to the affected PCI config space was attempted before the 100ms window, at which point they would be blocked waiting for the timeout to expire. A well-behaved domain shouldn't try to access the PCI config space either ahead the 100ms window expiring.

This approach seems reasonable for me, but this would require big changes in vPCI logic, as now pci_add_device() needs ability to defer all config space accesses till VFs are ready and in meantime we'll have to deal with half-initialized pdev. PCI/vPCI logic is already convoluted enough and adding more intermediate states, which need to be dealt with in different places will make things even worse. Unless I miss some easy fix, of course...

What I am trying to say is that your suggestion is technically doable, but requires lots of work, and we don't need resources for this right now. So, what's your opinion on existing approach? Is relying on a domain to introduce VFs such a bad idea?

Quoted 15 lines · Mykyta Poturai

Quoted 13 lines · Jan Beulich

Quoted 3 lines · Volodymyr Babchuk

Okay, so let's clear this. If I remember correct, you discussed this with Mykyta in the previous version and suggested to put the vCPU to sleep for 100ms.

I don't think I did (except perhaps from a very abstract perspective), precisely because of ...

Quoted 2 lines

I don't think that this is a good idea, because guest kernel will not be happy about that.

... this. Instead iirc I suggested to refuse (short-circuit) handling VF register accesses for the next 100ms.

Quoted 6 lines

Do you have any suggestions on how to ensure that we accurately catch the window where 100ms have already passed, but guests haven’t tried to read anything yet, to flip this back? As I mentioned in the previous version, Linux, for example, doesn’t attempt to re-read anything if the first read failed after 100ms. So it appears to me that this approach would be prone to racing with the guest for getting to the VF first.

Quoted 8 lines

One
approach I can think of is to somehow swap the register handlers back in-flight during the first read by the guest if 100ms have already passed. However, this would still depend on Dom0 for registering VFs, but in a more convoluted way. We also can’t add the VFs before 100ms have passed and add timing checks to all register handlers, because pci_add_device and everything below it expects the device to be functional at the moment of addition.

We could maybe do some middle ground here, kind of similar to what Linux does. The overall idea would be to put on hold any accesses to the device(s) PCI config space for 100ms, that would include the PF and any VFs.

At the point when VF enable is set Xen already knows the
position of the VFs in the PCI config space.

Any PCI config space access attempts to the PF or VFs during that 100ms window would cause the guest vCPU to be put on hold, and the access would only be retried once the 100ms window has passed and Xen has registered the VFs with vPCI. This approach needs extra logic to put vPCI accesses on hold, similar to what Xen does when mapping a BAR into the p2m, and a timer to defer the adding of the Vfs and the unlocking of the affected PCI config space region.

Quoted 16 lines · Roger Pau Monné

Quoted 15 lines · Mykyta Poturai

Quoted 13 lines · Jan Beulich

Quoted 3 lines · Volodymyr Babchuk

Okay, so let's clear this. If I remember correct, you discussed this with Mykyta in the previous version and suggested to put the vCPU to sleep for 100ms.

I don't think I did (except perhaps from a very abstract perspective), precisely because of ...

Quoted 2 lines

I don't think that this is a good idea, because guest kernel will not be happy about that.

... this. Instead iirc I suggested to refuse (short-circuit) handling VF register accesses for the next 100ms.

Quoted 6 lines

Quoted 6 lines

Do you have any suggestions on how to ensure that we accurately catch the window where 100ms have already passed, but guests haven’t tried to read anything yet, to flip this back? As I mentioned in the previous version, Linux, for example, doesn’t attempt to re-read anything if the first read failed after 100ms. So it appears to me that this approach would be prone to racing with the guest for getting to the VF first.

When we do the write to the control register in Xen, our timer will start ticking before the guest's. Hence our 100ms will be over (slightly) earlier, and a well-behaved guest (having waited for the full 100ms according to its own tracking) will be handled fine.

Quoted 8 lines

Quoted 8 lines

One
approach I can think of is to somehow swap the register handlers back in-flight during the first read by the guest if 100ms have already passed. However, this would still depend on Dom0 for registering VFs, but in a more convoluted way. We also can’t add the VFs before 100ms have passed and add timing checks to all register handlers, because pci_add_device and everything below it expects the device to be functional at the moment of addition.

I fear I'm not following this.

Quoted 4 lines

We could maybe do some middle ground here, kind of similar to what Linux does. The overall idea would be to put on hold any accesses to the device(s) PCI config space for 100ms, that would include the PF and any VFs.

For the PF, at most parts of the SR-IOV capability should be thus constrained, I think.

Quoted 10 lines

At the point when VF enable is set Xen already knows the
position of the VFs in the PCI config space.

Any PCI config space access attempts to the PF or VFs during that 100ms window would cause the guest vCPU to be put on hold, and the access would only be retried once the 100ms window has passed and Xen has registered the VFs with vPCI. This approach needs extra logic to put vPCI accesses on hold, similar to what Xen does when mapping a BAR into the p2m, and a timer to defer the adding of the Vfs and the unlocking of the affected PCI config space region.

I was meaning to have this done in even simpler a way: Simply record when the VFs were configured, and within the next 100ms terminate all accesses (read all ones, discard writes).

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. Also force VFs to always use emulated reads for command register, this is needed to prevent some drivers accidentally unmapping BARs. Discovery of VFs is done by Dom0, which must register them with Xen.

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

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

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

---
v2->v3:
* rework adding VFs with multiple mapping operations support
* use private.h
* style fixes
* fixup cleanup_sriov
* emulate command register read for VFs

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  |   1 +
 xen/drivers/vpci/header.c  |   6 +-
 xen/drivers/vpci/private.h |   1 +
 xen/drivers/vpci/sriov.c   | 314 +++++++++++++++++++++++++++++++++++++
 xen/include/xen/vpci.h     |   7 +-
 5 files changed, 327 insertions(+), 2 deletions(-)
 create mode 100644 xen/drivers/vpci/sriov.c

xen/drivers/vpci/Makefile +1 -0

index 9793a4f9b0..8b0e3c03a7 100644
@@ -1,3 +1,4 @@
 obj-y += cap.o
 obj-y += vpci.o header.o rebar.o
+obj-y += sriov.o
 obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o

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

index 451cdd3a6f..a36285672e 100644

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

                                 cmd_write, PCI_COMMAND, 2, header, 0, 0,
                                 is_hwdom ? 0
                                          : PCI_COMMAND_RSVDP_MASK |

@@ -900,6 +901,9 @@ int vpci_init_header(struct pci_dev *pdev)
 
     header->guest_cmd = cmd;
 
+    if ( pdev->info.is_virtfn )
+        return vpci_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);

xen/drivers/vpci/private.h +1 -0

index f012fd160d..8e0043ddbe 100644
@@ -45,6 +45,7 @@ typedef struct {
     REGISTER_VPCI_CAPABILITY(PCI_EXT_CAP_ID_##name, name, finit, fclean, true)
 
 int __must_check vpci_init_header(struct pci_dev *pdev);
+int __must_check vpci_vf_init_header(struct pci_dev *pdev);
 
 int vpci_init_capabilities(struct pci_dev *pdev, bool ext_only);
 void vpci_cleanup_capabilities(struct pci_dev *pdev, bool ext_only);

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

new file mode 100644
index 0000000000..ec6a7b84d5

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

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

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

+        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;
+}
+
+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)
+    {
+        rc = vpci_modify_bars(vf_pdev, cmd, false);

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

+    }
+
+    return 0;
+}
+
+static int size_vf_bars(const struct pci_dev *pf_pdev, unsigned int sriov_pos,
+                        uint64_t *vf_rlen)
+{

xen/drivers/vpci/sriov.c new file +35 -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:

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

+
+    return rc;
+}
+
+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 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;
+    int rc;
+
+    ASSERT(!pdev->info.is_virtfn);
+
+    if ( new_enabled == enabled && new_mem_enabled == mem_enabled )
+    {
+        pci_conf_write16(pdev->sbdf, reg, val);
+        return;
+    }
+
+    if ( mem_enabled && !new_mem_enabled )
+        map_vfs(pdev, 0);
+
+    if ( !enabled && new_enabled )
+    {
+        size_vf_bars(pdev, sriov_pos, (uint64_t *)data);
+
+        /*
+         * 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);
+    }
+
+    if ( !mem_enabled && new_mem_enabled )
+    {
+        rc = map_vfs(pdev, PCI_COMMAND_MEMORY);
+
+        if ( rc )
+            map_vfs(pdev, 0);
+    }
+
+    pci_conf_write16(pdev->sbdf, reg, val);
+}
+
+int vpci_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);
+        if ( rc )
+            return rc;
+
+        vf_pdev->vpci->header.guest_cmd |= PCI_COMMAND_MEMORY;
+    }
+
+    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);
+
+    if ( !pos )
+        return 0;
+
+    if ( xsm_resource_setup_pci(XSM_PRIV, pdev->sbdf.bdf) )
+    {
+        printk(XENLOG_ERR
+               "%pp: SR-IOV configuration unsupported for unpriv %pd\n",
+               &pdev->sbdf, pdev->domain);
+        return -EACCES;
+    }
+
+    pdev->vpci->sriov = xzalloc(struct vpci_sriov);
+    if ( !pdev->vpci->sriov )
+        return -ENOMEM;
+
+    /*
+     * 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 +37 -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;
+    }
+    XFREE(pdev->vpci->sriov);
+
+    /*
+     * Unprivileged domains have a deny by default register access policy, no
+     * need to add any further handlers for them.
+     */
+    if ( !is_hardware_domain(pdev->domain) )
+        return 0;
+
+    rc = vpci_add_register(pdev->vpci, vpci_hw_read16, NULL,
+                           pos + PCI_SRIOV_CTRL, 2, NULL);
+    if ( rc )
+        printk(XENLOG_ERR "%pd %pp: fail to add SRIOV ctrl handler rc=%d\n",
+               pdev->domain, &pdev->sbdf, rc);
+
+    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/include/xen/vpci.h +0 -0

index e34f7abe6d..fc8a943abe 100644

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. Also force VFs to always use emulated reads for command register, this is needed to prevent some drivers accidentally unmapping BARs. Discovery of VFs is done by Dom0, which must register them with Xen.

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

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

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

---
v2->v3:
* rework adding VFs with multiple mapping operations support
* use private.h
* style fixes
* fixup cleanup_sriov
* emulate command register read for VFs

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  |   1 +
 xen/drivers/vpci/header.c  |   6 +-
 xen/drivers/vpci/private.h |   1 +
 xen/drivers/vpci/sriov.c   | 314 +++++++++++++++++++++++++++++++++++++
 xen/include/xen/vpci.h     |   7 +-
 5 files changed, 327 insertions(+), 2 deletions(-)
 create mode 100644 xen/drivers/vpci/sriov.c

xen/drivers/vpci/Makefile +1 -0

index 9793a4f9b0..8b0e3c03a7 100644
@@ -1,3 +1,4 @@
 obj-y += cap.o
 obj-y += vpci.o header.o rebar.o
+obj-y += sriov.o
 obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o

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

index 451cdd3a6f..a36285672e 100644
@@ -869,7 +869,8 @@ int vpci_init_header(struct pci_dev *pdev)
      * PCI_COMMAND_PARITY, PCI_COMMAND_SERR, and PCI_COMMAND_FAST_BACK.
      */
     rc = vpci_add_register_mask(pdev->vpci,
-                                is_hwdom ? vpci_hw_read16 : guest_cmd_read,
+                                is_hwdom && !pdev->info.is_virtfn
+                                ? vpci_hw_read16 : guest_cmd_read,
                                 cmd_write, PCI_COMMAND, 2, header, 0, 0,
                                 is_hwdom ? 0
                                          : PCI_COMMAND_RSVDP_MASK |

@@ -900,6 +901,9 @@ int vpci_init_header(struct pci_dev *pdev)
 
     header->guest_cmd = cmd;
 
+    if ( pdev->info.is_virtfn )
+        return vpci_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);

xen/drivers/vpci/private.h +1 -0

index f012fd160d..8e0043ddbe 100644
@@ -45,6 +45,7 @@ typedef struct {
     REGISTER_VPCI_CAPABILITY(PCI_EXT_CAP_ID_##name, name, finit, fclean, true)
 
 int __must_check vpci_init_header(struct pci_dev *pdev);
+int __must_check vpci_vf_init_header(struct pci_dev *pdev);
 
 int vpci_init_capabilities(struct pci_dev *pdev, bool ext_only);
 void vpci_cleanup_capabilities(struct pci_dev *pdev, bool ext_only);

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

new file mode 100644
index 0000000000..ec6a7b84d5
@@ -0,0 +1,314 @@
+/* SPDX-License-Identifier: GPL-2.0-only */

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

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

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

+        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;
+}
+
+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)
+    {
+        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;
+        }
+
+        vf_pdev->vpci->header.guest_cmd &= ~PCI_COMMAND_MEMORY;
+        vf_pdev->vpci->header.guest_cmd |= (cmd & PCI_COMMAND_MEMORY);
+    }
+
+    return 0;
+}
+
+static int size_vf_bars(const struct pci_dev *pf_pdev, unsigned int sriov_pos,
+                        uint64_t *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.
+         */
+        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;
+}
+
+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 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;
+    int rc;
+
+    ASSERT(!pdev->info.is_virtfn);
+
+    if ( new_enabled == enabled && new_mem_enabled == mem_enabled )
+    {
+        pci_conf_write16(pdev->sbdf, reg, val);
+        return;
+    }
+
+    if ( mem_enabled && !new_mem_enabled )
+        map_vfs(pdev, 0);
+
+    if ( !enabled && new_enabled )
+    {
+        size_vf_bars(pdev, sriov_pos, (uint64_t *)data);
+
+        /*
+         * 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);
+    }
+
+    if ( !mem_enabled && new_mem_enabled )
+    {
+        rc = map_vfs(pdev, PCI_COMMAND_MEMORY);
+
+        if ( rc )
+            map_vfs(pdev, 0);
+    }
+
+    pci_conf_write16(pdev->sbdf, reg, val);
+}
+
+int vpci_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);
+        if ( rc )
+            return rc;
+
+        vf_pdev->vpci->header.guest_cmd |= PCI_COMMAND_MEMORY;
+    }
+
+    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);
+
+    if ( !pos )
+        return 0;