[PATCH] x86, efi: Calling __pa() with an ioremap'd address is invalid

[PATCH] x86, efi: Calling __pa() with an ioremap'd address is invalid

[Matt Fleming]

From: Matt Fleming <matt.fleming@intel.com>

If we encounter an efi_memory_desc_t without EFI_MEMORY_WB set in
->attribute we currently call set_memory_uc(), which in turn calls
__pa() on a potentially ioremap'd address. On CONFIG_X86_32 this is
invalid, resulting in the following oops,

  BUG: unable to handle kernel paging request at f7f22280
  IP: [<c10257b9>] reserve_ram_pages_type+0x89/0x210
  *pdpt = 0000000001978001 *pde = 0000000001ffb067 *pte = 0000000000000000
  Oops: 0000 [#1] PREEMPT SMP
  Modules linked in:

  Pid: 0, comm: swapper Not tainted 3.0.0-acpi-efi-0805 #3
   EIP: 0060:[<c10257b9>] EFLAGS: 00010202 CPU: 0
   EIP is at reserve_ram_pages_type+0x89/0x210
   EAX: 0070e280 EBX: 38714000 ECX: f7814000 EDX: 00000000
   ESI: 00000000 EDI: 38715000 EBP: c189fef0 ESP: c189fea8
   DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
  Process swapper (pid: 0, ti=c189e000 task=c18bbe60 task.ti=c189e000)
  Stack:
   80000200 ff108000 00000000 c189ff00 00038714 00000000 00000000 c189fed0
   c104f8ca 00038714 00000000 00038715 00000000 00000000 00038715 00000000
   00000010 38715000 c189ff48 c1025aff 38715000 00000000 00000010 00000000
  Call Trace:
   [<c104f8ca>] ? page_is_ram+0x1a/0x40
   [<c1025aff>] reserve_memtype+0xdf/0x2f0
   [<c1024dc9>] set_memory_uc+0x49/0xa0
   [<c19334d0>] efi_enter_virtual_mode+0x1c2/0x3aa
   [<c19216d4>] start_kernel+0x291/0x2f2
   [<c19211c7>] ? loglevel+0x1b/0x1b
   [<c19210bf>] i386_start_kernel+0xbf/0xc8

Let efi_ioremap() handle marking regions as uncached, whether that is
by calling ioremap_nocache() or calling set_memory_uc() on a
non-ioremap'd address.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <ming@elte.hu>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Huang Ying <huang.ying.caritas@gmail.com>
Cc: stable@kernel.org
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 arch/x86/include/asm/efi.h     |   11 +++++++++--
 arch/x86/platform/efi/efi.c    |   21 +++++++++++----------
 arch/x86/platform/efi/efi_64.c |   12 ++++++++++--
 3 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 7093e4a..d0ccbc4 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -33,7 +33,14 @@ 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)		ioremap_cache(addr, size)
+static inline void __iomem *efi_ioremap(unsigned long addr, unsigned long size,
+					u32 type, u64 attr)
+{
+	if (type == EFI_MEMORY_MAPPED_IO || !(attr & EFI_MEMORY_WB))
+		return ioremap_nocache(addr, size);
+	else
+		return ioremap_cache(addr, size);
+}
 
 #else /* !CONFIG_X86_32 */
 
@@ -85,7 +92,7 @@ extern u64 efi_call6(void *fp, u64 arg1, u64 arg2, u64 arg3,
 		  (u64)(a3), (u64)(a4), (u64)(a5), (u64)(a6))
 
 extern void __iomem *efi_ioremap(unsigned long addr, unsigned long size,
-				 u32 type);
+				 u32 type, u64 attr);
 
 #endif /* CONFIG_X86_32 */
 
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 3ae4128..87f2295 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -670,10 +670,18 @@ void __init efi_enter_virtual_mode(void)
 		end_pfn = PFN_UP(end);
 		if (end_pfn <= max_low_pfn_mapped
 		    || (end_pfn > (1UL << (32 - PAGE_SHIFT))
-			&& end_pfn <= max_pfn_mapped))
+			&& end_pfn <= max_pfn_mapped)) {
 			va = __va(md->phys_addr);
-		else
-			va = efi_ioremap(md->phys_addr, size, md->type);
+
+			if (!(md->attribute & EFI_MEMORY_WB)) {
+				addr = (u64) (unsigned long)va;
+				npages = md->num_pages;
+				memrange_efi_to_native(&addr, &npages);
+				set_memory_uc(addr, npages);
+			}
+		} else
+			va = efi_ioremap(md->phys_addr, size,
+					 md->type, md->attribute);
 
 		md->virt_addr = (u64) (unsigned long) va;
 
@@ -683,13 +691,6 @@ void __init efi_enter_virtual_mode(void)
 			continue;
 		}
 
-		if (!(md->attribute & EFI_MEMORY_WB)) {
-			addr = md->virt_addr;
-			npages = md->num_pages;
-			memrange_efi_to_native(&addr, &npages);
-			set_memory_uc(addr, npages);
-		}
-
 		systab = (u64) (unsigned long) efi_phys.systab;
 		if (md->phys_addr <= systab && systab < end) {
 			systab += md->virt_addr - md->phys_addr;
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index ac3aa54..6d6e877 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -82,9 +82,10 @@ void __init efi_call_phys_epilog(void)
 }
 
 void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size,
-				 u32 type)
+				 u32 type, u64 attr)
 {
 	unsigned long last_map_pfn;
+	u64 addr, npages;
 
 	if (type == EFI_MEMORY_MAPPED_IO)
 		return ioremap(phys_addr, size);
@@ -92,7 +93,14 @@ void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size,
 	last_map_pfn = init_memory_mapping(phys_addr, phys_addr + size);
 	if ((last_map_pfn << PAGE_SHIFT) < phys_addr + size) {
 		unsigned long top = last_map_pfn << PAGE_SHIFT;
-		efi_ioremap(top, size - (top - phys_addr), type);
+		efi_ioremap(top, size - (top - phys_addr), type, attr);
+	}
+
+	if (!(attr & EFI_MEMORY_WB)) {
+		addr = (u64)(unsigned long)__va(phys_addr);
+		npages = size >> EFI_PAGE_SHIFT;
+		memrange_efi_to_native(&addr, &npages);
+		set_memory_uc(addr, npages);
 	}
 
 	return (void __iomem *)__va(phys_addr);
-- 
1.7.4.4

Re: [PATCH] x86, efi: Calling __pa() with an ioremap'd address is invalid

[Matthew Garrett]

Is there any chance of unifying the 32 and 64-bit code here as well?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] x86, efi: Calling __pa() with an ioremap'd address is invalid

[Matt Fleming]

On Tue, 2011-10-11 at 14:03 +0100, Matthew Garrett wrote:
> Is there any chance of unifying the 32 and 64-bit code here as well?

I was under the impression that this wasn't possible for the cached case
because the 32-bit code path uses ioremap_cache() whereas 64-bit uses
init_memory_mapping() and __va().

Unless I'm missing something?

Re: [PATCH] x86, efi: Calling __pa() with an ioremap'd address is invalid

[hpanvin@gmail.com]

I think ioremap_cache() should work for either.

Matt Fleming <matt@console-pimps.org> wrote:

>On Tue, 2011-10-11 at 14:03 +0100, Matthew Garrett wrote:
>> Is there any chance of unifying the 32 and 64-bit code here as well?
>
>I was under the impression that this wasn't possible for the cached
>case
>because the 32-bit code path uses ioremap_cache() whereas 64-bit uses
>init_memory_mapping() and __va().
>
>Unless I'm missing something?

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH] x86, efi: Calling __pa() with an ioremap'd address is invalid

[Matt Fleming]

On Tue, 2011-10-11 at 08:45 -0700, hpanvin@gmail.com wrote:
> I think ioremap_cache() should work for either.

Sorry yeah, it will work but I don't think it's desirable (that's what I
meant, though I realise it's not what I wrote).

The reason being: aren't you doing an extra step of indirection because
you have to jump through the vmalloc space?

With 32-bit this is obviously necessary because of the lack of space for
the direct kernel map, but on 64-bit we can just extend the kernel map
and do quick conversions between physical and virtual space with __va()
and __pa().

Maybe the extra indirection isn't that bad and no one will care? If so,
then sure, a unified version sounds like a plan. I was just hesitant of
changing the existing 64-bit implementation for fear of receiving
replies like "you've now made the 64-bit path slower".

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] x86, efi: Calling __pa() with an ioremap'd address is invalid

[Matthew Garrett]

On Tue, Oct 11, 2011 at 04:58:09PM +0100, Matt Fleming wrote:

> Maybe the extra indirection isn't that bad and no one will care? If so,
> then sure, a unified version sounds like a plan. I was just hesitant of
> changing the existing 64-bit implementation for fear of receiving
> replies like "you've now made the 64-bit path slower".

If we have UEFI calls on the fast path then I think we're already 
losing. I'd favour simplicity over performance here, personally.

-- 
Matthew Garrett | mjg59@srcf.ucam.org