Re: [PATCH for-4.17] x86/efi: don't translate EFI memory map MMIO regions to e820 RESERVED

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.10.2022 14:17, Roger Pau Monné wrote:
> On Tue, Oct 04, 2022 at 12:40:10PM +0200, Jan Beulich wrote:
>> On 04.10.2022 11:27, Roger Pau Monné wrote:
>>> On Tue, Oct 04, 2022 at 11:01:18AM +0200, Jan Beulich wrote:
>>>> On 30.09.2022 16:17, Roger Pau Monne wrote:
>>>>> The EFI memory map contains two memory types (EfiMemoryMappedIO and
>>>>> EfiMemoryMappedIOPortSpace) used to describe IO memory areas of
>>>>> devices used by EFI.
>>>>>
>>>>> The current parsing of the EFI memory map was translating
>>>>> EfiMemoryMappedIO and EfiMemoryMappedIOPortSpace to E820_RESERVED on
>>>>> x86.  This is an issue because device MMIO regions (BARs) should not
>>>>> be positioned on reserved regions.  Any BARs positioned on non-hole
>>>>> areas of the memory map will cause is_memory_hole() to return false,
>>>>> which would then cause memory decoding to be disabled for such device.
>>>>> This leads to EFI firmware malfunctions when using runtime services.
>>>>>
>>>>> 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 not adding regions with type EfiMemoryMappedIO or
>>>>> EfiMemoryMappedIOPortSpace to the e820 memory map, that allows BARs to
>>>>> be positioned there.
>>>>>
>>>>> Fixes: 75cc460a1b ('xen/pci: detect when BARs are not suitably 
>>>>> positioned')
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>>
>>>> In the best case this is moving us from one way of being wrong to another:
>>>> So far we wrongly include BARs in E820_RESERVED (_if_ they can be
>>>> legitimately covered by a EfiMemoryMappedIO region in the first place,
>>>> which I'm not sure is actually permitted - iirc just like E820_RESERVED
>>>> may not be used for BARs, this memory type also may not be), whereas with
>>>> your change we would no longer report non-BAR MMIO space (chipset specific
>>>> ranges for example) as reserved. In fact I think the example you provide
>>>> is at least partly due to bogus firmware behavior: The BAR is put in space
>>>> normally used for firmware specific memory (MMIO) ranges. I think firmware
>>>> should either assign the BAR differently or exclude the range from the
>>>> memory map.
>>>
>>> Hm, I'm not sure the example is bogus, how would firmware request a BAR
>>> to be mapped for run time services to access it otherwise if it's not
>>> using EfiMemoryMappedIO?
>>>
>>> Not adding the BAR to the memory map in any way would mean the OS is
>>> free to not map it for runtime services to access.
>>
>> My view is that BARs should not be marked for runtime services use. Doing
>> so requires awareness of the driver inside the OS, which I don't think
>> one can expect. If firmware needs to make use of a device in a system, it
>> ought to properly hide it from the OS. Note how the potential sharing of
>> an RTC requires special provisions in the spec, mandating driver awareness.
>>
>> Having a BAR expressed in the memory map also contradicts the ability of
>> an OS to relocate all BARs of all devices, if necessary.
> 
> I've failed to figure out if there's a way in UEFI to report a device
> is in use by the firmware.  I've already looked before sending the
> patch (see also the post commit notes about for example not passing
> through the device to any guest for obvious reason).
> 
> I've got no idea if Linux has any checks to avoid trying to move BARs
> residing in EfiMemoryMappedIO ranges, we have now observed this
> behavior in two systems already.
> 
> Maybe we could do a special check for PCI devices and allow them
> having BARs in EfiMemoryMappedIO, together with printing a warning
> message.

Right, that's one of the possible quirk workarounds I was thinking of.
At the risk of stating the obvious - the same would presumably apply to
E820_RESERVED on non-EFI systems then.

Jan