[patch] x86, pat: fix reserve_memtype() for legacy 1MB range

[patch] x86, pat: fix reserve_memtype() for legacy 1MB range

[Suresh Siddha]

From: Suresh Siddha <suresh.b.siddha@intel.com>
Subject: x86, pat: fix reserve_memtype() for legacy 1MB range

Thierry Vignaud reported:
> http://bugzilla.kernel.org/show_bug.cgi?id=12372
>
> On P4 with an SiS motherboard (video card is a SiS 651)
> X server fails to start with error:
> xf86MapVidMem: Could not mmap framebuffer (0x00000000,0x2000) (Invalid
> argument)

Here X is trying to map first 8KB of memory using /dev/mem. Existing
code treats first 0-4KB of memory as non-RAM and 4KB-8KB as RAM. Recent
code changes don't allow to map memory with different attributes
at the same time.

Fix this by treating the first 1MB legacy region as special and always
track the attribute requests with in this region using linear linked
list (and don't bother if the range is RAM or non-RAM or mixed)

Reported-and-tested-by: Thierry Vignaud <tvignaud@mandriva.com>
Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Cc: stable@kernel.org
---

diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index 85cbd3c..c959a4d 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -333,11 +333,20 @@ int reserve_memtype(u64 start, u64 end, unsigned long req_type,
 					      req_type & _PAGE_CACHE_MASK);
 	}
 
-	is_range_ram = pagerange_is_ram(start, end);
-	if (is_range_ram == 1)
-		return reserve_ram_pages_type(start, end, req_type, new_type);
-	else if (is_range_ram < 0)
-		return -EINVAL;
+	/*
+	 * For legacy reasons, some parts of the physical address range in the
+	 * legacy 1MB region is treated as non-RAM (even when listed as RAM in
+	 * the e820 tables).  So we will track the memory attributes of this
+	 * legacy 1MB region using the linear memtype_list always.
+	 */
+	if (end >= ISA_END_ADDRESS) {
+		is_range_ram = pagerange_is_ram(start, end);
+		if (is_range_ram == 1)
+			return reserve_ram_pages_type(start, end, req_type,
+						      new_type);
+		else if (is_range_ram < 0)
+			return -EINVAL;
+	}
 
 	new  = kmalloc(sizeof(struct memtype), GFP_KERNEL);
 	if (!new)

Re: [patch] x86, pat: fix reserve_memtype() for legacy 1MB range

[H. Peter Anvin]

Suresh Siddha wrote:
> 
> Here X is trying to map first 8KB of memory using /dev/mem. Existing
> code treats first 0-4KB of memory as non-RAM and 4KB-8KB as RAM. Recent
> code changes don't allow to map memory with different attributes
> at the same time.
> 

Why was 0-4 KB marked as non-RAM?  It is most definitely RAM, and should 
be WB.

	-hpa

Re: [patch] x86, pat: fix reserve_memtype() for legacy 1MB range

[Suresh Siddha]

On Fri, Jan 09, 2009 at 02:39:31PM -0800, H. Peter Anvin wrote:
> Suresh Siddha wrote:
> >
> > Here X is trying to map first 8KB of memory using /dev/mem. Existing
> > code treats first 0-4KB of memory as non-RAM and 4KB-8KB as RAM. Recent
> > code changes don't allow to map memory with different attributes
> > at the same time.
> >
> 
> Why was 0-4 KB marked as non-RAM?  It is most definitely RAM, and should
> be WB.

While in reality it is RAM, we have CONFIG_STRICT_DEVMEM which doesn't allow
apps to map RAM pages using /dev/mem. And to allow app's to map the
legacy 0-4KB bios data page, we consider it as non-RAM.

thanks,
suresh

Re: [patch] x86, pat: fix reserve_memtype() for legacy 1MB range

[H. Peter Anvin]

Suresh Siddha wrote:
> On Fri, Jan 09, 2009 at 02:39:31PM -0800, H. Peter Anvin wrote:
>> Suresh Siddha wrote:
>>> Here X is trying to map first 8KB of memory using /dev/mem. Existing
>>> code treats first 0-4KB of memory as non-RAM and 4KB-8KB as RAM. Recent
>>> code changes don't allow to map memory with different attributes
>>> at the same time.
>>>
>> Why was 0-4 KB marked as non-RAM?  It is most definitely RAM, and should
>> be WB.
> 
> While in reality it is RAM, we have CONFIG_STRICT_DEVMEM which doesn't allow
> apps to map RAM pages using /dev/mem. And to allow app's to map the
> legacy 0-4KB bios data page, we consider it as non-RAM.

Permission to map and memory type should not be connected.  Just to 
clarify, when mapped, was it still WB?

	-hpa

Re: [patch] x86, pat: fix reserve_memtype() for legacy 1MB range

[Suresh Siddha]

On Fri, Jan 09, 2009 at 02:55:55PM -0800, H. Peter Anvin wrote:
> Suresh Siddha wrote:
> > On Fri, Jan 09, 2009 at 02:39:31PM -0800, H. Peter Anvin wrote:
> >> Suresh Siddha wrote:
> >>> Here X is trying to map first 8KB of memory using /dev/mem. Existing
> >>> code treats first 0-4KB of memory as non-RAM and 4KB-8KB as RAM. Recent
> >>> code changes don't allow to map memory with different attributes
> >>> at the same time.
> >>>
> >> Why was 0-4 KB marked as non-RAM?  It is most definitely RAM, and should
> >> be WB.
> > 
> > While in reality it is RAM, we have CONFIG_STRICT_DEVMEM which doesn't allow
> > apps to map RAM pages using /dev/mem. And to allow app's to map the
> > legacy 0-4KB bios data page, we consider it as non-RAM.
> 
> Permission to map and memory type should not be connected.  Just to 
> clarify, when mapped, was it still WB?

yes it is. It's just treated and tracked differently.

Re: [patch] x86, pat: fix reserve_memtype() for legacy 1MB range

[H. Peter Anvin]

Suresh Siddha wrote:
>> Permission to map and memory type should not be connected.  Just to 
>> clarify, when mapped, was it still WB?
> 
> yes it is. It's just treated and tracked differently.

OK, cool.  Just wanted to check.

	-hpa

Re: [patch] x86, pat: fix reserve_memtype() for legacy 1MB range

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> Suresh Siddha wrote:
>>> Permission to map and memory type should not be connected.  Just to  
>>> clarify, when mapped, was it still WB?
>>
>> yes it is. It's just treated and tracked differently.
>
> OK, cool.  Just wanted to check.

ok - i've applied Suresh's fix to tip/x86/urgent, thanks guys!

	Ingo

Re: [patch] x86, pat: fix reserve_memtype() for legacy 1MB range

[Pavel Machek]

On Fri 2009-01-09 14:48:04, Suresh Siddha wrote:
> On Fri, Jan 09, 2009 at 02:39:31PM -0800, H. Peter Anvin wrote:
> > Suresh Siddha wrote:
> > >
> > > Here X is trying to map first 8KB of memory using /dev/mem. Existing
> > > code treats first 0-4KB of memory as non-RAM and 4KB-8KB as RAM. Recent
> > > code changes don't allow to map memory with different attributes
> > > at the same time.
> > >
> > 
> > Why was 0-4 KB marked as non-RAM?  It is most definitely RAM, and should
> > be WB.
> 
> While in reality it is RAM, we have CONFIG_STRICT_DEVMEM which doesn't allow
> apps to map RAM pages using /dev/mem. And to allow app's to map the
> legacy 0-4KB bios data page, we consider it as non-RAM.

Fix config_strict_devmem? Ram is ram, and we should not li for
strict_devmem benefit...

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch] x86, pat: fix reserve_memtype() for legacy 1MB range

[H. Peter Anvin]

Pavel Machek wrote:
> On Fri 2009-01-09 14:48:04, Suresh Siddha wrote:
>> On Fri, Jan 09, 2009 at 02:39:31PM -0800, H. Peter Anvin wrote:
>>> Suresh Siddha wrote:
>>>> Here X is trying to map first 8KB of memory using /dev/mem. Existing
>>>> code treats first 0-4KB of memory as non-RAM and 4KB-8KB as RAM. Recent
>>>> code changes don't allow to map memory with different attributes
>>>> at the same time.
>>>>
>>> Why was 0-4 KB marked as non-RAM?  It is most definitely RAM, and should
>>> be WB.
>> While in reality it is RAM, we have CONFIG_STRICT_DEVMEM which doesn't allow
>> apps to map RAM pages using /dev/mem. And to allow app's to map the
>> legacy 0-4KB bios data page, we consider it as non-RAM.
> 
> Fix config_strict_devmem? Ram is ram, and we should not li for
> strict_devmem benefit...
> 

*As far as I understand* this is only considered non-RAM for the purpose
of strict_devmem?

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [patch] x86, pat: fix reserve_memtype() for legacy 1MB range

[Pavel Machek]

On Sat 2009-01-17 15:24:39, H. Peter Anvin wrote:
> Pavel Machek wrote:
> > On Fri 2009-01-09 14:48:04, Suresh Siddha wrote:
> >> On Fri, Jan 09, 2009 at 02:39:31PM -0800, H. Peter Anvin wrote:
> >>> Suresh Siddha wrote:
> >>>> Here X is trying to map first 8KB of memory using /dev/mem. Existing
> >>>> code treats first 0-4KB of memory as non-RAM and 4KB-8KB as RAM. Recent
> >>>> code changes don't allow to map memory with different attributes
> >>>> at the same time.
> >>>>
> >>> Why was 0-4 KB marked as non-RAM?  It is most definitely RAM, and should
> >>> be WB.
> >> While in reality it is RAM, we have CONFIG_STRICT_DEVMEM which doesn't allow
> >> apps to map RAM pages using /dev/mem. And to allow app's to map the
> >> legacy 0-4KB bios data page, we consider it as non-RAM.
> > 
> > Fix config_strict_devmem? Ram is ram, and we should not li for
> > strict_devmem benefit...
> 
> *As far as I understand* this is only considered non-RAM for the purpose
> of strict_devmem?

I did not look *too* closely, but the patch at the beggining of this
thread was for pat.c or something like that. I'd expect it to change
/dev/mem code...
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch] x86, pat: fix reserve_memtype() for legacy 1MB range

[Suresh Siddha]

On Sat, Jan 17, 2009 at 09:42:25AM -0800, Pavel Machek wrote:
> On Fri 2009-01-09 14:48:04, Suresh Siddha wrote:
> > On Fri, Jan 09, 2009 at 02:39:31PM -0800, H. Peter Anvin wrote:
> > > Suresh Siddha wrote:
> > > >
> > > > Here X is trying to map first 8KB of memory using /dev/mem. Existing
> > > > code treats first 0-4KB of memory as non-RAM and 4KB-8KB as RAM. Recent
> > > > code changes don't allow to map memory with different attributes
> > > > at the same time.
> > > >
> > >
> > > Why was 0-4 KB marked as non-RAM?  It is most definitely RAM, and should
> > > be WB.
> >
> > While in reality it is RAM, we have CONFIG_STRICT_DEVMEM which doesn't allow
> > apps to map RAM pages using /dev/mem. And to allow app's to map the
> > legacy 0-4KB bios data page, we consider it as non-RAM.
> 
> Fix config_strict_devmem? Ram is ram, and we should not li for
> strict_devmem benefit...

This is not just related to CONFIG_STRICT_DEVMEM. This is how we track
the PAT attribute for this 1MB physical address range even in the case of
!CONFIG_STRICT_DEVMEM

In some portions of the x86 code, we treat legacy first 1MB as special.
CONFIG_STRICT_DEVMEM is one of them.

For this range, we don't really follow what e820 says about this region. For
example, for some of the systems, e820 table doesn't even include some portions
of this 1MB region etc.

I looked at including this 1MB check in page_is_ram(). But this routine
is called at various generic places (like mem_init() where it really needs
to know what e820 says etc).

thanks,
suresh