linux-kernel - Re: [PATCH 8/10 -tip] x86: Add cpufeature for Microcode update

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: <20090513054627.GB9991@alberich.amd.com>
Date:	Wed, 13 May 2009 07:46:27 +0200
From:	Andreas Herrmann <andreas.herrmann3@....com>
To:	Jaswinder Singh Rajput <jaswinder@...nel.org>
CC:	Ingo Molnar <mingo@...e.hu>, "H. Peter Anvin" <hpa@...nel.org>,
	Robert Richter <robert.richter@....com>,
	Dave Jones <davej@...hat.com>,
	LKML <linux-kernel@...r.kernel.org>,
	x86 maintainers <x86@...nel.org>
Subject: Re: [PATCH 8/10 -tip] x86: Add cpufeature for Microcode update

On Tue, May 12, 2009 at 09:13:16PM +0530, Jaswinder Singh Rajput wrote:
> 
> Setting microcode update feature to friendly access of UCODE MSRs like:
> 1. IA32_PLATFORM_ID (Intel)
> 2. IA32_UCODE_WRITE (Intel)
> 3. IA32_UCODE_REV (Intel)
> 4. MSR_AMD64_PATCH_LEVEL (AMD)
> 5. MSR_AMD64_PATCH_LOADER (AMD)
> 
> Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@...il.com>

NAK! There is absolutely no point in moving the CPU
family/model/revision checks from the microcode drivers into generic
setup code.

Please explain what's the benefit of this?

>  arch/x86/include/asm/cpufeature.h |    1 +
>  arch/x86/kernel/cpu/amd.c         |   12 ++++++++++++
>  arch/x86/kernel/cpu/intel.c       |   11 +++++++++++
>  3 files changed, 24 insertions(+), 0 deletions(-)

I just see that it adds additional lines of code without any benefit.
You don't even check for this flag in microcode_core code which would
have introduced a "minor use case" of this.

Instead of checking CPU family/model/revision in those drivers (that
are really requiring this information and really know what to check
for) you disperse the handling of it over the kernel.

Last not least. The check in the AMD microcode loader driver ensures
that only AMD CPU families >= 0x10 are handled by the driver. It's a
mystery to me how this information is useful to set a generic
microcode feature flag "to friendly access of UCODE MSRs".

Maybe you wanted to introduce a generic flag to indicate that certain
MSRs are available. Then this check is just bogus.

Please, if you want to access microcode related MSRs in other parts of
the kernel, introduce the right CPU family and revision checks at the new
respective places.

Regards,
Andreas

-- 
Operating | Advanced Micro Devices GmbH
  System  | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. München, Germany
 Research | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
  Center  | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
  (OSRC)  | Registergericht München, HRB Nr. 43632

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