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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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