Re: [PATCH] x86: simplify mtrr_bp_init()

[Jan Beulich]

>>> On 07.07.12 at 00:02, Yinghai Lu <yinghai@xxxxxxxxxx> wrote:
> On Fri, Jul 6, 2012 at 7:18 AM, Jan Beulich <JBeulich@xxxxxxxx> 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@xxxxxxxx>
>>
>> ---
>>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/