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

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

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.