LinuxLists.cc - [PATCH] Remove warning in efi_enter_virtual

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

(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.

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. They do this because after efi_free_boot_services() we want
these regions to be available for general allocation.

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);

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.

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.

> Mapping only memory EFI_RUNTIME_MEMORY is is consistent with the code in
> arch/ia64/kernel/efi.c:efi_enter_virtual_mode();
>
> Signed-off-by: Bryan O'Donoghue <[email protected]>
> ---
> arch/x86/platform/efi/efi-bgrt.c | 20 +++++++++++++++-----
> arch/x86/platform/efi/efi.c | 12 +++++++++---
> include/linux/efi-bgrt.h | 2 ++
> 3 files changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/platform/efi/efi-bgrt.c b/arch/x86/platform/efi/efi-bgrt.c
> index 7145ec6..3655d62 100644
> --- a/arch/x86/platform/efi/efi-bgrt.c
> +++ b/arch/x86/platform/efi/efi-bgrt.c
> @@ -25,19 +25,29 @@ struct bmp_header {
> u32 size;
> } __packed;
>
> -void __init efi_bgrt_init(void)
> +bool __init efi_bgrt_probe(void)
> {
> acpi_status status;
> - void __iomem *image;
> - bool ioremapped = false;
> - struct bmp_header bmp_header;
>
> if (acpi_disabled)
> - return;
> + return false;
>
> + bgrt_tab = NULL;
> status = acpi_get_table("BGRT", 0,
> (struct acpi_table_header **)&bgrt_tab);
> if (ACPI_FAILURE(status))
> + return false;
> +
> + return true;
> +}
> +
> +void __init efi_bgrt_init(void)
> +{
> + void __iomem *image;
> + bool ioremapped = false;
> + struct bmp_header bmp_header;
> +
> + if (acpi_disabled || bgrt_tab == NULL)
> return;
>
> if (bgrt_tab->header.length < sizeof(*bgrt_tab))
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 5f2ecaf..7c64a2f 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -842,6 +842,7 @@ void __init efi_enter_virtual_mode(void)
> u64 end, systab, start_pfn, end_pfn;
> void *p, *va, *new_memmap = NULL;
> int count = 0;
> + bool bgrt_map;
>
> efi.systab = NULL;
>
> @@ -855,6 +856,11 @@ void __init efi_enter_virtual_mode(void)
> return;
> }
>
> + /*
> + * Determine if mapping EFI boot code/data is required for BGRT mapping
> + */
> + bgrt_map = efi_bgrt_probe();
> +
> /* Merge contiguous regions of the same type and attribute */
> for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
> u64 prev_size;
> @@ -884,9 +890,9 @@ void __init efi_enter_virtual_mode(void)
>
> 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)
> + if (!((md->attribute & EFI_MEMORY_RUNTIME) || (bgrt_map &&
> + (md->type == EFI_BOOT_SERVICES_CODE ||
> + md->type == EFI_BOOT_SERVICES_DATA))))
> continue;
>
> size = md->num_pages << EFI_PAGE_SHIFT;
> diff --git a/include/linux/efi-bgrt.h b/include/linux/efi-bgrt.h
> index 051b21f..165426b 100644
> --- a/include/linux/efi-bgrt.h
> +++ b/include/linux/efi-bgrt.h
> @@ -6,6 +6,7 @@
> #include <linux/acpi.h>
>
> void efi_bgrt_init(void);
> +bool efi_bgrt_probe(void);
>
> /* The BGRT data itself; only valid if bgrt_image != NULL. */
> extern void *bgrt_image;
> @@ -15,6 +16,7 @@ extern struct acpi_table_bgrt *bgrt_tab;
> #else /* !CONFIG_ACPI_BGRT */
>
> static inline void efi_bgrt_init(void) {}
> +static inline bool efi_bgrt_probe(void) { return false; }
>
> #endif /* !CONFIG_ACPI_BGRT */
>
>

--
Matt Fleming, Intel Open Source Technology Center

2013-04-17 21:58:22

by Bryan O'Donoghue

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

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

2013-04-18 11:00:37

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On 17/04/13 23:00, Bryan O'Donoghue wrote:
> 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.

No, that's incorrect. The patch that introduced the mapping of
EFI_BOOT_SERVICES_{CODE,DATA} was committed before support for bgrt
existed. git blame is a good tool to use when doing one of these
historical digs, and in this case it shows that the above lines from
efi_enter_virtual_mode() were introduced in the following commit,

commit 916f676f8dc016103f983c7ec54c18ecdbb6e349
Author: Matthew Garrett <[email protected]>
Date: Wed May 25 09:53:13 2011 -0400

x86, efi: Retain boot service code until after switching to virtual mode

UEFI stands for "Unified Extensible Firmware Interface", where "Firmware"
is an ancient African word meaning "Why do something right when you can
do it so wrong that children will weep and brave adults will cower before
you", and "UEI" is Celtic for "We missed DOS so we burned it into your
ROMs". The UEFI specification provides for runtime services (ie, another
way for the operating system to be forced to depend on the firmware) and
we rely on these for certain trivial tasks such as setting up the
bootloader. But some hardware fails to work if we attempt to use these
runtime services from physical mode, and so we have to switch into virtual
mode. So far so dreadful.

The specification makes it clear that the operating system is free to do
whatever it wants with boot services code after ExitBootServices() has been
called. SetVirtualAddressMap() can't be called until ExitBootServices() has
been. So, obviously, a whole bunch of EFI implementations call into boot
services code when we do that. Since we've been charmingly naive and
trusted that the specification may be somehow relevant to the real world,
we've already stuffed a picture of a penguin or something in that address
space. And just to make things more entertaining, we've also marked it
non-executable.

This patch allocates the boot services regions during EFI init and makes
sure that they're executable. Then, after SetVirtualAddressMap(), it
discards them and everyone lives happily ever after. Except for the ones
who have to work on EFI, who live sad lives haunted by the knowledge that
someone's eventually going to write yet another firmware specification.

[ hpa: adding this to urgent with a stable tag since it fixes currently-broken
hardware. However, I do not know what the dependencies are and so I do
not know which -stable versions this may be a candidate for. ]

Signed-off-by: Matthew Garrett <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: H. Peter Anvin <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: <[email protected]>

Yes the bgrt code accesses the Boot Service mappings, but that isn't the
only reason we want to map those regions.

> 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.

No, we've never seen an ia64 firmware implementation with the "access
EFI Boot Services Code/Data after ExitBootServices() bug", and it
doesn't suffer from the same virtual address space limitations that i386
does.

> 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.

You assume that mapping of the Boot Services regions is done purely for
the benefit of pulling out the bgrt image - it's not, see the above
commit log - and I assumed that you had an ACPI bgrt pointer in your
memory map, but you don't.

Darren, Josh, have you ever seen an i386 machine with a bgrt pointer? If
not, and given that we've never seen an i386 firmware that requires the
above workaround from Matthew, combined with the fact that there are so
few i386 implementations out there, I'm inclined to apply the patch
below, because anything else is a lot more work. We can address this
properly if we ever start seeing i386 machines with bgrt pointers that
reference highmem.

---

>From 37ba0d4f43e0f8ec3e3342bc1331e36125495d3e Mon Sep 17 00:00:00 2001
From: Matt Fleming <[email protected]>
Date: Thu, 18 Apr 2013 09:57:55 +0100
Subject: [PATCH] x86, efi: Refuse to ioremap highmem on i386

Bryan reports running into the following warning on i386,

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

Due to the workaround described in commit 916f676f8 ("x86, efi: Retain
boot service code until after switching to virtual mode") EFI Boot
Service regions are mapped for a period during boot. Unfortunately, with
the limited size of the i386 direct kernel map it's possible that some
of the Boot Service regions will not be directly accessible, which
causes them to be ioremap()'d, triggering the above warning as the
regions are marked as E820_RAM in the e820 memmap.

The simplest fix is to refuse to map ram (and therefore, any EFI Boot
Service regions) in an i386-specific version of efi_ioremap(), because
there is no method of mapping arbitrary sizes of highmem at contiguous
virtual addresses in the kernel address space.

There are currently only two situations where we need to map EFI Boot
Service regions,

1. To workaround the firmware bug described in 916f676f8
2. To access the ACPI BGRT image

but since we haven't seen an i386 implementation that requires either,
this simple fix should suffice for now. Item 2. above does still work on
i386 provided that the BGRT image is not in highmem.

Reported-by: Bryan O'Donoghue <[email protected]>
Cc: Josh Triplett <[email protected]>
Cc: Darren Hart <[email protected]>
Cc: Matthew Garrett <[email protected]>
Cc: Josh Boyer <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: <[email protected]>
Signed-off-by: Matt Fleming <[email protected]>
---
arch/x86/include/asm/efi.h | 7 ++-----
arch/x86/platform/efi/efi_32.c | 15 +++++++++++++++
2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 2fb5d58..8f042bf 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -35,8 +35,6 @@ extern unsigned long asmlinkage efi_call_phys(void *, ...);
#define efi_call_virt6(f, a1, a2, a3, a4, a5, a6) \
efi_call_virt(f, a1, a2, a3, a4, a5, a6)

-#define efi_ioremap(addr, size, type, attr) ioremap_cache(addr, size)
-
#else /* !CONFIG_X86_32 */

#define EFI_LOADER_SIGNATURE "EL64"
@@ -88,9 +86,6 @@ extern u64 efi_call6(void *fp, u64 arg1, u64 arg2, u64 arg3,
efi_call6((void *)(efi.systab->runtime->f), (u64)(a1), (u64)(a2), \
(u64)(a3), (u64)(a4), (u64)(a5), (u64)(a6))

-extern void __iomem *efi_ioremap(unsigned long addr, unsigned long size,
- u32 type, u64 attribute);
-
#endif /* CONFIG_X86_32 */

extern int add_efi_memmap;
@@ -101,6 +96,8 @@ extern void efi_call_phys_prelog(void);
extern void efi_call_phys_epilog(void);
extern void efi_unmap_memmap(void);
extern void efi_memory_uc(u64 addr, unsigned long size);
+extern void __iomem *efi_ioremap(unsigned long addr, unsigned long size,
+ u32 type, u64 attribute);

struct efi_var_bootdata {
struct setup_data data;
diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c
index 40e4469..fbdfa97 100644
--- a/arch/x86/platform/efi/efi_32.c
+++ b/arch/x86/platform/efi/efi_32.c
@@ -67,3 +67,18 @@ void efi_call_phys_epilog(void)

local_irq_restore(efi_rt_eflags);
}
+
+void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size,
+ u32 type, u64 attribute)
+{
+ /*
+ * If we've been asked to map a chunk of ram, that means it is
+ * highmem and isn't accessible via the direct kernel mapping.
+ * i386 doesn't have a method for mapping arbitrary sized chunks
+ * of highmem into contiguous virtual addresses.
+ */
+ if (page_is_ram(phys_addr))
+ return NULL;
+
+ return ioremap_cache(phys_addr, size);
+}
--
1.7.10.4

--
Matt Fleming, Intel Open Source Technology Center

2013-04-18 13:41:23

by Josh Boyer

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On Thu, Apr 18, 2013 at 12:00:26PM +0100, Matt Fleming wrote:
> On 17/04/13 23:00, Bryan O'Donoghue wrote:
> > 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.
>
> No, that's incorrect. The patch that introduced the mapping of
> EFI_BOOT_SERVICES_{CODE,DATA} was committed before support for bgrt
> existed. git blame is a good tool to use when doing one of these
> historical digs, and in this case it shows that the above lines from
> efi_enter_virtual_mode() were introduced in the following commit,
>
> commit 916f676f8dc016103f983c7ec54c18ecdbb6e349
> Author: Matthew Garrett <[email protected]>
> Date: Wed May 25 09:53:13 2011 -0400
>
> x86, efi: Retain boot service code until after switching to virtual mode
>
> UEFI stands for "Unified Extensible Firmware Interface", where "Firmware"
> is an ancient African word meaning "Why do something right when you can
> do it so wrong that children will weep and brave adults will cower before
> you", and "UEI" is Celtic for "We missed DOS so we burned it into your
> ROMs". The UEFI specification provides for runtime services (ie, another
> way for the operating system to be forced to depend on the firmware) and
> we rely on these for certain trivial tasks such as setting up the
> bootloader. But some hardware fails to work if we attempt to use these
> runtime services from physical mode, and so we have to switch into virtual
> mode. So far so dreadful.
>
> The specification makes it clear that the operating system is free to do
> whatever it wants with boot services code after ExitBootServices() has been
> called. SetVirtualAddressMap() can't be called until ExitBootServices() has
> been. So, obviously, a whole bunch of EFI implementations call into boot
> services code when we do that. Since we've been charmingly naive and
> trusted that the specification may be somehow relevant to the real world,
> we've already stuffed a picture of a penguin or something in that address
> space. And just to make things more entertaining, we've also marked it
> non-executable.
>
> This patch allocates the boot services regions during EFI init and makes
> sure that they're executable. Then, after SetVirtualAddressMap(), it
> discards them and everyone lives happily ever after. Except for the ones
> who have to work on EFI, who live sad lives haunted by the knowledge that
> someone's eventually going to write yet another firmware specification.
>
> [ hpa: adding this to urgent with a stable tag since it fixes currently-broken
> hardware. However, I do not know what the dependencies are and so I do
> not know which -stable versions this may be a candidate for. ]
>
> Signed-off-by: Matthew Garrett <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> Signed-off-by: H. Peter Anvin <[email protected]>
> Cc: Tony Luck <[email protected]>
> Cc: <[email protected]>
>
> Yes the bgrt code accesses the Boot Service mappings, but that isn't the
> only reason we want to map those regions.
>
> > 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.
>
> No, we've never seen an ia64 firmware implementation with the "access
> EFI Boot Services Code/Data after ExitBootServices() bug", and it
> doesn't suffer from the same virtual address space limitations that i386
> does.
>
> > 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.
>
> You assume that mapping of the Boot Services regions is done purely for
> the benefit of pulling out the bgrt image - it's not, see the above
> commit log - and I assumed that you had an ACPI bgrt pointer in your
> memory map, but you don't.
>
> Darren, Josh, have you ever seen an i386 machine with a bgrt pointer? If
> not, and given that we've never seen an i386 firmware that requires the
> above workaround from Matthew, combined with the fact that there are so
> few i386 implementations out there, I'm inclined to apply the patch
> below, because anything else is a lot more work. We can address this
> properly if we ever start seeing i386 machines with bgrt pointers that
> reference highmem.

Hm. I'm probably the least clueful person to ask on this one. Fedora
has a number of 32-bit bug reports, but we explicitly don't support
32-bit UEFI.

BGRT is a new addition in ACPI 5.0, right? Hopefully with it being
relatively recent, and new 32-bit firmware being somewhat rare, it won't
be a problem.

josh

2013-04-18 14:51:38

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On 04/18/2013 04:00 AM, Matt Fleming wrote:
> On 17/04/13 23:00, Bryan O'Donoghue wrote:
>> 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.
>
> No, that's incorrect. The patch that introduced the mapping of
> EFI_BOOT_SERVICES_{CODE,DATA} was committed before support for bgrt
> existed. git blame is a good tool to use when doing one of these
> historical digs, and in this case it shows that the above lines from
> efi_enter_virtual_mode() were introduced in the following commit,
>
> commit 916f676f8dc016103f983c7ec54c18ecdbb6e349
> Author: Matthew Garrett <[email protected]>
> Date: Wed May 25 09:53:13 2011 -0400
>
> x86, efi: Retain boot service code until after switching to virtual mode
>
> UEFI stands for "Unified Extensible Firmware Interface", where "Firmware"
> is an ancient African word meaning "Why do something right when you can
> do it so wrong that children will weep and brave adults will cower before
> you", and "UEI" is Celtic for "We missed DOS so we burned it into your
> ROMs". The UEFI specification provides for runtime services (ie, another
> way for the operating system to be forced to depend on the firmware) and
> we rely on these for certain trivial tasks such as setting up the
> bootloader. But some hardware fails to work if we attempt to use these
> runtime services from physical mode, and so we have to switch into virtual
> mode. So far so dreadful.
>
> The specification makes it clear that the operating system is free to do
> whatever it wants with boot services code after ExitBootServices() has been
> called. SetVirtualAddressMap() can't be called until ExitBootServices() has
> been. So, obviously, a whole bunch of EFI implementations call into boot
> services code when we do that. Since we've been charmingly naive and
> trusted that the specification may be somehow relevant to the real world,
> we've already stuffed a picture of a penguin or something in that address
> space. And just to make things more entertaining, we've also marked it
> non-executable.
>
> This patch allocates the boot services regions during EFI init and makes
> sure that they're executable. Then, after SetVirtualAddressMap(), it
> discards them and everyone lives happily ever after. Except for the ones
> who have to work on EFI, who live sad lives haunted by the knowledge that
> someone's eventually going to write yet another firmware specification.
>
> [ hpa: adding this to urgent with a stable tag since it fixes currently-broken
> hardware. However, I do not know what the dependencies are and so I do
> not know which -stable versions this may be a candidate for. ]
>
> Signed-off-by: Matthew Garrett <[email protected]>
> Link: http://lkml.kernel.org/r/[email protected]
> Signed-off-by: H. Peter Anvin <[email protected]>
> Cc: Tony Luck <[email protected]>
> Cc: <[email protected]>
>
> Yes the bgrt code accesses the Boot Service mappings, but that isn't the
> only reason we want to map those regions.
>
>> 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.
>
> No, we've never seen an ia64 firmware implementation with the "access
> EFI Boot Services Code/Data after ExitBootServices() bug", and it
> doesn't suffer from the same virtual address space limitations that i386
> does.
>
>> 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.
>
> You assume that mapping of the Boot Services regions is done purely for
> the benefit of pulling out the bgrt image - it's not, see the above
> commit log - and I assumed that you had an ACPI bgrt pointer in your
> memory map, but you don't.
>
> Darren, Josh, have you ever seen an i386 machine with a bgrt pointer? If
> not, and given that we've never seen an i386 firmware that requires the
> above workaround from Matthew, combined with the fact that there are so
> few i386 implementations out there, I'm inclined to apply the patch
> below, because anything else is a lot more work. We can address this
> properly if we ever start seeing i386 machines with bgrt pointers that
> reference highmem.

I don't believe I have seen a 32-bit EFI system with a BGRT, but then
again, I had to look it up today! That said, I suspect the MinnowBoard
would benefit from such a thing, so we should have an example of it
there in the near future.

Is this in anyway related to the following patch from Josh Boyer? We're
carrying this in the Yocto Project trees currently.

commit 6f3e186bc7721c5b24ad90d4a751cccfccd445e6
Author: Josh Boyer <[email protected]>
Date: Fri Aug 5 08:47:23 2011 -0400

Add patch to fix 32bit EFI service mapping (rhbz 726701)

Signed-off-by: Tom Zanussi <[email protected]>
Signed-off-by: Darren Hart <[email protected]>

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 928bf83..6d7ecb5 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -888,10 +888,13 @@ void __init efi_enter_virtual_mode(void)

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;
+ if (!(md->attribute & EFI_MEMORY_RUNTIME)) {
+#ifdef CONFIG_X86_64
+ if (md->type != EFI_BOOT_SERVICES_CODE &&
+ md->type != EFI_BOOT_SERVICES_DATA)
+#endif
+ continue;
+ }

size = md->num_pages << EFI_PAGE_SHIFT;
end = md->phys_addr + size;

>
> ---
>
> From 37ba0d4f43e0f8ec3e3342bc1331e36125495d3e Mon Sep 17 00:00:00 2001
> From: Matt Fleming <[email protected]>
> Date: Thu, 18 Apr 2013 09:57:55 +0100
> Subject: [PATCH] x86, efi: Refuse to ioremap highmem on i386
>
> Bryan reports running into the following warning on i386,
>
> 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
>
> Due to the workaround described in commit 916f676f8 ("x86, efi: Retain
> boot service code until after switching to virtual mode") EFI Boot
> Service regions are mapped for a period during boot. Unfortunately, with
> the limited size of the i386 direct kernel map it's possible that some
> of the Boot Service regions will not be directly accessible, which
> causes them to be ioremap()'d, triggering the above warning as the
> regions are marked as E820_RAM in the e820 memmap.
>
> The simplest fix is to refuse to map ram (and therefore, any EFI Boot
> Service regions) in an i386-specific version of efi_ioremap(), because
> there is no method of mapping arbitrary sizes of highmem at contiguous
> virtual addresses in the kernel address space.
>
> There are currently only two situations where we need to map EFI Boot
> Service regions,
>
> 1. To workaround the firmware bug described in 916f676f8
> 2. To access the ACPI BGRT image
>
> but since we haven't seen an i386 implementation that requires either,
> this simple fix should suffice for now. Item 2. above does still work on
> i386 provided that the BGRT image is not in highmem.
>
> Reported-by: Bryan O'Donoghue <[email protected]>
> Cc: Josh Triplett <[email protected]>
> Cc: Darren Hart <[email protected]>
> Cc: Matthew Garrett <[email protected]>
> Cc: Josh Boyer <[email protected]>
> Cc: H. Peter Anvin <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Matt Fleming <[email protected]>
> ---
> arch/x86/include/asm/efi.h | 7 ++-----
> arch/x86/platform/efi/efi_32.c | 15 +++++++++++++++
> 2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index 2fb5d58..8f042bf 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -35,8 +35,6 @@ extern unsigned long asmlinkage efi_call_phys(void *, ...);
> #define efi_call_virt6(f, a1, a2, a3, a4, a5, a6) \
> efi_call_virt(f, a1, a2, a3, a4, a5, a6)
>
> -#define efi_ioremap(addr, size, type, attr) ioremap_cache(addr, size)
> -
> #else /* !CONFIG_X86_32 */
>
> #define EFI_LOADER_SIGNATURE "EL64"
> @@ -88,9 +86,6 @@ extern u64 efi_call6(void *fp, u64 arg1, u64 arg2, u64 arg3,
> efi_call6((void *)(efi.systab->runtime->f), (u64)(a1), (u64)(a2), \
> (u64)(a3), (u64)(a4), (u64)(a5), (u64)(a6))
>
> -extern void __iomem *efi_ioremap(unsigned long addr, unsigned long size,
> - u32 type, u64 attribute);
> -
> #endif /* CONFIG_X86_32 */
>
> extern int add_efi_memmap;
> @@ -101,6 +96,8 @@ extern void efi_call_phys_prelog(void);
> extern void efi_call_phys_epilog(void);
> extern void efi_unmap_memmap(void);
> extern void efi_memory_uc(u64 addr, unsigned long size);
> +extern void __iomem *efi_ioremap(unsigned long addr, unsigned long size,
> + u32 type, u64 attribute);
>
> struct efi_var_bootdata {
> struct setup_data data;
> diff --git a/arch/x86/platform/efi/efi_32.c b/arch/x86/platform/efi/efi_32.c
> index 40e4469..fbdfa97 100644
> --- a/arch/x86/platform/efi/efi_32.c
> +++ b/arch/x86/platform/efi/efi_32.c
> @@ -67,3 +67,18 @@ void efi_call_phys_epilog(void)
>
> local_irq_restore(efi_rt_eflags);
> }
> +
> +void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size,
> + u32 type, u64 attribute)
> +{
> + /*
> + * If we've been asked to map a chunk of ram, that means it is
> + * highmem and isn't accessible via the direct kernel mapping.
> + * i386 doesn't have a method for mapping arbitrary sized chunks
> + * of highmem into contiguous virtual addresses.
> + */
> + if (page_is_ram(phys_addr))
> + return NULL;
> +
> + return ioremap_cache(phys_addr, size);
> +}
>

--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

2013-04-18 15:01:19

by Matthew Garrett

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On Thu, 2013-04-18 at 09:40 -0400, Josh Boyer wrote:

> BGRT is a new addition in ACPI 5.0, right? Hopefully with it being
> relatively recent, and new 32-bit firmware being somewhat rare, it won't
> be a problem.

New 32-bit UEFI firmware is fairly common. Thanks, Intel.

--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2013-04-18 15:17:29

by Josh Boyer

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On Thu, Apr 18, 2013 at 03:01:12PM +0000, Matthew Garrett wrote:
> On Thu, 2013-04-18 at 09:40 -0400, Josh Boyer wrote:
>
> > BGRT is a new addition in ACPI 5.0, right? Hopefully with it being
> > relatively recent, and new 32-bit firmware being somewhat rare, it won't
> > be a problem.
>
> New 32-bit UEFI firmware is fairly common. Thanks, Intel.

See. I said I was the wrong person to weigh in. And I'm now also sad.

josh

2013-04-18 16:20:03

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On 18/04/13 15:51, Darren Hart wrote:
> I don't believe I have seen a 32-bit EFI system with a BGRT, but then
> again, I had to look it up today! That said, I suspect the MinnowBoard
> would benefit from such a thing, so we should have an example of it
> there in the near future.

That's fine and things will work as-is provided that the BGRT image is
not in highmem.

> Is this in anyway related to the following patch from Josh Boyer? We're
> carrying this in the Yocto Project trees currently.
>
> commit 6f3e186bc7721c5b24ad90d4a751cccfccd445e6
> Author: Josh Boyer <[email protected]>
> Date: Fri Aug 5 08:47:23 2011 -0400
>
> Add patch to fix 32bit EFI service mapping (rhbz 726701)
>
> Signed-off-by: Tom Zanussi <[email protected]>
> Signed-off-by: Darren Hart <[email protected]>

Yep, it's basically the same fix. My patch just avoids the #ifdef and
prints an error message if we have EFI Boot services regions we can't
access directly. The error message will at least be useful if we do
start seeing BGRT pointers in highmem.

Could you give it a spin on your MinnowBoard?

--
Matt Fleming, Intel Open Source Technology Center

2013-04-18 16:33:42

by Josh Triplett

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On Thu, Apr 18, 2013 at 12:00:26PM +0100, Matt Fleming wrote:
> > 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.
>
> You assume that mapping of the Boot Services regions is done purely for
> the benefit of pulling out the bgrt image - it's not, see the above
> commit log - and I assumed that you had an ACPI bgrt pointer in your
> memory map, but you don't.
>
> Darren, Josh, have you ever seen an i386 machine with a bgrt pointer? If
> not, and given that we've never seen an i386 firmware that requires the
> above workaround from Matthew, combined with the fact that there are so
> few i386 implementations out there, I'm inclined to apply the patch
> below, because anything else is a lot more work. We can address this
> properly if we ever start seeing i386 machines with bgrt pointers that
> reference highmem.

The machine I developed the BGRT changes on kept the image below the 4G
mark, inside one of the memory regions reclaimable via
ExitBootServices().

- Josh Triplett

2013-04-18 16:38:33

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On 04/18/2013 09:33 AM, Josh Triplett wrote:
> On Thu, Apr 18, 2013 at 12:00:26PM +0100, Matt Fleming wrote:
>>> 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.
>>
>> You assume that mapping of the Boot Services regions is done purely for
>> the benefit of pulling out the bgrt image - it's not, see the above
>> commit log - and I assumed that you had an ACPI bgrt pointer in your
>> memory map, but you don't.
>>
>> Darren, Josh, have you ever seen an i386 machine with a bgrt pointer? If
>> not, and given that we've never seen an i386 firmware that requires the
>> above workaround from Matthew, combined with the fact that there are so
>> few i386 implementations out there, I'm inclined to apply the patch
>> below, because anything else is a lot more work. We can address this
>> properly if we ever start seeing i386 machines with bgrt pointers that
>> reference highmem.
>
> The machine I developed the BGRT changes on kept the image below the 4G
> mark, inside one of the memory regions reclaimable via
> ExitBootServices().
>

Well, highmem is >= ~896M. Do you have a machine with BGRT over the
highmem mark?

-hpa

2013-04-18 16:45:13

by Josh Triplett

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On Thu, Apr 18, 2013 at 09:38:04AM -0700, H. Peter Anvin wrote:
> On 04/18/2013 09:33 AM, Josh Triplett wrote:
> > On Thu, Apr 18, 2013 at 12:00:26PM +0100, Matt Fleming wrote:
> >>> 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.
> >>
> >> You assume that mapping of the Boot Services regions is done purely for
> >> the benefit of pulling out the bgrt image - it's not, see the above
> >> commit log - and I assumed that you had an ACPI bgrt pointer in your
> >> memory map, but you don't.
> >>
> >> Darren, Josh, have you ever seen an i386 machine with a bgrt pointer? If
> >> not, and given that we've never seen an i386 firmware that requires the
> >> above workaround from Matthew, combined with the fact that there are so
> >> few i386 implementations out there, I'm inclined to apply the patch
> >> below, because anything else is a lot more work. We can address this
> >> properly if we ever start seeing i386 machines with bgrt pointers that
> >> reference highmem.
> >
> > The machine I developed the BGRT changes on kept the image below the 4G
> > mark, inside one of the memory regions reclaimable via
> > ExitBootServices().
>
> Well, highmem is >= ~896M. Do you have a machine with BGRT over the
> highmem mark?

I don't have the machine in question anymore, and I don't remember.

- Josh Triplett

2013-04-18 19:55:26

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On 04/18/2013 05:44 PM, Josh Triplett wrote:
>>> The machine I developed the BGRT changes on kept the image below the 4G
>>> mark, inside one of the memory regions reclaimable via
>>> ExitBootServices().
>>
>> Well, highmem is >= ~896M. Do you have a machine with BGRT over the
>> highmem mark?
>
> I don't have the machine in question anymore, and I don't remember.

Sorry, I should have been more clear - having a BGRT image in highmem
has never worked for the reasons I outlined in my previous mail. What I
was really asking was: is it OK that we now explicitly don't support
that case? I'm working on the assumption that it's pointless writing
support for the BGRT in highmem because no such i386 machines exist. If
the BGRT code works for your i386 right now, the address isn't in highmem.

If there are machines out there that would require us to write support,
it's probably worth doing now instead of punting. But it sounds like
there aren't any.

--
Matt Fleming, Intel Open Source Technology Center

2013-04-18 19:58:23

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On 04/18/2013 12:55 PM, Matt Fleming wrote:
> On 04/18/2013 05:44 PM, Josh Triplett wrote:
>>>> The machine I developed the BGRT changes on kept the image below the 4G
>>>> mark, inside one of the memory regions reclaimable via
>>>> ExitBootServices().
>>>
>>> Well, highmem is >= ~896M. Do you have a machine with BGRT over the
>>> highmem mark?
>>
>> I don't have the machine in question anymore, and I don't remember.
>
> Sorry, I should have been more clear - having a BGRT image in highmem
> has never worked for the reasons I outlined in my previous mail. What I
> was really asking was: is it OK that we now explicitly don't support
> that case? I'm working on the assumption that it's pointless writing
> support for the BGRT in highmem because no such i386 machines exist. If
> the BGRT code works for your i386 right now, the address isn't in highmem.
>
> If there are machines out there that would require us to write support,
> it's probably worth doing now instead of punting. But it sounds like
> there aren't any.
>

I suspect if there aren't any yet there WILL be.

-hpa

2013-04-18 19:58:46

by Matthew Garrett

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On Thu, 2013-04-18 at 20:55 +0100, Matt Fleming wrote:

> If there are machines out there that would require us to write support,
> it's probably worth doing now instead of punting. But it sounds like
> there aren't any.

I would expect that if there are any 32-bit UEFI systems that ship with
BGRT support (and Darren makes it sound like that's a possibility),
there's a realistic chance of the BGRT ending up allocated above the
highmem barrier.

--
Matthew Garrett | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2013-04-18 20:11:22

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On 04/18/2013 12:58 PM, Matthew Garrett wrote:
> On Thu, 2013-04-18 at 20:55 +0100, Matt Fleming wrote:
>
>> If there are machines out there that would require us to write support,
>> it's probably worth doing now instead of punting. But it sounds like
>> there aren't any.
>
> I would expect that if there are any 32-bit UEFI systems that ship with
> BGRT support (and Darren makes it sound like that's a possibility),
> there's a realistic chance of the BGRT ending up allocated above the
> highmem barrier.

I can certainly ensure we sit below that barrier on the MinnowBoard. We
can also speak with the Intel UEFI firmware development teams to see
about making that a requirement. I don't know if we'll be successful,
but Matt, Peter, and I have recently coaxed some changes of that nature in.

--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

2013-04-18 20:13:55

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On 04/18/2013 01:11 PM, Darren Hart wrote:
>>
>> I would expect that if there are any 32-bit UEFI systems that ship with
>> BGRT support (and Darren makes it sound like that's a possibility),
>> there's a realistic chance of the BGRT ending up allocated above the
>> highmem barrier.
>
> I can certainly ensure we sit below that barrier on the MinnowBoard. We
> can also speak with the Intel UEFI firmware development teams to see
> about making that a requirement. I don't know if we'll be successful,
> but Matt, Peter, and I have recently coaxed some changes of that nature in.
>

No, that is the wrong approach. The HIGHMEM barrier is a Linux kernel
construct and isn't even guaranteed to be the same from one boot *of the
same kernel* to another. The value 896M is merely the default, but it
can be affected by both compile time and command line options.

-hpa

2013-04-18 20:18:13

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On 04/18/2013 01:13 PM, H. Peter Anvin wrote:
> On 04/18/2013 01:11 PM, Darren Hart wrote:
>>>
>>> I would expect that if there are any 32-bit UEFI systems that ship with
>>> BGRT support (and Darren makes it sound like that's a possibility),
>>> there's a realistic chance of the BGRT ending up allocated above the
>>> highmem barrier.
>>
>> I can certainly ensure we sit below that barrier on the MinnowBoard. We
>> can also speak with the Intel UEFI firmware development teams to see
>> about making that a requirement. I don't know if we'll be successful,
>> but Matt, Peter, and I have recently coaxed some changes of that nature in.
>>
>
> No, that is the wrong approach. The HIGHMEM barrier is a Linux kernel
> construct and isn't even guaranteed to be the same from one boot *of the
> same kernel* to another. The value 896M is merely the default, but it
> can be affected by both compile time and command line options.
>
> -hpa

Ah, well then I've misunderstood the nature of the problem a bit. Will
have to spend some time understanding this better.

--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

2013-04-18 20:46:27

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On 04/18/2013 01:17 PM, Darren Hart wrote:
>
> Ah, well then I've misunderstood the nature of the problem a bit. Will
> have to spend some time understanding this better.
>

Way, way too many people misunderstand HIGHMEM.

-hpa

2013-04-18 22:04:38

by Bryan O'Donoghue

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

> UEFI stands for "Unified Extensible Firmware Interface", where "Firmware"
> is an ancient African word meaning "Why do something right when you can
> do it so wrong that children will weep and brave adults will cower before
> you", and "UEI" is Celtic for "We missed DOS so we burned it into your
> ROMs". The UEFI specification provides for runtime services (ie, another
> way for the operating system to be forced to depend on the firmware) and
> we rely on these for certain trivial tasks such as setting up the
> bootloader. But some hardware fails to work if we attempt to use these
> runtime services from physical mode, and so we have to switch into virtual
> mode. So far so dreadful.
>

"UEI" is Celtic for "We missed DOS so we burned it into your ROMS"

I love it "maith an fear"

> There are currently only two situations where we need to map EFI Boot
> Service regions,
>
> 1. To workaround the firmware bug described in 916f676f8
> 2. To access the ACPI BGRT image
>
> but since we haven't seen an i386 implementation that requires either,
> this simple fix should suffice for now. Item 2. above does still work on
> i386 provided that the BGRT image is not in highmem.

Matt, Peter, Josh, Darren.

Given it's not possible to guarantee someone won't stuff a BGRT into
EFI_BOOT_MEMORY >= highmem eventually (and indeed the axioms of the
universe pretty much guarantee eventually it will be so) - I'd suggest
version 2.

A kernel parameter - rather than a probe for BGRT - since we anticipate
BIOS bugs on the way.

Version 2 of the submitted path introduces an early kernel parameter
"virt_mapboot" - which is true by default (maintaining the current
behavior of mapping EFI_BOOT_MEMORY by default) - but which can be set
to false - if your IA32 BIOS is not buggy.

Perhaps it would be better to be optimistic.

Change the behavior of efi_enter_virtual_mode() to do the right thing
re: the standard and require passing of a parameter to switch on
work-arounds for non-standards conformant BIOS. Note: this approach
would break BGRT code - requiring addition of kernel parameters to
existing systems - which from a user-friendliness POV is probably
verboten....

2013-04-18 23:01:33

by Josh Triplett

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On Thu, Apr 18, 2013 at 11:07:06PM +0100, Bryan O'Donoghue wrote:
>
> > UEFI stands for "Unified Extensible Firmware Interface", where "Firmware"
> > is an ancient African word meaning "Why do something right when you can
> > do it so wrong that children will weep and brave adults will cower before
> > you", and "UEI" is Celtic for "We missed DOS so we burned it into your
> > ROMs". The UEFI specification provides for runtime services (ie, another
> > way for the operating system to be forced to depend on the firmware) and
> > we rely on these for certain trivial tasks such as setting up the
> > bootloader. But some hardware fails to work if we attempt to use these
> > runtime services from physical mode, and so we have to switch into virtual
> > mode. So far so dreadful.
> >
>
> "UEI" is Celtic for "We missed DOS so we burned it into your ROMS"
>
> I love it "maith an fear"
>
> >There are currently only two situations where we need to map EFI Boot
> >Service regions,
> >
> > 1. To workaround the firmware bug described in 916f676f8
> > 2. To access the ACPI BGRT image
> >
> >but since we haven't seen an i386 implementation that requires either,
> >this simple fix should suffice for now. Item 2. above does still work on
> >i386 provided that the BGRT image is not in highmem.
>
> Matt, Peter, Josh, Darren.
>
> Given it's not possible to guarantee someone won't stuff a BGRT into
> EFI_BOOT_MEMORY >= highmem eventually (and indeed the axioms of the
> universe pretty much guarantee eventually it will be so) - I'd
> suggest version 2.
>
> A kernel parameter - rather than a probe for BGRT - since we
> anticipate BIOS bugs on the way.
>
> Version 2 of the submitted path introduces an early kernel parameter
> "virt_mapboot" - which is true by default (maintaining the current
> behavior of mapping EFI_BOOT_MEMORY by default) - but which can be
> set to false - if your IA32 BIOS is not buggy.
>
> Perhaps it would be better to be optimistic.
>
> Change the behavior of efi_enter_virtual_mode() to do the right
> thing re: the standard and require passing of a parameter to switch
> on work-arounds for non-standards conformant BIOS. Note: this
> approach would break BGRT code - requiring addition of kernel
> parameters to existing systems - which from a user-friendliness POV
> is probably verboten....

I'd much rather see the code do the right thing automatically, rather
than requiring an "unbreak me" command-line parameter.

But in any case, since BGRT is a "make my system look prettier" feature
rather than core functionality, giving up on it on 32-bit EFI seems
fairly reasonable, especially since it may still work if stored
sufficiently low in memory.

- Josh Triplett

2013-04-18 23:42:45

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

>
> "UEI" is Celtic for "We missed DOS so we burned it into your ROMS"
>
> I love it "maith an fear"
>
>> There are currently only two situations where we need to map EFI Boot
>> Service regions,
>>
>> 1. To workaround the firmware bug described in 916f676f8
>> 2. To access the ACPI BGRT image
>>
>> but since we haven't seen an i386 implementation that requires either,
>> this simple fix should suffice for now. Item 2. above does still work on
>> i386 provided that the BGRT image is not in highmem.
>
> Matt, Peter, Josh, Darren.
>
> Given it's not possible to guarantee someone won't stuff a BGRT into
> EFI_BOOT_MEMORY >= highmem eventually (and indeed the axioms of the
> universe pretty much guarantee eventually it will be so) - I'd suggest
> version 2.
>

Quite frankly, it is quite likely.

> A kernel parameter - rather than a probe for BGRT - since we anticipate
> BIOS bugs on the way.

Pardon? Kernel parameter for what?

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2013-04-18 23:45:11

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On 04/18/2013 04:01 PM, Josh Triplett wrote:
>
> I'd much rather see the code do the right thing automatically, rather
> than requiring an "unbreak me" command-line parameter.
>
> But in any case, since BGRT is a "make my system look prettier" feature
> rather than core functionality, giving up on it on 32-bit EFI seems
> fairly reasonable, especially since it may still work if stored
> sufficiently low in memory.
>

No, it really isn't. Quite frankly, I don't understand why HIGHMEM
makes it any harder... in a lot of ways HIGHMEM memory is easier to deal
with because it is touched much later in the process. Sure, it has to
be mapped before you can copy it out, but that usually isn't a big deal.

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.

2013-04-19 00:18:09

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On 04/18/2013 09:19 AM, Matt Fleming wrote:
> On 18/04/13 15:51, Darren Hart wrote:
>> I don't believe I have seen a 32-bit EFI system with a BGRT, but then
>> again, I had to look it up today! That said, I suspect the MinnowBoard
>> would benefit from such a thing, so we should have an example of it
>> there in the near future.
>
> That's fine and things will work as-is provided that the BGRT image is
> not in highmem.
>
>> Is this in anyway related to the following patch from Josh Boyer? We're
>> carrying this in the Yocto Project trees currently.
>>
>> commit 6f3e186bc7721c5b24ad90d4a751cccfccd445e6
>> Author: Josh Boyer <[email protected]>
>> Date: Fri Aug 5 08:47:23 2011 -0400
>>
>> Add patch to fix 32bit EFI service mapping (rhbz 726701)
>>
>> Signed-off-by: Tom Zanussi <[email protected]>
>> Signed-off-by: Darren Hart <[email protected]>
>
> Yep, it's basically the same fix. My patch just avoids the #ifdef and
> prints an error message if we have EFI Boot services regions we can't
> access directly. The error message will at least be useful if we do
> start seeing BGRT pointers in highmem.
>
> Could you give it a spin on your MinnowBoard?

I've removed the patch I reference above and applied your patch to my
3.8.4 MinnowBoard dev tree. It panics with:

------------[ cut here ]------------
WARNING: at arch/x86/mm/ioremap.c:102 __ioremap_caller+0x2cb/0x2f0()
Hardware name: Minnow
Modules linked in:
Pid: 0, comm: swapper/0 Not tainted 3.8.4-yocto-standard+ #111
Call Trace:
[<c102faf3>] warn_slowpath_common+0x73/0xa0
[<c102752b>] ? __ioremap_caller+0x2cb/0x2f0
[<c102752b>] ? __ioremap_caller+0x2cb/0x2f0
[<c102fb43>] warn_slowpath_null+0x23/0x30
[<c102752b>] __ioremap_caller+0x2cb/0x2f0
[<c110b248>] ? kmem_cache_alloc_trace+0x58/0x120
[<c110b33f>] ? sysfs_slab_alias+0x2f/0x80
[<c165d948>] ? add_preempt_count+0x8/0x80
[<c165d8c8>] ? sub_preempt_count+0x8/0x80
[<c10387aa>] ? walk_system_ram_range+0xfa/0x110
[<c1027599>] ioremap_cache+0x19/0x20
[<c1932889>] ? efi_ioremap+0x1b/0x23
[<c1932889>] efi_ioremap+0x1b/0x23
[<c19326bd>] efi_enter_virtual_mode+0x195/0x346
[<c19208b8>] start_kernel+0x288/0x30b
[<c1920462>] ? repair_env_string+0x51/0x51
[<c19202a2>] i386_start_kernel+0x78/0x7d
---[ end trace f74542647d553317 ]---
efi: ioremap of 0x3DE3F000 failed!
efi: ioremap of 0x3E601000 failed!
efi: ioremap of 0x3E602000 failed!
efi: ioremap of 0x3E609000 failed!
efi: ioremap of 0x3E60C000 failed!
efi: ioremap of 0x3E614000 failed!
efi: ioremap of 0x3E617000 failed!
efi: ioremap of 0x3E621000 failed!
efi: ioremap of 0x3E622000 failed!
efi: ioremap of 0x3E627000 failed!
efi: ioremap of 0x3E628000 failed!
efi: ioremap of 0x3E62D000 failed!
efi: ioremap of 0x3E62E000 failed!
efi: ioremap of 0x3E631000 failed!
efi: ioremap of 0x3E632000 failed!
efi: ioremap of 0x3E63E000 failed!
efi: ioremap of 0x3EA0E000 failed!
efi: ioremap of 0x3EA0F000 failed!
efi: ioremap of 0x3EA11000 failed!
efi: ioremap of 0x3EA1A000 failed!
efi: ioremap of 0x3EA1B000 failed!
efi: ioremap of 0x3EA1D000 failed!
efi: ioremap of 0x3EC0B000 failed!
efi: ioremap of 0x3EC0F000 failed!
efi: ioremap of 0x3F33F000 failed!
efi: ioremap of 0xFED1C000 failed!
------------[ cut here ]------------
kernel BUG at arch/x86/platform/efi/efi.c:933!
invalid opcode: 0000 [#1] PREEMPT SMP
Modules linked in:
Pid: 0, comm: swapper/0 Tainted: G W 3.8.4-yocto-standard+
#111 Circuitco Minnow/Minnow Board

EIP: 0060:[<c193276c>] EFLAGS: 00010246 CPU: 0
EIP is at efi_enter_virtual_mode+0x244/0x346
EAX: ee012930 EBX: 00000030 ECX: 00000000 EDX: 000000d0
ESI: ffe190b0 EDI: ee012960 EBP: c18adfc4 ESP: c18adf7c
DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
CR0: 8005003b CR2: ffe17000 CR3: 019b4000 CR4: 00000690
DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
DR6: ffff0ff0 DR7: 00000400
Process swapper/0 (pid: 0, ti=c18ac000 task=c18b70e0 task.ti=c18ac000)
Stack:
00000000 80000000 00000000 00040000 00040000 80000000 0000a000 00000000
0002effe 00000000 00000032 ee012000 fffaa000 00000000 ffe190b0 c195b3c0
0008a800 c18ae800 c18adfe4 c19208b8 00000113 ffffffff ffffffff c1920462
Call Trace:
[<c19208b8>] start_kernel+0x288/0x30b
[<c1920462>] ? repair_env_string+0x51/0x51
[<c19202a2>] i386_start_kernel+0x78/0x7d
Code: 03 45 e4 89 c7 f3 a4 89 5d e0 8b 1d fc d7 9b c1 01 5d f0 8b 75 f0
39 35 f0 d7 9b c1 0f 87 82 fe ff ff 83 3d a0 be 91 c1 00 75 02 <0f> 0b
8b 1d fc d7 9b c1 8b 75 e0 0f af f3 8b 3d f8 d7 9b c1 e8

EIP: [<c193276c>] efi_enter_virtual_mode+0x244/0x346 SS:ESP 0068:c18adf7c
---[ end trace f74542647d553318 ]---
Kernel panic - not syncing: Attempted to kill the idle task!

--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

2013-04-19 07:50:25

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On 04/19/2013 01:18 AM, Darren Hart wrote:
> On 04/18/2013 09:19 AM, Matt Fleming wrote:
>>
>> Could you give it a spin on your MinnowBoard?
>
> I've removed the patch I reference above and applied your patch to my
> 3.8.4 MinnowBoard dev tree. It panics with:

D'oh. OK, at this point I'm inclined to apply Josh Boyer's patch on top
of my urgent branch which will address the WARNING people are hitting on
i386. I updated the commit message a little.

Josh (Boyer), are you guys still carrying this patch and have you seen
any fallout? I notice your SoB isn't on the patch that Darren posted, am
I OK to add it?

---

>From 40f053eb6ccb3f0c462ef7a23c44c3264d87a0d4 Mon Sep 17 00:00:00 2001
From: Josh Boyer <[email protected]>
Date: Thu, 18 Apr 2013 07:51:34 -0700
Subject: [PATCH] x86, efi: Don't map Boot Services on i386

Add patch to fix 32bit EFI service mapping (rhbz 726701)

Multiple people are reporting hitting the following WARNING on i386,

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

Due to the workaround described in commit 916f676f8 ("x86, efi: Retain
boot service code until after switching to virtual mode") EFI Boot
Service regions are mapped for a period during boot. Unfortunately, with
the limited size of the i386 direct kernel map it's possible that some
of the Boot Service regions will not be directly accessible, which
causes them to be ioremap()'d, triggering the above warning as the
regions are marked as E820_RAM in the e820 memmap.

There are currently only two situations where we need to map EFI Boot
Service regions,

1. To workaround the firmware bug described in 916f676f8
2. To access the ACPI BGRT image

but since we haven't seen an i386 implementation that requires either,
this simple fix should suffice for now.

[ Added to changelog - Matt ]

Reported-by: Bryan O'Donoghue <[email protected]>
Acked-by: Tom Zanussi <[email protected]>
Acked-by: Darren Hart <[email protected]>
Cc: Josh Triplett <[email protected]>
Cc: Matthew Garrett <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: <[email protected]>
Signed-off-by: Josh Boyer <[email protected]>
Signed-off-by: Matt Fleming <[email protected]>
---
arch/x86/platform/efi/efi.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index e4a86a6..b9876aa 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -997,10 +997,13 @@ void __init efi_enter_virtual_mode(void)

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;
+ if (!(md->attribute & EFI_MEMORY_RUNTIME)) {
+#ifdef CONFIG_X86_64
+ if (md->type != EFI_BOOT_SERVICES_CODE &&
+ md->type != EFI_BOOT_SERVICES_DATA)
+#endif
+ continue;
+ }

size = md->num_pages << EFI_PAGE_SHIFT;
end = md->phys_addr + size;
--
1.8.1.4

--
Matt Fleming, Intel Open Source Technology Center

2013-04-19 12:01:44

by Josh Boyer

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On Fri, Apr 19, 2013 at 08:50:14AM +0100, Matt Fleming wrote:
> On 04/19/2013 01:18 AM, Darren Hart wrote:
> > On 04/18/2013 09:19 AM, Matt Fleming wrote:
> >>
> >> Could you give it a spin on your MinnowBoard?
> >
> > I've removed the patch I reference above and applied your patch to my
> > 3.8.4 MinnowBoard dev tree. It panics with:
>
> D'oh. OK, at this point I'm inclined to apply Josh Boyer's patch on top
> of my urgent branch which will address the WARNING people are hitting on
> i386. I updated the commit message a little.
>
> Josh (Boyer), are you guys still carrying this patch and have you seen
> any fallout? I notice your SoB isn't on the patch that Darren posted, am
> I OK to add it?

Yeah, we're still carrying it. We've had it around since August of 2011
to cover the bug mentioned. I'm not aware of any further fallout it
might have caused, but again we don't support 32-bit EFI in Fedora.
That's not to say Fedora won't work, we just don't focus on it.

I don't actually remember authoring the patch, but it's been a while so:

Signed-off-by: Josh Boyer <[email protected]>

josh

2013-09-10 17:43:33

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On Fri, 2013-04-19 at 08:50 +0100, Matt Fleming wrote:
> On 04/19/2013 01:18 AM, Darren Hart wrote:
> > On 04/18/2013 09:19 AM, Matt Fleming wrote:
> >>
> >> Could you give it a spin on your MinnowBoard?
> >
> > I've removed the patch I reference above and applied your patch to my
> > 3.8.4 MinnowBoard dev tree. It panics with:
>
> D'oh. OK, at this point I'm inclined to apply Josh Boyer's patch on top
> of my urgent branch which will address the WARNING people are hitting on
> i386. I updated the commit message a little.
>
> Josh (Boyer), are you guys still carrying this patch and have you seen
> any fallout? I notice your SoB isn't on the patch that Darren posted, am
> I OK to add it?

Josh OK'd this, but as far as I can tell, it hasn't made it upstream
yet. Matt was there an alternate fixed pushed?

The patch from Josh we're referring to here I believe is:

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index e4a86a6..b9876aa 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -997,10 +997,13 @@ void __init efi_enter_virtual_mode(void)

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;
+ if (!(md->attribute & EFI_MEMORY_RUNTIME)) {
+#ifdef CONFIG_X86_64
+ if (md->type != EFI_BOOT_SERVICES_CODE &&
+ md->type != EFI_BOOT_SERVICES_DATA)
+#endif
+ continue;
+ }

size = md->num_pages << EFI_PAGE_SHIFT;
end = md->phys_addr + size;

--
Darren

>
> ---
>
> From 40f053eb6ccb3f0c462ef7a23c44c3264d87a0d4 Mon Sep 17 00:00:00 2001
> From: Josh Boyer <[email protected]>
> Date: Thu, 18 Apr 2013 07:51:34 -0700
> Subject: [PATCH] x86, efi: Don't map Boot Services on i386
>
> Add patch to fix 32bit EFI service mapping (rhbz 726701)
>
> Multiple people are reporting hitting the following WARNING on i386,
>
> 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
>
> Due to the workaround described in commit 916f676f8 ("x86, efi: Retain
> boot service code until after switching to virtual mode") EFI Boot
> Service regions are mapped for a period during boot. Unfortunately, with
> the limited size of the i386 direct kernel map it's possible that some
> of the Boot Service regions will not be directly accessible, which
> causes them to be ioremap()'d, triggering the above warning as the
> regions are marked as E820_RAM in the e820 memmap.
>
> There are currently only two situations where we need to map EFI Boot
> Service regions,
>
> 1. To workaround the firmware bug described in 916f676f8
> 2. To access the ACPI BGRT image
>
> but since we haven't seen an i386 implementation that requires either,
> this simple fix should suffice for now.
>
> [ Added to changelog - Matt ]
>
> Reported-by: Bryan O'Donoghue <[email protected]>
> Acked-by: Tom Zanussi <[email protected]>
> Acked-by: Darren Hart <[email protected]>
> Cc: Josh Triplett <[email protected]>
> Cc: Matthew Garrett <[email protected]>
> Cc: H. Peter Anvin <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Josh Boyer <[email protected]>
> Signed-off-by: Matt Fleming <[email protected]>
> ---
> arch/x86/platform/efi/efi.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index e4a86a6..b9876aa 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -997,10 +997,13 @@ void __init efi_enter_virtual_mode(void)
>
> 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;
> + if (!(md->attribute & EFI_MEMORY_RUNTIME)) {
> +#ifdef CONFIG_X86_64
> + if (md->type != EFI_BOOT_SERVICES_CODE &&
> + md->type != EFI_BOOT_SERVICES_DATA)
> +#endif
> + continue;
> + }
>
> size = md->num_pages << EFI_PAGE_SHIFT;
> end = md->phys_addr + size;
> --
> 1.8.1.4
>

--
Darren Hart
Intel Open Source Technology Center
Yocto Project - Linux Kernel

2013-09-12 07:55:26

[permalink] [raw]

Subject: Re: [PATCH] Remove warning in efi_enter_virtual_mode

On Tue, 10 Sep, at 10:43:27AM, Darren Hart wrote:
> Josh OK'd this, but as far as I can tell, it hasn't made it upstream
> yet. Matt was there an alternate fixed pushed?

Nope, this one slipped through the cracks. Thanks for following up!

Here's the patch I just applied on the 'urgent' branch.

---

>From e285df5368b33efcc9a0ca5b4ed87b0e0c590218 Mon Sep 17 00:00:00 2001
From: Josh Boyer <[email protected]>
Date: Thu, 18 Apr 2013 07:51:34 -0700
Subject: [PATCH] x86, efi: Don't map Boot Services on i386

Add patch to fix 32bit EFI service mapping (rhbz 726701)

Multiple people are reporting hitting the following WARNING on i386,

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

Due to the workaround described in commit 916f676f8 ("x86, efi: Retain
boot service code until after switching to virtual mode") EFI Boot
Service regions are mapped for a period during boot. Unfortunately, with
the limited size of the i386 direct kernel map it's possible that some
of the Boot Service regions will not be directly accessible, which
causes them to be ioremap()'d, triggering the above warning as the
regions are marked as E820_RAM in the e820 memmap.

There are currently only two situations where we need to map EFI Boot
Service regions,

1. To workaround the firmware bug described in 916f676f8
2. To access the ACPI BGRT image

but since we haven't seen an i386 implementation that requires either,
this simple fix should suffice for now.

[ Added to changelog - Matt ]

Reported-by: Bryan O'Donoghue <[email protected]>
Acked-by: Tom Zanussi <[email protected]>
Acked-by: Darren Hart <[email protected]>
Cc: Josh Triplett <[email protected]>
Cc: Matthew Garrett <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: <[email protected]>
Signed-off-by: Josh Boyer <[email protected]>
Signed-off-by: Matt Fleming <[email protected]>
---
arch/x86/platform/efi/efi.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 90f6ed1..c7e22ab 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -912,10 +912,13 @@ void __init efi_enter_virtual_mode(void)

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;
+ if (!(md->attribute & EFI_MEMORY_RUNTIME)) {
+#ifdef CONFIG_X86_64
+ if (md->type != EFI_BOOT_SERVICES_CODE &&
+ md->type != EFI_BOOT_SERVICES_DATA)
+#endif
+ continue;
+ }

size = md->num_pages << EFI_PAGE_SHIFT;
end = md->phys_addr + size;
--
1.8.1.4

--
Matt Fleming, Intel Open Source Technology Center

2013-09-12 22:31:20