Messages in this thread | | | Date | Thu, 13 Jun 2013 15:11:20 -0700 | Subject | Re: [PATCH] x86: Fix /proc/mtrr with base/size more than 44bits | From | Yinghai Lu <> |
| |
On Thu, Jun 13, 2013 at 1:47 PM, H. Peter Anvin <hpa@zytor.com> wrote: > On 06/13/2013 11:53 AM, Yinghai Lu wrote: >> >> - if (base & size_or_mask || size & size_or_mask) { >> + if (base >> (boot_cpu_data.x86_phys_bits - PAGE_SHIFT) || >> + base > (base + size) || >> + (base + size - 1) >> (boot_cpu_data.x86_phys_bits - PAGE_SHIFT)) { >> pr_warning("mtrr: base or size exceeds the MTRR width\n"); >> return -EINVAL; >> } > > Most of this patch looks good as far as being a minimal patch, but I'm > really confused about this bit. Could you explain the reason for why > the original doesn't work? (To be fair: I am not even sure the original > does anything useful so it could just be a "this is just too broken to > live" kind of thing.)
all because I update size_of_mask for old cpus that does not have cpuid 80000008. by make high 32bits to be all 1s. otherwise size = -mask trick will not work.
then check those range size_or_mask using, found that is not right.
as base and size could be all small, but base + size -1 could be big enough. then the original will not detect that is out of boundary.
also we could even use x86_phys_bits directly.
> > The first and third clause of the test can be simplified, however: > > (base | (base + size - 1)) >> (boot_cpu_data.x86_phys_bits - PAGE_SHIFT) > > ... although it would be cleaner to put boot_cpu_data.x86_phys_bits - > PAGE_SHIFT into a variable.
Yes.
also we can drop base > (base + size) checking, as base and size are already shifted with PAGE_SHIFT to pfn. so base+size can not be wrapped.
> > A lot of the mask_hi/mask_lo stuff should just get removed by using > rdmsrl/wrmsrl, but that is not stable material obviously.
yes.
will send updated version shortly.
Thanks
Yinghai
| |