[PATCH v5 0/9] vpci: allow queueing of mapping operations

[PATCH v5 0/9] vpci: allow queueing of mapping operations

[Stewart Hildebrand]

Introduce the ability to queue p2m mapping operations in vPCI. This is
pre-requisite for both vPCI SR-IOV [2] and the ability to handle BAR writes
with memory decoding enabled [1]. I thought I might wait to submit the BAR-
write-with-memory-decoding-enabled patch until the prerequisite is reviewed
acked/merged, but if you'd find it more helpful for me to submit it right away
I'd be happy to do so (I've already pushed it to the gitlab pipeline [0]).

When multiple operations are queued, there are some non-trivial complexities
with the deferred setting of bar->enabled, header->rom_enabled, and
header->bars_mapped flags. If deferred, these flags wouldn't be updated until
the p2m op queue is processing, but we need to consult them in modify_bars()
at queue time, so they wouldn't be accurate when adding multiple tasks to the
queue. In order to simplify the deferred mapping state, and in particular
avoid complexities with the bar->enabled, header->rom_enabled, and
header->bars_mapped flags, I have picked up Roger's vPCI fixes series [3]. The
overall result is now that _only_ p2m operations are deferred. PCI config
space writes, flag-setting, and everything else is handled at queuing time.

1. vpci: move BAR mapping permissions checks
2. vpci: make BAR mapping more resilient for the hardware domain
3. vpci: only check BAR validity once
4. vpci/msix: move MSI-X hole punching as a result of memory decoding enable
5. vpci: simplify handling of memory decoding and ROM enable writes
6. vpci: create mem local variables
7. vpci: split vpci_process_pending()
8. vpci: allow queueing of mapping operations
9. vpci: don't pass command value to modify_bars()

Patch 1 is code movement, and patch 2 is hardening map_range(). While these
 first 2 are not strictly pre-requisites for queued mapping, they simplify the
 subsequent prepwork.
Patches 3-5 progressively remove modify_decoding().
Patches 6-7 are non-functional changes pulled out from patch 8.
Patch 8 is the core feature: pre-requisite for SR-IOV and BAR-write-with-
 memory-decoding-enabled.
Patch 9 is cleanup I chose to put at the end for lower diffstat.

Patches 2-5 are borrowed from Roger's vPCI fixes series [3]. These patches
have gone directly from v1 to v5, but otherwise revlogs are written assuming
this is a continuation of the BAR-write-with-memory-decoding-enabled series.

v4->v5:
* don't defer anything that's not p2m related
* drop ("vpci: allow 32-bit BAR writes with memory decoding enabled") for now
  to focus on the pre-requisite patches first
* split non-functional changes

v3->v4:
* switch back to dynamically allocated queue elements

v2->v3:
* add ("vpci: Use pervcpu ranges for BAR mapping")
* rework with fixed array of map/unmap slots

v1->v2:
* new approach with queued p2m operations

RFC->v1:
* rework BAR mapping machinery to support unmap-then-map operation

[0] Pipeline: https://gitlab.com/xen-project/people/stewarthildebrand/xen/-/pipelines/2662651745

[1] BAR write with memory decoding enabled
v4: https://lore.kernel.org/xen-devel/20260406191203.97662-1-stewart.hildebrand@amd.com/T/#t
v3: https://lore.kernel.org/xen-devel/20260324030513.700217-1-stewart.hildebrand@amd.com/T/#t
v2: https://lore.kernel.org/xen-devel/20250723163744.13095-1-stewart.hildebrand@amd.com/T/#t
v1: https://lore.kernel.org/xen-devel/20250531125405.268984-1-stewart.hildebrand@amd.com/T/#t
RFC: https://lore.kernel.org/xen-devel/20250312195019.382926-1-stewart.hildebrand@amd.com/T/#t
Gitlab: https://gitlab.com/xen-project/xen/-/issues/197

[2] vPCI SR-IOV
v3: https://lore.kernel.org/xen-devel/cover.1775742115.git.mykyta_poturai@epam.com/T/#t
v2: https://lore.kernel.org/xen-devel/cover.1772806036.git.mykyta_poturai@epam.com/T/#t
v1: https://lore.kernel.org/xen-devel/cover.1753450965.git.mykyta_poturai@epam.com/T/#t
2018 v2 https://lore.kernel.org/xen-devel/20180717094830.54806-1-roger.pau@citrix.com/T/#t

[3] vPCI miscellaneous fixes
https://lore.kernel.org/xen-devel/20250814160358.95543-1-roger.pau@citrix.com/T/#t


 xen/common/domain.c        |   5 +
 xen/drivers/vpci/header.c  | 443 +++++++++++++++++++++----------------
 xen/drivers/vpci/private.h |   1 +
 xen/drivers/vpci/vpci.c    |  17 +-
 xen/include/xen/vpci.h     |  15 +-
 5 files changed, 279 insertions(+), 202 deletions(-)


base-commit: 03851197a7b2cd58851b9ec2293bdf4a68c47b66
-- 
2.54.0

[PATCH v5 1/9] vpci: move BAR mapping permissions checks

[Stewart Hildebrand]

There's no need to defer the permissions checks. Perform them right away
in modify_bars().

As a result of the move, the permissions checks will now cover the whole
BAR before any holes are removed.

Carry over the domain_crash() in the error path for domUs.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v5: new patch
---
 xen/drivers/vpci/header.c | 40 +++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index d1c92cf77f8a..5ac525036672 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -58,24 +58,6 @@ static int cf_check map_range(
          * offset of the current address from the BAR start.
          */
         unsigned long map_mfn = start_mfn + s - start_gfn;
-        unsigned long m_end = map_mfn + size - 1;
-
-        if ( !iomem_access_permitted(map->d, map_mfn, m_end) )
-        {
-            printk(XENLOG_G_WARNING
-                   "%pd denied access to MMIO range [%#lx, %#lx]\n",
-                   map->d, map_mfn, m_end);
-            return -EPERM;
-        }
-
-        rc = xsm_iomem_mapping_vpci(XSM_HOOK, map->d, map_mfn, m_end, map->map);
-        if ( rc )
-        {
-            printk(XENLOG_G_WARNING
-                   "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
-                   map->d, map_mfn, m_end, rc);
-            return rc;
-        }
 
         /*
          * ARM TODOs:
@@ -369,6 +351,28 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
             return -EINVAL;
         }
 
+        if ( !iomem_access_permitted(pdev->domain, start, end) )
+        {
+            printk(XENLOG_G_WARNING
+                   "%pd denied access to MMIO range [%#lx, %#lx]\n",
+                   pdev->domain, start, end);
+            if ( !is_hardware_domain(pdev->domain) )
+                domain_crash(pdev->domain);
+            return -EPERM;
+        }
+
+        rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end,
+                                    !!(cmd & PCI_COMMAND_MEMORY));
+        if ( rc )
+        {
+            printk(XENLOG_G_WARNING
+                   "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
+                   pdev->domain, start, end, rc);
+            if ( !is_hardware_domain(pdev->domain) )
+                domain_crash(pdev->domain);
+            return rc;
+        }
+
         rc = rangeset_add_range(bar->mem, start_guest, end_guest);
         if ( rc )
         {
-- 
2.54.0

[PATCH v5 2/9] vpci: make BAR mapping more resilient for the hardware domain

[Stewart Hildebrand]

From: Roger Pau Monne <roger.pau@citrix.com>

The logic in map_range() will bubble up failures to the upper layer, which
will result in any remaining regions being skip, and for the non-hardware
domain case the owner domain of the device would be destroyed.  However for
the hardware domain the intent is to continue execution, hoping the
failure to modify the p2m could be worked around by the hardware domain.

To accomplish that in a better way, ignore failures and skip the range in
that case, possibly continuing to map further ranges.

Since the error path in vpci_process_pending() should only be used by domUs
now, and it will unconditionally end up calling domain_crash(), simplify
it: there's no need to cleanup if the domain will be destroyed.

Memory decoding may be left enabled in case of mapping error for devices
assigned to domUs.

No functional change for domUs intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
Although not strictly a pre-requisite for queued p2m mapping, I've
included this patch from Roger's vPCI fixes series since it simplifies
the error path in vpci_process_pending().

v1->v5:
* move "*c += size;" outside is_hardware_domain condition
* add note to commit message about not disabling memory decoding on failure
---
 xen/drivers/vpci/header.c | 45 +++++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 5ac525036672..9f2d0c8a5452 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -70,17 +70,26 @@ static int cf_check map_range(
 
         rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, _mfn(map_mfn))
                       : unmap_mmio_regions(map->d, _gfn(s), size, _mfn(map_mfn));
-        if ( rc == 0 )
-        {
-            *c += size;
-            break;
-        }
         if ( rc < 0 )
         {
             printk(XENLOG_G_WARNING
                    "Failed to %smap [%lx %lx] -> [%lx %lx] for %pd: %d\n",
                    map->map ? "" : "un", s, e, map_mfn,
                    map_mfn + size, map->d, rc);
+            goto done;
+        }
+        if ( rc == 0 )
+        {
+ done:
+            if ( is_hardware_domain(map->d) )
+                /*
+                 * Ignore failures for the hardware domain and skip the range.
+                 * Do it as a best effort workaround to attempt to get the
+                 * hardware domain to boot.
+                 */
+                rc = 0;
+
+            *c += size;
             break;
         }
         ASSERT(rc < size);
@@ -196,28 +205,22 @@ bool vpci_process_pending(struct vcpu *v)
             return true;
         }
 
-        if ( rc )
+        if ( rc && !is_hardware_domain(v->domain) )
         {
-            spin_lock(&pdev->vpci->lock);
-            /* Disable memory decoding unconditionally on failure. */
-            modify_decoding(pdev, v->vpci.cmd & ~PCI_COMMAND_MEMORY,
-                            false);
-            spin_unlock(&pdev->vpci->lock);
-
-            /* 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);
-
-            v->vpci.pdev = NULL;
-
             read_unlock(&v->domain->pci_lock);
 
-            if ( !is_hardware_domain(v->domain) )
-                domain_crash(v->domain);
+            domain_crash(v->domain);
 
             return false;
         }
+        ASSERT(!rc);
+        /*
+         * Purge rangeset to deal with the hardware domain having triggered an
+         * error.  It shouldn't be possible, as map_range() will always swallow
+         * errors for hardware domain owned devices, and
+         * rangeset_consume_ranges() itself doesn't generate any errors.
+         */
+        rangeset_purge(bar->mem);
     }
     v->vpci.pdev = NULL;
 
-- 
2.54.0

[PATCH v5 3/9] vpci: only check BAR validity once

[Stewart Hildebrand]

From: Roger Pau Monne <roger.pau@citrix.com>

The BAR validity checks in modify_decoding() are redundant since they
are already done in modify_bars().

Avoid multiple calls to pci_check_bar() for the same memory decoding
related operation, as each call can possibly print a warning message
about a BAR being in an invalid position.

Further, there's no need to defer setting bar->enabled until p2m
operations are finished. It can safely be set in modify_bars() itself.
This slightly changes the meaning of the bar->enabled flag: when true,
it means a BAR is either mapped or queued to be mapped. Store whether
the BAR is validly positioned in modify_bars(), and use the cached value
for setting bar->enabled.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Fixes: 4acab25a9300 ('x86/vpci: fix handling of BAR overlaps with non-hole regions')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v1->v5:
* rebase
* don't defer setting bar->enabled
* use local variable for bar_valid flag

Original report [1]:
[1] https://lore.kernel.org/xen-devel/dbc003a2-9202-46ec-bf87-2829d8a63d53@citrix.com/
---
 xen/drivers/vpci/header.c | 18 +++++++++---------
 xen/include/xen/vpci.h    |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 9f2d0c8a5452..1fd4832033e9 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -128,19 +128,10 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
             uint32_t val = bar->addr |
                            (map ? PCI_ROM_ADDRESS_ENABLE : 0);
 
-            if ( pci_check_bar(pdev, _mfn(PFN_DOWN(bar->addr)),
-                               _mfn(PFN_DOWN(bar->addr + bar->size - 1))) )
-                bar->enabled = map;
             header->rom_enabled = map;
             pci_conf_write32(pdev->sbdf, rom_pos, val);
             return;
         }
-
-        if ( !rom_only &&
-             (bar->type != VPCI_BAR_ROM || header->rom_enabled) &&
-             pci_check_bar(pdev, _mfn(PFN_DOWN(bar->addr)),
-                           _mfn(PFN_DOWN(bar->addr + bar->size - 1))) )
-            bar->enabled = map;
     }
 
     /*
@@ -296,6 +287,7 @@ static int 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;
+    bool bar_valid[ARRAY_SIZE(header->bars)] = { false };
     unsigned int i, j;
     int rc;
 
@@ -341,6 +333,8 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
 
         ASSERT(rangeset_is_empty(bar->mem));
 
+        bar_valid[i] = true;
+
         /*
          * Make sure that the guest set address has the same page offset
          * as the physical address on the host or otherwise things won't work as
@@ -506,6 +500,12 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
         d = dom_xen;
     }
 
+    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
+    {
+        if ( bar_valid[i] )
+            header->bars[i].enabled = cmd & PCI_COMMAND_MEMORY;
+    }
+
     if ( system_state < SYS_STATE_active )
     {
         /*
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 877aa391d178..ab94eb60d65f 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -64,7 +64,7 @@ struct vpci {
                 VPCI_BAR_ROM,
             } type;
             bool prefetchable : 1;
-            /* Store whether the BAR is mapped into guest p2m. */
+            /* Whether the BAR is mapped or queued for mapping in guest p2m. */
             bool enabled      : 1;
         } bars[PCI_HEADER_NORMAL_NR_BARS + 1];
         /* At most 6 BARS + 1 expansion ROM BAR. */
-- 
2.54.0

[PATCH v5 4/9] vpci/msix: move MSI-X hole punching as a result of memory decoding enable

[Stewart Hildebrand]

From: Roger Pau Monne <roger.pau@citrix.com>

Deferring the p2m unpopulate of the MSI-X table region to after the BARs are
mapped in the p2m is not needed.  The aim of vpci_make_msix_hole() is to
cope with the BAR containing the MSI-X table being positioned over a
reserved e820 region, by making sure the MSI-X table region is
unpopulated on the p2m. Note that reserved e820 regions are identity mapped
in the hardware domain p2m by default.

Doing it before or after the BAR p2m changes are done is irrelevant, hence
do it ahead of the p2m changes, as that's simpler.

Note that vpci_make_msix_hole() should be invoked strictly after setting
bar->enabled. The movement of vpci_make_msix_hole() is OK since
bar->enabled is set in modify_bars().

Take the opportunity to store the memory decoding enabled bit in a local
variable.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
Note that this should not be committed until after the bar->enabled
setting movement in ("xen/vpci: only check BAR validity once")

I tested this with hot-plug as described in 7c24d58a6e64 ("vpci/msix:
check for BARs enabled in vpci_make_msix_hole").

The return at the end of the function will gain a purpose again after a
later code movement patch in the series.

The new_enabled variable will gain 2 additional uses later in the series.

v1->v5:
* order after ("xen/vpci: only check BAR validity once")
* preserve behavior of only invoking vpci_make_msix_hole() when memory
  decoding toggles from 0 to 1
* add note about vpci_make_msix_hole() invocation after setting bar->enabled
* use local variable
* Jan offered an R-b tag in [1], though I hesitated to pick it up due to
  changes, and it was offered prior to 7c24d58a6e64 going upstream
[1] https://lore.kernel.org/xen-devel/e2c4944d-d089-490c-879d-b6baec5e4cae@suse.com/
---
 xen/drivers/vpci/header.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 1fd4832033e9..cf9d0bac8876 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -134,19 +134,6 @@ static void modify_decoding(const struct pci_dev *pdev, uint16_t cmd,
         }
     }
 
-    /*
-     * Make sure there are no mappings in the MSIX MMIO areas, so that accesses
-     * can be trapped (and emulated) by Xen when the memory decoding bit is
-     * enabled.
-     *
-     * FIXME: punching holes after the p2m has been set up might be racy for
-     * DomU usage, needs to be revisited.
-     */
-#ifdef CONFIG_HAS_PCI_MSI
-    if ( map && !rom_only && vpci_make_msix_hole(pdev) )
-        return;
-#endif
-
     if ( !rom_only )
     {
         pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
@@ -529,6 +516,7 @@ static void cf_check cmd_write(
     const struct pci_dev *pdev, unsigned int reg, uint32_t cmd, void *data)
 {
     struct vpci_header *header = data;
+    bool new_enabled = cmd & PCI_COMMAND_MEMORY;
 
     if ( !is_hardware_domain(pdev->domain) )
     {
@@ -546,7 +534,7 @@ static void cf_check cmd_write(
      * decoding one. Bits that are not allowed for DomU are already
      * handled above and by the rsvdp_mask.
      */
-    if ( header->bars_mapped != !!(cmd & PCI_COMMAND_MEMORY) )
+    if ( header->bars_mapped != new_enabled )
         /*
          * Ignore the error. No memory has been added or removed from the p2m
          * (because the actual p2m changes are deferred in defer_map) and the
@@ -556,6 +544,12 @@ static void cf_check cmd_write(
         modify_bars(pdev, cmd, false);
     else
         pci_conf_write16(pdev->sbdf, reg, cmd);
+
+#ifdef CONFIG_HAS_PCI_MSI
+    /* Unpopulate MSI-X table region, so accesses trap into Xen. */
+    if ( !header->bars_mapped && new_enabled && vpci_make_msix_hole(pdev) )
+        return;
+#endif
 }
 
 static uint32_t cf_check guest_cmd_read(
-- 
2.54.0

[PATCH v5 5/9] vpci: simplify handling of memory decoding and ROM enable writes

[Stewart Hildebrand]

From: Roger Pau Monne <roger.pau@citrix.com>

Deferring the actual write of the PCI register bit, either the memory
decoding or the ROM enable is not helpful, and adds an unnecessary amount of
complexity to the preemptible handling of BAR related p2m modifications.

In the hardware domain case, whether the PCI register write is done ahead
or after the p2m changes doesn't matter, a hardware domain has plenty of
ways to mess with the PCI register state if it wants to.  Any poking at the
BAR p2m regions ahead of the guest write having completed will be
undefined.

On the other hand, for domUs the memory decoding bit shouldn't really
change as a result of guest actions, and should always be enabled.  Guest
toggling the memory decoding command register should only result in p2m
modifications, but no propagation to the device PCI registers.  Having
memory decoding unconditionally enabled ensures the domU attempting to
perform p2m accesses while the p2m changes are taking place will always
access the BAR contents. This is not the current behavior for domUs, so add
a note that it would preferably done that way.

This allows to get rid of modify_decoding(), as writing the command
register can easily be done without the need for an external helper.

Resolves: https://gitlab.com/xen-project/xen/-/issues/98
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
Since the full command register value is no longer needed after p2m
operations, we could reasonably replace the cmd parameter to
modify_bars() with 'bool map' immediately after this patch. However, I
chose to wait until the end of the series to reduce the diffstat and
make review easier.

Note that in rom_write(), when rom->enabled and new_enabled are both
false, rom->addr/guest_addr will be written twice. This quirk is
preexisting, so I didn't think it was in the scope for this patch to
address it. Do you think it's worth addressing separately?

v1->v5:
* rebase
* reorder within series
* in rom_write(), retain bars_mapped / enable toggle gate for invoking
  modify_bars()
* add Resolves: tag
---
 xen/drivers/vpci/header.c | 80 +++++++++++----------------------------
 1 file changed, 22 insertions(+), 58 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index cf9d0bac8876..908adf0b3f4c 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -102,47 +102,6 @@ static int cf_check map_range(
     return rc;
 }
 
-/*
- * The rom_only parameter is used to signal the map/unmap helpers that the ROM
- * 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)
-{
-    struct vpci_header *header = &pdev->vpci->header;
-    bool map = cmd & PCI_COMMAND_MEMORY;
-    unsigned int i;
-
-    for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
-    {
-        struct vpci_bar *bar = &header->bars[i];
-
-        if ( !MAPPABLE_BAR(bar) )
-            continue;
-
-        if ( rom_only && bar->type == VPCI_BAR_ROM )
-        {
-            unsigned int rom_pos = (i == PCI_HEADER_NORMAL_NR_BARS)
-                                   ? PCI_ROM_ADDRESS : PCI_ROM_ADDRESS1;
-            uint32_t val = bar->addr |
-                           (map ? PCI_ROM_ADDRESS_ENABLE : 0);
-
-            header->rom_enabled = map;
-            pci_conf_write32(pdev->sbdf, rom_pos, val);
-            return;
-        }
-    }
-
-    if ( !rom_only )
-    {
-        pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
-        header->bars_mapped = map;
-    }
-    else
-        ASSERT_UNREACHABLE();
-}
-
 bool vpci_process_pending(struct vcpu *v)
 {
     const struct pci_dev *pdev = v->vpci.pdev;
@@ -202,10 +161,6 @@ bool vpci_process_pending(struct vcpu *v)
     }
     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;
@@ -241,8 +196,6 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
             write_lock(&d->pci_lock);
         }
     }
-    if ( !rc )
-        modify_decoding(pdev, cmd, false);
 
     return rc;
 }
@@ -534,22 +487,29 @@ static void cf_check cmd_write(
      * decoding one. Bits that are not allowed for DomU are already
      * handled above and by the rsvdp_mask.
      */
-    if ( header->bars_mapped != new_enabled )
+    if ( header->bars_mapped != new_enabled &&
+         modify_bars(pdev, cmd, false) )
         /*
          * Ignore the error. No memory has been added or removed from the p2m
          * (because the actual p2m changes are deferred in defer_map) and the
          * 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);
-    else
-        pci_conf_write16(pdev->sbdf, reg, cmd);
+        return;
 
 #ifdef CONFIG_HAS_PCI_MSI
     /* Unpopulate MSI-X table region, so accesses trap into Xen. */
     if ( !header->bars_mapped && new_enabled && vpci_make_msix_hole(pdev) )
         return;
 #endif
+
+    /*
+     * FIXME: for domUs we don't want the guest toggling the memory decoding
+     * bit.  It should be set in vpci_init_header() and guest attempts to
+     * modify it should only lead to guest p2m changes.
+     */
+    header->bars_mapped = new_enabled;
+    pci_conf_write16(pdev->sbdf, reg, cmd);
 }
 
 static uint32_t cf_check guest_cmd_read(
@@ -705,17 +665,12 @@ static void cf_check rom_write(
         rom->guest_addr = rom->addr;
     }
 
-    if ( !header->bars_mapped || rom->enabled == new_enabled )
-    {
-        /* Just update the ROM BAR field. */
-        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) )
+    if ( header->bars_mapped && rom->enabled != new_enabled &&
+         modify_bars(pdev, new_enabled ? PCI_COMMAND_MEMORY : 0, true) )
         /*
          * 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
@@ -726,6 +681,8 @@ static void cf_check rom_write(
          */
         return;
 
+    header->rom_enabled = new_enabled;
+    pci_conf_write32(pdev->sbdf, reg, val);
     if ( !new_enabled )
     {
         rom->addr = val & PCI_ROM_ADDRESS_MASK;
@@ -927,6 +884,13 @@ int vpci_init_header(struct pci_dev *pdev)
             goto fail;
     }
 
+    if ( cmd & PCI_COMMAND_MEMORY )
+    {
+        /* Restore command register value. */
+        header->bars_mapped = true;
+        pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
+    }
+
     return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false) : 0;
 
  fail:
-- 
2.54.0

[PATCH v5 6/9] vpci: create mem local variables

[Stewart Hildebrand]

Use local variables to reference struct rangeset *mem in preparation of
moving it out of struct vpci. No functional change.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
v4->v5:
* Patch adapted from [1] ("vpci: Use pervcpu ranges for BAR mapping").
  I kept Mykyta's S-o-b tag since many of the changes are similar.

[1] https://lore.kernel.org/xen-devel/20260406191203.97662-2-stewart.hildebrand@amd.com/
---
 xen/drivers/vpci/header.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 908adf0b3f4c..28e051f2779c 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -124,6 +124,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 = bar->mem;
         struct map_data data = {
             .d = v->domain,
             .map = v->vpci.cmd & PCI_COMMAND_MEMORY,
@@ -131,10 +132,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 )
         {
@@ -178,13 +179,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 = bar->mem;
         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
@@ -248,12 +250,13 @@ static int 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 = bar->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);
 
-        if ( !bar->mem )
+        if ( !mem )
             continue;
 
         if ( !MAPPABLE_BAR(bar) ||
@@ -271,7 +274,7 @@ static int 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));
 
         bar_valid[i] = true;
 
@@ -310,7 +313,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
             return rc;
         }
 
-        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",
@@ -321,12 +324,12 @@ static int 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 = header->bars[j].mem;
 
-            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,
@@ -336,7 +339,7 @@ static int 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,
@@ -355,12 +358,12 @@ static int 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 = header->bars[j].mem;
 
-            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,
@@ -410,8 +413,9 @@ static int 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 = bar->mem;
 
-                    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
@@ -422,7 +426,7 @@ static int 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,
-- 
2.54.0

[PATCH v5 7/9] vpci: split vpci_process_pending()

[Stewart Hildebrand]

Create a new function to separate the mapping logic from the pdev
validation and lock-acquiring logic. Slightly consolidate exit paths for
hardware_domain and !hardware_domain.

No functional change.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v4->v5:
* new patch
---
 xen/drivers/vpci/header.c | 65 +++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 27 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 28e051f2779c..f7eaf9c6ddf2 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -102,23 +102,12 @@ static int cf_check map_range(
     return rc;
 }
 
-bool vpci_process_pending(struct vcpu *v)
+static int process_map_task(const struct pci_dev *pdev, bool map)
 {
-    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++ )
@@ -126,8 +115,8 @@ bool vpci_process_pending(struct vcpu *v)
         struct vpci_bar *bar = &header->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 = map,
             .bar = bar,
         };
         int rc;
@@ -136,21 +125,12 @@ bool vpci_process_pending(struct vcpu *v)
             continue;
 
         rc = rangeset_consume_ranges(mem, map_range, &data);
-
         if ( rc == -ERESTART )
-        {
-            read_unlock(&v->domain->pci_lock);
-            return true;
-        }
-
-        if ( rc && !is_hardware_domain(v->domain) )
-        {
-            read_unlock(&v->domain->pci_lock);
+            return rc;
 
-            domain_crash(v->domain);
+        if ( rc && !is_hardware_domain(pdev->domain) )
+            return rc;
 
-            return false;
-        }
         ASSERT(!rc);
         /*
          * Purge rangeset to deal with the hardware domain having triggered an
@@ -160,10 +140,41 @@ bool vpci_process_pending(struct vcpu *v)
          */
         rangeset_purge(bar->mem);
     }
+
+    return 0;
+}
+
+bool vpci_process_pending(struct vcpu *v)
+{
+    const struct pci_dev *pdev = v->vpci.pdev;
+    int rc;
+
+    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;
+    }
+
+    rc = process_map_task(pdev, v->vpci.cmd & PCI_COMMAND_MEMORY);
+    if ( rc == -ERESTART )
+    {
+        read_unlock(&v->domain->pci_lock);
+        return true;
+    }
+
     v->vpci.pdev = NULL;
 
     read_unlock(&v->domain->pci_lock);
 
+    if ( rc && !is_hardware_domain(v->domain) )
+        domain_crash(v->domain);
+
     return false;
 }
 
-- 
2.54.0

[PATCH v5 8/9] vpci: allow queueing of mapping operations

[Stewart Hildebrand]

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

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

While queueing multiple operations is valid, at the moment, only a
single operation will be queued.

Removed rangeset names, since RANGESETF_no_print was set anyway.

Amends: 622bdd962822 ("vpci/header: handle p2m range sets per BAR")
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
---
As a side effect of this, the ASSERT(rangeset_is_empty(mem)) in
modify_bars() is no longer bogus (see [2]).

v4->v5:
* split non-functional pre-requisites out into separate patches
* move pdev into struct map_task in anticipation of mapping different pdevs
  corresponding to different VFs
* move call to vpci_vcpu_destroy() from vcpu_destroy() to vcpu_teardown()

v3->v4:
* switch back to dynamically allocated queue elements

v2->v3:
* rebase on ("vpci: Use pervcpu ranges for BAR mapping") from [1]
* rework with fixed array of map/unmap slots

[1] https://lore.kernel.org/xen-devel/cover.1772806036.git.mykyta_poturai@epam.com/T/#t
[2] https://lore.kernel.org/xen-devel/20250814160358.95543-2-roger.pau@citrix.com/

v1->v2:
* new patch
---
 xen/common/domain.c        |   5 +
 xen/drivers/vpci/header.c  | 222 +++++++++++++++++++++++++------------
 xen/drivers/vpci/private.h |   1 +
 xen/drivers/vpci/vpci.c    |  17 ++-
 xen/include/xen/vpci.h     |  13 ++-
 5 files changed, 180 insertions(+), 78 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 53ac1d6c4034..d576c2a40fdf 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -36,6 +36,7 @@
 #include <xen/argo.h>
 #include <xen/llc-coloring.h>
 #include <xen/xvmalloc.h>
+#include <xen/vpci.h>
 #include <asm/p2m.h>
 #include <asm/processor.h>
 #include <public/sched.h>
@@ -439,6 +440,8 @@ static int vmtrace_alloc_buffer(struct vcpu *v)
  */
 static int vcpu_teardown(struct vcpu *v)
 {
+    vpci_vcpu_destroy(v);
+
     vmtrace_free_buffer(v);
 
     return 0;
@@ -508,6 +511,8 @@ struct vcpu *vcpu_create(struct domain *d, unsigned int vcpu_id)
     if ( arch_vcpu_create(v) != 0 )
         goto fail_sched;
 
+    vpci_vcpu_init(v);
+
     d->vcpu[vcpu_id] = v;
     if ( vcpu_id != 0 )
     {
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index f7eaf9c6ddf2..99ce67053068 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -23,6 +23,7 @@
 #include <xen/lib.h>
 #include <xen/sched.h>
 #include <xen/softirq.h>
+#include <xen/xvmalloc.h>
 
 #include <xsm/xsm.h>
 
@@ -33,9 +34,21 @@
     ((x)->type == VPCI_BAR_MEM32 || (x)->type == VPCI_BAR_MEM64_LO ||   \
      (x)->type == VPCI_BAR_ROM)
 
+struct map_task {
+    /* Structure to store state while {un}mapping PCI BARs. */
+    struct list_head next; /* List element in v->vpci.task_queue */
+    const struct pci_dev *pdev;
+    struct bar_map {
+        uint64_t addr;
+        uint64_t guest_addr;
+        struct rangeset *mem;
+    } bars[ARRAY_SIZE(((struct vpci_header *)NULL)->bars)];
+    bool map : 1;
+};
+
 struct map_data {
     struct domain *d;
-    const struct vpci_bar *bar;
+    const struct bar_map *bar;
     bool map;
 };
 
@@ -102,21 +115,82 @@ static int cf_check map_range(
     return rc;
 }
 
-static int process_map_task(const struct pci_dev *pdev, bool map)
+static void destroy_map_task(struct map_task *task)
+{
+    unsigned int i;
+
+    for ( i = 0; i < ARRAY_SIZE(task->bars); i++ )
+        rangeset_destroy(task->bars[i].mem);
+
+    xvfree(task);
+}
+
+void vpci_clear_map_queue(struct vcpu *v, const struct pci_dev *pdev)
+{
+    struct map_task *task, *tmp;
+
+    list_for_each_entry_safe ( task, tmp, &v->vpci.task_queue, next )
+    {
+        if ( !pdev || task->pdev == pdev )
+        {
+            list_del(&task->next);
+            destroy_map_task(task);
+        }
+    }
+}
+
+static struct map_task *alloc_map_task(const struct pci_dev *pdev, bool map,
+                                       bool rom_only)
+{
+    struct map_task *task;
+    unsigned int i;
+
+    task = xvzalloc(struct map_task);
+    if ( !task )
+        return NULL;
+
+    task->pdev = pdev;
+
+    for ( i = 0; i < ARRAY_SIZE(task->bars); i++ )
+    {
+        if ( !MAPPABLE_BAR(&pdev->vpci->header.bars[i]) )
+            continue;
+
+        if ( rom_only && pdev->vpci->header.bars[i].type != VPCI_BAR_ROM )
+            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->map = map;
+
+    return task;
+}
+
+static int process_map_task(struct map_task *task)
 {
-    struct vpci_header *header = NULL;
+    const struct pci_dev *pdev = task->pdev;
     unsigned int i;
 
     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 bar_map *bar = &task->bars[i];
         struct rangeset *mem = bar->mem;
         struct map_data data = {
             .d = pdev->domain,
-            .map = map,
+            .map = task->map,
             .bar = bar,
         };
         int rc;
@@ -146,29 +220,47 @@ static int process_map_task(const struct pci_dev *pdev, bool map)
 
 bool vpci_process_pending(struct vcpu *v)
 {
-    const struct pci_dev *pdev = v->vpci.pdev;
-    int rc;
-
-    if ( !pdev )
-        return false;
+    struct map_task *task;
+    int rc = 0;
 
     read_lock(&v->domain->pci_lock);
 
-    if ( !pdev->vpci || (v->domain != pdev->domain) )
+    while ( (task = list_first_entry_or_null(&v->vpci.task_queue,
+                                             struct map_task, next)) != NULL )
     {
-        v->vpci.pdev = NULL;
-        read_unlock(&v->domain->pci_lock);
-        return false;
-    }
+        const struct pci_dev *pdev = task->pdev;
 
-    rc = process_map_task(pdev, v->vpci.cmd & PCI_COMMAND_MEMORY);
-    if ( rc == -ERESTART )
-    {
-        read_unlock(&v->domain->pci_lock);
-        return true;
-    }
+        if ( !pdev )
+        {
+            ASSERT_UNREACHABLE();
+            vpci_clear_map_queue(v, NULL);
+            rc = -EILSEQ;
+            break;
+        }
 
-    v->vpci.pdev = NULL;
+        if ( !pdev->vpci || (v->domain != pdev->domain) )
+        {
+            list_del(&task->next);
+            destroy_map_task(task);
+            continue;
+        }
+
+        rc = process_map_task(task);
+        if ( rc == -ERESTART )
+        {
+            read_unlock(&v->domain->pci_lock);
+            return true;
+        }
+
+        list_del(&task->next);
+        destroy_map_task(task);
+
+        if ( rc )
+        {
+            vpci_clear_map_queue(v, pdev);
+            break;
+        }
+    }
 
     read_unlock(&v->domain->pci_lock);
 
@@ -178,18 +270,17 @@ bool vpci_process_pending(struct vcpu *v)
     return false;
 }
 
-static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
-                            uint16_t cmd)
+static int __init apply_map(struct map_task *task)
 {
-    struct vpci_header *header = &pdev->vpci->header;
+    struct domain *d = task->pdev->domain;
     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 bar_map *bar = &task->bars[i];
         struct rangeset *mem = bar->mem;
         struct map_data data = { .d = d, .map = true, .bar = bar };
 
@@ -213,7 +304,7 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
     return rc;
 }
 
-static void defer_map(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
+static void defer_map(struct map_task *task)
 {
     struct vcpu *curr = current;
 
@@ -223,9 +314,8 @@ static void defer_map(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
      * is mapped. This can lead to parallel mapping operations being
      * started for the same device if the domain is not well-behaved.
      */
-    curr->vpci.pdev = pdev;
-    curr->vpci.cmd = cmd;
-    curr->vpci.rom_only = rom_only;
+    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
@@ -240,12 +330,17 @@ static int 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 map_task *task;
     bool bar_valid[ARRAY_SIZE(header->bars)] = { false };
     unsigned int i, j;
     int rc;
 
     ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
 
+    task = alloc_map_task(pdev, cmd & PCI_COMMAND_MEMORY, 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
@@ -261,7 +356,7 @@ static int 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 = bar->mem;
+        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);
@@ -299,7 +394,8 @@ static int 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;
         }
 
         if ( !iomem_access_permitted(pdev->domain, start, end) )
@@ -309,7 +405,8 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
                    pdev->domain, start, end);
             if ( !is_hardware_domain(pdev->domain) )
                 domain_crash(pdev->domain);
-            return -EPERM;
+            rc = -EPERM;
+            goto fail;
         }
 
         rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end,
@@ -321,7 +418,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
                    pdev->domain, start, end, rc);
             if ( !is_hardware_domain(pdev->domain) )
                 domain_crash(pdev->domain);
-            return rc;
+            goto fail;
         }
 
         rc = rangeset_add_range(mem, start_guest, end_guest);
@@ -329,13 +426,13 @@ static int 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 = header->bars[j].mem;
+            struct rangeset *prev_mem = task->bars[j].mem;
 
             if ( rangeset_is_empty(prev_mem) )
                 continue;
@@ -346,7 +443,7 @@ static int 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;
             }
         }
 
@@ -356,7 +453,7 @@ static int 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;
         }
     }
 
@@ -367,9 +464,9 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
         unsigned long end = PFN_DOWN(vmsix_table_addr(pdev->vpci, i) +
                                      vmsix_table_size(pdev->vpci, i) - 1);
 
-        for ( j = 0; j < ARRAY_SIZE(header->bars); j++ )
+        for ( j = 0; j < ARRAY_SIZE(task->bars); j++ )
         {
-            struct rangeset *mem = header->bars[j].mem;
+            struct rangeset *mem = task->bars[j].mem;
 
             if ( rangeset_is_empty(mem) )
                 continue;
@@ -380,7 +477,7 @@ static int 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;
             }
         }
     }
@@ -424,7 +521,7 @@ static int 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 = bar->mem;
+                    struct rangeset *mem = task->bars[j].mem;
 
                     if ( !rangeset_overlaps_range(mem, start, end) ||
                          /*
@@ -443,7 +540,7 @@ static int 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;
                     }
                 }
             }
@@ -472,12 +569,19 @@ static int 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(task);
+        destroy_map_task(task);
+        return rc;
     }
 
-    defer_map(pdev, cmd, rom_only);
+    defer_map(task);
 
     return 0;
+
+ fail:
+    destroy_map_task(task);
+
+    return rc;
 }
 
 static void cf_check cmd_write(
@@ -705,18 +809,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;
@@ -826,10 +918,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 )
@@ -882,12 +970,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 )
     {
diff --git a/xen/drivers/vpci/private.h b/xen/drivers/vpci/private.h
index 2907f6b40fce..52f26394745b 100644
--- a/xen/drivers/vpci/private.h
+++ b/xen/drivers/vpci/private.h
@@ -44,6 +44,7 @@ typedef struct {
 #define REGISTER_VPCI_EXTCAP(name, finit, fclean) \
     REGISTER_VPCI_CAPABILITY(PCI_EXT_CAP_ID_##name, name, finit, fclean, true)
 
+void vpci_clear_map_queue(struct vcpu *v, const struct pci_dev *pdev);
 int __must_check vpci_init_header(struct pci_dev *pdev);
 
 int vpci_init_capabilities(struct pci_dev *pdev, bool ext_only);
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 0ac9ec8b0475..226c7870dd6c 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -24,6 +24,17 @@
 
 #ifdef __XEN__
 
+void vpci_vcpu_destroy(struct vcpu *v)
+{
+    if ( v->vpci.task_queue.next )
+        vpci_clear_map_queue(v, NULL);
+}
+
+void vpci_vcpu_init(struct vcpu *v)
+{
+    INIT_LIST_HEAD(&v->vpci.task_queue);
+}
+
 #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
 static int assign_virtual_sbdf(struct pci_dev *pdev)
 {
@@ -89,7 +100,7 @@ struct vpci_register *vpci_get_register(const struct vpci *vpci,
 
 void vpci_deassign_device(struct pci_dev *pdev)
 {
-    unsigned int i;
+    struct vcpu *v;
 
     ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
 
@@ -116,8 +127,8 @@ 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);
+    for_each_vcpu ( pdev->domain, v )
+        vpci_clear_map_queue(v, pdev);
 
     xfree(pdev->vpci);
     pdev->vpci = NULL;
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index ab94eb60d65f..6bb09afd4eb0 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -19,6 +19,9 @@
  */
 #define VPCI_MAX_VIRT_DEV       (PCI_SLOT(~0) + 1)
 
+void vpci_vcpu_destroy(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);
 
@@ -54,7 +57,6 @@ struct vpci {
             uint64_t guest_addr;
             uint64_t size;
             uint64_t resizable_sizes;
-            struct rangeset *mem;
             enum {
                 VPCI_BAR_EMPTY,
                 VPCI_BAR_IO,
@@ -153,10 +155,8 @@ struct vpci {
 };
 
 struct vpci_vcpu {
-    /* Per-vcpu structure to store state while {un}mapping of PCI BARs. */
-    const struct pci_dev *pdev;
-    uint16_t cmd;
-    bool rom_only : 1;
+    /* List head with struct map_task elements */
+    struct list_head task_queue;
 };
 
 #ifdef __XEN__
@@ -204,6 +204,9 @@ bool vpci_ecam_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int len,
 #else /* !CONFIG_HAS_VPCI */
 struct vpci_vcpu {};
 
+static inline void vpci_vcpu_destroy(struct vcpu *v) { }
+static inline void vpci_vcpu_init(struct vcpu *v) { }
+
 static inline int vpci_reinit_ext_capabilities(struct pci_dev *pdev)
 {
     return 0;
-- 
2.54.0

[PATCH v5 9/9] vpci: don't pass command value to modify_bars()

[Stewart Hildebrand]

Now that writing the command register is no longer deferred, there's no
need to pass the cmd value to modify_bars(). Replace it with a bool to
indicate map/unmap.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v4->v5:
* adapted from: ("vpci: allow BAR map/unmap without affecting memory
  decoding bit")
* significant rework

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 | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 99ce67053068..187de3a12abb 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -324,7 +324,7 @@ static void defer_map(struct 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, bool map, bool rom_only)
 {
     struct vpci_header *header = &pdev->vpci->header;
     struct pci_dev *tmp;
@@ -337,7 +337,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 & PCI_COMMAND_MEMORY, rom_only);
+    task = alloc_map_task(pdev, map, rom_only);
     if ( !task )
         return -ENOMEM;
 
@@ -369,7 +369,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
              (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->enabled == map )
             continue;
 
         if ( !pci_check_bar(pdev, _mfn(start), _mfn(end)) )
@@ -409,8 +409,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
             goto fail;
         }
 
-        rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end,
-                                    !!(cmd & PCI_COMMAND_MEMORY));
+        rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end, map);
         if ( rc )
         {
             printk(XENLOG_G_WARNING
@@ -555,7 +554,7 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
     for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
     {
         if ( bar_valid[i] )
-            header->bars[i].enabled = cmd & PCI_COMMAND_MEMORY;
+            header->bars[i].enabled = map;
     }
 
     if ( system_state < SYS_STATE_active )
@@ -568,7 +567,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(task);
         destroy_map_task(task);
         return rc;
@@ -607,7 +606,7 @@ static void cf_check cmd_write(
      * handled above and by the rsvdp_mask.
      */
     if ( header->bars_mapped != new_enabled &&
-         modify_bars(pdev, cmd, false) )
+         modify_bars(pdev, new_enabled, false) )
         /*
          * Ignore the error. No memory has been added or removed from the p2m
          * (because the actual p2m changes are deferred in defer_map) and the
@@ -784,12 +783,8 @@ static void cf_check rom_write(
         rom->guest_addr = rom->addr;
     }
 
-    /*
-     * 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.
-     */
     if ( header->bars_mapped && rom->enabled != new_enabled &&
-         modify_bars(pdev, new_enabled ? PCI_COMMAND_MEMORY : 0, true) )
+         modify_bars(pdev, new_enabled, true) )
         /*
          * 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
@@ -988,7 +983,7 @@ int vpci_init_header(struct pci_dev *pdev)
         pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
     }
 
-    return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, cmd, false) : 0;
+    return (cmd & PCI_COMMAND_MEMORY) ? modify_bars(pdev, true, false) : 0;
 
  fail:
     pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
-- 
2.54.0

Re: [PATCH v5 1/9] vpci: move BAR mapping permissions checks

[Jan Beulich]

On 08.07.2026 23:02, Stewart Hildebrand wrote:
> There's no need to defer the permissions checks. Perform them right away
> in modify_bars().
> 
> As a result of the move, the permissions checks will now cover the whole
> BAR before any holes are removed.

Is this a good idea though? A really good toolstack would avoid granting
access to e.g. the MSI-X table.

> Carry over the domain_crash() in the error path for domUs.

I don't think this is warranted.

> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -58,24 +58,6 @@ static int cf_check map_range(
>           * offset of the current address from the BAR start.
>           */
>          unsigned long map_mfn = start_mfn + s - start_gfn;
> -        unsigned long m_end = map_mfn + size - 1;
> -
> -        if ( !iomem_access_permitted(map->d, map_mfn, m_end) )
> -        {
> -            printk(XENLOG_G_WARNING
> -                   "%pd denied access to MMIO range [%#lx, %#lx]\n",
> -                   map->d, map_mfn, m_end);
> -            return -EPERM;
> -        }
> -
> -        rc = xsm_iomem_mapping_vpci(XSM_HOOK, map->d, map_mfn, m_end, map->map);
> -        if ( rc )
> -        {
> -            printk(XENLOG_G_WARNING
> -                   "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
> -                   map->d, map_mfn, m_end, rc);

I realize you literally only take what's here, but ...

> @@ -369,6 +351,28 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>              return -EINVAL;
>          }
>  
> +        if ( !iomem_access_permitted(pdev->domain, start, end) )
> +        {
> +            printk(XENLOG_G_WARNING
> +                   "%pd denied access to MMIO range [%#lx, %#lx]\n",
> +                   pdev->domain, start, end);
> +            if ( !is_hardware_domain(pdev->domain) )
> +                domain_crash(pdev->domain);
> +            return -EPERM;
> +        }
> +
> +        rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end,
> +                                    !!(cmd & PCI_COMMAND_MEMORY));
> +        if ( rc )
> +        {
> +            printk(XENLOG_G_WARNING
> +                   "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
> +                   pdev->domain, start, end, rc);

... is the logging of rc actually useful? Afaict it's only ever going to be
-EPERM.

Jan

Re: [PATCH v5 2/9] vpci: make BAR mapping more resilient for the hardware domain

[Jan Beulich]

On 08.07.2026 23:02, Stewart Hildebrand wrote:
> From: Roger Pau Monne <roger.pau@citrix.com>
> 
> The logic in map_range() will bubble up failures to the upper layer, which
> will result in any remaining regions being skip, and for the non-hardware
> domain case the owner domain of the device would be destroyed.  However for
> the hardware domain the intent is to continue execution, hoping the
> failure to modify the p2m could be worked around by the hardware domain.
> 
> To accomplish that in a better way, ignore failures and skip the range in
> that case, possibly continuing to map further ranges.
> 
> Since the error path in vpci_process_pending() should only be used by domUs
> now, and it will unconditionally end up calling domain_crash(), simplify
> it: there's no need to cleanup if the domain will be destroyed.

Aren't we increasing the number of places adjustments will need making if
that (not really nice) crashing of the domain was sorted at some point?

> Memory decoding may be left enabled in case of mapping error for devices
> assigned to domUs.

I'm similarly concerned of this. I also can't spot why this would be for
DomU-s only. The original code did it for all domains.

> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -70,17 +70,26 @@ static int cf_check map_range(
>  
>          rc = map->map ? map_mmio_regions(map->d, _gfn(s), size, _mfn(map_mfn))
>                        : unmap_mmio_regions(map->d, _gfn(s), size, _mfn(map_mfn));
> -        if ( rc == 0 )
> -        {
> -            *c += size;
> -            break;
> -        }
>          if ( rc < 0 )
>          {
>              printk(XENLOG_G_WARNING
>                     "Failed to %smap [%lx %lx] -> [%lx %lx] for %pd: %d\n",
>                     map->map ? "" : "un", s, e, map_mfn,
>                     map_mfn + size, map->d, rc);
> +            goto done;
> +        }
> +        if ( rc == 0 )
> +        {
> + done:
> +            if ( is_hardware_domain(map->d) )
> +                /*
> +                 * Ignore failures for the hardware domain and skip the range.
> +                 * Do it as a best effort workaround to attempt to get the
> +                 * hardware domain to boot.
> +                 */
> +                rc = 0;

Why would this not move ahead of the "goto done"? For the if() here it's
a no-op. Then (really: independently) ...

> +            *c += size;
>              break;
>          }

... the remaining piece could be in "if ( rc <= 0 )".

Jan

Re: [PATCH v5 1/9] vpci: move BAR mapping permissions checks

[Roger Pau Monné]

On Thu, Jul 23, 2026 at 04:44:26PM +0200, Jan Beulich wrote:
> On 08.07.2026 23:02, Stewart Hildebrand wrote:
> > There's no need to defer the permissions checks. Perform them right away
> > in modify_bars().
> > 
> > As a result of the move, the permissions checks will now cover the whole
> > BAR before any holes are removed.
> 
> Is this a good idea though? A really good toolstack would avoid granting
> access to e.g. the MSI-X table.
> 
> > Carry over the domain_crash() in the error path for domUs.
> 
> I don't think this is warranted.
> 
> > --- a/xen/drivers/vpci/header.c
> > +++ b/xen/drivers/vpci/header.c
> > @@ -58,24 +58,6 @@ static int cf_check map_range(
> >           * offset of the current address from the BAR start.
> >           */
> >          unsigned long map_mfn = start_mfn + s - start_gfn;
> > -        unsigned long m_end = map_mfn + size - 1;
> > -
> > -        if ( !iomem_access_permitted(map->d, map_mfn, m_end) )
> > -        {
> > -            printk(XENLOG_G_WARNING
> > -                   "%pd denied access to MMIO range [%#lx, %#lx]\n",
> > -                   map->d, map_mfn, m_end);
> > -            return -EPERM;
> > -        }
> > -
> > -        rc = xsm_iomem_mapping_vpci(XSM_HOOK, map->d, map_mfn, m_end, map->map);
> > -        if ( rc )
> > -        {
> > -            printk(XENLOG_G_WARNING
> > -                   "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
> > -                   map->d, map_mfn, m_end, rc);
> 
> I realize you literally only take what's here, but ...
> 
> > @@ -369,6 +351,28 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
> >              return -EINVAL;
> >          }
> >  
> > +        if ( !iomem_access_permitted(pdev->domain, start, end) )
> > +        {
> > +            printk(XENLOG_G_WARNING
> > +                   "%pd denied access to MMIO range [%#lx, %#lx]\n",
> > +                   pdev->domain, start, end);
> > +            if ( !is_hardware_domain(pdev->domain) )
> > +                domain_crash(pdev->domain);
> > +            return -EPERM;
> > +        }
> > +
> > +        rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end,
> > +                                    !!(cmd & PCI_COMMAND_MEMORY));
> > +        if ( rc )
> > +        {
> > +            printk(XENLOG_G_WARNING
> > +                   "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
> > +                   pdev->domain, start, end, rc);
> 
> ... is the logging of rc actually useful? Afaict it's only ever going to be
> -EPERM.

I think Flask can also return -EACCES, not that it's very relevant I'm
afraid.  It's likely too late now, but those XSM checks should instead
return a bool to avoid this kind of confusion with the returned
error? (or whether diverse return codes are indeed expected).

Thanks, Roger.

Re: [PATCH v5 1/9] vpci: move BAR mapping permissions checks

[Jan Beulich]

On 23.07.2026 16:57, Roger Pau Monné wrote:
> On Thu, Jul 23, 2026 at 04:44:26PM +0200, Jan Beulich wrote:
>> On 08.07.2026 23:02, Stewart Hildebrand wrote:
>>> --- a/xen/drivers/vpci/header.c
>>> +++ b/xen/drivers/vpci/header.c
>>> @@ -58,24 +58,6 @@ static int cf_check map_range(
>>>           * offset of the current address from the BAR start.
>>>           */
>>>          unsigned long map_mfn = start_mfn + s - start_gfn;
>>> -        unsigned long m_end = map_mfn + size - 1;
>>> -
>>> -        if ( !iomem_access_permitted(map->d, map_mfn, m_end) )
>>> -        {
>>> -            printk(XENLOG_G_WARNING
>>> -                   "%pd denied access to MMIO range [%#lx, %#lx]\n",
>>> -                   map->d, map_mfn, m_end);
>>> -            return -EPERM;
>>> -        }
>>> -
>>> -        rc = xsm_iomem_mapping_vpci(XSM_HOOK, map->d, map_mfn, m_end, map->map);
>>> -        if ( rc )
>>> -        {
>>> -            printk(XENLOG_G_WARNING
>>> -                   "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
>>> -                   map->d, map_mfn, m_end, rc);
>>
>> I realize you literally only take what's here, but ...
>>
>>> @@ -369,6 +351,28 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>>>              return -EINVAL;
>>>          }
>>>  
>>> +        if ( !iomem_access_permitted(pdev->domain, start, end) )
>>> +        {
>>> +            printk(XENLOG_G_WARNING
>>> +                   "%pd denied access to MMIO range [%#lx, %#lx]\n",
>>> +                   pdev->domain, start, end);
>>> +            if ( !is_hardware_domain(pdev->domain) )
>>> +                domain_crash(pdev->domain);
>>> +            return -EPERM;
>>> +        }
>>> +
>>> +        rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end,
>>> +                                    !!(cmd & PCI_COMMAND_MEMORY));
>>> +        if ( rc )
>>> +        {
>>> +            printk(XENLOG_G_WARNING
>>> +                   "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
>>> +                   pdev->domain, start, end, rc);
>>
>> ... is the logging of rc actually useful? Afaict it's only ever going to be
>> -EPERM.
> 
> I think Flask can also return -EACCES, not that it's very relevant I'm
> afraid.

Hmm, looks like this can indeed happen. Please disregard this comment of
mine then.

>  It's likely too late now, but those XSM checks should instead
> return a bool to avoid this kind of confusion with the returned
> error? (or whether diverse return codes are indeed expected).

Depends on whether having distinct error codes is wanted. Daniel?

Jan

Re: [PATCH v5 3/9] vpci: only check BAR validity once

[Jan Beulich]

On 08.07.2026 23:02, Stewart Hildebrand wrote:
> From: Roger Pau Monne <roger.pau@citrix.com>
> 
> The BAR validity checks in modify_decoding() are redundant since they
> are already done in modify_bars().
> 
> Avoid multiple calls to pci_check_bar() for the same memory decoding
> related operation, as each call can possibly print a warning message
> about a BAR being in an invalid position.
> 
> Further, there's no need to defer setting bar->enabled until p2m
> operations are finished. It can safely be set in modify_bars() itself.
> This slightly changes the meaning of the bar->enabled flag: when true,
> it means a BAR is either mapped or queued to be mapped.

But are all consumers of the flag okay with this semantic change? What
would clear the flag if the mapping failed? It looks to me as if e.g.
msix_find()'s behavior would change then.

> @@ -296,6 +287,7 @@ static int 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;
> +    bool bar_valid[ARRAY_SIZE(header->bars)] = { false };

Just {} ?

Jan

Re: [PATCH v5 4/9] vpci/msix: move MSI-X hole punching as a result of memory decoding enable

[Jan Beulich]

On 08.07.2026 23:02, Stewart Hildebrand wrote:
> From: Roger Pau Monne <roger.pau@citrix.com>
> 
> Deferring the p2m unpopulate of the MSI-X table region to after the BARs are
> mapped in the p2m is not needed.  The aim of vpci_make_msix_hole() is to
> cope with the BAR containing the MSI-X table being positioned over a
> reserved e820 region, by making sure the MSI-X table region is
> unpopulated on the p2m. Note that reserved e820 regions are identity mapped
> in the hardware domain p2m by default.
> 
> Doing it before or after the BAR p2m changes are done is irrelevant, hence
> do it ahead of the p2m changes, as that's simpler.
> 
> Note that vpci_make_msix_hole() should be invoked strictly after setting
> bar->enabled. The movement of vpci_make_msix_hole() is OK since
> bar->enabled is set in modify_bars().
> 
> Take the opportunity to store the memory decoding enabled bit in a local
> variable.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> Note that this should not be committed until after the bar->enabled
> setting movement in ("xen/vpci: only check BAR validity once")
> 
> I tested this with hot-plug as described in 7c24d58a6e64 ("vpci/msix:
> check for BARs enabled in vpci_make_msix_hole").
> 
> The return at the end of the function will gain a purpose again after a
> later code movement patch in the series.
> 
> The new_enabled variable will gain 2 additional uses later in the series.
> 
> v1->v5:
> * order after ("xen/vpci: only check BAR validity once")
> * preserve behavior of only invoking vpci_make_msix_hole() when memory
>   decoding toggles from 0 to 1
> * add note about vpci_make_msix_hole() invocation after setting bar->enabled
> * use local variable
> * Jan offered an R-b tag in [1], though I hesitated to pick it up due to
>   changes, and it was offered prior to 7c24d58a6e64 going upstream
> [1] https://lore.kernel.org/xen-devel/e2c4944d-d089-490c-879d-b6baec5e4cae@suse.com/

If there wasn't ...

> @@ -556,6 +544,12 @@ static void cf_check cmd_write(
>          modify_bars(pdev, cmd, false);
>      else
>          pci_conf_write16(pdev->sbdf, reg, cmd);
> +
> +#ifdef CONFIG_HAS_PCI_MSI
> +    /* Unpopulate MSI-X table region, so accesses trap into Xen. */
> +    if ( !header->bars_mapped && new_enabled && vpci_make_msix_hole(pdev) )
> +        return;
> +#endif

... the new first part of the conditional, I'd re-offer R-b right away. With
that check added, aren't we at risk of leaving the range mapped when it
shouldn't be?

Jan

Re: [PATCH v5 1/9] vpci: move BAR mapping permissions checks

[Daniel P. Smith]

On 7/23/26 10:57 AM, Roger Pau Monné wrote:
> On Thu, Jul 23, 2026 at 04:44:26PM +0200, Jan Beulich wrote:
>> On 08.07.2026 23:02, Stewart Hildebrand wrote:
>>> There's no need to defer the permissions checks. Perform them right away
>>> in modify_bars().
>>>
>>> As a result of the move, the permissions checks will now cover the whole
>>> BAR before any holes are removed.
>>
>> Is this a good idea though? A really good toolstack would avoid granting
>> access to e.g. the MSI-X table.
>>
>>> Carry over the domain_crash() in the error path for domUs.
>>
>> I don't think this is warranted.
>>
>>> --- a/xen/drivers/vpci/header.c
>>> +++ b/xen/drivers/vpci/header.c
>>> @@ -58,24 +58,6 @@ static int cf_check map_range(
>>>            * offset of the current address from the BAR start.
>>>            */
>>>           unsigned long map_mfn = start_mfn + s - start_gfn;
>>> -        unsigned long m_end = map_mfn + size - 1;
>>> -
>>> -        if ( !iomem_access_permitted(map->d, map_mfn, m_end) )
>>> -        {
>>> -            printk(XENLOG_G_WARNING
>>> -                   "%pd denied access to MMIO range [%#lx, %#lx]\n",
>>> -                   map->d, map_mfn, m_end);
>>> -            return -EPERM;
>>> -        }
>>> -
>>> -        rc = xsm_iomem_mapping_vpci(XSM_HOOK, map->d, map_mfn, m_end, map->map);
>>> -        if ( rc )
>>> -        {
>>> -            printk(XENLOG_G_WARNING
>>> -                   "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
>>> -                   map->d, map_mfn, m_end, rc);
>>
>> I realize you literally only take what's here, but ...
>>
>>> @@ -369,6 +351,28 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>>>               return -EINVAL;
>>>           }
>>>   
>>> +        if ( !iomem_access_permitted(pdev->domain, start, end) )
>>> +        {
>>> +            printk(XENLOG_G_WARNING
>>> +                   "%pd denied access to MMIO range [%#lx, %#lx]\n",
>>> +                   pdev->domain, start, end);
>>> +            if ( !is_hardware_domain(pdev->domain) )
>>> +                domain_crash(pdev->domain);
>>> +            return -EPERM;
>>> +        }
>>> +
>>> +        rc = xsm_iomem_mapping_vpci(XSM_HOOK, pdev->domain, start, end,
>>> +                                    !!(cmd & PCI_COMMAND_MEMORY));
>>> +        if ( rc )
>>> +        {
>>> +            printk(XENLOG_G_WARNING
>>> +                   "%pd XSM denied access to MMIO range [%#lx, %#lx]: %d\n",
>>> +                   pdev->domain, start, end, rc);
>>
>> ... is the logging of rc actually useful? Afaict it's only ever going to be
>> -EPERM.
> 
> I think Flask can also return -EACCES, not that it's very relevant I'm
> afraid.  It's likely too late now, but those XSM checks should instead
> return a bool to avoid this kind of confusion with the returned
> error? (or whether diverse return codes are indeed expected).

An XSM access check may return -EACCES as one of any number of error 
codes depending on which XSM module is loaded. I haven't traced this 
specific path but a quick grep for the direct FLASK hooks shows EPERM, 
EINVAL, ENOMEM, and ENOENT directly in the hooks themselves. This does 
not even include the error codes that could come from the AVC. This is a 
consist interface provided by all XSM calls and fairly standard behavior 
across any number of respectable kernels such as Linux.

v/r,
dps