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

[Jan Beulich <jbeulich@xxxxxxxx>]

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