[PATCH for-4.17] x86/pci: allow BARs to be positioned on e820 reserved regions

[PATCH for-4.17] x86/pci: allow BARs to be positioned on e820 reserved regions

[Roger Pau Monne]

The EFI memory map contains two memory types (EfiMemoryMappedIO and
EfiMemoryMappedIOPortSpace) used to describe IO memory areas used by
EFI firmware.

The current parsing of the EFI memory map is translating
EfiMemoryMappedIO to E820_RESERVED on x86.  This causes issues on some
boxes as the firmware is relying on using those regions to position
the BARs of devices being used (possibly during runtime) for the
firmware.

Xen will disable memory decoding on any device that has BARs
positioned over any regions on the e820 memory map, hence the firmware
will malfunction after Xen turning off memory decoding for the
device(s) that have BARs mapped in EfiMemoryMappedIO regions.

The system under which this was observed has:

EFI memory map:
[...]
 00000fd000000-00000fe7fffff type=11 attr=800000000000100d
[...]
0000:00:1f.5 disabled: BAR [0xfe010, 0xfe010] overlaps with memory map

The device behind this BAR is:

00:1f.5 Serial bus controller [0c80]: Intel Corporation Lewisburg SPI Controller (rev 09)
	Subsystem: Super Micro Computer Inc Device 091c
	Flags: fast devsel
	Memory at fe010000 (32-bit, non-prefetchable) [size=4K]well

For the record, the symptom observed in that machine was a hard freeze
when attempting to set an EFI variable (XEN_EFI_set_variable).

Fix by allowing BARs of PCI devices to be positioned over reserved
memory regions, but print a warning message about such overlap.

Fixes: 75cc460a1b ('xen/pci: detect when BARs are not suitably positioned')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/arm/include/asm/pci.h |  2 --
 xen/arch/x86/include/asm/pci.h | 10 ----------
 xen/arch/x86/pci.c             | 27 +++++++++++++++++++++++++++
 xen/include/xen/pci.h          |  1 +
 4 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 8cb46f6b71..80a2431804 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -126,8 +126,6 @@ int pci_host_iterate_bridges_and_count(struct domain *d,
 
 int pci_host_bridge_mappings(struct domain *d);
 
-bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end);
-
 #else   /*!CONFIG_HAS_PCI*/
 
 struct arch_pci_dev { };
diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
index f4a58c8acf..c8e1a9ecdb 100644
--- a/xen/arch/x86/include/asm/pci.h
+++ b/xen/arch/x86/include/asm/pci.h
@@ -57,14 +57,4 @@ 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);
-}
-
 #endif /* __X86_PCI_H__ */
diff --git a/xen/arch/x86/pci.c b/xen/arch/x86/pci.c
index 97b792e578..6920bf2168 100644
--- a/xen/arch/x86/pci.c
+++ b/xen/arch/x86/pci.c
@@ -98,3 +98,30 @@ 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)
+{
+    unsigned long mfn;
+
+    /*
+     * Check if BAR is not overlapping with any memory region defined
+     * in the memory map.
+     */
+    if ( is_memory_hole(start, end) )
+        return true;
+
+    /*
+     * Also allow BARs placed on reserved regions in order to deal with EFI
+     * firmware using EfiMemoryMappedIO regions to place the BARs of devices
+     * that can be used during runtime.  But print a warning when doing so.
+     */
+    for ( mfn = mfn_x(start); mfn <= mfn_x(end); mfn++ )
+        if ( !page_is_ram_type(mfn, RAM_TYPE_RESERVED) )
+            return false;
+
+    printk(XENLOG_WARNING
+           "%pp: BAR [%#" PRI_mfn ", %#" PRI_mfn "] overlaps reserved region\n",
+           &pdev->sbdf, mfn_x(start), mfn_x(end));
+
+    return true;
+}
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 5975ca2f30..64995fc68d 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -211,6 +211,7 @@ unsigned int pci_size_mem_bar(pci_sbdf_t sbdf, unsigned int pos,
 
 void pci_intx(const struct pci_dev *, bool enable);
 bool_t pcie_aer_get_firmware_first(const struct pci_dev *);
+bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end);
 
 struct pirq;
 int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t gtable);
-- 
2.37.3

Re: [PATCH for-4.17] x86/pci: allow BARs to be positioned on e820 reserved regions

[Jan Beulich]

On 04.10.2022 17:36, Roger Pau Monne wrote:
> The EFI memory map contains two memory types (EfiMemoryMappedIO and
> EfiMemoryMappedIOPortSpace) used to describe IO memory areas used by
> EFI firmware.
> 
> The current parsing of the EFI memory map is translating
> EfiMemoryMappedIO to E820_RESERVED on x86.  This causes issues on some
> boxes as the firmware is relying on using those regions to position
> the BARs of devices being used (possibly during runtime) for the
> firmware.
> 
> Xen will disable memory decoding on any device that has BARs
> positioned over any regions on the e820 memory map, hence the firmware
> will malfunction after Xen turning off memory decoding for the
> device(s) that have BARs mapped in EfiMemoryMappedIO regions.
> 
> The system under which this was observed has:
> 
> EFI memory map:
> [...]
>  00000fd000000-00000fe7fffff type=11 attr=800000000000100d
> [...]
> 0000:00:1f.5 disabled: BAR [0xfe010, 0xfe010] overlaps with memory map
> 
> The device behind this BAR is:
> 
> 00:1f.5 Serial bus controller [0c80]: Intel Corporation Lewisburg SPI Controller (rev 09)
> 	Subsystem: Super Micro Computer Inc Device 091c
> 	Flags: fast devsel
> 	Memory at fe010000 (32-bit, non-prefetchable) [size=4K]well
> 
> For the record, the symptom observed in that machine was a hard freeze
> when attempting to set an EFI variable (XEN_EFI_set_variable).
> 
> Fix by allowing BARs of PCI devices to be positioned over reserved
> memory regions, but print a warning message about such overlap.

Somewhat hesitantly I might ack this change, but I'd like to give
others (Andrew in particular) some time to voice their views. As
said during the earlier discussion - I think we're relaxing things
too much by going this route.

> --- a/xen/arch/x86/pci.c
> +++ b/xen/arch/x86/pci.c
> @@ -98,3 +98,30 @@ 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)
> +{
> +    unsigned long mfn;
> +
> +    /*
> +     * Check if BAR is not overlapping with any memory region defined
> +     * in the memory map.
> +     */
> +    if ( is_memory_hole(start, end) )
> +        return true;
> +
> +    /*
> +     * Also allow BARs placed on reserved regions in order to deal with EFI
> +     * firmware using EfiMemoryMappedIO regions to place the BARs of devices
> +     * that can be used during runtime.  But print a warning when doing so.
> +     */
> +    for ( mfn = mfn_x(start); mfn <= mfn_x(end); mfn++ )
> +        if ( !page_is_ram_type(mfn, RAM_TYPE_RESERVED) )
> +            return false;

We don't need to be arch-independent here and hence instead of this
(potentially long) loop can't we use a single call to e820_all_mapped()?

Jan

Re: [PATCH for-4.17] x86/pci: allow BARs to be positioned on e820 reserved regions

[Roger Pau Monné]

On Tue, Oct 04, 2022 at 06:11:50PM +0200, Jan Beulich wrote:
> On 04.10.2022 17:36, Roger Pau Monne wrote:
> > The EFI memory map contains two memory types (EfiMemoryMappedIO and
> > EfiMemoryMappedIOPortSpace) used to describe IO memory areas used by
> > EFI firmware.
> > 
> > The current parsing of the EFI memory map is translating
> > EfiMemoryMappedIO to E820_RESERVED on x86.  This causes issues on some
> > boxes as the firmware is relying on using those regions to position
> > the BARs of devices being used (possibly during runtime) for the
> > firmware.
> > 
> > Xen will disable memory decoding on any device that has BARs
> > positioned over any regions on the e820 memory map, hence the firmware
> > will malfunction after Xen turning off memory decoding for the
> > device(s) that have BARs mapped in EfiMemoryMappedIO regions.
> > 
> > The system under which this was observed has:
> > 
> > EFI memory map:
> > [...]
> >  00000fd000000-00000fe7fffff type=11 attr=800000000000100d
> > [...]
> > 0000:00:1f.5 disabled: BAR [0xfe010, 0xfe010] overlaps with memory map
> > 
> > The device behind this BAR is:
> > 
> > 00:1f.5 Serial bus controller [0c80]: Intel Corporation Lewisburg SPI Controller (rev 09)
> > 	Subsystem: Super Micro Computer Inc Device 091c
> > 	Flags: fast devsel
> > 	Memory at fe010000 (32-bit, non-prefetchable) [size=4K]well
> > 
> > For the record, the symptom observed in that machine was a hard freeze
> > when attempting to set an EFI variable (XEN_EFI_set_variable).
> > 
> > Fix by allowing BARs of PCI devices to be positioned over reserved
> > memory regions, but print a warning message about such overlap.
> 
> Somewhat hesitantly I might ack this change, but I'd like to give
> others (Andrew in particular) some time to voice their views. As
> said during the earlier discussion - I think we're relaxing things
> too much by going this route.

Another option would be to explicitly check in efi_memmap for
EfiMemoryMappedIO regions and BAR overlap and only allow those.  That
would be more cumbersome code wise AFAICT.

> > --- a/xen/arch/x86/pci.c
> > +++ b/xen/arch/x86/pci.c
> > @@ -98,3 +98,30 @@ 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)
> > +{
> > +    unsigned long mfn;
> > +
> > +    /*
> > +     * Check if BAR is not overlapping with any memory region defined
> > +     * in the memory map.
> > +     */
> > +    if ( is_memory_hole(start, end) )
> > +        return true;
> > +
> > +    /*
> > +     * Also allow BARs placed on reserved regions in order to deal with EFI
> > +     * firmware using EfiMemoryMappedIO regions to place the BARs of devices
> > +     * that can be used during runtime.  But print a warning when doing so.
> > +     */
> > +    for ( mfn = mfn_x(start); mfn <= mfn_x(end); mfn++ )
> > +        if ( !page_is_ram_type(mfn, RAM_TYPE_RESERVED) )
> > +            return false;
> 
> We don't need to be arch-independent here and hence instead of this
> (potentially long) loop can't we use a single call to e820_all_mapped()?

Sure, was searching for a range checking functions but wrongly looked
into mm.c instead of e820.  I would have to make the function
non-init, but I think that's fine.

Thanks, Roger.

Re: [PATCH for-4.17] x86/pci: allow BARs to be positioned on e820 reserved regions

[Jan Beulich]

On 05.10.2022 10:37, Roger Pau Monné wrote:
> On Tue, Oct 04, 2022 at 06:11:50PM +0200, Jan Beulich wrote:
>> On 04.10.2022 17:36, Roger Pau Monne wrote:
>>> The EFI memory map contains two memory types (EfiMemoryMappedIO and
>>> EfiMemoryMappedIOPortSpace) used to describe IO memory areas used by
>>> EFI firmware.
>>>
>>> The current parsing of the EFI memory map is translating
>>> EfiMemoryMappedIO to E820_RESERVED on x86.  This causes issues on some
>>> boxes as the firmware is relying on using those regions to position
>>> the BARs of devices being used (possibly during runtime) for the
>>> firmware.
>>>
>>> Xen will disable memory decoding on any device that has BARs
>>> positioned over any regions on the e820 memory map, hence the firmware
>>> will malfunction after Xen turning off memory decoding for the
>>> device(s) that have BARs mapped in EfiMemoryMappedIO regions.
>>>
>>> The system under which this was observed has:
>>>
>>> EFI memory map:
>>> [...]
>>>  00000fd000000-00000fe7fffff type=11 attr=800000000000100d
>>> [...]
>>> 0000:00:1f.5 disabled: BAR [0xfe010, 0xfe010] overlaps with memory map
>>>
>>> The device behind this BAR is:
>>>
>>> 00:1f.5 Serial bus controller [0c80]: Intel Corporation Lewisburg SPI Controller (rev 09)
>>> 	Subsystem: Super Micro Computer Inc Device 091c
>>> 	Flags: fast devsel
>>> 	Memory at fe010000 (32-bit, non-prefetchable) [size=4K]well
>>>
>>> For the record, the symptom observed in that machine was a hard freeze
>>> when attempting to set an EFI variable (XEN_EFI_set_variable).
>>>
>>> Fix by allowing BARs of PCI devices to be positioned over reserved
>>> memory regions, but print a warning message about such overlap.
>>
>> Somewhat hesitantly I might ack this change, but I'd like to give
>> others (Andrew in particular) some time to voice their views. As
>> said during the earlier discussion - I think we're relaxing things
>> too much by going this route.
> 
> Another option would be to explicitly check in efi_memmap for
> EfiMemoryMappedIO regions and BAR overlap and only allow those.  That
> would be more cumbersome code wise AFAICT.

Indeed there's a question of balancing well here, between two outcomes
neither of which is really desirable.

Jan

Re: [PATCH for-4.17] x86/pci: allow BARs to be positioned on e820 reserved regions

[Roger Pau Monné]

On Wed, Oct 05, 2022 at 10:53:38AM +0200, Jan Beulich wrote:
> On 05.10.2022 10:37, Roger Pau Monné wrote:
> > On Tue, Oct 04, 2022 at 06:11:50PM +0200, Jan Beulich wrote:
> >> On 04.10.2022 17:36, Roger Pau Monne wrote:
> >>> The EFI memory map contains two memory types (EfiMemoryMappedIO and
> >>> EfiMemoryMappedIOPortSpace) used to describe IO memory areas used by
> >>> EFI firmware.
> >>>
> >>> The current parsing of the EFI memory map is translating
> >>> EfiMemoryMappedIO to E820_RESERVED on x86.  This causes issues on some
> >>> boxes as the firmware is relying on using those regions to position
> >>> the BARs of devices being used (possibly during runtime) for the
> >>> firmware.
> >>>
> >>> Xen will disable memory decoding on any device that has BARs
> >>> positioned over any regions on the e820 memory map, hence the firmware
> >>> will malfunction after Xen turning off memory decoding for the
> >>> device(s) that have BARs mapped in EfiMemoryMappedIO regions.
> >>>
> >>> The system under which this was observed has:
> >>>
> >>> EFI memory map:
> >>> [...]
> >>>  00000fd000000-00000fe7fffff type=11 attr=800000000000100d
> >>> [...]
> >>> 0000:00:1f.5 disabled: BAR [0xfe010, 0xfe010] overlaps with memory map
> >>>
> >>> The device behind this BAR is:
> >>>
> >>> 00:1f.5 Serial bus controller [0c80]: Intel Corporation Lewisburg SPI Controller (rev 09)
> >>> 	Subsystem: Super Micro Computer Inc Device 091c
> >>> 	Flags: fast devsel
> >>> 	Memory at fe010000 (32-bit, non-prefetchable) [size=4K]well
> >>>
> >>> For the record, the symptom observed in that machine was a hard freeze
> >>> when attempting to set an EFI variable (XEN_EFI_set_variable).
> >>>
> >>> Fix by allowing BARs of PCI devices to be positioned over reserved
> >>> memory regions, but print a warning message about such overlap.
> >>
> >> Somewhat hesitantly I might ack this change, but I'd like to give
> >> others (Andrew in particular) some time to voice their views. As
> >> said during the earlier discussion - I think we're relaxing things
> >> too much by going this route.
> > 
> > Another option would be to explicitly check in efi_memmap for
> > EfiMemoryMappedIO regions and BAR overlap and only allow those.  That
> > would be more cumbersome code wise AFAICT.
> 
> Indeed there's a question of balancing well here, between two outcomes
> neither of which is really desirable.

Hm, I have the following diff attached below which is more limited,
and not so bad I think.  Initially I wanted to introduce an
efi_all_mapped() helper, but that would require exposing EFI_MEMORY_TYPE
which is quite intrusive.

Let me know if you think the proposal below is better and I will
formally send a patch with it.

Thanks, Roger.
---
diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h
index f4a58c8acf..c8e1a9ecdb 100644
--- a/xen/arch/x86/include/asm/pci.h
+++ b/xen/arch/x86/include/asm/pci.h
@@ -57,14 +57,4 @@ 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);
-}
-
 #endif /* __X86_PCI_H__ */
diff --git a/xen/arch/x86/pci.c b/xen/arch/x86/pci.c
index 97b792e578..c3737e226d 100644
--- a/xen/arch/x86/pci.c
+++ b/xen/arch/x86/pci.c
@@ -4,6 +4,7 @@
  * Architecture-dependent PCI access functions.
  */
 
+#include <xen/efi.h>
 #include <xen/spinlock.h>
 #include <xen/pci.h>
 #include <asm/io.h>
@@ -98,3 +99,28 @@ 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) )
+        return true;
+
+    /*
+     * Also allow BARs placed on EfiMemoryMappedIO regions in order to deal
+     * with EFI firmware using those regions to place the BARs of devices that
+     * can be used during runtime.  But print a warning when doing so.
+     */
+    if ( !efi_all_runtime_mmio(mfn_to_maddr(start),
+                               mfn_to_maddr(mfn_add(end, 1))) )
+        return false;
+
+    printk(XENLOG_WARNING
+           "%pp: BAR [%#" PRI_mfn ", %#" PRI_mfn "] overlaps reserved region\n",
+           &pdev->sbdf, mfn_x(start), mfn_x(end));
+
+    return true;
+}
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 13b0975866..b69c710ce3 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -78,6 +78,30 @@ bool efi_enabled(unsigned int feature)
     return test_bit(feature, &efi_flags);
 }
 
+/*
+ * This function checks if the entire range [start,end) is contained inside of
+ * a single EfiMemoryMappedIO descriptor with the runtime attribute set.
+ */
+bool efi_all_runtime_mmio(uint64_t start, uint64_t end)
+{
+    unsigned int i;
+
+    for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
+    {
+        EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
+        uint64_t len = desc->NumberOfPages << EFI_PAGE_SHIFT;
+
+        if ( desc->Type != EfiMemoryMappedIO ||
+             !(desc->Attribute & EFI_MEMORY_RUNTIME) )
+            continue;
+
+        if ( start >= desc->PhysicalStart && end <= desc->PhysicalStart + len )
+            return true;
+    }
+
+    return false;
+}
+
 #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
 
 struct efi_rs_state efi_rs_enter(void)
diff --git a/xen/include/xen/efi.h b/xen/include/xen/efi.h
index 94a7e547f9..f29ea1a320 100644
--- a/xen/include/xen/efi.h
+++ b/xen/include/xen/efi.h
@@ -45,6 +45,8 @@ int efi_runtime_call(struct xenpf_efi_runtime_call *);
 int efi_compat_get_info(uint32_t idx, union compat_pf_efi_info *);
 int efi_compat_runtime_call(struct compat_pf_efi_runtime_call *);
 
+bool efi_all_runtime_mmio(uint64_t start, uint64_t end);
+
 #endif /* !__ASSEMBLY__ */
 
 #endif /* __XEN_EFI_H__ */
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 5975ca2f30..64995fc68d 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -211,6 +211,7 @@ unsigned int pci_size_mem_bar(pci_sbdf_t sbdf, unsigned int pos,
 
 void pci_intx(const struct pci_dev *, bool enable);
 bool_t pcie_aer_get_firmware_first(const struct pci_dev *);
+bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end);
 
 struct pirq;
 int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t gtable);

Re: [PATCH for-4.17] x86/pci: allow BARs to be positioned on e820 reserved regions

[Jan Beulich]

On 05.10.2022 16:12, Roger Pau Monné wrote:
> Hm, I have the following diff attached below which is more limited,
> and not so bad I think.  Initially I wanted to introduce an
> efi_all_mapped() helper, but that would require exposing EFI_MEMORY_TYPE
> which is quite intrusive.
> 
> Let me know if you think the proposal below is better and I will
> formally send a patch with it.

Hmm, personally I like this slightly better for, as you say, its more
limited effect. Objectively, however, I'm still unconvinced of making
this an EFI special case. How would non-EFI firmware go about
communicating that it is going to access a device at runtime (which,
as said before, I consider a no-go in the first place)? Likely by
putting its BAR range(s) in an E820_RESERVED region.

Plus the MMIO range covered on the system in question is pretty large.
That way we're still allowing pretty wide an abuse by the firmware.
Furthermore the MCFG range would also be covered by an
EfiMemoryMappedIO descriptor (in fact that's the only use of the type
I had been aware of so far). IOW the change specifically permits an
overlap of a BAR with an MCFG range.

Who's the manufacturer of the system? Or put in different words - how
likely is it that we could first gain understanding on their
intentions with this region? You did say the system hangs hard without
some kind of workaround, but that doesn't clarify to me in how far a
use of the device by the firmware was actually involved there.

Have you considered other routes towards dealing with the issue? One
approach coming to mind would build on top of what you've been doing
so far (either variant): Besides avoiding the turning off of memory
decode, also invoke pci_ro_device(), thus protecting it from having
its BARs relocated, and also preventing any driver in Dom0 to gain
control of it, thus avoiding two parties competing for the device.

Relocating the BAR outside of the reserved region would be nice, but
will likely not resolve the hang.

In any event I'm still hoping to have a 3rd view on the situation as a
whole, irrespective of specific ideas towards possible workarounds ...

Independent of the above a couple of purely cosmetic comments:

> @@ -98,3 +99,28 @@ 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) )
> +        return true;
> +
> +    /*
> +     * Also allow BARs placed on EfiMemoryMappedIO regions in order to deal
> +     * with EFI firmware using those regions to place the BARs of devices that
> +     * can be used during runtime.  But print a warning when doing so.
> +     */
> +    if ( !efi_all_runtime_mmio(mfn_to_maddr(start),
> +                               mfn_to_maddr(mfn_add(end, 1))) )
> +        return false;
> +
> +    printk(XENLOG_WARNING
> +           "%pp: BAR [%#" PRI_mfn ", %#" PRI_mfn "] overlaps reserved region\n",
> +           &pdev->sbdf, mfn_x(start), mfn_x(end));

Perhaps re-word the message now that the check is a different one?

> --- a/xen/common/efi/runtime.c
> +++ b/xen/common/efi/runtime.c
> @@ -78,6 +78,30 @@ bool efi_enabled(unsigned int feature)
>      return test_bit(feature, &efi_flags);
>  }
>  
> +/*
> + * This function checks if the entire range [start,end) is contained inside of
> + * a single EfiMemoryMappedIO descriptor with the runtime attribute set.
> + */
> +bool efi_all_runtime_mmio(uint64_t start, uint64_t end)
> +{
> +    unsigned int i;
> +
> +    for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
> +    {
> +        EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;

const?

> +        uint64_t len = desc->NumberOfPages << EFI_PAGE_SHIFT;
> +
> +        if ( desc->Type != EfiMemoryMappedIO ||
> +             !(desc->Attribute & EFI_MEMORY_RUNTIME) )
> +            continue;
> +
> +        if ( start >= desc->PhysicalStart && end <= desc->PhysicalStart + len )
> +            return true;
> +    }
> +
> +    return false;
> +}
> +
>  #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */

Perhaps put the function inside this #ifdef?

Jan

Re: [PATCH for-4.17] x86/pci: allow BARs to be positioned on e820 reserved regions

[Roger Pau Monné]

On Wed, Oct 05, 2022 at 05:42:08PM +0200, Jan Beulich wrote:
> On 05.10.2022 16:12, Roger Pau Monné wrote:
> > Hm, I have the following diff attached below which is more limited,
> > and not so bad I think.  Initially I wanted to introduce an
> > efi_all_mapped() helper, but that would require exposing EFI_MEMORY_TYPE
> > which is quite intrusive.
> > 
> > Let me know if you think the proposal below is better and I will
> > formally send a patch with it.
> 
> Hmm, personally I like this slightly better for, as you say, its more
> limited effect. Objectively, however, I'm still unconvinced of making
> this an EFI special case. How would non-EFI firmware go about
> communicating that it is going to access a device at runtime (which,
> as said before, I consider a no-go in the first place)? Likely by
> putting its BAR range(s) in an E820_RESERVED region.

I've never encountered a non-EFI firmware that attempts to access the
BAR of a device so far (or at least none that caused problems with
Xen I should say), so I would worry about such use-case when we find
one.

> Plus the MMIO range covered on the system in question is pretty large.
> That way we're still allowing pretty wide an abuse by the firmware.
> Furthermore the MCFG range would also be covered by an
> EfiMemoryMappedIO descriptor (in fact that's the only use of the type
> I had been aware of so far). IOW the change specifically permits an
> overlap of a BAR with an MCFG range.

Additionally I could check if the device overlaps with any known MCFG
regions in pci_check_bar(), again not sure if that's more fine grained
than needed.

> 
> Who's the manufacturer of the system?

It's from Supermicro: "Supermicro X11DPU BIOS"

> Or put in different words - how
> likely is it that we could first gain understanding on their
> intentions with this region? You did say the system hangs hard without
> some kind of workaround, but that doesn't clarify to me in how far a
> use of the device by the firmware was actually involved there.

It's a black box to me, so I have no idea what the firmware attempts
to do.  It looks like that BAR is used to store some information
related to the boot sequence, the hang happened when trying to create
a new variable bootnum using efibootmgr (that's just a guess
really).

> Have you considered other routes towards dealing with the issue? One
> approach coming to mind would build on top of what you've been doing
> so far (either variant): Besides avoiding the turning off of memory
> decode, also invoke pci_ro_device(), thus protecting it from having
> its BARs relocated, and also preventing any driver in Dom0 to gain
> control of it, thus avoiding two parties competing for the device.

IMO using pci_ro_device() would be going too far here - it's not Xen
the entity using the device, so Xen doesn't really know whether
there's already some interface between the firmware and the OS driver
(or just OS) in order to signal that the device is being used by the
firmware.

> Relocating the BAR outside of the reserved region would be nice, but
> will likely not resolve the hang.

Not an option for Xen due to not having enough information about the
memory layout, and it's risky IMO anyway.

> In any event I'm still hoping to have a 3rd view on the situation as a
> whole, irrespective of specific ideas towards possible workarounds ...

Thanks for the detailed feedback.

Roger.

Re: [PATCH for-4.17] x86/pci: allow BARs to be positioned on e820 reserved regions

[Jan Beulich]

On 06.10.2022 10:48, Roger Pau Monné wrote:
> On Wed, Oct 05, 2022 at 05:42:08PM +0200, Jan Beulich wrote:
>> On 05.10.2022 16:12, Roger Pau Monné wrote:
>>> Hm, I have the following diff attached below which is more limited,
>>> and not so bad I think.  Initially I wanted to introduce an
>>> efi_all_mapped() helper, but that would require exposing EFI_MEMORY_TYPE
>>> which is quite intrusive.
>>>
>>> Let me know if you think the proposal below is better and I will
>>> formally send a patch with it.
>>
>> Hmm, personally I like this slightly better for, as you say, its more
>> limited effect. Objectively, however, I'm still unconvinced of making
>> this an EFI special case. How would non-EFI firmware go about
>> communicating that it is going to access a device at runtime (which,
>> as said before, I consider a no-go in the first place)? Likely by
>> putting its BAR range(s) in an E820_RESERVED region.
> 
> I've never encountered a non-EFI firmware that attempts to access the
> BAR of a device so far (or at least none that caused problems with
> Xen I should say), so I would worry about such use-case when we find
> one.
> 
>> Plus the MMIO range covered on the system in question is pretty large.
>> That way we're still allowing pretty wide an abuse by the firmware.
>> Furthermore the MCFG range would also be covered by an
>> EfiMemoryMappedIO descriptor (in fact that's the only use of the type
>> I had been aware of so far). IOW the change specifically permits an
>> overlap of a BAR with an MCFG range.
> 
> Additionally I could check if the device overlaps with any known MCFG
> regions in pci_check_bar(), again not sure if that's more fine grained
> than needed.
> 
>>
>> Who's the manufacturer of the system?
> 
> It's from Supermicro: "Supermicro X11DPU BIOS"
> 
>> Or put in different words - how
>> likely is it that we could first gain understanding on their
>> intentions with this region? You did say the system hangs hard without
>> some kind of workaround, but that doesn't clarify to me in how far a
>> use of the device by the firmware was actually involved there.
> 
> It's a black box to me, so I have no idea what the firmware attempts
> to do.

Right - I don't expect there's a realistic chance of getting hold of a
firmware person of theirs.

>  It looks like that BAR is used to store some information
> related to the boot sequence, the hang happened when trying to create
> a new variable bootnum using efibootmgr (that's just a guess
> really).
> 
>> Have you considered other routes towards dealing with the issue? One
>> approach coming to mind would build on top of what you've been doing
>> so far (either variant): Besides avoiding the turning off of memory
>> decode, also invoke pci_ro_device(), thus protecting it from having
>> its BARs relocated, and also preventing any driver in Dom0 to gain
>> control of it, thus avoiding two parties competing for the device.
> 
> IMO using pci_ro_device() would be going too far here - it's not Xen
> the entity using the device, so Xen doesn't really know whether
> there's already some interface between the firmware and the OS driver
> (or just OS) in order to signal that the device is being used by the
> firmware.

Fair point (I did think of that, but ended up not spelling it out).

Jan

Re: [PATCH for-4.17] x86/pci: allow BARs to be positioned on e820 reserved regions

[Roger Pau Monné]

On Thu, Oct 06, 2022 at 11:02:32AM +0200, Jan Beulich wrote:
> On 06.10.2022 10:48, Roger Pau Monné wrote:
> > On Wed, Oct 05, 2022 at 05:42:08PM +0200, Jan Beulich wrote:
> >> Or put in different words - how
> >> likely is it that we could first gain understanding on their
> >> intentions with this region? You did say the system hangs hard without
> >> some kind of workaround, but that doesn't clarify to me in how far a
> >> use of the device by the firmware was actually involved there.
> > 
> > It's a black box to me, so I have no idea what the firmware attempts
> > to do.
> 
> Right - I don't expect there's a realistic chance of getting hold of a
> firmware person of theirs.

Even if somehow I could manage to do that (which I think it's
unlikely), it's not going to happen before the release, and there are
production boxes out there that have this behavior and we need to deal
with them.

Thanks, Roger.