RE: [PATCH v2 1/2] x86: Enable fast strings on Intel if BIOS hasn'talready

[Yu, Fenghua]

> -----Original Message-----
> From: amluto@xxxxxxxxx [mailto:amluto@xxxxxxxxx] On Behalf Of Andrew
> Lutomirski
> Sent: Saturday, August 06, 2011 4:32 PM
> To: Yu, Fenghua
> Cc: x86@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Matthew Garrett; Len
> Brown; linux-acpi@xxxxxxxxxxxxxxx; Ingo Molnar
> Subject: Re: [PATCH v2 1/2] x86: Enable fast strings on Intel if BIOS
> hasn't already
> 
> On Sat, Aug 6, 2011 at 3:33 PM, Yu, Fenghua <fenghua.yu@xxxxxxxxx>
> wrote:
> >> -----Original Message-----
> >> From: Andy Lutomirski [mailto:luto@xxxxxxx]
> >> Sent: Saturday, August 06, 2011 4:43 AM
> >> To: x86@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> >> Cc: Yu, Fenghua; Matthew Garrett; Len Brown; linux-
> >> acpi@xxxxxxxxxxxxxxx; 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@xxxxxxx>
> >> ---
> >>  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...

Ok. I see.

Still I would suggest to remove this printk. This info will be printed on every single CPU if BIOS doesn't enable fast string. This could flood the log buffer on big machines or many threads machines. And the info doesn't really provide useful message because we enable fast string in OS anyway. Reporting BIOS issue by this info probably will be viewed as low priority or ignored. 

The following info is more important because it reports an abnormal behavior.

Thanks.

-Fenghua
--
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/