* [PATCH] x86: simplify mtrr_bp_init()
@ 2012-07-06 14:18 Jan Beulich
2012-07-06 22:02 ` Yinghai Lu
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2012-07-06 14:18 UTC (permalink / raw)
To: mingo, tglx, hpa; +Cc: linux-kernel
Now that the x86_phys_bits cpuinfo field is uniformly available on
32- and 64-bit, the function no longer needs to determine this anew.
Additionally, both size_or_mask and size_and_mask can be set once at
the end of the function instead of individually in each special case,
as their value solely depends on the physical address size determined
to be used here.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
arch/x86/kernel/cpu/mtrr/main.c | 47 ++++++++++------------------------------
1 file changed, 12 insertions(+), 35 deletions(-)
--- 3.5-rc5/arch/x86/kernel/cpu/mtrr/main.c
+++ 3.5-rc5-x86-mtrr-init-simplify/arch/x86/kernel/cpu/mtrr/main.c
@@ -592,67 +592,41 @@ int __initdata changed_by_mtrr_cleanup;
*/
void __init mtrr_bp_init(void)
{
- u32 phys_addr;
+ unsigned int phys_addr = boot_cpu_data.x86_phys_bits;
init_ifs();
- phys_addr = 32;
-
if (cpu_has_mtrr) {
mtrr_if = &generic_mtrr_ops;
- size_or_mask = 0xff000000; /* 36 bits */
- size_and_mask = 0x00f00000;
- phys_addr = 36;
-
- /*
- * This is an AMD specific MSR, but we assume(hope?) that
- * Intel will implement it to when they extend the address
- * bus of the Xeon.
- */
- if (cpuid_eax(0x80000000) >= 0x80000008) {
- phys_addr = cpuid_eax(0x80000008) & 0xff;
- /* CPUID workaround for Intel 0F33/0F34 CPU */
- if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
- boot_cpu_data.x86 == 0xF &&
- boot_cpu_data.x86_model == 0x3 &&
- (boot_cpu_data.x86_mask == 0x3 ||
- boot_cpu_data.x86_mask == 0x4))
- phys_addr = 36;
-
- 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) {
+
+ if (boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR &&
+ boot_cpu_data.x86 == 6) {
/*
* VIA C* family have Intel style MTRRs,
* but don't support PAE
*/
- size_or_mask = 0xfff00000; /* 32 bits */
- size_and_mask = 0;
phys_addr = 32;
- }
+ } else if (phys_addr < 36)
+ phys_addr = 36;
} else {
switch (boot_cpu_data.x86_vendor) {
case X86_VENDOR_AMD:
if (cpu_has_k6_mtrr) {
/* Pre-Athlon (K6) AMD CPU MTRRs */
mtrr_if = mtrr_ops[X86_VENDOR_AMD];
- size_or_mask = 0xfff00000; /* 32 bits */
- size_and_mask = 0;
+ phys_addr = 32;
}
break;
case X86_VENDOR_CENTAUR:
if (cpu_has_centaur_mcr) {
mtrr_if = mtrr_ops[X86_VENDOR_CENTAUR];
- size_or_mask = 0xfff00000; /* 32 bits */
- size_and_mask = 0;
+ phys_addr = 32;
}
break;
case X86_VENDOR_CYRIX:
if (cpu_has_cyrix_arr) {
mtrr_if = mtrr_ops[X86_VENDOR_CYRIX];
- size_or_mask = 0xfff00000; /* 32 bits */
- size_and_mask = 0;
+ phys_addr = 32;
}
break;
default:
@@ -660,6 +634,9 @@ void __init mtrr_bp_init(void)
}
}
+ size_or_mask = ~((1ULL << (phys_addr - PAGE_SHIFT)) - 1);
+ size_and_mask = ~size_or_mask & 0xfffff00000ULL;
+
if (mtrr_if) {
set_num_var_ranges();
init_table();
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] x86: simplify mtrr_bp_init()
2012-07-06 14:18 [PATCH] x86: simplify mtrr_bp_init() Jan Beulich
@ 2012-07-06 22:02 ` Yinghai Lu
2012-07-25 7:59 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Yinghai Lu @ 2012-07-06 22:02 UTC (permalink / raw)
To: Jan Beulich; +Cc: mingo, tglx, hpa, linux-kernel
On Fri, Jul 6, 2012 at 7:18 AM, Jan Beulich <JBeulich@suse.com> wrote:
> Now that the x86_phys_bits cpuinfo field is uniformly available on
> 32- and 64-bit, the function no longer needs to determine this anew.
>
> Additionally, both size_or_mask and size_and_mask can be set once at
> the end of the function instead of individually in each special case,
> as their value solely depends on the physical address size determined
> to be used here.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> ---
> arch/x86/kernel/cpu/mtrr/main.c | 47 ++++++++++------------------------------
> 1 file changed, 12 insertions(+), 35 deletions(-)
>
> --- 3.5-rc5/arch/x86/kernel/cpu/mtrr/main.c
> +++ 3.5-rc5-x86-mtrr-init-simplify/arch/x86/kernel/cpu/mtrr/main.c
> @@ -592,67 +592,41 @@ int __initdata changed_by_mtrr_cleanup;
> */
> void __init mtrr_bp_init(void)
> {
> - u32 phys_addr;
> + unsigned int phys_addr = boot_cpu_data.x86_phys_bits;
>
> init_ifs();
>
> - phys_addr = 32;
> -
> if (cpu_has_mtrr) {
> mtrr_if = &generic_mtrr_ops;
> - size_or_mask = 0xff000000; /* 36 bits */
> - size_and_mask = 0x00f00000;
> - phys_addr = 36;
> -
> - /*
> - * This is an AMD specific MSR, but we assume(hope?) that
> - * Intel will implement it to when they extend the address
> - * bus of the Xeon.
> - */
> - if (cpuid_eax(0x80000000) >= 0x80000008) {
> - phys_addr = cpuid_eax(0x80000008) & 0xff;
> - /* CPUID workaround for Intel 0F33/0F34 CPU */
> - if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
> - boot_cpu_data.x86 == 0xF &&
> - boot_cpu_data.x86_model == 0x3 &&
> - (boot_cpu_data.x86_mask == 0x3 ||
> - boot_cpu_data.x86_mask == 0x4))
> - phys_addr = 36;
> -
> - 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) {
> +
> + if (boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR &&
> + boot_cpu_data.x86 == 6) {
> /*
> * VIA C* family have Intel style MTRRs,
> * but don't support PAE
> */
> - size_or_mask = 0xfff00000; /* 32 bits */
> - size_and_mask = 0;
> phys_addr = 32;
> - }
> + } else if (phys_addr < 36)
> + phys_addr = 36;
> } else {
> switch (boot_cpu_data.x86_vendor) {
> case X86_VENDOR_AMD:
> if (cpu_has_k6_mtrr) {
> /* Pre-Athlon (K6) AMD CPU MTRRs */
> mtrr_if = mtrr_ops[X86_VENDOR_AMD];
> - size_or_mask = 0xfff00000; /* 32 bits */
> - size_and_mask = 0;
> + phys_addr = 32;
> }
> break;
> case X86_VENDOR_CENTAUR:
> if (cpu_has_centaur_mcr) {
> mtrr_if = mtrr_ops[X86_VENDOR_CENTAUR];
> - size_or_mask = 0xfff00000; /* 32 bits */
> - size_and_mask = 0;
> + phys_addr = 32;
> }
> break;
> case X86_VENDOR_CYRIX:
> if (cpu_has_cyrix_arr) {
> mtrr_if = mtrr_ops[X86_VENDOR_CYRIX];
> - size_or_mask = 0xfff00000; /* 32 bits */
> - size_and_mask = 0;
> + phys_addr = 32;
should drop all phys_addr assignment in this function.
x86_phys_bits should have all correct value?
Yinghai
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] x86: simplify mtrr_bp_init()
2012-07-06 22:02 ` Yinghai Lu
@ 2012-07-25 7:59 ` Jan Beulich
2012-07-25 16:57 ` H. Peter Anvin
0 siblings, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2012-07-25 7:59 UTC (permalink / raw)
To: Yinghai Lu; +Cc: mingo, tglx, linux-kernel, hpa
>>> On 07.07.12 at 00:02, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Jul 6, 2012 at 7:18 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> Now that the x86_phys_bits cpuinfo field is uniformly available on
>> 32- and 64-bit, the function no longer needs to determine this anew.
>>
>> Additionally, both size_or_mask and size_and_mask can be set once at
>> the end of the function instead of individually in each special case,
>> as their value solely depends on the physical address size determined
>> to be used here.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> ---
>> arch/x86/kernel/cpu/mtrr/main.c | 47 ++++++++++------------------------------
>> 1 file changed, 12 insertions(+), 35 deletions(-)
>>
>> --- 3.5-rc5/arch/x86/kernel/cpu/mtrr/main.c
>> +++ 3.5-rc5-x86-mtrr-init-simplify/arch/x86/kernel/cpu/mtrr/main.c
>> @@ -592,67 +592,41 @@ int __initdata changed_by_mtrr_cleanup;
>> */
>> void __init mtrr_bp_init(void)
>> {
>> - u32 phys_addr;
>> + unsigned int phys_addr = boot_cpu_data.x86_phys_bits;
>>
>> init_ifs();
>>
>> - phys_addr = 32;
>> -
>> if (cpu_has_mtrr) {
>> mtrr_if = &generic_mtrr_ops;
>> - size_or_mask = 0xff000000; /* 36 bits */
>> - size_and_mask = 0x00f00000;
>> - phys_addr = 36;
>> -
>> - /*
>> - * This is an AMD specific MSR, but we assume(hope?) that
>> - * Intel will implement it to when they extend the address
>> - * bus of the Xeon.
>> - */
>> - if (cpuid_eax(0x80000000) >= 0x80000008) {
>> - phys_addr = cpuid_eax(0x80000008) & 0xff;
>> - /* CPUID workaround for Intel 0F33/0F34 CPU */
>> - if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
>> - boot_cpu_data.x86 == 0xF &&
>> - boot_cpu_data.x86_model == 0x3 &&
>> - (boot_cpu_data.x86_mask == 0x3 ||
>> - boot_cpu_data.x86_mask == 0x4))
>> - phys_addr = 36;
>> -
>> - 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) {
>> +
>> + if (boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR &&
>> + boot_cpu_data.x86 == 6) {
>> /*
>> * VIA C* family have Intel style MTRRs,
>> * but don't support PAE
>> */
>> - size_or_mask = 0xfff00000; /* 32 bits */
>> - size_and_mask = 0;
>> phys_addr = 32;
>> - }
>> + } else if (phys_addr < 36)
>> + phys_addr = 36;
>> } else {
>> switch (boot_cpu_data.x86_vendor) {
>> case X86_VENDOR_AMD:
>> if (cpu_has_k6_mtrr) {
>> /* Pre-Athlon (K6) AMD CPU MTRRs */
>> mtrr_if = mtrr_ops[X86_VENDOR_AMD];
>> - size_or_mask = 0xfff00000; /* 32 bits */
>> - size_and_mask = 0;
>> + phys_addr = 32;
>> }
>> break;
>> case X86_VENDOR_CENTAUR:
>> if (cpu_has_centaur_mcr) {
>> mtrr_if = mtrr_ops[X86_VENDOR_CENTAUR];
>> - size_or_mask = 0xfff00000; /* 32 bits */
>> - size_and_mask = 0;
>> + phys_addr = 32;
>> }
>> break;
>> case X86_VENDOR_CYRIX:
>> if (cpu_has_cyrix_arr) {
>> mtrr_if = mtrr_ops[X86_VENDOR_CYRIX];
>> - size_or_mask = 0xfff00000; /* 32 bits */
>> - size_and_mask = 0;
>> + phys_addr = 32;
>
> should drop all phys_addr assignment in this function.
>
> x86_phys_bits should have all correct value?
Is it certain that all special cases (setting phys_addr to 32) are
covered by those CPUs not having PAE/PSE36? One would
think that this is valid to imply, but getting cpu_info's phys_bits
wrong isn't fatal as long as no addresses beyond 4G would ever
be encountered anywhere, whereas using too large an address
width here would result in the MTRR writes causing #GP. So
when I did this adjustment (a couple of years ago already - this
isn't the first submission), I decided to remain on the safe side.
Does any of the maintainers have an opinion either way?
Jan
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] x86: simplify mtrr_bp_init()
2012-07-25 7:59 ` Jan Beulich
@ 2012-07-25 16:57 ` H. Peter Anvin
2012-07-26 7:01 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: H. Peter Anvin @ 2012-07-25 16:57 UTC (permalink / raw)
To: Jan Beulich; +Cc: Yinghai Lu, mingo, tglx, linux-kernel
On 07/25/2012 12:59 AM, Jan Beulich wrote:
>>
>> should drop all phys_addr assignment in this function.
>>
>> x86_phys_bits should have all correct value?
>
> Is it certain that all special cases (setting phys_addr to 32) are
> covered by those CPUs not having PAE/PSE36? One would
> think that this is valid to imply, but getting cpu_info's phys_bits
> wrong isn't fatal as long as no addresses beyond 4G would ever
> be encountered anywhere, whereas using too large an address
> width here would result in the MTRR writes causing #GP. So
> when I did this adjustment (a couple of years ago already - this
> isn't the first submission), I decided to remain on the safe side.
>
> Does any of the maintainers have an opinion either way?
>
There are definitely CPUs which have PAE but only has a 32-bit address
bus. On the other hand there are tons of chipsets which arbitrary
address caps that almost nothing in the system knows about, so I don't
think this matters.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86: simplify mtrr_bp_init()
2012-07-25 16:57 ` H. Peter Anvin
@ 2012-07-26 7:01 ` Jan Beulich
0 siblings, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2012-07-26 7:01 UTC (permalink / raw)
To: H. Peter Anvin; +Cc: mingo, Yinghai Lu, tglx, linux-kernel
>>> On 25.07.12 at 18:57, "H. Peter Anvin" <hpa@zytor.com> wrote:
> On 07/25/2012 12:59 AM, Jan Beulich wrote:
>>>
>>> should drop all phys_addr assignment in this function.
>>>
>>> x86_phys_bits should have all correct value?
>>
>> Is it certain that all special cases (setting phys_addr to 32) are
>> covered by those CPUs not having PAE/PSE36? One would
>> think that this is valid to imply, but getting cpu_info's phys_bits
>> wrong isn't fatal as long as no addresses beyond 4G would ever
>> be encountered anywhere, whereas using too large an address
>> width here would result in the MTRR writes causing #GP. So
>> when I did this adjustment (a couple of years ago already - this
>> isn't the first submission), I decided to remain on the safe side.
>>
>> Does any of the maintainers have an opinion either way?
>>
>
> There are definitely CPUs which have PAE but only has a 32-bit address
> bus. On the other hand there are tons of chipsets which arbitrary
> address caps that almost nothing in the system knows about, so I don't
> think this matters.
The first sentence implies to me that you consider the patch
fine as is, yet the last phrase makes me rather think you want
it adjusted as per Yinghai's response.
In any case, address capping by the chipset doesn't matter
here, all we're after is determining how may bits the MTRRs
(or equivalents) implement (so that size_or_mask and
size_and_mask end up being correct).
Bottom line - I'm confused as to what (if any) adjustments
the patch needs in order to be acceptable.
Jan
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-07-26 7:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-07-06 14:18 [PATCH] x86: simplify mtrr_bp_init() Jan Beulich
2012-07-06 22:02 ` Yinghai Lu
2012-07-25 7:59 ` Jan Beulich
2012-07-25 16:57 ` H. Peter Anvin
2012-07-26 7:01 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox