[patch] x86, pat: allow ISA memory range uncacheable mapping requests

[patch] x86, pat: allow ISA memory range uncacheable mapping requests

[Suresh Siddha]

Max Vozeler reported:
>  Bug 13877 -  bogl-term broken with CONFIG_X86_PAT=y, works with =n  
>
>  strace of bogl-term:
>  814   mmap2(NULL, 65536, PROT_READ|PROT_WRITE, MAP_SHARED, 4, 0)
>				 = -1 EAGAIN (Resource temporarily unavailable)
>  814   write(2, "bogl: mmaping /dev/fb0: Resource temporarily unavailable\n",
>	       57) = 57

PAT code maps the ISA memory range as WB in the PAT attribute, so that
fixed range MTRR registers define the actual memory type (UC/WC/WT etc).

But the upper level is_new_memtype_allowed() API checks are failing,
as the request here is for UC and the return tracked type is WB (Tracked type is
WB as MTRR type for this legacy range potentially will be different for each
4k page).

Fix is_new_memtype_allowed() by always succeeding the ISA address range
checks, as the null PAT (WB) and def MTRR fixed range register settings
satisfy the memory type needs of the applications that map the ISA address
range.

Reported-and-Tested-by: Max Vozeler <xam@debian.org>
Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
---

diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 9de8729..5d2774e 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -2,6 +2,7 @@
 #define _ASM_X86_PGTABLE_H
 
 #include <asm/page.h>
+#include <asm/e820.h>
 
 #include <asm/pgtable_types.h>
 
@@ -274,10 +275,17 @@ static inline pgprot_t pgprot_modify(pgprot_t oldprot, pgprot_t newprot)
 
 #define canon_pgprot(p) __pgprot(massage_pgprot(p))
 
-static inline int is_new_memtype_allowed(unsigned long flags,
-						unsigned long new_flags)
+static inline int is_new_memtype_allowed(u64 paddr, unsigned long size,
+					 unsigned long flags,
+					 unsigned long new_flags)
 {
 	/*
+ 	 * PAT type is always WB for ISA. So no need to check.
+ 	 */
+	if (is_ISA_range(paddr, paddr + size - 1))
+		return 1;
+	
+	/*
 	 * Certain new memtypes are not allowed with certain
 	 * requested memtype:
 	 * - request is uncached, return cannot be write-back
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index e6718bb..352aa9e 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -623,7 +623,8 @@ static int reserve_pfn_range(u64 paddr, unsigned long size, pgprot_t *vma_prot,
 		return ret;
 
 	if (flags != want_flags) {
-		if (strict_prot || !is_new_memtype_allowed(want_flags, flags)) {
+		if (strict_prot ||
+		    !is_new_memtype_allowed(paddr, size, want_flags, flags)) {
 			free_memtype(paddr, paddr + size);
 			printk(KERN_ERR "%s:%d map pfn expected mapping type %s"
 				" for %Lx-%Lx, got %s\n",

Re: [patch] x86, pat: allow ISA memory range uncacheable mapping requests

[H. Peter Anvin]

On 08/17/2009 01:23 PM, Suresh Siddha wrote:
> Max Vozeler reported:
>>  Bug 13877 -  bogl-term broken with CONFIG_X86_PAT=y, works with =n  
>>
>>  strace of bogl-term:
>>  814   mmap2(NULL, 65536, PROT_READ|PROT_WRITE, MAP_SHARED, 4, 0)
>> 				 = -1 EAGAIN (Resource temporarily unavailable)
>>  814   write(2, "bogl: mmaping /dev/fb0: Resource temporarily unavailable\n",
>> 	       57) = 57
> 
> PAT code maps the ISA memory range as WB in the PAT attribute, so that
> fixed range MTRR registers define the actual memory type (UC/WC/WT etc).
> 
> But the upper level is_new_memtype_allowed() API checks are failing,
> as the request here is for UC and the return tracked type is WB (Tracked type is
> WB as MTRR type for this legacy range potentially will be different for each
> 4k page).
> 
> Fix is_new_memtype_allowed() by always succeeding the ISA address range
> checks, as the null PAT (WB) and def MTRR fixed range register settings
> satisfy the memory type needs of the applications that map the ISA address
> range.

This patch seems correct in that it matches the current behavior of the
code.  I have, though, to ask what the logic behind treating the ISA
region in this way is.  From a hardware perspective it makes sense --
these addresses have the Legacy MTRRs which are like a
physical-address-based PAT, but it seems somewhat odd that'd we would
expect applications to use different APIs for this region.

I think the patch is definitely OK for x86/urgent, but I'd like some
thoughts about if this really is The Right Thing in the long term?

	-hpa

Re: [patch] x86, pat: allow ISA memory range uncacheable mapping requests

[Suresh Siddha]

On Mon, 2009-08-17 at 14:11 -0700, H. Peter Anvin wrote:
> On 08/17/2009 01:23 PM, Suresh Siddha wrote:
> > Max Vozeler reported:
> >>  Bug 13877 -  bogl-term broken with CONFIG_X86_PAT=y, works with =n  
> >>
> >>  strace of bogl-term:
> >>  814   mmap2(NULL, 65536, PROT_READ|PROT_WRITE, MAP_SHARED, 4, 0)
> >> 				 = -1 EAGAIN (Resource temporarily unavailable)
> >>  814   write(2, "bogl: mmaping /dev/fb0: Resource temporarily unavailable\n",
> >> 	       57) = 57
> > 
> > PAT code maps the ISA memory range as WB in the PAT attribute, so that
> > fixed range MTRR registers define the actual memory type (UC/WC/WT etc).
> > 
> > But the upper level is_new_memtype_allowed() API checks are failing,
> > as the request here is for UC and the return tracked type is WB (Tracked type is
> > WB as MTRR type for this legacy range potentially will be different for each
> > 4k page).
> > 
> > Fix is_new_memtype_allowed() by always succeeding the ISA address range
> > checks, as the null PAT (WB) and def MTRR fixed range register settings
> > satisfy the memory type needs of the applications that map the ISA address
> > range.
> 
> This patch seems correct in that it matches the current behavior of the
> code.  I have, though, to ask what the logic behind treating the ISA
> region in this way is.

With the (legacy) applications mapping this ISA region we have observed

a) they typically map more than what they want. some of this might be
covering some RAM backed pages  which are at < 1MB

b) they really don't care about the requested attribute but some specify
uncached and some don't specify any attribute

And mapped ISA range can have different memory attributes (UC, WT, WP
etc) based on the MTRR fixed range registers.

> From a hardware perspective it makes sense --
> these addresses have the Legacy MTRRs which are like a
> physical-address-based PAT, but it seems somewhat odd that'd we would
> expect applications to use different APIs for this region.

We are using the same pat API but the API handles this legacy range
differently.

Atleast some of the special ISA checks can be simplified by tracking the
memory range page by page (based on the attribute set by mtrr etc) for
this region and not at the granularity of the user mapping request. But
we need to pass the per page attributes to the higher level API's like
ioremap() or remap_pfn_range()  who has to set different attributes for
these pfn's. Not sure if this is all worth for some legacy applications
which are not performance sensitive and all they want is to just access
those memory ranges.

thanks,
suresh