Skip to contentSkip to conversation

x86: add executable mapping support to ioremap

PATCH Applied Huang, Ying 4 participants 9 messages Jan 31, 2008

Subsystems (2)
x86 x86 MM
lore
Share & tools
lore

Huang, Ying

author
#0 lore raw

PATCH 4/5 x86: add executable mapping support to ioremap

This patch makes ioremap() can be used to map pages as executable, this is needed by EFI support.

Signed-off-by Huang Ying <[email protected]>
---
 arch/x86/mm/ioremap.c   |   35 +++++++++++++++++++++++------------
 include/asm-x86/io_32.h |   14 ++++++++++++++
 include/asm-x86/io_64.h |   14 ++++++++++++++
 3 files changed, 51 insertions(+), 12 deletions(-)
arch/x86/mm/ioremap.c +23 -12
@@ -20,11 +20,6 @@
 #include <asm/tlbflush.h>
 #include <asm/pgalloc.h>
 
-enum ioremap_mode {
-	IOR_MODE_UNCACHED,
-	IOR_MODE_CACHED,
-};
-
 #ifdef CONFIG_X86_64
 
 unsigned long __phys_addr(unsigned long x)

@@ -71,7 +66,8 @@ int page_is_ram(unsigned long pagenr)
  * conflicts.
  */
 static int ioremap_change_attr(unsigned long paddr, unsigned long size,
-			       enum ioremap_mode mode)
+			       enum ioremap_mode mode,
+			       enum ioremap_xmode xmode)
 {
 	unsigned long vaddr = (unsigned long)__va(paddr);
 	unsigned long nrpages = size >> PAGE_SHIFT;

@@ -98,6 +94,16 @@ static int ioremap_change_attr(unsigned 
 		break;
 	}
 
+	if (err)
+		return err;
+
+	if (__supported_pte_mask & _PAGE_NX) {
+		if (xmode == IOR_XMODE_EXEC)
+			err = set_memory_x(vaddr, nrpages);
+		else
+			err = set_memory_nx(vaddr, nrpages);
+	}
+
 	return err;
 }
 

@@ -110,8 +116,9 @@ static int ioremap_change_attr(unsigned 
  * have to convert them into an offset in a page-aligned mapping, but the
  * caller shouldn't need to know that small detail.
  */
-static void __iomem *__ioremap(unsigned long phys_addr, unsigned long size,
-			       enum ioremap_mode mode)
+void __iomem *__ioremap(unsigned long phys_addr, unsigned long size,
+			enum ioremap_mode mode,
+			enum ioremap_xmode xmode)
 {
 	void __iomem *addr;
 	struct vm_struct *area;

@@ -148,6 +155,9 @@ static void __iomem *__ioremap(unsigned 
 		break;
 	}
 
+	if ((__supported_pte_mask & _PAGE_NX) && xmode == IOR_XMODE_EXEC)
+		prot = __pgprot(pgprot_val(prot) & ~_PAGE_NX);
+
 	/*
 	 * Mappings have to be page-aligned
 	 */

@@ -169,7 +179,7 @@ static void __iomem *__ioremap(unsigned 
 		return NULL;
 	}
 
-	if (ioremap_change_attr(phys_addr, size, mode) < 0) {
+	if (ioremap_change_attr(phys_addr, size, mode, xmode) < 0) {
 		vunmap(addr);
 		return NULL;
 	}

@@ -200,13 +210,13 @@ static void __iomem *__ioremap(unsigned 
  */
 void __iomem *ioremap_nocache(unsigned long phys_addr, unsigned long size)
 {
-	return __ioremap(phys_addr, size, IOR_MODE_UNCACHED);
+	return __ioremap(phys_addr, size, IOR_MODE_UNCACHED, IOR_XMODE_UNEXEC);
 }
 EXPORT_SYMBOL(ioremap_nocache);
 
 void __iomem *ioremap_cache(unsigned long phys_addr, unsigned long size)
 {
-	return __ioremap(phys_addr, size, IOR_MODE_CACHED);
+	return __ioremap(phys_addr, size, IOR_MODE_CACHED, IOR_XMODE_UNEXEC);
 }
 EXPORT_SYMBOL(ioremap_cache);
 

@@ -254,7 +264,8 @@ void iounmap(volatile void __iomem *addr
 	}
 
 	/* Reset the direct mapping. Can block */
-	ioremap_change_attr(p->phys_addr, p->size, IOR_MODE_CACHED);
+	ioremap_change_attr(p->phys_addr, p->size,
+			    IOR_MODE_CACHED, IOR_XMODE_UNEXEC);
 
 	/* Finally remove it */
 	o = remove_vm_area((void *)addr);
include/asm-x86/io_64.h +14 -0
@@ -153,6 +153,20 @@ static inline void * phys_to_virt(unsign
 extern void *early_ioremap(unsigned long addr, unsigned long size);
 extern void early_iounmap(void *addr, unsigned long size);
 
+enum ioremap_mode {
+	IOR_MODE_UNCACHED,
+	IOR_MODE_CACHED,
+};
+
+enum ioremap_xmode {
+	IOR_XMODE_EXEC,
+	IOR_XMODE_UNEXEC,
+};
+
+extern void __iomem *__ioremap(unsigned long phys_addr, unsigned long size,
+			       enum ioremap_mode mode,
+			       enum ioremap_xmode xmode);
+
 /*
  * This one maps high address device memory and turns off caching for that area.
  * it's useful if some control registers are in such an area and write combining
include/asm-x86/io_32.h +14 -0
@@ -100,6 +100,20 @@ static inline void * phys_to_virt(unsign
  */
 #define page_to_phys(page)    ((dma_addr_t)page_to_pfn(page) << PAGE_SHIFT)
 
+enum ioremap_mode {
+	IOR_MODE_UNCACHED,
+	IOR_MODE_CACHED,
+};
+
+enum ioremap_xmode {
+	IOR_XMODE_EXEC,
+	IOR_XMODE_UNEXEC,
+};
+
+extern void __iomem *__ioremap(unsigned long phys_addr, unsigned long size,
+			       enum ioremap_mode mode,
+			       enum ioremap_xmode xmode);
+
 /**
  * ioremap     -   map bus memory into CPU space
  * @offset:    bus address of the memory
#1 lore raw

include/asm-x86/io_64.h static inline void * phys_to_virt(unsign · line 169

Quoted 7 lines · Ying

This patch makes ioremap() can be used to map pages as executable, this is needed by EFI support.

include/asm-x86/io_64.h +4 -0
+extern void __iomem *__ioremap(unsigned long phys_addr, unsigned long size,
+			       enum ioremap_mode mode,
+			       enum ioremap_xmode xmode);
+

Why do you want to add a new API?

    addr = ioremap(phys_addr, len);
    set_memory_x(addr);

should do what you want already. I think Arjan was not very clear in his reply yesterday.

We tried to avoid the intermingling of ioremap and the page attribute settings. We just kept the cached/uncached part.

Now thinking about it further, we should probably fully decouple the mapping and the attribute change and remove the enum argument from __ioremap and change the implementations of ioremap_cached and ioremap_uncached to do the attribute setting after the remap.

That way ioremap_chached and ioremap_uncached just would become what they should be: conveniance functions for the developers.

Thanks,
tglx

#2 lore raw
Quoted 12 lines · Thomas Gleixner

include/asm-x86/io_64.h static inline void * phys_to_virt(unsign · line 169

Quoted 7 lines · Ying

This patch makes ioremap() can be used to map pages as executable, this is needed by EFI support.

include/asm-x86/io_64.h +4 -0
+extern void __iomem *__ioremap(unsigned long phys_addr, unsigned long size,
+                            enum ioremap_mode mode,
+                            enum ioremap_xmode xmode);
+

Why do you want to add a new API?

Quoted 5 lines
    addr = ioremap(phys_addr, len);
    set_memory_x(addr);

should do what you want already. I think Arjan was not very clear in his reply yesterday.

In current implementation, change_page_attr() can be used to change identity map only. At least I can find __pa() are used in change_page_attr_addr() to get physical address. So I think change_page_attr() should be fixed firstly.

Quoted 10 lines

We tried to avoid the intermingling of ioremap and the page attribute settings. We just kept the cached/uncached part.

Now thinking about it further, we should probably fully decouple the mapping and the attribute change and remove the enum argument from __ioremap and change the implementations of ioremap_cached and ioremap_uncached to do the attribute setting after the remap.

That way ioremap_chached and ioremap_uncached just would become what they should be: conveniance functions for the developers.

If the memory area be ioremap() has identity map too. We need two set_memory_xxx(addr). That is, we need:

addr = ioremap(phys_addr, len);
set_memory_x(addr);
if (phys_addr < (end_pfn_mapped >> PAGE_SHIFT))

        set_memory_x(__va(phys_addr));

Maybe we can hide the fact of identity map into set_memory_xxx(), just specify the physical address to set_memory_xxx() too.

Best Regards,
Huang Ying

#3 lore raw
Quoted 5 lines

addr = ioremap(phys_addr, len);
set_memory_x(addr);
if (phys_addr < (end_pfn_mapped >> PAGE_SHIFT))

        set_memory_x(__va(phys_addr));

If you just need the ioremap executable there is no reason to split up the direct mapping for this too. The x86 architecture does not require x bits to be coherent between mappings; only the caching attributes.

-Andi

#4 lore raw
Quoted 8 lines · Andi Kleen
Quoted 4 lines

addr = ioremap(phys_addr, len);
set_memory_x(addr);
if (phys_addr < (end_pfn_mapped >> PAGE_SHIFT))

        set_memory_x(__va(phys_addr));

If you just need the ioremap executable there is no reason to split up the direct mapping for this too. The x86 architecture does not require x bits to be coherent between mappings; only the caching attributes.

Thanks for pointing this out.

tglx

#5 lore raw
In reply to huang ying
Quoted 16 lines · huang ying
Quoted 15 lines · Thomas Gleixner

include/asm-x86/io_64.h static inline void * phys_to_virt(unsign · line 169

Quoted 7 lines · Ying

This patch makes ioremap() can be used to map pages as executable, this is needed by EFI support.

include/asm-x86/io_64.h +4 -0
+extern void __iomem *__ioremap(unsigned long phys_addr, unsigned long size,
+                            enum ioremap_mode mode,
+                            enum ioremap_xmode xmode);
+

Why do you want to add a new API?

    addr = ioremap(phys_addr, len);
    set_memory_x(addr);
Quoted 7 lines
Quoted 2 lines

should do what you want already. I think Arjan was not very clear in his reply yesterday.

In current implementation, change_page_attr() can be used to change identity map only. At least I can find __pa() are used in change_page_attr_addr() to get physical address. So I think change_page_attr() should be fixed firstly.

There is nothing to fix. It works on virtual addresses.

The __pa() in change_page_attr_addr() is only used to check for the high alias mapping of the kernel, but the call to change_page_attr uses the virtual address.

Thanks,

tglx

#6 lore raw
Quoted 5 lines

There is nothing to fix. It works on virtual addresses.

The __pa() in change_page_attr_addr() is only used to check for the high alias mapping of the kernel, but the call to change_page_attr uses the virtual address.

But __pa() doesn't work for ioremap ...

huang is completely right.

-Andi

#7 lore raw
Quoted 7 lines · Andi Kleen
Quoted 5 lines

There is nothing to fix. It works on virtual addresses.

The __pa() in change_page_attr_addr() is only used to check for the high alias mapping of the kernel, but the call to change_page_attr uses the virtual address.

But __pa() doesn't work for ioremap ...

If it does not, then __pa() needs to be fixed, nothing else.

Thanks,
tglx

#8 lore raw
Quoted 11 lines · Thomas Gleixner
Quoted 7 lines · Andi Kleen
Quoted 5 lines

There is nothing to fix. It works on virtual addresses.

The __pa() in change_page_attr_addr() is only used to check for the high alias mapping of the kernel, but the call to change_page_attr uses the virtual address.

But __pa() doesn't work for ioremap ...

If it does not, then __pa() needs to be fixed, nothing else.

No if, it doesn't.

Changing that seems like a slippery rope. Would you want to make it work for user space addresses then too? It would certainly become
much more heavyweight because it requires much more checks
and then a page table lookup.

For a fairly uncommon case.

I always thought the cheap direct va<->pa conversions to be one of the basic design principles of the Linux VM. It's surprising you want to throw that overboard that quickly.

Better check at least with Linus first, it's a fairly fundamental change.

To be honest IMHO the simplest way to fix this problem would be to just put the pgprot_t argument back into __ioremap(). Then you wouldn't have all these problems.

-Andi