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

Eric W. Biederman
2007-02-06T00:27:58Z

"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