[patch] mtrr: fix issues with large addresses

[patch] mtrr: fix issues with large addresses

[Andreas Herrmann]

Hi,

This is a repost of a mail sent to Richard Gooch and lkml some time
ago. Meanwhile I noticed that Richard has a new email address. And it
seems that he does not maintain the mtrr code anymore. (So how about
updating the MAINTAINERS file?)

Here we go again -- with new recipient and a slightly modified
version of the patch.


Regards,

Andreas


mtrr: fix issues with large addresses

Fixes some issues with /proc/mtrr interface:
o If physical address size crosses the 44 bit boundary
  size_or_mask is evaluated wrong
o size_and_mask limits physical base
  address for an MTRR to be less than 44 bit
o added check to restrict base address to 36 bit on i386

Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>

--
diff --git a/arch/i386/kernel/cpu/mtrr/generic.c b/arch/i386/kernel/cpu/mtrr/generic.c
index f77fc53..aa21d15 100644
--- a/arch/i386/kernel/cpu/mtrr/generic.c
+++ b/arch/i386/kernel/cpu/mtrr/generic.c
@@ -172,7 +172,7 @@ int generic_get_free_region(unsigned long base, unsigned long size, int replace_
 static void generic_get_mtrr(unsigned int reg, unsigned long *base,
 			     unsigned long *size, mtrr_type *type)
 {
-	unsigned int mask_lo, mask_hi, base_lo, base_hi;
+	unsigned long mask_lo, mask_hi, base_lo, base_hi;
 
 	rdmsr(MTRRphysMask_MSR(reg), mask_lo, mask_hi);
 	if ((mask_lo & 0x800) == 0) {
diff --git a/arch/i386/kernel/cpu/mtrr/if.c b/arch/i386/kernel/cpu/mtrr/if.c
index 5ae1705..3abc3f1 100644
--- a/arch/i386/kernel/cpu/mtrr/if.c
+++ b/arch/i386/kernel/cpu/mtrr/if.c
@@ -137,6 +137,10 @@ mtrr_write(struct file *file, const char __user *buf, size_t len, loff_t * ppos)
 	for (i = 0; i < MTRR_NUM_TYPES; ++i) {
 		if (strcmp(ptr, mtrr_strings[i]))
 			continue;
+#ifndef CONFIG_X86_64
+		if (base > 0xfffffffffULL)
+			return -EINVAL;
+#endif
 		base >>= PAGE_SHIFT;
 		size >>= PAGE_SHIFT;
 		err =
diff --git a/arch/i386/kernel/cpu/mtrr/main.c b/arch/i386/kernel/cpu/mtrr/main.c
index 16bb7ea..0acfb6a 100644
--- a/arch/i386/kernel/cpu/mtrr/main.c
+++ b/arch/i386/kernel/cpu/mtrr/main.c
@@ -50,7 +50,7 @@ u32 num_var_ranges = 0;
 unsigned int *usage_table;
 static DEFINE_MUTEX(mtrr_mutex);
 
-u32 size_or_mask, size_and_mask;
+u64 size_or_mask, size_and_mask;
 
 static struct mtrr_ops * mtrr_ops[X86_VENDOR_NUM] = {};
 
@@ -662,8 +662,8 @@ void __init mtrr_bp_init(void)
 			     boot_cpu_data.x86_mask == 0x4))
 				phys_addr = 36;
 
-			size_or_mask = ~((1 << (phys_addr - PAGE_SHIFT)) - 1);
-			size_and_mask = ~size_or_mask & 0xfff00000;
+			size_or_mask = ~((1ULL << (phys_addr - PAGE_SHIFT)) - 1);
+			size_and_mask = ~size_or_mask & 0xfffff00000ULL;
 		} else if (boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR &&
 			   boot_cpu_data.x86 == 6) {
 			/* VIA C* family have Intel style MTRRs, but
diff --git a/arch/i386/kernel/cpu/mtrr/mtrr.h b/arch/i386/kernel/cpu/mtrr/mtrr.h
index d61ea9d..289dfe6 100644
--- a/arch/i386/kernel/cpu/mtrr/mtrr.h
+++ b/arch/i386/kernel/cpu/mtrr/mtrr.h
@@ -84,7 +84,7 @@ void get_mtrr_state(void);
 
 extern void set_mtrr_ops(struct mtrr_ops * ops);
 
-extern u32 size_or_mask, size_and_mask;
+extern u64 size_or_mask, size_and_mask;
 extern struct mtrr_ops * mtrr_if;
 
 #define is_cpu(vnd)	(mtrr_if && mtrr_if->vendor == X86_VENDOR_##vnd)

Re: [patch] mtrr: fix issues with large addresses

[Siddha, Suresh B]

On Mon, Feb 05, 2007 at 06:19:59PM +0100, Andreas Herrmann wrote:
> o added check to restrict base address to 36 bit on i386

Why is this? It can go upto implemented physical bits, right?

thanks,
suresh

Re: [patch] mtrr: fix issues with large addresses

[Eric W. Biederman]

"Andreas Herrmann" <andreas.herrmann3@amd.com> writes:

> Hi,
>
> This is a repost of a mail sent to Richard Gooch and lkml some time
> ago. Meanwhile I noticed that Richard has a new email address. And it
> seems that he does not maintain the mtrr code anymore. (So how about
> updating the MAINTAINERS file?)
>
> Here we go again -- with new recipient and a slightly modified
> version of the patch.
>
>
> Regards,
>
> Andreas
>
>
> mtrr: fix issues with large addresses
>
> Fixes some issues with /proc/mtrr interface:
> o If physical address size crosses the 44 bit boundary
>   size_or_mask is evaluated wrong
> o size_and_mask limits physical base
>   address for an MTRR to be less than 44 bit
> o added check to restrict base address to 36 bit on i386

The limit is per cpu not per architecture.  So if you run a
cpu that can run in 64bit mode in 32bit mode the limit
is not 36 bits.  Even PAE in 32bit mode doesn't have that limit.

> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
>
> --
> diff --git a/arch/i386/kernel/cpu/mtrr/generic.c
> b/arch/i386/kernel/cpu/mtrr/generic.c
> index f77fc53..aa21d15 100644
> --- a/arch/i386/kernel/cpu/mtrr/generic.c
> +++ b/arch/i386/kernel/cpu/mtrr/generic.c
> @@ -172,7 +172,7 @@ int generic_get_free_region(unsigned long base, unsigned
> long size, int replace_
>  static void generic_get_mtrr(unsigned int reg, unsigned long *base,
>  			     unsigned long *size, mtrr_type *type)
>  {
> -	unsigned int mask_lo, mask_hi, base_lo, base_hi;
> +	unsigned long mask_lo, mask_hi, base_lo, base_hi;

Why?  Given the low and the high I am assuming these are all implicitly
32bit quantities.  unsigned int is fine.
  
>  	rdmsr(MTRRphysMask_MSR(reg), mask_lo, mask_hi);
>  	if ((mask_lo & 0x800) == 0) {
> diff --git a/arch/i386/kernel/cpu/mtrr/if.c b/arch/i386/kernel/cpu/mtrr/if.c
> index 5ae1705..3abc3f1 100644
> --- a/arch/i386/kernel/cpu/mtrr/if.c
> +++ b/arch/i386/kernel/cpu/mtrr/if.c
> @@ -137,6 +137,10 @@ mtrr_write(struct file *file, const char __user *buf,
> size_t len, loff_t * ppos)
>  	for (i = 0; i < MTRR_NUM_TYPES; ++i) {
>  		if (strcmp(ptr, mtrr_strings[i]))
>  			continue;
> +#ifndef CONFIG_X86_64
> +		if (base > 0xfffffffffULL)
> +			return -EINVAL;
> +#endif

That is just silly.  If the cpu is running in long mode or should
not affect this capability.

>  		base >>= PAGE_SHIFT;
>  		size >>= PAGE_SHIFT;
>  		err =
> diff --git a/arch/i386/kernel/cpu/mtrr/main.c b/arch/i386/kernel/cpu/mtrr/main.c
> index 16bb7ea..0acfb6a 100644
> --- a/arch/i386/kernel/cpu/mtrr/main.c
> +++ b/arch/i386/kernel/cpu/mtrr/main.c
> @@ -50,7 +50,7 @@ u32 num_var_ranges = 0;
>  unsigned int *usage_table;
>  static DEFINE_MUTEX(mtrr_mutex);
>  
> -u32 size_or_mask, size_and_mask;
> +u64 size_or_mask, size_and_mask;
>  
>  static struct mtrr_ops * mtrr_ops[X86_VENDOR_NUM] = {};
>  
> @@ -662,8 +662,8 @@ void __init mtrr_bp_init(void)
>  			     boot_cpu_data.x86_mask == 0x4))
>  				phys_addr = 36;
>  
> -			size_or_mask = ~((1 << (phys_addr - PAGE_SHIFT)) - 1);
> -			size_and_mask = ~size_or_mask & 0xfff00000;
> + size_or_mask = ~((1ULL << (phys_addr - PAGE_SHIFT)) - 1);
> +			size_and_mask = ~size_or_mask & 0xfffff00000ULL;

Don't you want to make this hard coded mask 0xfffffffffff00000ULL?

Eric

Re: [patch] mtrr: fix issues with large addresses

[Andi Kleen]

On Monday 05 February 2007 23:50, Siddha, Suresh B wrote:
> On Mon, Feb 05, 2007 at 06:19:59PM +0100, Andreas Herrmann wrote:
> > o added check to restrict base address to 36 bit on i386
> 
> Why is this? It can go upto implemented physical bits, right?

In theory it can, but Linux doesn't support it.

-Andi 

Re: [discuss] [patch] mtrr: fix issues with large addresses

[Jan Beulich]

>>> Andi Kleen <ak@suse.de> 06.02.07 08:53 >>>
>On Monday 05 February 2007 23:50, Siddha, Suresh B wrote:
>> On Mon, Feb 05, 2007 at 06:19:59PM +0100, Andreas Herrmann wrote:
>> > o added check to restrict base address to 36 bit on i386
>> 
>> Why is this? It can go upto implemented physical bits, right?
>
>In theory it can, but Linux doesn't support it.

I don't think I remember a restriction here, at least not below 44 bits
(that's where pfn-s would need to become 64-bit wide).

Jan

Re: [discuss] [patch] mtrr: fix issues with large addresses

[Andi Kleen]

On Tuesday 06 February 2007 10:31, Jan Beulich wrote:
> >>> Andi Kleen <ak@suse.de> 06.02.07 08:53 >>>
> >On Monday 05 February 2007 23:50, Siddha, Suresh B wrote:
> >> On Mon, Feb 05, 2007 at 06:19:59PM +0100, Andreas Herrmann wrote:
> >> > o added check to restrict base address to 36 bit on i386
> >> 
> >> Why is this? It can go upto implemented physical bits, right?
> >
> >In theory it can, but Linux doesn't support it.
> 
> I don't think I remember a restriction here, at least not below 44 bits
> (that's where pfn-s would need to become 64-bit wide).

The i386 mm code only supports 4 entries in the PGD, so more than 36bit cannot 
be mapped right now.

Also even 64MB barely works (many boxes don't boot), you would likely
need at least the 4:4 patch to go >64GB. Also we know there are tons
of possible deadlocks in various subsystems when the lowmem:highmem ratio 
gets so out of hand.

Ok it could be probably all fixed with some work (at least the mm part,
the deadlocks would be more tricky), but would seem fairly 
pointless to me because all machines with >36bits support are 64bit capable.

-Andi

Re: [discuss] [patch] mtrr: fix issues with large addresses

[Jan Beulich]

>> I don't think I remember a restriction here, at least not below 44 bits
>> (that's where pfn-s would need to become 64-bit wide).
>
>The i386 mm code only supports 4 entries in the PGD, so more than 36bit cannot 
>be mapped right now.

That has nothing to do with the number of physical address bits.

>Also even 64MB barely works (many boxes don't boot), you would likely
>need at least the 4:4 patch to go >64GB. Also we know there are tons
>of possible deadlocks in various subsystems when the lowmem:highmem ratio 
>gets so out of hand.
>
>Ok it could be probably all fixed with some work (at least the mm part,
>the deadlocks would be more tricky), but would seem fairly 
>pointless to me because all machines with >36bits support are 64bit capable.

That's a different story, and certainly a limiting factor. But this shouldn't
e.g. disallow (hypothetical?) systems that have a very sparse memory map
extending beyond 64G.

Jan

Re: [discuss] [patch] mtrr: fix issues with large addresses

[Andi Kleen]

On Tuesday 06 February 2007 10:53, Jan Beulich wrote:
> >> I don't think I remember a restriction here, at least not below 44 bits
> >> (that's where pfn-s would need to become 64-bit wide).
> >
> >The i386 mm code only supports 4 entries in the PGD, so more than 36bit cannot 
> >be mapped right now.
> 
> That has nothing to do with the number of physical address bits.

You couldn't use the memory in any ways.

Anyways I give up -- the check is probably not needed, unless Andreas
comes up with a good reason.

> 
> >Also even 64MB barely works (many boxes don't boot), you would likely
> >need at least the 4:4 patch to go >64GB. Also we know there are tons
> >of possible deadlocks in various subsystems when the lowmem:highmem ratio 
> >gets so out of hand.
> >
> >Ok it could be probably all fixed with some work (at least the mm part,
> >the deadlocks would be more tricky), but would seem fairly 
> >pointless to me because all machines with >36bits support are 64bit capable.
> 
> That's a different story, and certainly a limiting factor. But this shouldn't
> e.g. disallow (hypothetical?) systems that have a very sparse memory map
> extending beyond 64G.

They would need a discontig kernel to boot most likely, otherwise
mem_map would fill up their memory. 

And I was told Windows doesn't like that, so it's unlikely there will ever be such
x86 machines.

-Andi

Re: [patch] mtrr: fix issues with large addresses

[Andreas Herrmann]

On Mon, Feb 05, 2007 at 05:26:12PM -0700, ebiederm@xmission.com wrote:
> "Andreas Herrmann" <andreas.herrmann3@amd.com> writes:
> > mtrr: fix issues with large addresses
> >
> > Fixes some issues with /proc/mtrr interface:
> > o If physical address size crosses the 44 bit boundary
> >   size_or_mask is evaluated wrong
> > o size_and_mask limits physical base
> >   address for an MTRR to be less than 44 bit
> > o added check to restrict base address to 36 bit on i386
> 
> The limit is per cpu not per architecture.  So if you run a
> cpu that can run in 64bit mode in 32bit mode the limit
> is not 36 bits.  Even PAE in 32bit mode doesn't have that limit.
> 

Good point.

I totally ignored that on 64 bit cpus in legacy mode
- PAE-paging means up to 52 physical address bits respectively
"physical address size of the underlying implementation"
- for non-PAE-paging with PSE enabled we have 40 bits for AMD and
with PSE36 36 bits for Intel

What a mess.
(Hope anyone knows for sure which paging methods are relevant for
Linux if compiled for i386 and w/o CONFIG_X86_64?)

(Seems that in my mind this legacy stuff is still tied to 36 and 32
bits :(

> > diff --git a/arch/i386/kernel/cpu/mtrr/generic.c
> > b/arch/i386/kernel/cpu/mtrr/generic.c
> > index f77fc53..aa21d15 100644
> > --- a/arch/i386/kernel/cpu/mtrr/generic.c
> > +++ b/arch/i386/kernel/cpu/mtrr/generic.c
> > @@ -172,7 +172,7 @@ int generic_get_free_region(unsigned long base, unsigned
> > long size, int replace_
> >  static void generic_get_mtrr(unsigned int reg, unsigned long *base,
> >  			     unsigned long *size, mtrr_type *type)
> >  {
> > -	unsigned int mask_lo, mask_hi, base_lo, base_hi;
> > +	unsigned long mask_lo, mask_hi, base_lo, base_hi;
> 
> Why?  Given the low and the high I am assuming these are all implicitly
> 32bit quantities.  unsigned int is fine.

It is not, please refer to the function body, e.g.

	*base = base_hi << (32 - PAGE_SHIFT) | base_lo >> PAGE_SHIFT;

All leading 20 bits of "unsigned int" base_hi are snipped away. Thus
limiting base to use 44 bit instead of 52 bit in 64 bit mode. An
option would have been to use a type cast while shifting.

(Hmm, having your first remark in mind I have to admit that my fix is
mainly focused on 64 bit mode not on 64 bit cpu running in 32 bit ...)

> > diff --git a/arch/i386/kernel/cpu/mtrr/if.c b/arch/i386/kernel/cpu/mtrr/if.c
> > index 5ae1705..3abc3f1 100644
> > --- a/arch/i386/kernel/cpu/mtrr/if.c
> > +++ b/arch/i386/kernel/cpu/mtrr/if.c
> > @@ -137,6 +137,10 @@ mtrr_write(struct file *file, const char __user *buf,
> > size_t len, loff_t * ppos)
> >  	for (i = 0; i < MTRR_NUM_TYPES; ++i) {
> >  		if (strcmp(ptr, mtrr_strings[i]))
> >  			continue;
> > +#ifndef CONFIG_X86_64
> > +		if (base > 0xfffffffffULL)
> > +			return -EINVAL;
> > +#endif
> 
> That is just silly.  If the cpu is running in long mode or should
> not affect this capability.

Yes, that check is wrong -- due to my wrong assumption.
Base can use 52 bits not just 36 bits in 32 bit mode on 64 bit cpu.

My intention was to avoid the silent truncation of base address
in the following lines:

		base >>= PAGE_SHIFT;
		size >>= PAGE_SHIFT;
		err =
		    mtrr_add_page((unsigned long) base, ...

where base is cut at bit 44 due to the type cast.

The user doing
	#> echo 0x100000000000 size=0x0815000 type=uncachable >/proc/mtrr
will end up with a new MTRR pair having PhysBase==0x0. (At least this
will give him some time to get a coffee when his system reboots after
the crash.)

So it seems that some more stuff needs to be fixed in the mtrr code.
All unsigned long base addresses used in this code implicitly restrict
the address to 44 bit (taking the PAGE_SHIFT into account).

So I could do one of the following:
(1) prepare new patch omitting this silly hunk (-> old behaviour)
(2) check for 44 bit address size instead of 36 bit address size to
reflect the implicit truncation (-> avoid silent truncation)
(3) fix all mtrr code to be able to use up to 52 bit width physical
addresses instead of 44 bit ones if running in 32 bit mode on 64 bit
cpus.

I prefer to do (2).
(IMHO those who have the need for n>44 bit width base address in an MTRR
should stick to 64 bit mode.)

> > diff --git a/arch/i386/kernel/cpu/mtrr/main.c b/arch/i386/kernel/cpu/mtrr/main.c
> > index 16bb7ea..0acfb6a 100644
> > --- a/arch/i386/kernel/cpu/mtrr/main.c
> > +++ b/arch/i386/kernel/cpu/mtrr/main.c
> > @@ -50,7 +50,7 @@ u32 num_var_ranges = 0;
> >  unsigned int *usage_table;
> >  static DEFINE_MUTEX(mtrr_mutex);
> >  
> > -u32 size_or_mask, size_and_mask;
> > +u64 size_or_mask, size_and_mask;
> >  
> >  static struct mtrr_ops * mtrr_ops[X86_VENDOR_NUM] = {};
> >  
> > @@ -662,8 +662,8 @@ void __init mtrr_bp_init(void)
> >  			     boot_cpu_data.x86_mask == 0x4))
> >  				phys_addr = 36;
> >  
> > -			size_or_mask = ~((1 << (phys_addr - PAGE_SHIFT)) - 1);
> > -			size_and_mask = ~size_or_mask & 0xfff00000;
> > + size_or_mask = ~((1ULL << (phys_addr - PAGE_SHIFT)) - 1);
> > +			size_and_mask = ~size_or_mask & 0xfffff00000ULL;
> 
> Don't you want to make this hard coded mask 0xfffffffffff00000ULL?
> 

No.

(That mask is used for values already right shifted by PAGE_SHIFT. So
at least the upper three nibbles should be zero. And I recommend to
zero out also all bits betwenn 52 and 63.)

AFAIK size_and_mask is used to mask the address bits above 32 bits
in PhysBase and PhysMask.
0xfffff00000 << PAGE_SHIFT will mask the upper bits of PhysBase and
PhysMask up to the 52nd bit (counted from 1).

And 52 bit physical address size is exactly what both AMD and Intel
specify as the "architectural limit" in their programmer manuals for
64 bit mode.

> Eric
> 

Thanks for your comments.


Regards,

Andreas

-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Re: [discuss] [patch] mtrr: fix issues with large addresses

[Andreas Herrmann]

On Tue, Feb 06, 2007 at 09:31:45AM +0000, Jan Beulich wrote:
> >>> Andi Kleen <ak@suse.de> 06.02.07 08:53 >>>
> >On Monday 05 February 2007 23:50, Siddha, Suresh B wrote:
> >> On Mon, Feb 05, 2007 at 06:19:59PM +0100, Andreas Herrmann wrote:
> >> > o added check to restrict base address to 36 bit on i386
> >> 
> >> Why is this? It can go upto implemented physical bits, right?
> >
> >In theory it can, but Linux doesn't support it.
> 
> I don't think I remember a restriction here, at least not below 44 bits
> (that's where pfn-s would need to become 64-bit wide).
> 
> Jan
> 

Hi all,

shame on me.
Wanted to fix and interface issue where base address is truncated
at 44 bit in mtrr_write().

(And just thought 36 bit would be more than enough for that 32 bit
Linux version :)


Regards,

Andreas

-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Re: [discuss] [patch] mtrr: fix issues with large addresses

[Andreas Herrmann]

On Tue, Feb 06, 2007 at 11:54:57AM +0100, Andi Kleen wrote:
> On Tuesday 06 February 2007 10:53, Jan Beulich wrote:
> > >> I don't think I remember a restriction here, at least not below 44 bits
> > >> (that's where pfn-s would need to become 64-bit wide).
> > >
> > >The i386 mm code only supports 4 entries in the PGD, so more than 36bit cannot 
> > >be mapped right now.
> > 
> > That has nothing to do with the number of physical address bits.
> 
> You couldn't use the memory in any ways.
> 
> Anyways I give up -- the check is probably not needed, unless Andreas
> comes up with a good reason.

No, I haven't a good reason to restrict the base address to fewer
than 44 bits.

So the question is, should I completely remove that check or adapt it
to check for 44 bit instead of 36 bit?


Regards,

Andreas

-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Re: [patch] mtrr: fix issues with large addresses

[Eric W. Biederman]

"Andreas Herrmann" <andreas.herrmann3@amd.com> writes:

> On Mon, Feb 05, 2007 at 05:26:12PM -0700, ebiederm@xmission.com wrote:
>> "Andreas Herrmann" <andreas.herrmann3@amd.com> writes:
>> > mtrr: fix issues with large addresses
>> >
>> > Fixes some issues with /proc/mtrr interface:
>> > o If physical address size crosses the 44 bit boundary
>> >   size_or_mask is evaluated wrong
>> > o size_and_mask limits physical base
>> >   address for an MTRR to be less than 44 bit
>> > o added check to restrict base address to 36 bit on i386
>> 
>> The limit is per cpu not per architecture.  So if you run a
>> cpu that can run in 64bit mode in 32bit mode the limit
>> is not 36 bits.  Even PAE in 32bit mode doesn't have that limit.
>> 
>
> Good point.
>
> I totally ignored that on 64 bit cpus in legacy mode
> - PAE-paging means up to 52 physical address bits respectively
> "physical address size of the underlying implementation"
> - for non-PAE-paging with PSE enabled we have 40 bits for AMD and
> with PSE36 36 bits for Intel

For non PAE-paging you have 32bits.

The real question is how many physical bits does the cpu implemented
for bus transactions.  The latest Intel (non-ia64) cpus are at 40 bits
I believe, and the cutting edge AMD cpus are moving to 48bits.  The
Intel bus protocol supports 50 bits physical at least in it's ia64
incarnation so I believe some of the chipsets may actually support
that.

> What a mess.
> (Hope anyone knows for sure which paging methods are relevant for
> Linux if compiled for i386 and w/o CONFIG_X86_64?)

PAE does get used on i386, but except that it is a related phenomenon
it isn't relevant.

> (Seems that in my mind this legacy stuff is still tied to 36 and 32
> bits :(

Basically Intel had 36 bits implemented for so long it just got
stuck in everyones heads.  Forget that.  It is a question of
how many physical address bits the cpu uses when generating bus
cycles.  Nothing about the mode the cpu is running in matters.

>> > diff --git a/arch/i386/kernel/cpu/mtrr/generic.c
>> > b/arch/i386/kernel/cpu/mtrr/generic.c
>> > index f77fc53..aa21d15 100644
>> > --- a/arch/i386/kernel/cpu/mtrr/generic.c
>> > +++ b/arch/i386/kernel/cpu/mtrr/generic.c
>> > @@ -172,7 +172,7 @@ int generic_get_free_region(unsigned long base, unsigned
>> > long size, int replace_
>> >  static void generic_get_mtrr(unsigned int reg, unsigned long *base,
>> >  			     unsigned long *size, mtrr_type *type)
>> >  {
>> > -	unsigned int mask_lo, mask_hi, base_lo, base_hi;
>> > +	unsigned long mask_lo, mask_hi, base_lo, base_hi;
>> 
>> Why?  Given the low and the high I am assuming these are all implicitly
>> 32bit quantities.  unsigned int is fine.
>
> It is not, please refer to the function body, e.g.
>
> 	*base = base_hi << (32 - PAGE_SHIFT) | base_lo >> PAGE_SHIFT;
>
> All leading 20 bits of "unsigned int" base_hi are snipped away. Thus
> limiting base to use 44 bit instead of 52 bit in 64 bit mode. An
> option would have been to use a type cast while shifting.
>
> (Hmm, having your first remark in mind I have to admit that my fix is
> mainly focused on 64 bit mode not on 64 bit cpu running in 32 bit ...)

Yes.  So base needs to be come a u64. 

So base = ((base_hi << 32) | base_lo) >> PAGE_SHIFT.

I see where the 44bit limit comes in.  Do you actually have boxes
with > 16TB?

Regardless it looks like base and possibly size needs to become
a u64.  At which time the extra >> PAGE_SHIFT could be meaningless.
Either that or because base and size need to be sized in something like
megabytes.

I suspect making it a u64 sized in bytes will get the job done and
result in simpler code.


>> > diff --git a/arch/i386/kernel/cpu/mtrr/if.c b/arch/i386/kernel/cpu/mtrr/if.c
>> > index 5ae1705..3abc3f1 100644
>> > --- a/arch/i386/kernel/cpu/mtrr/if.c
>> > +++ b/arch/i386/kernel/cpu/mtrr/if.c
>> > @@ -137,6 +137,10 @@ mtrr_write(struct file *file, const char __user *buf,
>> > size_t len, loff_t * ppos)
>> >  	for (i = 0; i < MTRR_NUM_TYPES; ++i) {
>> >  		if (strcmp(ptr, mtrr_strings[i]))
>> >  			continue;
>> > +#ifndef CONFIG_X86_64
>> > +		if (base > 0xfffffffffULL)
>> > +			return -EINVAL;
>> > +#endif
>> 
>> That is just silly.  If the cpu is running in long mode or should
>> not affect this capability.
>
> Yes, that check is wrong -- due to my wrong assumption.
> Base can use 52 bits not just 36 bits in 32 bit mode on 64 bit cpu.
>
> My intention was to avoid the silent truncation of base address
> in the following lines:
>
> 		base >>= PAGE_SHIFT;
> 		size >>= PAGE_SHIFT;
> 		err =
> 		    mtrr_add_page((unsigned long) base, ...
>
> where base is cut at bit 44 due to the type cast.
>
> The user doing
> 	#> echo 0x100000000000 size=0x0815000 type=uncachable >/proc/mtrr
> will end up with a new MTRR pair having PhysBase==0x0. (At least this
> will give him some time to get a coffee when his system reboots after
> the crash.)
>
> So it seems that some more stuff needs to be fixed in the mtrr code.
> All unsigned long base addresses used in this code implicitly restrict
> the address to 44 bit (taking the PAGE_SHIFT into account).
>
> So I could do one of the following:
> (1) prepare new patch omitting this silly hunk (-> old behaviour)
> (2) check for 44 bit address size instead of 36 bit address size to
> reflect the implicit truncation (-> avoid silent truncation)
> (3) fix all mtrr code to be able to use up to 52 bit width physical
> addresses instead of 44 bit ones if running in 32 bit mode on 64 bit
> cpus.
>
> I prefer to do (2).
> (IMHO those who have the need for n>44 bit width base address in an MTRR
> should stick to 64 bit mode.)

I prefer (3).  Since the code is shared between 32 and 64bit mode it
should behave the same in both.  I know there are people who regularly
test 32bit kernels on boxes with 128 cpus and 128MB of ram.

People sometimes want crazy things and since it just a matter of changing
the type it should be no real work to get the code to work in 32bit mode.

>> > diff --git a/arch/i386/kernel/cpu/mtrr/main.c
> b/arch/i386/kernel/cpu/mtrr/main.c
>> > index 16bb7ea..0acfb6a 100644
>> > --- a/arch/i386/kernel/cpu/mtrr/main.c
>> > +++ b/arch/i386/kernel/cpu/mtrr/main.c
>> > @@ -50,7 +50,7 @@ u32 num_var_ranges = 0;
>> >  unsigned int *usage_table;
>> >  static DEFINE_MUTEX(mtrr_mutex);
>> >  
>> > -u32 size_or_mask, size_and_mask;
>> > +u64 size_or_mask, size_and_mask;
>> >  
>> >  static struct mtrr_ops * mtrr_ops[X86_VENDOR_NUM] = {};
>> >  
>> > @@ -662,8 +662,8 @@ void __init mtrr_bp_init(void)
>> >  			     boot_cpu_data.x86_mask == 0x4))
>> >  				phys_addr = 36;
>> >  
>> > -			size_or_mask = ~((1 << (phys_addr - PAGE_SHIFT)) - 1);
>> > -			size_and_mask = ~size_or_mask & 0xfff00000;
>> > + size_or_mask = ~((1ULL << (phys_addr - PAGE_SHIFT)) - 1);
>> > +			size_and_mask = ~size_or_mask & 0xfffff00000ULL;
>> 
>> Don't you want to make this hard coded mask 0xfffffffffff00000ULL?
>> 
>
> No.
>
> (That mask is used for values already right shifted by PAGE_SHIFT. So
> at least the upper three nibbles should be zero. And I recommend to
> zero out also all bits betwenn 52 and 63.)
>
> AFAIK size_and_mask is used to mask the address bits above 32 bits
> in PhysBase and PhysMask.
> 0xfffff00000 << PAGE_SHIFT will mask the upper bits of PhysBase and
> PhysMask up to the 52nd bit (counted from 1).
>
> And 52 bit physical address size is exactly what both AMD and Intel
> specify as the "architectural limit" in their programmer manuals for
> 64 bit mode.

Ok. I had earlier missed the PAGE_SHIFT shenanigans the code is playing.

Eric

Re: [discuss] [patch] mtrr: fix issues with large addresses

[Andreas Herrmann]

On Tue, Feb 06, 2007 at 10:54:23AM -0700, ebiederm@xmission.com wrote:
> "Andreas Herrmann" <andreas.herrmann3@amd.com> writes:
> > On Mon, Feb 05, 2007 at 05:26:12PM -0700, ebiederm@xmission.com wrote:
> >> "Andreas Herrmann" <andreas.herrmann3@amd.com> writes:
> >> >
> >> The limit is per cpu not per architecture.  So if you run a
> >> cpu that can run in 64bit mode in 32bit mode the limit
> >> is not 36 bits.  Even PAE in 32bit mode doesn't have that limit.
> >> 
> > Good point.
> >
> > I totally ignored that on 64 bit cpus in legacy mode
> > - PAE-paging means up to 52 physical address bits respectively
> > "physical address size of the underlying implementation"
> > - for non-PAE-paging with PSE enabled we have 40 bits for AMD and
> > with PSE36 36 bits for Intel
> 
> For non PAE-paging you have 32bits.

You are referring to current Linux implementation?
The AMD64 architecture increased physical address size in PSE mode to
40 bits. So at least it would be possible to use more than 32 bits.

> >> > diff --git a/arch/i386/kernel/cpu/mtrr/generic.c
> >> > b/arch/i386/kernel/cpu/mtrr/generic.c
> >> > index f77fc53..aa21d15 100644
> >> > --- a/arch/i386/kernel/cpu/mtrr/generic.c
> >> > +++ b/arch/i386/kernel/cpu/mtrr/generic.c
> >> > @@ -172,7 +172,7 @@ int generic_get_free_region(unsigned long base, unsigned
> >> > long size, int replace_
> >> >  static void generic_get_mtrr(unsigned int reg, unsigned long *base,
> >> >  			     unsigned long *size, mtrr_type *type)
> >> >  {
> >> > -	unsigned int mask_lo, mask_hi, base_lo, base_hi;
> >> > +	unsigned long mask_lo, mask_hi, base_lo, base_hi;
> >> 
> >> Why?  Given the low and the high I am assuming these are all implicitly
> >> 32bit quantities.  unsigned int is fine.
> >
> > It is not, please refer to the function body, e.g.
> >
> > 	*base = base_hi << (32 - PAGE_SHIFT) | base_lo >> PAGE_SHIFT;
> >
> > All leading 20 bits of "unsigned int" base_hi are snipped away. Thus
> > limiting base to use 44 bit instead of 52 bit in 64 bit mode. An
> > option would have been to use a type cast while shifting.
> >
> > (Hmm, having your first remark in mind I have to admit that my fix is
> > mainly focused on 64 bit mode not on 64 bit cpu running in 32 bit ...)
> 
> Yes.  So base needs to be come a u64. 

I was afraid you'ld say that.

> So base = ((base_hi << 32) | base_lo) >> PAGE_SHIFT.
> 
> I see where the 44bit limit comes in.  Do you actually have boxes
> with > 16TB?

No, I don't have access to such a box. Would be nice though.

> 
> Regardless it looks like base and possibly size needs to become
> a u64.  At which time the extra >> PAGE_SHIFT could be meaningless.
> Either that or because base and size need to be sized in something like
> megabytes.
> 
> I suspect making it a u64 sized in bytes will get the job done and
> result in simpler code.

Right you are!

> >> > diff --git a/arch/i386/kernel/cpu/mtrr/if.c b/arch/i386/kernel/cpu/mtrr/if.c
> >> > index 5ae1705..3abc3f1 100644
> >> > --- a/arch/i386/kernel/cpu/mtrr/if.c
> >> > +++ b/arch/i386/kernel/cpu/mtrr/if.c
> >> > @@ -137,6 +137,10 @@ mtrr_write(struct file *file, const char __user *buf,
> >> > size_t len, loff_t * ppos)
> >> >  	for (i = 0; i < MTRR_NUM_TYPES; ++i) {
> >> >  		if (strcmp(ptr, mtrr_strings[i]))
> >> >  			continue;
> >> > +#ifndef CONFIG_X86_64
> >> > +		if (base > 0xfffffffffULL)
> >> > +			return -EINVAL;
> >> > +#endif
> >> 
> >> That is just silly.  If the cpu is running in long mode or should
> >> not affect this capability.

	<snip>

> > So I could do one of the following:
> > (1) prepare new patch omitting this silly hunk (-> old behaviour)
> > (2) check for 44 bit address size instead of 36 bit address size to
> > reflect the implicit truncation (-> avoid silent truncation)
> > (3) fix all mtrr code to be able to use up to 52 bit width physical
> > addresses instead of 44 bit ones if running in 32 bit mode on 64 bit
> > cpus.
> >
> > I prefer to do (2).
> > (IMHO those who have the need for n>44 bit width base address in an MTRR
> > should stick to 64 bit mode.)
> 
> I prefer (3).  Since the code is shared between 32 and 64bit mode it
> should behave the same in both.  I know there are people who regularly
> test 32bit kernels on boxes with 128 cpus and 128MB of ram.
> 
> People sometimes want crazy things and since it just a matter of changing
> the type it should be no real work to get the code to work in 32bit mode.

Ok, it is best to do (3).
I will come up with another patch asap.


> Eric


Regards,

Andreas

-- 
AMD Saxony, Dresden, Germany
Operating System Research Center

Re: [discuss] [patch] mtrr: fix issues with large addresses

[Eric W. Biederman]

"Andreas Herrmann" <andreas.herrmann3@amd.com> writes:

> On Tue, Feb 06, 2007 at 10:54:23AM -0700, ebiederm@xmission.com wrote:
>> "Andreas Herrmann" <andreas.herrmann3@amd.com> writes:
>> > On Mon, Feb 05, 2007 at 05:26:12PM -0700, ebiederm@xmission.com wrote:
>> >> "Andreas Herrmann" <andreas.herrmann3@amd.com> writes:
>> >> >
>> >> The limit is per cpu not per architecture.  So if you run a
>> >> cpu that can run in 64bit mode in 32bit mode the limit
>> >> is not 36 bits.  Even PAE in 32bit mode doesn't have that limit.
>> >> 
>> > Good point.
>> >
>> > I totally ignored that on 64 bit cpus in legacy mode
>> > - PAE-paging means up to 52 physical address bits respectively
>> > "physical address size of the underlying implementation"
>> > - for non-PAE-paging with PSE enabled we have 40 bits for AMD and
>> > with PSE36 36 bits for Intel
>> 
>> For non PAE-paging you have 32bits.
>
> You are referring to current Linux implementation?
> The AMD64 architecture increased physical address size in PSE mode to
> 40 bits. So at least it would be possible to use more than 32 bits.

How do you get 40 physical bits in a 32bit page table entry? My memory
is that the low bits in the page table entry were well defined and
accounted for. I'm pretty certain I can account for 6 of the low bits
off the top of my head.  PSE is the page size extension allowing pages 2MB/4MB
pages.  

PAE (physical address extension) gives you a 64bit page table entry
and where you have a place for all of those extra physical bits kick
as I recall.  The limit is 52 bits and current cpus talk about support
40 bits with AMD in the process of going to 48 bits.

Is there a feature I have overlooked?  
That would allow 40 bits with PSE?

>> 
>> Yes.  So base needs to be come a u64. 
>
> I was afraid you'ld say that.
>
>> So base = ((base_hi << 32) | base_lo) >> PAGE_SHIFT.
>> 
>> I see where the 44bit limit comes in.  Do you actually have boxes
>> with > 16TB?
>
> No, I don't have access to such a box. Would be nice though.
>
>> 
>> Regardless it looks like base and possibly size needs to become
>> a u64.  At which time the extra >> PAGE_SHIFT could be meaningless.
>> Either that or because base and size need to be sized in something like
>> megabytes.
>> 
>> I suspect making it a u64 sized in bytes will get the job done and
>> result in simpler code.
>
> Right you are!

> Ok, it is best to do (3).
> I will come up with another patch asap.

Thanks.  Sorry for being a pain.

Eric

Re: [discuss] [patch] mtrr: fix issues with large addresses

[Joerg Roedel]

On Tue, Feb 06, 2007 at 12:08:12PM -0700, ebiederm@xmission.com wrote:
> "Andreas Herrmann" <andreas.herrmann3@amd.com> writes:
> > You are referring to current Linux implementation?
> > The AMD64 architecture increased physical address size in PSE mode to
> > 40 bits. So at least it would be possible to use more than 32 bits.
> 
> How do you get 40 physical bits in a 32bit page table entry? My memory
> is that the low bits in the page table entry were well defined and
> accounted for. I'm pretty certain I can account for 6 of the low bits
> off the top of my head.  PSE is the page size extension allowing pages 2MB/4MB
> pages.

The access to 40 physical address bits is only possible using large pages
(4MB on 32bit without PAE). In those page tables entrys you only use
bits 22:31 for encoding the physical address. The bits 12:21 are
unused. These unused bits are reused to encode bits 32:39 of the 40 bit
physical address.

Joerg

-- 
Joerg Roedel
Operating System Research Center
AMD Saxony LLC & Co. KG

Re: [discuss] [patch] mtrr: fix issues with large addresses

[Eric W. Biederman]

"Joerg Roedel" <joerg.roedel@amd.com> writes:

> On Tue, Feb 06, 2007 at 12:08:12PM -0700, ebiederm@xmission.com wrote:
>> "Andreas Herrmann" <andreas.herrmann3@amd.com> writes:
>> > You are referring to current Linux implementation?
>> > The AMD64 architecture increased physical address size in PSE mode to
>> > 40 bits. So at least it would be possible to use more than 32 bits.
>> 
>> How do you get 40 physical bits in a 32bit page table entry? My memory
>> is that the low bits in the page table entry were well defined and
>> accounted for. I'm pretty certain I can account for 6 of the low bits
>> off the top of my head.  PSE is the page size extension allowing pages 2MB/4MB
>> pages.
>
> The access to 40 physical address bits is only possible using large pages
> (4MB on 32bit without PAE). In those page tables entrys you only use
> bits 22:31 for encoding the physical address. The bits 12:21 are
> unused. These unused bits are reused to encode bits 32:39 of the 40 bit
> physical address.

Yep.  I missed that feature, and I do see it in AMD documentation now
that I look.

I'm not certain what that would be useful for though.

I'm pretty certain doesn't use this feature, we just enable PAE mode.

Eric