EFI: Do not use __pa() to get the physical address of an ioremapped memory range

PATCH In review Zhang Rui 4 participants 11 messages Sep 1, 2011 – Sep 15, 2011

Subsystems (2)

x86 EFI

lore

Share & tools

lore

Zhang Rui

author

2011-09-01 06:55 UTC #0 lore raw

PATCH 1/1 EFI: Do not use __pa() to get the physical address of an ioremapped memory range

From: Zhang Rui <[email protected]>
Date: Wed, 31 Aug 2011 09:59:01 +0800
Subject: Do not use __pa() to get the physical address of an ioremapped memory range.

set_memory_uc uses __pa() to translate the virtual address to the physical address. This breaks a EFI_MEMORY_MAPPED_IO memory region in my case as it was ioremapped first.

<1>[    0.029956] BUG: unable to handle kernel paging request at f7f22280
<1>[    0.042049] IP: [<c10257b9>] reserve_ram_pages_type+0x89/0x210
<4>[    0.053465] *pdpt = 0000000001978001 *pde = 0000000001ffb067 *pte = 0000000000000000
<0>[    0.068781] Oops: 0000 [#1] PREEMPT SMP
<4>[    0.076441] Modules linked in:
<4>[    0.082399]
<4>[    0.085304] Pid: 0, comm: swapper Not tainted 3.0.0-acpi-efi-0805 #3
<4>[    0.098067] EIP: 0060:[<c10257b9>] EFLAGS: 00010202 CPU: 0
<4>[    0.108810] EIP is at reserve_ram_pages_type+0x89/0x210
<4>[    0.119022] EAX: 0070e280 EBX: 38714000 ECX: f7814000 EDX: 00000000
<4>[    0.131284] ESI: 00000000 EDI: 38715000 EBP: c189fef0 ESP: c189fea8
<4>[    0.143543]  DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
<0>[    0.154099] Process swapper (pid: 0, ti=c189e000 task=c18bbe60 task.ti=c189e000)
<0>[    0.168576] Stack:
<4>[    0.172475]  80000200 ff108000 00000000 c189ff00 00038714 00000000 00000000 c189fed0
<4>[    0.187623]  c104f8ca 00038714 00000000 00038715 00000000 00000000 00038715 00000000
<4>[    0.202777]  00000010 38715000 c189ff48 c1025aff 38715000 00000000 00000010 00000000
<0>[    0.217930] Call Trace:
<4>[    0.222722]  [<c104f8ca>] ? page_is_ram+0x1a/0x40
<4>[    0.231916]  [<c1025aff>] reserve_memtype+0xdf/0x2f0
<4>[    0.241622]  [<c1024dc9>] set_memory_uc+0x49/0xa0
<4>[    0.250818]  [<c19334d0>] efi_enter_virtual_mode+0x1c2/0x3aa
<4>[    0.261887]  [<c19216d4>] start_kernel+0x291/0x2f2
<4>[    0.271245]  [<c19211c7>] ? loglevel+0x1b/0x1b
<4>[    0.279929]  [<c19210bf>] i386_start_kernel+0xbf/0xc8
---
 arch/x86/platform/efi/efi.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

arch/x86/platform/efi/efi.c +8 -1

Index: acpi50/arch/x86/platform/efi/efi.c
===================================================================
@@ -44,6 +44,7 @@
 #include <asm/cacheflush.h>
 #include <asm/tlbflush.h>
 #include <asm/x86_init.h>
+#include <asm/pat.h>
 
 #define EFI_DEBUG	1
 #define PFX 		"EFI: "

@@ -687,7 +688,13 @@ void __init efi_enter_virtual_mode(void)
 			addr = md->virt_addr;
 			npages = md->num_pages;
 			memrange_efi_to_native(&addr, &npages);
-			set_memory_uc(addr, npages);
+			if (reserve_memtype(md->phys_addr, md->phys_addr + npages * PAGE_SIZE,
+					_PAGE_CACHE_UC_MINUS, NULL)) {
+				printk(KERN_ERR PFX "failed to reserve region as uncacheable\n");
+			} elif (_set_memory_uc(addr, npages)) {
+				printk(KERN_ERR PFX "failed to set region to uncacheable\n");
+				free_memtype(md->phys_addr, md->phys_addr + npages * PAGE_SIZE);
+			}
 		}
 
 		systab = (u64) (unsigned long) efi_phys.systab;

Zhang Rui

author

2011-09-01 08:15 UTC #1 lore raw

Quoted 7 lines · Rui

From: Zhang Rui <[email protected]>
Date: Wed, 31 Aug 2011 09:59:01 +0800
Subject: Do not use __pa() to get the physical address of an ioremapped memory range.
set_memory_uc uses __pa() to translate the virtual address to the physical address. This breaks a EFI_MEMORY_MAPPED_IO memory region in my case as it was ioremapped first.

oops, wrong patch was attached.

here is the correct patch.

From: Zhang Rui <[email protected]>
Date: Wed, 31 Aug 2011 09:59:01 +0800
Subject: Do not use __pa() to get the physical address of an ioremapped memory range.

set_memory_uc uses __pa() to translate the virtual address to the physical address. This breaks a EFI_MEMORY_MAPPED_IO memory region in my case as it was ioremapped first.

<1>[    0.029956] BUG: unable to handle kernel paging request at f7f22280
<1>[    0.042049] IP: [<c10257b9>] reserve_ram_pages_type+0x89/0x210
<4>[    0.053465] *pdpt = 0000000001978001 *pde = 0000000001ffb067 *pte = 0000000000000000
<0>[    0.068781] Oops: 0000 [#1] PREEMPT SMP
<4>[    0.076441] Modules linked in:
<4>[    0.082399]
<4>[    0.085304] Pid: 0, comm: swapper Not tainted 3.0.0-acpi-efi-0805 #3
<4>[    0.098067] EIP: 0060:[<c10257b9>] EFLAGS: 00010202 CPU: 0
<4>[    0.108810] EIP is at reserve_ram_pages_type+0x89/0x210
<4>[    0.119022] EAX: 0070e280 EBX: 38714000 ECX: f7814000 EDX: 00000000
<4>[    0.131284] ESI: 00000000 EDI: 38715000 EBP: c189fef0 ESP: c189fea8
<4>[    0.143543]  DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
<0>[    0.154099] Process swapper (pid: 0, ti=c189e000 task=c18bbe60 task.ti=c189e000)
<0>[    0.168576] Stack:
<4>[    0.172475]  80000200 ff108000 00000000 c189ff00 00038714 00000000 00000000 c189fed0
<4>[    0.187623]  c104f8ca 00038714 00000000 00038715 00000000 00000000 00038715 00000000
<4>[    0.202777]  00000010 38715000 c189ff48 c1025aff 38715000 00000000 00000010 00000000
<0>[    0.217930] Call Trace:
<4>[    0.222722]  [<c104f8ca>] ? page_is_ram+0x1a/0x40
<4>[    0.231916]  [<c1025aff>] reserve_memtype+0xdf/0x2f0
<4>[    0.241622]  [<c1024dc9>] set_memory_uc+0x49/0xa0
<4>[    0.250818]  [<c19334d0>] efi_enter_virtual_mode+0x1c2/0x3aa
<4>[    0.261887]  [<c19216d4>] start_kernel+0x291/0x2f2
<4>[    0.271245]  [<c19211c7>] ? loglevel+0x1b/0x1b
<4>[    0.279929]  [<c19210bf>] i386_start_kernel+0xbf/0xc8

---

arch/x86/platform/efi/efi.c +8 -1

index 3ae4128..89586fe 100644
@@ -44,6 +44,7 @@
 #include <asm/cacheflush.h>
 #include <asm/tlbflush.h>
 #include <asm/x86_init.h>
+#include <asm/pat.h>
 
 #define EFI_DEBUG	1
 #define PFX 		"EFI: "

@@ -687,7 +688,13 @@ void __init efi_enter_virtual_mode(void)
 			addr = md->virt_addr;
 			npages = md->num_pages;
 			memrange_efi_to_native(&addr, &npages);
-			set_memory_uc(addr, npages);
+			if (reserve_memtype(md->phys_addr, md->phys_addr + npages * PAGE_SIZE,
+					_PAGE_CACHE_UC_MINUS, NULL)) {
+				printk(KERN_ERR PFX "failed to reserve region as uncacheable\n");
+			} else if (_set_memory_uc(addr, npages)) {
+				printk(KERN_ERR PFX "failed to set region to uncacheable\n");
+				free_memtype(md->phys_addr, md->phys_addr + npages * PAGE_SIZE);
+			}
 		}
 
 		systab = (u64) (unsigned long) efi_phys.systab;

Matt Fleming

2011-09-01 12:29 UTC #2 lore raw

Quoted 10 lines · Zhang Rui

Quoted 7 lines · Rui
From: Zhang Rui <[email protected]>
Date: Wed, 31 Aug 2011 09:59:01 +0800
Subject: Do not use __pa() to get the physical address of an ioremapped memory range.
set_memory_uc uses __pa() to translate the virtual address to the physical address. This breaks a EFI_MEMORY_MAPPED_IO memory region in my case as it was ioremapped first.
oops, wrong patch was attached.

Quoted 8 lines

here is the correct patch.
From: Zhang Rui <[email protected]>
Date: Wed, 31 Aug 2011 09:59:01 +0800
Subject: Do not use __pa() to get the physical address of an ioremapped memory range.
set_memory_uc uses __pa() to translate the virtual address to the physical address. This breaks a EFI_MEMORY_MAPPED_IO memory region in my case as it was ioremapped first.

Hmm.. does anyone know why we ioremap_cache() the memory on CONFIG_X86_32 instead of ioremap_nocache()? In the case of EFI_MEMORY_MAPPED_IO the memory really needs to be uncached. Then if we've ioremap'd the memory we should skip set_memory_uc() altogether, no?

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Zhang Rui

author

2011-09-02 02:19 UTC #3 lore raw

Quoted 17 lines · Matt Fleming

Quoted 16 lines · Zhang Rui
Quoted 7 lines · Rui
From: Zhang Rui <[email protected]>
Date: Wed, 31 Aug 2011 09:59:01 +0800
Subject: Do not use __pa() to get the physical address of an ioremapped memory range.
set_memory_uc uses __pa() to translate the virtual address to the physical address. This breaks a EFI_MEMORY_MAPPED_IO memory region in my case as it was ioremapped first.
oops, wrong patch was attached.
here is the correct patch.
From: Zhang Rui <[email protected]>
Date: Wed, 31 Aug 2011 09:59:01 +0800
Subject: Do not use __pa() to get the physical address of an ioremapped memory range.

Quoted 5 lines

Quoted 2 lines
set_memory_uc uses __pa() to translate the virtual address to the physical address. This breaks a EFI_MEMORY_MAPPED_IO memory region in my case as it was ioremapped first.
Hmm.. does anyone know why we ioremap_cache() the memory on CONFIG_X86_32 instead of ioremap_nocache()?

Perhaps this is because not all the memory range need ioremap_nocache(), e.g. EFI_BOOT_SERVICES_CODE, EFI_BOOT_SERVICES_DATA, etc?

So the current logic is to use ioremap_cache for all of them first, and then set all the non-write-back memory ranges to uncacheable.

Quoted 5 lines

In the case of
EFI_MEMORY_MAPPED_IO the memory really needs to be uncached. Then if we've ioremap'd the memory we should skip set_memory_uc() altogether, no?

In the beginning, I tried to fix efi_ioremap, i.e. use ioremap_nocache for EFI_MEMORY_MAPPED_IO and ioremap_cache for others.
But I'm not sure if that is the right FIX, as I don't know if there might be other memory ranges that also need to be set to uncached.

thanks,
rui

huang ying

2011-09-02 05:12 UTC #4 lore raw

In reply to Matt Fleming

Quoted 17 lines · Matt Fleming

Quoted 16 lines · Zhang Rui
Quoted 7 lines · Rui
From: Zhang Rui <[email protected]>
Date: Wed, 31 Aug 2011 09:59:01 +0800
Subject: Do not use __pa() to get the physical address of an ioremapped memory range.
set_memory_uc uses __pa() to translate the virtual address to the physical address. This breaks a EFI_MEMORY_MAPPED_IO memory region in my case as it was ioremapped first.
oops, wrong patch was attached.
here is the correct patch.
From: Zhang Rui <[email protected]>
Date: Wed, 31 Aug 2011 09:59:01 +0800
Subject: Do not use __pa() to get the physical address of an ioremapped memory range.

Quoted 8 lines

Quoted 2 lines
set_memory_uc uses __pa() to translate the virtual address to the physical address. This breaks a EFI_MEMORY_MAPPED_IO memory region in my case as it was ioremapped first.
Hmm.. does anyone know why we ioremap_cache() the memory on CONFIG_X86_32 instead of ioremap_nocache()? In the case of EFI_MEMORY_MAPPED_IO the memory really needs to be uncached. Then if we've ioremap'd the memory we should skip set_memory_uc() altogether, no?

Because whether the mapping should be cached is determined by md->attr instead of md->type. And besides UC, we may add WC, etc support.

Best Regards,
Huang Ying

Matt Fleming

2011-09-12 14:52 UTC #5 lore raw

Quoted 10 lines · huang ying

Quoted 6 lines · Matt Fleming
Hmm.. does anyone know why we ioremap_cache() the memory on CONFIG_X86_32 instead of ioremap_nocache()? In the case of EFI_MEMORY_MAPPED_IO the memory really needs to be uncached. Then if we've ioremap'd the memory we should skip set_memory_uc() altogether, no?
Because whether the mapping should be cached is determined by md->attr instead of md->type. And besides UC, we may add WC, etc support.

Confused.

The CONFIG_X86_64 version of efi_ioremap() looks like this,

void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size,

                                 u32 type)
{
        unsigned long last_map_pfn;

        if (type == EFI_MEMORY_MAPPED_IO)
                return ioremap(phys_addr, size);

Which uses md->type to figure out if we should call ioremap(), which on x86 is #define'd to ioremap_nocache(). CONFIG_X86_32 doesn't do this, but it looks to me like it should.

Zhang, I agree that calling __pa() on an ioremap()'d region is bogus, but I don't understand why no one is seeing this crash on x86-64. Is it something to do with the x86-64 memory map layout such that __pa() works on an ioremap()'d address?

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

huang ying

2011-09-13 08:48 UTC #6 lore raw

Quoted 21 lines · Matt Fleming

Quoted 10 lines · huang ying
Quoted 6 lines · Matt Fleming
Hmm.. does anyone know why we ioremap_cache() the memory on CONFIG_X86_32 instead of ioremap_nocache()? In the case of EFI_MEMORY_MAPPED_IO the memory really needs to be uncached. Then if we've ioremap'd the memory we should skip set_memory_uc() altogether, no?
Because whether the mapping should be cached is determined by md->attr instead of md->type. And besides UC, we may add WC, etc support.
Confused.
The CONFIG_X86_64 version of efi_ioremap() looks like this,
void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size, u32 type)
{
unsigned long last_map_pfn;

Quoted 6 lines

if (type == EFI_MEMORY_MAPPED_IO)
return ioremap(phys_addr, size);
Which uses md->type to figure out if we should call ioremap(), which on x86 is #define'd to ioremap_nocache(). CONFIG_X86_32 doesn't do this, but it looks to me like it should.

I don't think this type based solution is the perfect one.

Best Regards,
Huang Ying

Matt Fleming

2011-09-14 19:17 UTC #7 lore raw

Quoted 22 lines · huang ying

Quoted 21 lines · Matt Fleming
Quoted 10 lines · huang ying
Quoted 6 lines · Matt Fleming
Hmm.. does anyone know why we ioremap_cache() the memory on CONFIG_X86_32 instead of ioremap_nocache()? In the case of EFI_MEMORY_MAPPED_IO the memory really needs to be uncached. Then if we've ioremap'd the memory we should skip set_memory_uc() altogether, no?
Because whether the mapping should be cached is determined by md->attr instead of md->type. And besides UC, we may add WC, etc support.
Confused.
The CONFIG_X86_64 version of efi_ioremap() looks like this,
void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size,
                                u32 type)
{
       unsigned long last_map_pfn;

Quoted 8 lines

Quoted 6 lines
       if (type == EFI_MEMORY_MAPPED_IO)
               return ioremap(phys_addr, size);
Which uses md->type to figure out if we should call ioremap(), which on x86 is #define'd to ioremap_nocache(). CONFIG_X86_32 doesn't do this, but it looks to me like it should.
I don't think this type based solution is the perfect one.

Why? What else would you suggest?

Signature · 1 line

Matt Fleming, Intel Open Source Technology Center

Shaohua Li

2011-09-15 01:22 UTC #8 lore raw

In reply to Matt Fleming

2011/9/12 Matt Fleming <[email protected]>:

Quoted 21 lines · huang ying

Quoted 10 lines · Matt Fleming
Quoted 6 lines
Hmm.. does anyone know why we ioremap_cache() the memory on CONFIG_X86_32 instead of ioremap_nocache()? In the case of EFI_MEMORY_MAPPED_IO the memory really needs to be uncached. Then if we've ioremap'd the memory we should skip set_memory_uc() altogether, no?
Because whether the mapping should be cached is determined by md->attr instead of md->type. And besides UC, we may add WC, etc support.
Confused.
The CONFIG_X86_64 version of efi_ioremap() looks like this,
void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size, u32 type)
{
unsigned long last_map_pfn;

Quoted 6 lines

if (type == EFI_MEMORY_MAPPED_IO)
return ioremap(phys_addr, size);
Which uses md->type to figure out if we should call ioremap(), which on x86 is #define'd to ioremap_nocache(). CONFIG_X86_32 doesn't do this, but it looks to me like it should.

agree. mapping it wrong and fixing it later makes no sense. we should get the mapping correct at the first.

Quoted 4 lines

Zhang, I agree that calling __pa() on an ioremap()'d region is bogus, but I don't understand why no one is seeing this crash on x86-64. Is it something to do with the x86-64 memory map layout such that __pa() works on an ioremap()'d address?

x86-64 does identity mapping for larger space (from 0 to the last physical mem even there is hole). Maybe this is the reason.

Zhang Rui

author

2011-09-15 01:52 UTC #9 lore raw

Quoted 22 lines · Shaohua Li

2011/9/12 Matt Fleming <[email protected]>:
Quoted 21 lines · huang ying
Quoted 10 lines · Matt Fleming
Quoted 6 lines
Hmm.. does anyone know why we ioremap_cache() the memory on CONFIG_X86_32 instead of ioremap_nocache()? In the case of EFI_MEMORY_MAPPED_IO the memory really needs to be uncached. Then if we've ioremap'd the memory we should skip set_memory_uc() altogether, no?
Because whether the mapping should be cached is determined by md->attr instead of md->type. And besides UC, we may add WC, etc support.
Confused.
The CONFIG_X86_64 version of efi_ioremap() looks like this,
void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size,
                                u32 type)
{
       unsigned long last_map_pfn;

Quoted 9 lines

Quoted 6 lines
       if (type == EFI_MEMORY_MAPPED_IO)
               return ioremap(phys_addr, size);
Which uses md->type to figure out if we should call ioremap(), which on x86 is #define'd to ioremap_nocache(). CONFIG_X86_32 doesn't do this, but it looks to me like it should.
agree. mapping it wrong and fixing it later makes no sense. we should get the mapping correct at the first.

So what kinds of memory regions need ioremap_nocache?
should we make this decision based on the memory range type or attribute or even both of them?

-rui

Quoted 6 lines

Quoted 4 lines
Zhang, I agree that calling __pa() on an ioremap()'d region is bogus, but I don't understand why no one is seeing this crash on x86-64. Is it something to do with the x86-64 memory map layout such that __pa() works on an ioremap()'d address?
x86-64 does identity mapping for larger space (from 0 to the last physical mem even there is hole). Maybe this is the reason.

huang ying

2011-09-15 03:28 UTC #10 lore raw

Quoted 29 lines · Zhang Rui

Quoted 28 lines · Shaohua Li
2011/9/12 Matt Fleming <[email protected]>:
Quoted 27 lines · huang ying
Quoted 10 lines · Matt Fleming
Quoted 6 lines
Hmm.. does anyone know why we ioremap_cache() the memory on CONFIG_X86_32 instead of ioremap_nocache()? In the case of EFI_MEMORY_MAPPED_IO the memory really needs to be uncached. Then if we've ioremap'd the memory we should skip set_memory_uc() altogether, no?
Because whether the mapping should be cached is determined by md->attr instead of md->type. And besides UC, we may add WC, etc support.
Confused.
The CONFIG_X86_64 version of efi_ioremap() looks like this,
void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size, u32 type)
{
unsigned long last_map_pfn;
if (type == EFI_MEMORY_MAPPED_IO)
return ioremap(phys_addr, size);
Which uses md->type to figure out if we should call ioremap(), which on x86 is #define'd to ioremap_nocache(). CONFIG_X86_32 doesn't do this, but it looks to me like it should.

Quoted 6 lines

Quoted 3 lines
agree. mapping it wrong and fixing it later makes no sense. we should get the mapping correct at the first.
So what kinds of memory regions need ioremap_nocache?
should we make this decision based on the memory range type or attribute or even both of them?

I think the decision should be made based on attribute as follow:

- attribute & EFI_MEMORY_WB --> ioremap_cache
- attribute & EFI_MEMORY_WC --> ioremap_wc
- others -> ioremap_nocache

init_memory_mapping in efi_ioremap is used to make kexec support possible. But ioremap is used anyway, kexec support should be done in another way. So I think it is ok to just use ioremap_xxx in efi_ioremap. The efi_ioremap definition for 32bit need to be fixed too.

Best Regards,
Huang Ying