[PATCH v2] x86/vpci: fix handling of BAR overlaps with non-hole regions

[PATCH v2] x86/vpci: fix handling of BAR overlaps with non-hole regions

[Roger Pau Monne]

For once the message printed when a BAR overlaps with a non-hole regions is
not accurate on x86.  While the BAR won't be mapped by the vPCI logic, it
is quite likely overlapping with a reserved region in the memory map, and
already mapped as by default all reserved regions are identity mapped in
the p2m.  Fix the message so it just warns about the overlap, without
mentioning that the BAR won't be mapped, as this has caused confusion in
the past.

Secondly, when an overlap is detected the BAR 'enabled' field is not set,
hence other vPCI code that depends on it like vPCI MSI-X handling won't
function properly, as it sees the BAR as disabled, even when memory
decoding is enabled for the device and the BAR is likely mapped in the
p2m.  Change the handling of BARs that overlap non-hole regions to instead
remove any overlapped regions from the rangeset, so the resulting ranges to
map just contain the hole regions.  This requires introducing a new
pci_sanitize_bar_memory() that's implemented per-arch and sanitizes the
address range to add to the p2m.

For x86 pci_sanitize_bar_memory() removes any regions present in the host
memory map, for ARM this is currently left as a dummy handler to not change
existing behavior.

Ultimately the above changes should fix the vPCI MSI-X handlers not working
correctly when the BAR that contains the MSI-X table overlaps with a
non-hole region, as then the 'enabled' BAR bit won't be set and the MSI-X
traps won't handle accesses as expected.

Reported-by: Stefano Stabellini <stefano.stabellini@amd.com>
Fixes: 53d9133638c3 ('pci: do not disable memory decoding for devices')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Tested-by: Victor M Lira <victorm.lira@amd.com>
---
Changes since v1:
 - Use a forward declaration for struct rangeset instead of including the
   header.
---
 xen/arch/arm/include/asm/pci.h |  3 ++
 xen/arch/x86/include/asm/pci.h | 13 +++------
 xen/arch/x86/pci.c             | 50 ++++++++++++++++++++++++++++++++++
 xen/drivers/vpci/header.c      |  9 ++++++
 4 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 7f77226c9bbf..1605ec660d0b 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -128,6 +128,9 @@ int pci_host_bridge_mappings(struct domain *d);
 
 bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end);
 
+static inline int pci_sanitize_bar_memory(struct rangeset *r)
+{ return 0; }
+
 #else   /*!CONFIG_HAS_PCI*/
 
 struct pci_dev;
diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
index fd5480d67d43..bed99437cc3b 100644
--- a/xen/arch/x86/include/asm/pci.h
+++ b/xen/arch/x86/include/asm/pci.h
@@ -57,14 +57,9 @@ static always_inline bool is_pci_passthrough_enabled(void)
 
 void arch_pci_init_pdev(struct pci_dev *pdev);
 
-static inline bool pci_check_bar(const struct pci_dev *pdev,
-                                 mfn_t start, mfn_t end)
-{
-    /*
-     * Check if BAR is not overlapping with any memory region defined
-     * in the memory map.
-     */
-    return is_memory_hole(start, end);
-}
+bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end);
+
+struct rangeset;
+int pci_sanitize_bar_memory(struct rangeset *r);
 
 #endif /* __X86_PCI_H__ */
diff --git a/xen/arch/x86/pci.c b/xen/arch/x86/pci.c
index 97b792e578f1..afaf9fe1c053 100644
--- a/xen/arch/x86/pci.c
+++ b/xen/arch/x86/pci.c
@@ -98,3 +98,53 @@ int pci_conf_write_intercept(unsigned int seg, unsigned int bdf,
 
     return rc;
 }
+
+bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
+{
+    /*
+     * Check if BAR is not overlapping with any memory region defined
+     * in the memory map.
+     */
+    if ( !is_memory_hole(start, end) )
+        gdprintk(XENLOG_WARNING,
+                 "%pp: BAR at [%"PRI_mfn", %"PRI_mfn"] not in memory map hole\n",
+                 &pdev->sbdf, mfn_x(start), mfn_x(end));
+
+    /*
+     * Unconditionally return true, pci_sanitize_bar_memory() will remove any
+     * non-hole regions.
+     */
+    return true;
+}
+
+/* Remove overlaps with any ranges defined in the host memory map. */
+int pci_sanitize_bar_memory(struct rangeset *r)
+{
+    unsigned int i;
+
+    for ( i = 0; i < e820.nr_map; i++ )
+    {
+        const struct e820entry *entry = &e820.map[i];
+        int rc;
+
+        if ( !entry->size )
+            continue;
+
+        rc = rangeset_remove_range(r, PFN_DOWN(entry->addr),
+                                   PFN_UP(entry->addr + entry->size - 1));
+        if ( rc )
+            return rc;
+    }
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
index ef6c965c081c..1f48f2aac64e 100644
--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -394,6 +394,15 @@ static int modify_bars(const struct pci_dev *pdev, uint16_t cmd, bool rom_only)
                 return rc;
             }
         }
+
+        rc = pci_sanitize_bar_memory(bar->mem);
+        if ( rc )
+        {
+            gprintk(XENLOG_WARNING,
+                    "%pp: failed to sanitize BAR#%u memory: %d\n",
+                    &pdev->sbdf, i, rc);
+            return rc;
+        }
     }
 
     /* Remove any MSIX regions if present. */
-- 
2.48.1

Re: [PATCH v2] x86/vpci: fix handling of BAR overlaps with non-hole regions

[Jan Beulich]

On 16.05.2025 10:31, Roger Pau Monne wrote:
> For once the message printed when a BAR overlaps with a non-hole regions is
> not accurate on x86.  While the BAR won't be mapped by the vPCI logic, it
> is quite likely overlapping with a reserved region in the memory map, and
> already mapped as by default all reserved regions are identity mapped in
> the p2m.  Fix the message so it just warns about the overlap, without
> mentioning that the BAR won't be mapped, as this has caused confusion in
> the past.

I was trying to get back to this, but my attempt to use this "fixed
message" as an anchor failed: You don't modify any existing message, and
hence I was unable to determine which other message you refer to here.
None of the ones I looked at appear to fit this description.

Jan

Re: [PATCH v2] x86/vpci: fix handling of BAR overlaps with non-hole regions

[Roger Pau Monné]

On Wed, May 21, 2025 at 10:29:26AM +0200, Jan Beulich wrote:
> On 16.05.2025 10:31, Roger Pau Monne wrote:
> > For once the message printed when a BAR overlaps with a non-hole regions is
> > not accurate on x86.  While the BAR won't be mapped by the vPCI logic, it
> > is quite likely overlapping with a reserved region in the memory map, and
> > already mapped as by default all reserved regions are identity mapped in
> > the p2m.  Fix the message so it just warns about the overlap, without
> > mentioning that the BAR won't be mapped, as this has caused confusion in
> > the past.
> 
> I was trying to get back to this, but my attempt to use this "fixed
> message" as an anchor failed: You don't modify any existing message, and
> hence I was unable to determine which other message you refer to here.
> None of the ones I looked at appear to fit this description.

OK, it's a bit tricky.  Note how the new implementation of
pci_check_bar() unconditionally returns true, which means the message
in modify_bars() will never be printed on x86.  Instead a slightly
different warning message is printed in the x86 implementation of
pci_check_bar().

That's what the above paragraph attempts to explain.

Maybe I need to adjust the last sentence so it's:

"Avoiding printing the warning message in modify_bars(), and instead
print a more lax message in the x86 implementation of pci_check_bar()
to note the current BAR position overlaps with non-hole region(s)."

Does the above make it a bit clearer?

Thanks, Roger.

Re: [PATCH v2] x86/vpci: fix handling of BAR overlaps with non-hole regions

[Jan Beulich]

On 21.05.2025 13:34, Roger Pau Monné wrote:
> On Wed, May 21, 2025 at 10:29:26AM +0200, Jan Beulich wrote:
>> On 16.05.2025 10:31, Roger Pau Monne wrote:
>>> For once the message printed when a BAR overlaps with a non-hole regions is
>>> not accurate on x86.  While the BAR won't be mapped by the vPCI logic, it
>>> is quite likely overlapping with a reserved region in the memory map, and
>>> already mapped as by default all reserved regions are identity mapped in
>>> the p2m.  Fix the message so it just warns about the overlap, without
>>> mentioning that the BAR won't be mapped, as this has caused confusion in
>>> the past.
>>
>> I was trying to get back to this, but my attempt to use this "fixed
>> message" as an anchor failed: You don't modify any existing message, and
>> hence I was unable to determine which other message you refer to here.
>> None of the ones I looked at appear to fit this description.
> 
> OK, it's a bit tricky.  Note how the new implementation of
> pci_check_bar() unconditionally returns true, which means the message
> in modify_bars() will never be printed on x86.  Instead a slightly
> different warning message is printed in the x86 implementation of
> pci_check_bar().
> 
> That's what the above paragraph attempts to explain.
> 
> Maybe I need to adjust the last sentence so it's:
> 
> "Avoiding printing the warning message in modify_bars(), and instead
> print a more lax message in the x86 implementation of pci_check_bar()
> to note the current BAR position overlaps with non-hole region(s)."
> 
> Does the above make it a bit clearer?

Yes, it definitely does. And with use of that I'm now also feeling reasonably
confident to offer
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v2] x86/vpci: fix handling of BAR overlaps with non-hole regions

[Roger Pau Monné]

On Wed, May 21, 2025 at 04:31:33PM +0200, Jan Beulich wrote:
> On 21.05.2025 13:34, Roger Pau Monné wrote:
> > On Wed, May 21, 2025 at 10:29:26AM +0200, Jan Beulich wrote:
> >> On 16.05.2025 10:31, Roger Pau Monne wrote:
> >>> For once the message printed when a BAR overlaps with a non-hole regions is
> >>> not accurate on x86.  While the BAR won't be mapped by the vPCI logic, it
> >>> is quite likely overlapping with a reserved region in the memory map, and
> >>> already mapped as by default all reserved regions are identity mapped in
> >>> the p2m.  Fix the message so it just warns about the overlap, without
> >>> mentioning that the BAR won't be mapped, as this has caused confusion in
> >>> the past.
> >>
> >> I was trying to get back to this, but my attempt to use this "fixed
> >> message" as an anchor failed: You don't modify any existing message, and
> >> hence I was unable to determine which other message you refer to here.
> >> None of the ones I looked at appear to fit this description.
> > 
> > OK, it's a bit tricky.  Note how the new implementation of
> > pci_check_bar() unconditionally returns true, which means the message
> > in modify_bars() will never be printed on x86.  Instead a slightly
> > different warning message is printed in the x86 implementation of
> > pci_check_bar().
> > 
> > That's what the above paragraph attempts to explain.
> > 
> > Maybe I need to adjust the last sentence so it's:
> > 
> > "Avoiding printing the warning message in modify_bars(), and instead
> > print a more lax message in the x86 implementation of pci_check_bar()
> > to note the current BAR position overlaps with non-hole region(s)."
> > 
> > Does the above make it a bit clearer?
> 
> Yes, it definitely does. And with use of that I'm now also feeling reasonably
> confident to offer
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks, sorry this was a bit dense.

Roger.

Re: [PATCH v2] x86/vpci: fix handling of BAR overlaps with non-hole regions

[Julien Grall]

Hi Roger,

On 16/05/2025 09:31, Roger Pau Monne wrote:
> For once the message printed when a BAR overlaps with a non-hole regions is
> not accurate on x86.  While the BAR won't be mapped by the vPCI logic, it
> is quite likely overlapping with a reserved region in the memory map, and
> already mapped as by default all reserved regions are identity mapped in
> the p2m.  Fix the message so it just warns about the overlap, without
> mentioning that the BAR won't be mapped, as this has caused confusion in
> the past.
> 
> Secondly, when an overlap is detected the BAR 'enabled' field is not set,
> hence other vPCI code that depends on it like vPCI MSI-X handling won't
> function properly, as it sees the BAR as disabled, even when memory
> decoding is enabled for the device and the BAR is likely mapped in the
> p2m.  Change the handling of BARs that overlap non-hole regions to instead
> remove any overlapped regions from the rangeset, so the resulting ranges to
> map just contain the hole regions.  This requires introducing a new
> pci_sanitize_bar_memory() that's implemented per-arch and sanitizes the
> address range to add to the p2m.
> 
> For x86 pci_sanitize_bar_memory() removes any regions present in the host
> memory map, for ARM this is currently left as a dummy handler to not change
> existing behavior.
> 
> Ultimately the above changes should fix the vPCI MSI-X handlers not working
> correctly when the BAR that contains the MSI-X table overlaps with a
> non-hole region, as then the 'enabled' BAR bit won't be set and the MSI-X
> traps won't handle accesses as expected.
> 
> Reported-by: Stefano Stabellini <stefano.stabellini@amd.com>
> Fixes: 53d9133638c3 ('pci: do not disable memory decoding for devices')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Tested-by: Victor M Lira <victorm.lira@amd.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

[regression] Re: [PATCH v2] x86/vpci: fix handling of BAR overlaps with non-hole regions

[Andrew Cooper]

On 16/05/2025 9:31 am, Roger Pau Monne wrote:
> diff --git a/xen/arch/x86/pci.c b/xen/arch/x86/pci.c
> index 97b792e578f1..afaf9fe1c053 100644
> --- a/xen/arch/x86/pci.c
> +++ b/xen/arch/x86/pci.c
> @@ -98,3 +98,53 @@ int pci_conf_write_intercept(unsigned int seg, unsigned int bdf,
>  
>      return rc;
>  }
> +
> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
> +{
> +    /*
> +     * Check if BAR is not overlapping with any memory region defined
> +     * in the memory map.
> +     */
> +    if ( !is_memory_hole(start, end) )
> +        gdprintk(XENLOG_WARNING,
> +                 "%pp: BAR at [%"PRI_mfn", %"PRI_mfn"] not in memory map hole\n",
> +                 &pdev->sbdf, mfn_x(start), mfn_x(end));


This is causing substantial logspam now.  From adl-* in CI,

(XEN) [    4.131847] PCI add device 0000:00:14.0
[    0.227775] pci 0000:00:14.2: [8086:7aa7] type 00 class 0x050000
(XEN) [    4.131987] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.131996] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132013] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132015] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132062] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132070] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132084] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132086] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
[    0.227942] pci 0000:00:14.2: reg 0x10: [mem 0x00000000-0x00003fff 64bit]
(XEN) [    4.132130] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132138] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132153] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132155] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132202] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132210] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132224] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132226] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
[    0.228082] pci 0000:00:14.2: reg 0x18: [mem 0x00000000-0x00000fff 64bit]
(XEN) [    4.132270] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132277] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132291] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132294] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132322] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132330] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132343] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132346] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132360] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132368] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132382] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132384] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132412] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132420] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132433] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132436] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132451] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132458] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132474] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132476] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132528] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132536] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132550] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00003] not in memory map hole
(XEN) [    4.132552] arch/x86/pci.c:109:d0v0 0000:00:14.2: BAR at [00000, 00000] not in memory map hole
(XEN) [    4.132729] PCI add device 0000:00:14.2

It really needs to become less verbose.

~Andrew