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

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

[Huang, Ying]

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

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

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

--- a/arch/x86/mm/ioremap.c
+++ b/arch/x86/mm/ioremap.c
@@ -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);
--- a/include/asm-x86/io_64.h
+++ b/include/asm-x86/io_64.h
@@ -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
--- a/include/asm-x86/io_32.h
+++ b/include/asm-x86/io_32.h
@@ -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

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

[Thomas Gleixner]

On Thu, 31 Jan 2008, Huang, Ying wrote:

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

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

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

[huang ying]

On Jan 31, 2008 9:00 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 31 Jan 2008, Huang, Ying wrote:
>
> > This patch makes ioremap() can be used to map pages as executable,
> > this is needed by EFI support.
>
> > +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.

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.

> 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

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

[Andi Kleen]

> 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

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

[Thomas Gleixner]

On Thu, 31 Jan 2008, Andi Kleen wrote:
> > 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

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

[Thomas Gleixner]

On Thu, 31 Jan 2008, huang ying wrote:

> On Jan 31, 2008 9:00 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Thu, 31 Jan 2008, Huang, Ying wrote:
> >
> > > This patch makes ioremap() can be used to map pages as executable,
> > > this is needed by EFI support.
> >
> > > +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.
> 
> 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

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

[Andi Kleen]

> 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

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

[Thomas Gleixner]

On Thu, 31 Jan 2008, Andi Kleen wrote:

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

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

[Andi Kleen]

On Thu, Jan 31, 2008 at 05:37:05PM +0100, Thomas Gleixner wrote:
> On Thu, 31 Jan 2008, Andi Kleen wrote:
> 
> > > 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