Re: [PATCH] Remove warning in efi_enter_virtual_mode

[Bryan O'Donoghue <bryan.odonoghue.lkml-SyKdqv6vbfZdzvEItQ6vdLNAH6kLmebB@public.gmane.org>]

On 17/04/13 15:06, Matt Fleming wrote:
> (Cc'ing some more folks)
>
> On 16/04/13 16:58, Bryan O'Donoghue wrote:
>> This warning is caused by efi_enter_virtual_mode mapping memory marked
>> as !EFI_RUNTIME_MEMORY. The reason this memory is being mapped is to
>> work around buggy code that stores an ACPI object called the Boot
>> Graphics Resource Table - BGRT in memory of type EfiBootServices*.
>>
>> ------------[ cut here ]------------
>> WARNING: at arch/x86/mm/ioremap.c:102 __ioremap_caller+0x3d3/0x440()
>> Modules linked in:
>> Pid: 0, comm: swapper Not tainted 3.9.0-rc7+ #95
>> Call Trace:
>>   [<c102b6af>] warn_slowpath_common+0x5f/0x80
>>   [<c1023fb3>] ? __ioremap_caller+0x3d3/0x440
>>   [<c1023fb3>] ? __ioremap_caller+0x3d3/0x440
>>   [<c102b6ed>] warn_slowpath_null+0x1d/0x20
>>   [<c1023fb3>] __ioremap_caller+0x3d3/0x440
>>   [<c106007b>] ? get_usage_chars+0xfb/0x110
>>   [<c102d937>] ? vprintk_emit+0x147/0x480
>>   [<c1418593>] ? efi_enter_virtual_mode+0x1e4/0x3de
>>   [<c102406a>] ioremap_cache+0x1a/0x20
>>   [<c1418593>] ? efi_enter_virtual_mode+0x1e4/0x3de
>>   [<c1418593>] efi_enter_virtual_mode+0x1e4/0x3de
>>   [<c1407984>] start_kernel+0x286/0x2f4
>>   [<c1407535>] ? repair_env_string+0x51/0x51
>>   [<c1407362>] i386_start_kernel+0x12c/0x12f
>> ---[ end trace 4eaa2a86a8e2da22 ]---
>
> The warning is telling you that someone is trying to ioremap RAM, which
> is bad. It's not specifically an issue with the bgrt image, it's just
> that said object happens to occupy an EfiBootServicesData region which
> isn't mapped by the direct kernel mapping on i386.

I understand that.

In my mind the only memory that is relevant to efi_enter_virtual_mode is 
memory that the BIOS has marked as EFI_RUNTIME_SERVICE

md->attribute & 0x80000000_00000000

I couldn't quite understand why the code in

arch/x86/platform/efi/efi.c:enter_virtual_mode() looks like this

for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
                 md = p;
                 if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
                     md->type != EFI_BOOT_SERVICES_CODE &&
                     md->type != EFI_BOOT_SERVICES_DATA)
			continue;

While the code in

arch/ia64/kernel/efi.c:enter_virtual_mode

for (p = efi_map_start; p < efi_map_end; p += efi_desc_size) {
		md = p;
		if (md->attribute & EFI_MEMORY_RUNTIME) {

The ia64 version is consistent with the standard - but obviously isn't 
accounting for the work-around implemented to retrieve the BGRT on ia32.

Looking at the commit message associated with 
arch/x86/platform/efi/efi-bgrt.c

It's pretty obvious the mapping of boot code/data was done to facilitate 
BGRT.

Since the EFI memory map I'm using is clean - below - there's no reason 
for us to map the boot code/data.

> Most (all?) boot loaders mark EFI_BOOT_SERVICES_{CODE,DATA} as E820_RAM,
> which is why ioremap() complained about you trying to ioremap() a page
> of RAM.

But they aught to. It's entirely legitimate for the run-time to reclaim 
this memory - since after ExitBootServices() - none of the code/data 
inside of EFI_BOOT_SERVICES is of any use.

Caveat being the work-around that was done for BGRT.

They do this because after efi_free_boot_services() we want
> these regions to be available for general allocation.

Agree.

> On x86-64 you rarely hit the ioremap() path because all RAM regions are
> mapped by the kernel direct mapping, e.g __va() works on those
> addresses. On i386, with its reduced virtual address space, it's much
> more likely that those addresses are not part of the direct mapping, and
> so this is the chunk of code that causes problems in
> efi_enter_virtual_mode(),
>
>                  start_pfn = PFN_DOWN(md->phys_addr);
>                  end_pfn = PFN_UP(end);
>                  if (pfn_range_is_mapped(start_pfn, end_pfn)) {
>                          va = __va(md->phys_addr);
>
>                          if (!(md->attribute&  EFI_MEMORY_WB))
>                                  efi_memory_uc((u64)(unsigned long)va, size);
>                  } else
>                          va = efi_ioremap(md->phys_addr, size,
>                                           md->type, md->attribute);

Yes it fails sanity checking in efi_ioremap::__ioremap_caller on an 
"is_ram()" call.

> What we probably need is an i386-specific implementation of
> efi_ioremap() that checks to see whether we're mapping a RAM region. I
> thought of maybe using the kmap_high() functions, but I don't think
> repeated calls to the kmap*() functions are guaranteed to provide
> consecutive virtual addresses, and I doubt free_bootmem() (called from
> efi_free_boot_services()) understands kmap'd addresses.

That's one solution - you'd need to make the i386::efi_ioremap() aware 
of the BGRT work-around.

Presumably this work-around is also required on ia64 too.

> Maybe we should hot-add the EFI_BOOT_SERVICES_* regions once we've
> finished with them and only then mark them as RAM?
>
> x86 folks? Halp?
>
>> On systems that do not have a BGRT object, there's no reason to map this
>> memory in efi_enter_virtual_mode. This patch searches for the BGRT
>> object and if found - will continue to map the boot services memory,
>> else only memory with attribute EFI_RUNTIME_MEMORY will be mapped.
>
> Like I said above, it just so happens on your machine that a BGRT object
> occupies that chunk of memory, but this might not be the case on every
> EFI i386 machine. You may have other useful things in that region that
> you want to be mapped. It's also entirely possible that someone with the
> same memory map layout as you _will_ want the bgrt image mapped. This
> code needs fixing, instead of just working around the problem.

No, no - we *don't* have a BGRT object at all.

We have a completely clean memory map - but the BGRT code is causing the 
is_ram() failure.

Rather than just blow away the BGRT code the patch determines if a BGRT 
object exists.

If there is an ACPI::BGRT - then efi_enter_virtual_mode - will still 
continue to map EFI_BOOT_SERVICES*

If not then you get this

if (md->attribute & EFI_MEMORY_RUNTIME) {
	/* Only map EFI_RUNTIME_MEMORY here */
}

Which is what everybody who is !ACPI::BGRT really wants.

I've coded up a precautionary alternate version of the patch that passes 
a kernel parameter to switch off the offending code, though it would 
probably be better to pass a kernel parameter to switch it on !

Though that would require modification of the kernel command line for 
any system that currently relies on BGRT - so unfortunately I think 
switching off the - I hate to use the bug - seems the less 
user-impacting method.

I'll send this patch for reference - but, I do believe probing for BGRT 
is more appropriate than forcing a kernel parameter addition.

I think even if you make an i386 version of efi_ioremap() you still need 
to address the fundamental problem that only systems which want to 
implement the ACPI::BGRT work-around care about mapping 
EFI_BOOT_SERVICES, unless I've missed a trick here ?


Bryan