Re: [PATCH v2 1/2] x86: Enable fast strings on Intel if BIOS hasn't already

[Andrew Lutomirski <luto@mit.edu>]

On Sat, Aug 6, 2011 at 3:33 PM, Yu, Fenghua <fenghua.yu@intel.com> wrote:
>> -----Original Message-----
>> From: Andy Lutomirski [mailto:luto@MIT.EDU]
>> Sent: Saturday, August 06, 2011 4:43 AM
>> To: x86@kernel.org; linux-kernel@vger.kernel.org
>> Cc: Yu, Fenghua; Matthew Garrett; Len Brown; linux-
>> acpi@vger.kernel.org; Ingo Molnar; Andy Lutomirski
>> Subject: [PATCH v2 1/2] x86: Enable fast strings on Intel if BIOS
>> hasn't already
>>
>> Intel SDM volume 3A, 8.4.2 says:
>>
>>   Software can disable fast-string operation by clearing the
>>   fast-string-enable bit (bit 0) of IA32_MISC_ENABLE MSR.
>>   However, Intel recomments that system software always enable
>>   fast-string operation.
>>
>> The Intel DQ67SW board (with latest BIOS) disables fast string
>> operations if TXT is enabled.  A Lenovo X220 disables it regardless
>> of TXT setting.  I doubt I'm the only person with a dumb BIOS like
>> this.
>>
>> Signed-off-by: Andy Lutomirski <luto@mit.edu>
>> ---
>>  arch/x86/kernel/cpu/intel.c |   25 ++++++++++++++++++++++---
>>  1 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
>> index ed6086e..c80ab41 100644
>> --- a/arch/x86/kernel/cpu/intel.c
>> +++ b/arch/x86/kernel/cpu/intel.c
>> @@ -30,6 +30,7 @@
>>  static void __cpuinit early_init_intel(struct cpuinfo_x86 *c)
>>  {
>>       u64 misc_enable;
>> +     bool allow_fast_string = true;
>>
>>       /* Unmask CPUID levels if masked: */
>>       if (c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xd)) {
>> @@ -118,8 +119,9 @@ static void __cpuinit early_init_intel(struct
>> cpuinfo_x86 *c)
>>        * (model 2) with the same problem.
>>        */
>>       if (c->x86 == 15) {
>> -             rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
>> +             allow_fast_string = false;
>>
>> +             rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
>>               if (misc_enable & MSR_IA32_MISC_ENABLE_FAST_STRING) {
>>                       printk(KERN_INFO "kmemcheck: Disabling fast string
>> operations\n");
>>
>> @@ -130,11 +132,28 @@ static void __cpuinit early_init_intel(struct
>> cpuinfo_x86 *c)
>>  #endif
>>
>>       /*
>> -      * If fast string is not enabled in IA32_MISC_ENABLE for any
>> reason,
>> -      * clear the fast string and enhanced fast string CPU
>> capabilities.
>> +      * If BIOS didn't enable fast string operation, try to enable
>> +      * it ourselves.  If that fails, then clear the fast string
>> +      * and enhanced fast string CPU capabilities.
>>        */
>>       if (c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xd)) {
>>               rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
>> +
>> +             if (allow_fast_string &&
>> +                 !(misc_enable & MSR_IA32_MISC_ENABLE_FAST_STRING)) {
>> +                     misc_enable |= MSR_IA32_MISC_ENABLE_FAST_STRING;
>> +                     wrmsr_safe(MSR_IA32_MISC_ENABLE, (u32)misc_enable,
>> +                                (u32)(misc_enable >> 32));
>> +
>> +                     /* Re-read to make sure it stuck. */
>> +                     rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
>> +
>> +                     if (misc_enable & MSR_IA32_MISC_ENABLE_FAST_STRING)
>> +                             printk(KERN_WARNING FW_WARN "CPU #%d: "
>> +                                    "IA32_MISC_ENABLE.FAST_STRING_ENABLE "
>> +                                    "was not set", c->cpu_index);
> This printk is redundant because the same info is dumped in the below printk. Plus it's not firmware's issue if we can not set fast string bit 0 in MISC_ENABLE register. So I don't think FW_WARN is right.

Huh?  This is the success path -- the patch prints the warning if
flipping the bit worked...

>
> I would suggest to remove this printk (and the if () statement) and change KERN_INFO to KERN_WARNING in the following printk.
>
>> +             }
>> +
>>               if (!(misc_enable & MSR_IA32_MISC_ENABLE_FAST_STRING)) {
>>                       printk(KERN_INFO "Disabled fast string
>> operations\n");
>>                       setup_clear_cpu_cap(X86_FEATURE_REP_GOOD);

...and this (which could be more clear) is displayed if flipping the
bit did not work, in which case the problem isn't BIOS's fault.

--Andy