[PATCH 4/6] x86: fix some bugs about EFI runtime code mapping

[PATCH 4/6] x86: fix some bugs about EFI runtime code mapping

[Huang, Ying]

This patch fixes some bugs of making EFI runtime code executable.

- Use change_page_attr in i386 too. Because the runtime code may be
  mapped not through ioremap.

- If there is no _PAGE_NX in __supported_pte_mask, the change_page_attr
  is not called.

- Make efi_ioremap map pages as PAGE_KERNEL_EXEC, because EFI runtime
  code may be mapped through efi_ioremap.

Signed-off-by: Huang Ying <ying.huang@intel.com>

---
 arch/x86/kernel/efi.c    |   35 ++++++++++++++++++++++++++++++-----
 arch/x86/kernel/efi_64.c |   26 ++++++--------------------
 include/asm-x86/efi.h    |    1 -
 3 files changed, 36 insertions(+), 26 deletions(-)

--- a/arch/x86/kernel/efi.c
+++ b/arch/x86/kernel/efi.c
@@ -40,6 +40,8 @@
 #include <asm/setup.h>
 #include <asm/efi.h>
 #include <asm/time.h>
+#include <asm/cacheflush.h>
+#include <asm/tlbflush.h>
 
 #define EFI_DEBUG	1
 #define PFX 		"EFI: "
@@ -379,6 +381,32 @@ void __init efi_init(void)
 #endif
 }
 
+#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
+static void __init runtime_code_page_mkexec(void)
+{
+	efi_memory_desc_t *md;
+	unsigned long end;
+	void *p;
+
+	if (!(__supported_pte_mask & _PAGE_NX))
+		return;
+
+	/* Make EFI runtime service code area executable */
+	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
+		md = p;
+		end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT);
+		if (md->type == EFI_RUNTIME_SERVICES_CODE &&
+		    (end >> PAGE_SHIFT) <= end_pfn_map)
+			change_page_attr_addr(md->virt_addr,
+					      md->num_pages,
+					      PAGE_KERNEL_EXEC);
+	}
+	__flush_tlb_all();
+}
+#else
+static inline void __init runtime_code_page_mkexec(void) { }
+#endif
+
 /*
  * This function will switch the EFI runtime services to virtual mode.
  * Essentially, look through the EFI memmap and map every region that
@@ -399,9 +427,9 @@ void __init efi_enter_virtual_mode(void)
 		md = p;
 		if (!(md->attribute & EFI_MEMORY_RUNTIME))
 			continue;
+		end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT);
 		if ((md->attribute & EFI_MEMORY_WB) &&
-		    (((md->phys_addr + (md->num_pages<<EFI_PAGE_SHIFT)) >>
-		      PAGE_SHIFT) < end_pfn_map))
+		    ((end >> PAGE_SHIFT) <= end_pfn_map))
 			md->virt_addr = (unsigned long)__va(md->phys_addr);
 		else
 			md->virt_addr = (unsigned long)
@@ -410,7 +438,6 @@ void __init efi_enter_virtual_mode(void)
 		if (!md->virt_addr)
 			printk(KERN_ERR PFX "ioremap of 0x%llX failed!\n",
 			       (unsigned long long)md->phys_addr);
-		end = md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT);
 		if ((md->phys_addr <= (unsigned long)efi_phys.systab) &&
 		    ((unsigned long)efi_phys.systab < end))
 			efi.systab = (efi_system_table_t *)(unsigned long)
@@ -448,9 +475,7 @@ void __init efi_enter_virtual_mode(void)
 	efi.get_next_high_mono_count = virt_efi_get_next_high_mono_count;
 	efi.reset_system = virt_efi_reset_system;
 	efi.set_virtual_address_map = virt_efi_set_virtual_address_map;
-#ifdef CONFIG_X86_64
 	runtime_code_page_mkexec();
-#endif
 }
 
 /*
--- a/arch/x86/kernel/efi_64.c
+++ b/arch/x86/kernel/efi_64.c
@@ -33,7 +33,6 @@
 #include <asm/e820.h>
 #include <asm/pgtable.h>
 #include <asm/tlbflush.h>
-#include <asm/cacheflush.h>
 #include <asm/proto.h>
 #include <asm/efi.h>
 
@@ -55,7 +54,7 @@ static void __init early_mapping_set_exe
 		else
 			set_pte(kpte, __pte((pte_val(*kpte) | _PAGE_NX) & \
 					    __supported_pte_mask));
-		if (pte_huge(*kpte))
+		if (level == 4)
 			start = (start + PMD_SIZE) & PMD_MASK;
 		else
 			start = (start + PAGE_SIZE) & PAGE_MASK;
@@ -67,6 +66,9 @@ static void __init early_runtime_code_ma
 	efi_memory_desc_t *md;
 	void *p;
 
+	if (!(__supported_pte_mask & _PAGE_NX))
+		return;
+
 	/* Make EFI runtime service code area executable */
 	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
 		md = p;
@@ -116,22 +118,6 @@ void __init efi_reserve_bootmem(void)
 				memmap.nr_map * memmap.desc_size);
 }
 
-void __init runtime_code_page_mkexec(void)
-{
-	efi_memory_desc_t *md;
-	void *p;
-
-	/* Make EFI runtime service code area executable */
-	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
-		md = p;
-		if (md->type == EFI_RUNTIME_SERVICES_CODE)
-			change_page_attr_addr(md->virt_addr,
-					      md->num_pages,
-					      PAGE_KERNEL_EXEC);
-	}
-	__flush_tlb_all();
-}
-
 void __iomem * __init efi_ioremap(unsigned long offset,
 				  unsigned long size)
 {
@@ -146,8 +132,8 @@ void __iomem * __init efi_ioremap(unsign
 		return NULL;
 
 	for (i = 0; i < pages; i++) {
-		set_fixmap_nocache(FIX_EFI_IO_MAP_FIRST_PAGE - pages_mapped,
-				   offset);
+		__set_fixmap(FIX_EFI_IO_MAP_FIRST_PAGE - pages_mapped,
+			     offset, PAGE_KERNEL_EXEC);
 		offset += PAGE_SIZE;
 		pages_mapped++;
 	}
--- a/include/asm-x86/efi.h
+++ b/include/asm-x86/efi.h
@@ -95,6 +95,5 @@ extern void *efi_ioremap(unsigned long o
 extern void efi_reserve_bootmem(void);
 extern void efi_call_phys_prelog(void);
 extern void efi_call_phys_epilog(void);
-extern void runtime_code_page_mkexec(void);
 
 #endif

Re: [PATCH 4/6] x86: fix some bugs about EFI runtime code mapping

[Jeremy Fitzhardinge]

Huang, Ying wrote:
> This patch fixes some bugs of making EFI runtime code executable.
>
> - Use change_page_attr in i386 too. Because the runtime code may be
>   mapped not through ioremap.
>
> - If there is no _PAGE_NX in __supported_pte_mask, the change_page_attr
>   is not called.
>
> - Make efi_ioremap map pages as PAGE_KERNEL_EXEC, because EFI runtime
>   code may be mapped through efi_ioremap.
>
> Signed-off-by: Huang Ying <ying.huang@intel.com>
>
> ---
>  arch/x86/kernel/efi.c    |   35 ++++++++++++++++++++++++++++++-----
>  arch/x86/kernel/efi_64.c |   26 ++++++--------------------
>  include/asm-x86/efi.h    |    1 -
>  3 files changed, 36 insertions(+), 26 deletions(-)
>
> --- a/arch/x86/kernel/efi.c
> +++ b/arch/x86/kernel/efi.c
> @@ -40,6 +40,8 @@
>  #include <asm/setup.h>
>  #include <asm/efi.h>
>  #include <asm/time.h>
> +#include <asm/cacheflush.h>
> +#include <asm/tlbflush.h>
>  
>  #define EFI_DEBUG	1
>  #define PFX 		"EFI: "
> @@ -379,6 +381,32 @@ void __init efi_init(void)
>  #endif
>  }
>  
> +#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
> +static void __init runtime_code_page_mkexec(void)
> +{
> +	efi_memory_desc_t *md;
> +	unsigned long end;
> +	void *p;
> +
> +	if (!(__supported_pte_mask & _PAGE_NX))
> +		return;
>   

On 32-bit non-PAE, _PAGE_NX == 0, so this if() statement should be 
sufficient to disable the whole function at compile time without needing 
the outer #if defined wrapper.

    J

Re: [PATCH 4/6] x86: fix some bugs about EFI runtime code mapping

[Ingo Molnar]

* Huang, Ying <ying.huang@intel.com> wrote:

> This patch fixes some bugs of making EFI runtime code executable.
> 
> - Use change_page_attr in i386 too. Because the runtime code may be
>   mapped not through ioremap.
> 
> - If there is no _PAGE_NX in __supported_pte_mask, the change_page_attr
>   is not called.
> 
> - Make efi_ioremap map pages as PAGE_KERNEL_EXEC, because EFI runtime
>   code may be mapped through efi_ioremap.

thanks, applied.

note that here:

> -		set_fixmap_nocache(FIX_EFI_IO_MAP_FIRST_PAGE - pages_mapped,
> -				   offset);
> +		__set_fixmap(FIX_EFI_IO_MAP_FIRST_PAGE - pages_mapped,
> +			     offset, PAGE_KERNEL_EXEC);

you've changed it from nocache-noexec to cached-exec. I suspect that's 
what we want - except if an early EFI area can be non-prefetchable 
device memory. Can that ever happen? Would you like to have 
PAGE_KERNEL_NOCACHE_EXEC perhaps? I implemented that yesterday but did 
not commit it yet. (see the patch below)

	Ingo

----------------->
Subject: x86: add PAGE_KERNEL_EXEC_NOCACHE
From: Ingo Molnar <mingo@elte.hu>

add PAGE_KERNEL_EXEC_NOCACHE.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/asm-x86/pgtable.h |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-x86.q/include/asm-x86/pgtable.h
===================================================================
--- linux-x86.q.orig/include/asm-x86/pgtable.h
+++ linux-x86.q/include/asm-x86/pgtable.h
@@ -79,6 +79,7 @@ extern pteval_t __PAGE_KERNEL, __PAGE_KE
 
 #define __PAGE_KERNEL_RO		(__PAGE_KERNEL & ~_PAGE_RW)
 #define __PAGE_KERNEL_RX		(__PAGE_KERNEL_EXEC & ~_PAGE_RW)
+#define __PAGE_KERNEL_EXEC_NOCACHE	(__PAGE_KERNEL_EXEC | _PAGE_PCD | _PAGE_PWT)
 #define __PAGE_KERNEL_NOCACHE		(__PAGE_KERNEL | _PAGE_PCD | _PAGE_PWT)
 #define __PAGE_KERNEL_VSYSCALL		(__PAGE_KERNEL_RX | _PAGE_USER)
 #define __PAGE_KERNEL_VSYSCALL_NOCACHE	(__PAGE_KERNEL_VSYSCALL | _PAGE_PCD | _PAGE_PWT)
@@ -96,6 +97,7 @@ extern pteval_t __PAGE_KERNEL, __PAGE_KE
 #define PAGE_KERNEL_EXEC		MAKE_GLOBAL(__PAGE_KERNEL_EXEC)
 #define PAGE_KERNEL_RX			MAKE_GLOBAL(__PAGE_KERNEL_RX)
 #define PAGE_KERNEL_NOCACHE		MAKE_GLOBAL(__PAGE_KERNEL_NOCACHE)
+#define PAGE_KERNEL_EXEC_NOCACHE	MAKE_GLOBAL(__PAGE_KERNEL_EXEC_NOCACHE)
 #define PAGE_KERNEL_LARGE		MAKE_GLOBAL(__PAGE_KERNEL_LARGE)
 #define PAGE_KERNEL_LARGE_EXEC		MAKE_GLOBAL(__PAGE_KERNEL_LARGE_EXEC)
 #define PAGE_KERNEL_VSYSCALL		MAKE_GLOBAL(__PAGE_KERNEL_VSYSCALL)

Re: [PATCH 4/6] x86: fix some bugs about EFI runtime code mapping

[Ingo Molnar]

* Jeremy Fitzhardinge <jeremy@goop.org> wrote:

>>  +#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
>> +static void __init runtime_code_page_mkexec(void)
>> +{
>> +	efi_memory_desc_t *md;
>> +	unsigned long end;
>> +	void *p;
>> +
>> +	if (!(__supported_pte_mask & _PAGE_NX))
>> +		return;
>>   
>
> On 32-bit non-PAE, _PAGE_NX == 0, so this if() statement should be 
> sufficient to disable the whole function at compile time without 
> needing the outer #if defined wrapper.

good point. The patch fixes bugs and the consolidation it does is very 
nice so i've applied it already, but we could indeed further consolidate 
it and make it a nice #ifdef-less function. Could one of you send an 
add-on patch for this?

	Ingo

Re: [PATCH 4/6] x86: fix some bugs about EFI runtime code mapping

[Huang, Ying]

On Fri, 2008-01-25 at 10:16 +0100, Ingo Molnar wrote:
> * Huang, Ying <ying.huang@intel.com> wrote:
> 
> > This patch fixes some bugs of making EFI runtime code executable.
> > 
> > - Use change_page_attr in i386 too. Because the runtime code may be
> >   mapped not through ioremap.
> > 
> > - If there is no _PAGE_NX in __supported_pte_mask, the change_page_attr
> >   is not called.
> > 
> > - Make efi_ioremap map pages as PAGE_KERNEL_EXEC, because EFI runtime
> >   code may be mapped through efi_ioremap.
> 
> thanks, applied.
> 
> note that here:
> 
> > -		set_fixmap_nocache(FIX_EFI_IO_MAP_FIRST_PAGE - pages_mapped,
> > -				   offset);
> > +		__set_fixmap(FIX_EFI_IO_MAP_FIRST_PAGE - pages_mapped,
> > +			     offset, PAGE_KERNEL_EXEC);
> 
> you've changed it from nocache-noexec to cached-exec. I suspect that's 
> what we want - except if an early EFI area can be non-prefetchable 
> device memory. Can that ever happen? Would you like to have 
> PAGE_KERNEL_NOCACHE_EXEC perhaps? I implemented that yesterday but did 
> not commit it yet. (see the patch below)

Yes. EFI area can be non-prefetchable device memory. I should use
PAGE_KERNEL_NOCACHE_EXEC.

A question about this:

The MTRR on x86 should have set the memory area as un-cachable. Why do
we bother to set it in page table?

Best Regards,
Huang Ying

Re: [PATCH 4/6] x86: fix some bugs about EFI runtime code mapping

[Huang, Ying]

On Fri, 2008-01-25 at 10:19 +0100, Ingo Molnar wrote:
> * Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> 
> >>  +#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
> >> +static void __init runtime_code_page_mkexec(void)
> >> +{
> >> +	efi_memory_desc_t *md;
> >> +	unsigned long end;
> >> +	void *p;
> >> +
> >> +	if (!(__supported_pte_mask & _PAGE_NX))
> >> +		return;
> >>   
> >
> > On 32-bit non-PAE, _PAGE_NX == 0, so this if() statement should be 
> > sufficient to disable the whole function at compile time without 
> > needing the outer #if defined wrapper.
> 
> good point. The patch fixes bugs and the consolidation it does is very 
> nice so i've applied it already, but we could indeed further consolidate 
> it and make it a nice #ifdef-less function. Could one of you send an 
> add-on patch for this?

I will do it. But I have some other thing to do now, so I will send it
on next Monday.

Best Regards,
Huang Ying

Re: [PATCH 4/6] x86: fix some bugs about EFI runtime code mapping

[Ingo Molnar]

* Huang, Ying <ying.huang@intel.com> wrote:

> On Fri, 2008-01-25 at 10:16 +0100, Ingo Molnar wrote:
> > * Huang, Ying <ying.huang@intel.com> wrote:
> > 
> > > This patch fixes some bugs of making EFI runtime code executable.
> > > 
> > > - Use change_page_attr in i386 too. Because the runtime code may be
> > >   mapped not through ioremap.
> > > 
> > > - If there is no _PAGE_NX in __supported_pte_mask, the change_page_attr
> > >   is not called.
> > > 
> > > - Make efi_ioremap map pages as PAGE_KERNEL_EXEC, because EFI runtime
> > >   code may be mapped through efi_ioremap.
> > 
> > thanks, applied.
> > 
> > note that here:
> > 
> > > -		set_fixmap_nocache(FIX_EFI_IO_MAP_FIRST_PAGE - pages_mapped,
> > > -				   offset);
> > > +		__set_fixmap(FIX_EFI_IO_MAP_FIRST_PAGE - pages_mapped,
> > > +			     offset, PAGE_KERNEL_EXEC);
> > 
> > you've changed it from nocache-noexec to cached-exec. I suspect 
> > that's what we want - except if an early EFI area can be 
> > non-prefetchable device memory. Can that ever happen? Would you like 
> > to have PAGE_KERNEL_NOCACHE_EXEC perhaps? I implemented that 
> > yesterday but did not commit it yet. (see the patch below)
> 
> Yes. EFI area can be non-prefetchable device memory. I should use 
> PAGE_KERNEL_NOCACHE_EXEC.

ok, i fixed that in the patch and reordered the PAGE_KERNEL_NOCACHE_EXEC 
patch to come before yours.

> A question about this:
> 
> The MTRR on x86 should have set the memory area as un-cachable. Why do 
> we bother to set it in page table?

you are right in that there is no immediate correctness reason for it. 
The reason is to eventually get away from any MTRR dependencies. If some 
other OS uses PATs and the BIOS sets up the wrong MTRR, then the other 
OS might still having a working EFI, while Linux might crash and burn. 

So we try to get both the MTRRs and the pagetable attributes match the 
purpose of the area in question. If _both_ levels of attributes agree it 
cannot hurt, but if one of them is wrong, the other one still saves the 
day.

that's why we changed all ioremaps to default to cache-disabled (PCD) in 
latest x86.git as well. For years the x86 architecture set the ioremap 
pagetable entries to cacheable by default and only the MTRRs (and the 
BIOS writers) saved us from trouble. Now we try to be a bit more 
defensive and avoid "BIOS bug causes only Linux to crash and burn while 
other OSs work fine" type of scenarios.

	Ingo