Re: [PATCH] x86/PCI: Disable E820 reserved region clipping for Clevo NL4XLU laptops

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 10/11/22 19:40, Bjorn Helgaas wrote:
> On Mon, Oct 10, 2022 at 05:02:06PM +0200, Hans de Goede wrote:
>> Clevo NL4XLU barebones have the same E820 reservation covering
>> the entire _CRS 32-bit window issue as the Lenovo *IIL* and
>> Clevo X170KM-G models, relevant dmesg bits (with pci=no_e820):
>>
>>  BIOS-e820: [mem 0x000000005bc50000-0x00000000cfffffff] reserved
>>  pci_bus 0000:00: root bus resource [mem 0x6d800000-0xbfffffff window]
>>
>> Note how the e820 reservation covers the entire PCI root mem window.
>>
>> Add a no_e820 quirk for these models to fix the touchpad not working
>> (due to Linux being unable to assign a PCI BAR for the i2c-controller).
> 
> I do plan to apply this, but a little food for thought below.
> 
> I explored this issue a little bit with the ACPI/UEFI folks (see
> https://members.uefi.org/wg/aswg/mail/thread/9265 if you have access).  
> 
> One aspect I had glossed over earlier is that on most recent machines,
> the "E820 map" Linux uses is actually constructed internally by Linux
> based on the UEFI memory map, and that construction conflates several
> EFI types into E820_TYPE_RESERVED; see
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/firmware/efi/libstub/x86-stub.c?id=v5.19#n576
> 
> We don't have a dmesg log with "efi=debug" for this case, but in most
> cases where E820 says the entire root mem window is reserved, I think
> it's because an EfiMemoryMappedIO entry was converted to
> E820_TYPE_RESERVED.
> 
> From a response in the ACPI/UEFI discussion:
> 
>   The reason EfiMemoryMappedIO[1] exist in the UEFI memory map is to
>   request a virtual mapping for UEFI Runtime Services.
>   
>   For example if the EFI Runtime Service needed to write to FLASH then
>   the NOR FLASH would need a mapping. Also the NOR FLASH controller
>   might also need a mapping and this could include PCI Config Space if
>   needed registers existed in that space.
>   
>   Thus the EfiMemoryMappedIO entries just exist to pass up the
>   EFI_MEMORY_RUNTIME attribute in the UEFI Memory Map. This is the part
>   of the contract for UEFI Runtime Service to use virtual mappings
>   provided by the OS. So from an OS point of view EfiMemoryMappedIO has
>   no other purpose.
>   
>   [1] UEFI: Table 7-5 Memory Type Usage before ExitBootServices() "Used
>   by system firmware to request that a memory-mapped IO region be
>   mapped by the OS to a virtual address so it can be accessed by EFI
>   runtime services."
> 
> So the point here is that Linux currently converts EfiMemoryMappedIO
> to E820_TYPE_RESERVED, and that likely attaches more meaning to those
> regions than firmware intended.
> 
> I'm a little leery of changing that UEFI->E820 conversion because of
> other possible implications, but it may be that omitting
> EfiMemoryMappedIO entries from the E820 map and keeping the original
> "avoid E820 regions" (4dc2287c1805) would also solve this problem.

Actually during my many attempts to fix this I did write a patch
adding a new E820_TYPE_MMIO to the generated e820-memmap which
would only show up in the EFI -> E820 entry generation case
and then used that to not exclude that E820 region, see
this RFC series:

https://lore.kernel.org/linux-pci/20220214151759.98267-1-hdegoede@redhat.com/

Unfortunately this series caused the same X1 carbon gen2 suspend/resume
not working problem as the earlier set pci=no_e820 based on a BIOS date cutoff
attempt which I did earlier and which even briefly was in some -rc kernels
through Rafael's tree.

I also did another series which used the EfiMemoryMappedIO type as
an input to heuristics to automatically set pci=no_e820, see:

https://lore.kernel.org/linux-pci/20220228105259.230903-1-hdegoede@redhat.com/

IIRC that patch eventually got replaced by a similar but simpler
heuristic from you. Which IIRC eventually got dropped again because
it was causing regressions on some models again.

So we ended up with the current set pci=no_e820 using DMI based quirks +
try to enable it for all BIOS-es with date >= 2023 approach,
with the plan to do DMI quirks setting pci=use_e820 if any (buggy)
2023 BIOS-es show up which need this.

Regards,

Hans







>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216565
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  arch/x86/pci/acpi.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
>> index 2f82480fd430..45ef65d31a40 100644
>> --- a/arch/x86/pci/acpi.c
>> +++ b/arch/x86/pci/acpi.c
>> @@ -189,6 +189,19 @@ static const struct dmi_system_id pci_crs_quirks[] __initconst = {
>>  			DMI_MATCH(DMI_BOARD_NAME, "X170KM-G"),
>>  		},
>>  	},
>> +
>> +	/*
>> +	 * Clevo NL4XLU barebones have the same E820 reservation covering
>> +	 * the entire _CRS 32-bit window issue as the Lenovo *IIL* models.
>> +	 * See https://bugzilla.kernel.org/show_bug.cgi?id=216565
>> +	 */
>> +	{
>> +		.callback = set_no_e820,
>> +		.ident = "Clevo NL4XLU Barebone",
>> +		.matches = {
>> +			DMI_MATCH(DMI_BOARD_NAME, "NL4XLU"),
>> +		},
>> +	},
>>  	{}
>>  };
>>  
>> -- 
>> 2.37.3
>>
>