linux-kernel - Re: [PATCH -tip] x86: cpu/intel.c cleanup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090314153138.GA19459@elte.hu>
Date:	Sat, 14 Mar 2009 16:31:38 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Vegard Nossum <vegard.nossum@...il.com>
Cc:	Jaswinder Singh Rajput <jaswinder@...nel.org>,
	Sam Ravnborg <sam@...nborg.org>,
	x86 maintainers <x86@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH -tip] x86: cpu/intel.c cleanup


* Vegard Nossum <vegard.nossum@...il.com> wrote:

> 2009/3/14 Jaswinder Singh Rajput <jaswinder@...nel.org>:
> > From: Jaswinder Singh Rajput <jaswinderrajput@...il.com>
> > Date: Sat, 14 Mar 2009 17:47:38 +0530
> > Subject: [PATCH] x86: cpu/intel.c cleanup
> >
> > - fix various style problems
> >  - fix header files issues
> >
> 
> [...]
> 
> >  static void __cpuinit early_init_intel(struct cpuinfo_x86 *c)
> >  {
> > +       u64 misc_enable;
> > +
> >        /* Unmask CPUID levels if masked: */
> >        if (c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xd)) {
> > -               u64 misc_enable;
> >
> >                rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> >
> > @@ -44,16 +45,16 @@ static void __cpuinit early_init_intel(struct cpuinfo_x86 *c)
> >        }
> >
> >        if ((c->x86 == 0xf && c->x86_model >= 0x03) ||
> > -               (c->x86 == 0x6 && c->x86_model >= 0x0e))
> > +           (c->x86 == 0x6 && c->x86_model >= 0x0e))
> >                set_cpu_cap(c, X86_FEATURE_CONSTANT_TSC);
> >
> >  #ifdef CONFIG_X86_64
> >        set_cpu_cap(c, X86_FEATURE_SYSENTER32);
> > -#else
> > +#else /* CONFIG_X86_64 */
> >        /* Netburst reports 64 bytes clflush size, but does IO in 128 bytes */
> >        if (c->x86 == 15 && c->x86_cache_alignment == 64)
> >                c->x86_cache_alignment = 128;
> > -#endif
> > +#endif /* CONFIG_X86_64 */
> >
> >        /* CPUID workaround for 0F33/0F34 CPU */
> >        if (c->x86 == 0xF && c->x86_model == 0x3
> > @@ -96,19 +97,18 @@ static void __cpuinit early_init_intel(struct cpuinfo_x86 *c)
> >         * Ingo Molnar reported a Pentium D (model 6) and a Xeon
> >         * (model 2) with the same problem.
> >         */
> > -       if (c->x86 == 15) {
> > -               u64 misc_enable;
> > +       if (c->x86 != 15)
> > +               return;
> >
> > -               rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> > +       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");
> > +       if (misc_enable & MSR_IA32_MISC_ENABLE_FAST_STRING) {
> > +               pr_info("kmemcheck: Disabling fast string operations\n");
> >
> > -                       misc_enable &= ~MSR_IA32_MISC_ENABLE_FAST_STRING;
> > -                       wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> > -               }
> > +               misc_enable &= ~MSR_IA32_MISC_ENABLE_FAST_STRING;
> > +               wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
> >        }
> > -#endif
> > +#endif /* CONFIG_KMEMCHECK */
> >  }
> 
> I don't really like this change (last hunk). Doesn't it seem a 
> bit pointless? It breaks the symmetry with the masked CPUID 
> levels at the beginning of the function. If somebody wants to 
> add something else to this function, it might have to be 
> reindented again. Or is there a problem with too long lines 
> here?

yes, it would be cleaner to put the whole family 15 branch into 
a helper inline function instead.

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/