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

On Mon, Apr 06, 2026 at 03:11:57PM -0400, Stewart Hildebrand wrote:
> 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/

The deferred toggling of the command register bits seems overly
complex IMO.  It would be much simpler if domUs simply don't get to
play with the real decoding bits.  For the hardware domain we could
toggle the decoding bits ahead of the mapping operations, so that we
don't have to defer the toggling after mappings have been sorted.

Long term we should aim to remove the cmd field from vpci_map_task
struct.

I don't mind if you want to pick the patch in [1] in your series, as
we could then drop the cmd field and possibly simplify some of the
logic in this patch by no longer requiring the deferred write to the
command register?

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

task wants to be const here, unless further patches require modifying
task inside of modify_decoding().

Thanks, Roger.