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

[Andreas Herrmann <andreas.herrmann3@amd.com>]

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@gmail.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