[PATCH v4 3/4] vpci: allow BAR map/unmap without affecting memory decoding bit

Stewart Hildebrand posted 4 patches 3 days, 2 hours ago
[PATCH v4 3/4] vpci: allow BAR map/unmap without affecting memory decoding bit
Posted by Stewart Hildebrand 3 days, 2 hours ago
Introduce 'bool map' and allow invoking modify_bars() without changing
the memory decoding bit. This will allow hardware domain to reposition
BARs without affecting the memory decoding bit.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
This also lays some groundwork to allow domUs to toggle the guest view
without affecting hardware, a step toward addressing the FIXME in [1].

[1] https://lore.kernel.org/xen-devel/20250814160358.95543-4-roger.pau@citrix.com/

v3->v4:
* rebase on dynamically allocated map queue

v2->v3:
* use bool
* switch to task->map in more places

v1->v2:
* new patch
---
 xen/drivers/vpci/header.c | 43 +++++++++++++++++++++------------------
 xen/include/xen/vpci.h    |  1 +
 2 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 146915e28c50..20fe380552f4 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -117,11 +117,12 @@ static int cf_check map_range(
  * BAR's enable bit has changed with the memory decoding bit already enabled.
  * If rom_only is not set then it's the memory decoding bit that changed.
  */
-static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
-                            bool rom_only)
+static void modify_decoding(const struct pci_dev *pdev,
+                            struct vpci_map_task *task)
 {
     struct vpci_header *header = &pdev->vpci->header;
-    bool map = cmd & PCI_COMMAND_MEMORY;
+    bool rom_only = task->rom_only;
+    bool map = task->map;
     unsigned int i;
 
     for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
@@ -168,7 +169,7 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
 
     if ( !rom_only )
     {
-        pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
+        pci_conf_write16(pdev->sbdf, PCI_COMMAND, task->cmd);
         header->bars_mapped = map;
     }
     else
@@ -188,7 +189,7 @@ static int vpci_process_map_task(const struct pci_dev *pdev,
         struct rangeset *mem = bar->mem;
         struct map_data data = {
             .d = pdev->domain,
-            .map = task->cmd & PCI_COMMAND_MEMORY,
+            .map = task->map,
             .bar = bar,
         };
         int rc;
@@ -203,9 +204,11 @@ static int vpci_process_map_task(const struct pci_dev *pdev,
 
         if ( rc )
         {
-            spin_lock(&pdev->vpci->lock);
             /* Disable memory decoding unconditionally on failure. */
-            modify_decoding(pdev, task->cmd & ~PCI_COMMAND_MEMORY, false);
+            task->cmd &= ~PCI_COMMAND_MEMORY;
+            task->map = false;
+            spin_lock(&pdev->vpci->lock);
+            modify_decoding(pdev, task);
             spin_unlock(&pdev->vpci->lock);
 
             if ( !is_hardware_domain(pdev->domain) )
@@ -216,7 +219,7 @@ static int vpci_process_map_task(const struct pci_dev *pdev,
     }
 
     spin_lock(&pdev->vpci->lock);
-    modify_decoding(pdev, task->cmd, task->rom_only);
+    modify_decoding(pdev, task);
     spin_unlock(&pdev->vpci->lock);
 
     return 0;
@@ -328,13 +331,14 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
         }
     }
     if ( !rc )
-        modify_decoding(pdev, task->cmd, false);
+        modify_decoding(pdev, task);
 
     return rc;
 }
 
 static struct vpci_map_task *alloc_map_task(const struct pci_dev *pdev,
-                                            uint16_t cmd, bool rom_only)
+                                            uint16_t cmd, bool rom_only,
+                                            bool map)
 {
     struct vpci_map_task *task;
     unsigned int i;
@@ -364,6 +368,7 @@ static struct vpci_map_task *alloc_map_task(const struct pci_dev *pdev,
 
     task->cmd = cmd;
     task->rom_only = rom_only;
+    task->map = map;
 
     return task;
 }
@@ -391,7 +396,8 @@ static void defer_map(const struct pci_dev *pdev, struct vpci_map_task *task)
     raise_softirq(SCHEDULE_SOFTIRQ);
 }
 
-static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
+static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only,
+                       bool map)
 {
     struct vpci_header *header = &pdev->vpci->header;
     struct pci_dev *tmp;
@@ -403,7 +409,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
 
     ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
 
-    task = alloc_map_task(pdev, cmd, rom_only);
+    task = alloc_map_task(pdev, cmd, rom_only, map);
 
     if ( !task )
         return -ENOMEM;
@@ -602,7 +608,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
          * be called iff the memory decoding bit is enabled, thus the operation
          * will always be to establish mappings and process all the BARs.
          */
-        ASSERT((cmd & PCI_COMMAND_MEMORY) && !rom_only);
+        ASSERT(map && !rom_only);
         rc = apply_map(pdev->domain, pdev, task);
         destroy_map_task(task);
         return rc;
@@ -646,7 +652,7 @@ static void cf_check cmd_write(
          * memory decoding bit has not been changed, so leave everything as-is,
          * hoping the guest will realize and try again.
          */
-        modify_bars(pdev, cmd, false);
+        modify_bars(pdev, cmd, false, cmd & PCI_COMMAND_MEMORY);
     else
         pci_conf_write16(pdev->sbdf, reg, cmd);
 }
@@ -810,11 +816,8 @@ static void cf_check rom_write(
         header->rom_enabled = new_enabled;
         pci_conf_write32(pdev->sbdf, reg, val);
     }
-    /*
-     * Pass PCI_COMMAND_MEMORY or 0 to signal a map/unmap request, note that
-     * this fabricated command is never going to be written to the register.
-     */
-    else if ( modify_bars(pdev, new_enabled ? PCI_COMMAND_MEMORY : 0, true) )
+    /* Note that the command value 0 will never be written to the register */
+    else if ( modify_bars(pdev, 0, true, new_enabled) )
         /*
          * No memory has been added or removed from the p2m (because the actual
          * p2m changes are deferred in defer_map) and the ROM enable bit has
@@ -1004,7 +1007,7 @@ int vpci_init_header(struct pci_dev *pdev)
             goto fail;
     }
 
-    return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false) : 0;
+    return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false, true) : 0;
 
  fail:
     pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index e34f7abe6da2..e6d40827a43a 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -164,6 +164,7 @@ struct vpci_map_task {
     } bars[ARRAY_SIZE(((struct vpci_header *)NULL)->bars)];
     uint16_t cmd;
     bool rom_only : 1;
+    bool map : 1;
 };
 
 struct vpci_vcpu {
-- 
2.53.0