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

Store whether the BAR is validly positioned in modify_bars(), and used that
cached value for the whole operation.  This allows to get rid of
modify_decoding(), as the setting of whether the BAR is enabled or not can
be easily done in vpci_process_pending() itself, without the need for an
external helper.

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>
---
 xen/drivers/vpci/header.c | 69 ++++++++++-----------------------------
 xen/include/xen/vpci.h    |  5 +++
 2 files changed, 23 insertions(+), 51 deletions(-)

diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index 255c6d54b406..2071bed81676 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -121,46 +121,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);
-
-            if ( pci_check_bar(pdev, _mfn(PFN_DOWN(bar->addr)),
-                               _mfn(PFN_DOWN(bar->addr + bar->size - 1))) )
-                bar->enabled = map;
-            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;
-    }
-}
-
 bool vpci_process_pending(struct vcpu *v)
 {
     const struct pci_dev *pdev = v->vpci.pdev;
@@ -190,8 +150,10 @@ bool vpci_process_pending(struct vcpu *v)
         };
         int rc;
 
+        ASSERT(bar->valid || rangeset_is_empty(bar->mem));
+
         if ( rangeset_is_empty(bar->mem) )
-            continue;
+            goto next;
 
         rc = rangeset_consume_ranges(bar->mem, map_range, &data);
 
@@ -217,13 +179,13 @@ bool vpci_process_pending(struct vcpu *v)
          * rangeset_consume_ranges() itself doesn't generate any errors.
          */
         rangeset_purge(bar->mem);
+
+ next:
+        if ( bar->valid )
+            bar->enabled = v->vpci.cmd & PCI_COMMAND_MEMORY;
     }
     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;
@@ -243,10 +205,8 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
         struct vpci_bar *bar = &header->bars[i];
         struct map_data data = { .d = d, .map = true, .bar = bar };
 
-        if ( rangeset_is_empty(bar->mem) )
-            continue;
-
-        while ( (rc = rangeset_consume_ranges(bar->mem, map_range,
+        while ( bar->mem &&
+                (rc = rangeset_consume_ranges(bar->mem, map_range,
                                               &data)) == -ERESTART )
         {
             /*
@@ -258,9 +218,10 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
             process_pending_softirqs();
             write_lock(&d->pci_lock);
         }
+
+        if ( bar->valid )
+            bar->enabled = true;
     }
-    if ( !rc )
-        modify_decoding(pdev, cmd, false);
 
     return rc;
 }
@@ -326,6 +287,9 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
          */
         rangeset_purge(bar->mem);
 
+        /* Reset whether the BAR is valid, will be checked below. */
+        bar->valid = false;
+
         if ( !MAPPABLE_BAR(bar) ||
              (rom_only ? bar->type != VPCI_BAR_ROM
                        : (bar->type == VPCI_BAR_ROM && !header->rom_enabled)) ||
@@ -341,6 +305,8 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
             continue;
         }
 
+        bar->valid = 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
@@ -539,6 +505,7 @@ static void cf_check cmd_write(
     if ( (cmd & PCI_COMMAND_MEMORY) && 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
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 17cfecb0aabf..6bdbbb842f58 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -128,6 +128,11 @@ struct vpci {
             bool prefetchable : 1;
             /* Store whether the BAR is mapped into guest p2m. */
             bool enabled      : 1;
+            /*
+             * Is the BAR position valid?  Used to store intermediate state
+             * before BAR modifications are applied to the p2m.
+             */
+            bool valid        : 1;
         } bars[PCI_HEADER_NORMAL_NR_BARS + 1];
         /* At most 6 BARS + 1 expansion ROM BAR. */
 
-- 
2.49.0

On 14.08.2025 18:03, Roger Pau Monne wrote:
> Avoid multiple calls to pci_check_bar() for the same memory decoding
> related operation, as each call can possibly print a warning message avoid
> a BAR being in an invalid position.

s/avoid/about/ ?

> @@ -217,13 +179,13 @@ bool vpci_process_pending(struct vcpu *v)
>           * rangeset_consume_ranges() itself doesn't generate any errors.
>           */
>          rangeset_purge(bar->mem);
> +
> + next:
> +        if ( bar->valid )
> +            bar->enabled = v->vpci.cmd & PCI_COMMAND_MEMORY;

Isn't it at least latently risky to possibly leave ->enabled set to true
when ->valid is false?

> @@ -243,10 +205,8 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
>          struct vpci_bar *bar = &header->bars[i];
>          struct map_data data = { .d = d, .map = true, .bar = bar };
>  
> -        if ( rangeset_is_empty(bar->mem) )
> -            continue;
> -
> -        while ( (rc = rangeset_consume_ranges(bar->mem, map_range,
> +        while ( bar->mem &&
> +                (rc = rangeset_consume_ranges(bar->mem, map_range,
>                                                &data)) == -ERESTART )
>          {
>              /*
> @@ -258,9 +218,10 @@ static int __init apply_map(struct domain *d, const struct pci_dev *pdev,
>              process_pending_softirqs();
>              write_lock(&d->pci_lock);
>          }
> +
> +        if ( bar->valid )
> +            bar->enabled = true;
>      }
> -    if ( !rc )
> -        modify_decoding(pdev, cmd, false);
>  
>      return rc;
>  }
> @@ -326,6 +287,9 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>           */
>          rangeset_purge(bar->mem);
>  
> +        /* Reset whether the BAR is valid, will be checked below. */
> +        bar->valid = false;

Just that I can't spot any check further down. It's only ...

> @@ -341,6 +305,8 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
>              continue;
>          }
>  
> +        bar->valid = true;

... this setting to true.

> @@ -539,6 +505,7 @@ static void cf_check cmd_write(
>      if ( (cmd & PCI_COMMAND_MEMORY) && 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

This change was probably meant to go into an earlier patch?

Jan