Re: [PATCH 1/2] AMD errata checking framework

Re: [PATCH 1/2] AMD errata checking framework

[H. Peter Anvin]

On 06/11/2010 06:25 AM, Hans Rosenfeld wrote:
> +
> +/*
> + * Check for the presence of an AMD erratum.
> + * Arguments are defined in processor.h for each known erratum.
> + */
> +bool cpu_has_amd_erratum(const struct cpuinfo_x86 *cpu, bool osvw, ...)
> +{

/home/hpa/kernel/linux-2.6-tip.cpu/arch/x86/kernel/cpu/amd.c: In
function ‘cpu_has_amd_erratum’:
/home/hpa/kernel/linux-2.6-tip.cpu/arch/x86/kernel/cpu/amd.c:635: error:
expected expression before ‘do’
/home/hpa/kernel/linux-2.6-tip.cpu/arch/x86/kernel/cpu/amd.c:636: error:
expected expression before ‘do’
/home/hpa/kernel/linux-2.6-tip.cpu/arch/x86/kernel/cpu/amd.c:632:
warning: unused variable ‘t’

Also, the use of divide and modulo when shifts and masks work are
generally frowned upon in Linux as a matter of style.

	-hpa

Re: [PATCH 1/2] AMD errata checking framework

[H. Peter Anvin]

On 06/11/2010 06:25 AM, Hans Rosenfeld wrote:
> +		if (static_cpu_has(X86_FEATURE_OSVW) && 

static_cpu_has() is only well-defined for code that is only executed
after alternatives execution; that would not seem to be the case for
this code.

	-hpa

Re: [PATCH 1/2] AMD errata checking framework

[Hans Rosenfeld]

On Fri, Jun 11, 2010 at 04:14:31PM -0400, H. Peter Anvin wrote:
> On 06/11/2010 06:25 AM, Hans Rosenfeld wrote:
> > +
> > +/*
> > + * Check for the presence of an AMD erratum.
> > + * Arguments are defined in processor.h for each known erratum.
> > + */
> > +bool cpu_has_amd_erratum(const struct cpuinfo_x86 *cpu, bool osvw, ...)
> > +{
> 
> /home/hpa/kernel/linux-2.6-tip.cpu/arch/x86/kernel/cpu/amd.c: In
> function ?cpu_has_amd_erratum?:
> /home/hpa/kernel/linux-2.6-tip.cpu/arch/x86/kernel/cpu/amd.c:635: error:
> expected expression before ?do?
> /home/hpa/kernel/linux-2.6-tip.cpu/arch/x86/kernel/cpu/amd.c:636: error:
> expected expression before ?do?
> /home/hpa/kernel/linux-2.6-tip.cpu/arch/x86/kernel/cpu/amd.c:632:
> warning: unused variable ?t?

Umpf. It seems like I underestimated the brokenness of rdmsrl() etc..
A new patch set will follow shortly. I also added va_end() calls where
necessary and replaced static_cpu_has() with cpu_has().

> Also, the use of divide and modulo when shifts and masks work are
> generally frowned upon in Linux as a matter of style.

While I agree in principle, I think that in this special case the
divide and modulo by 64 make it easier to understand what the code
does on the first look. But if you insist I will send another patch
set that replaces divide and modulo with shifts and masks.


Hans


-- 
%SYSTEM-F-ANARCHISM, The operating system has been overthrown

Re: [PATCH 1/2] AMD errata checking framework

[H. Peter Anvin]

On 06/14/2010 05:33 AM, Hans Rosenfeld wrote:
> 
>> Also, the use of divide and modulo when shifts and masks work are
>> generally frowned upon in Linux as a matter of style.
> 
> While I agree in principle, I think that in this special case the
> divide and modulo by 64 make it easier to understand what the code
> does on the first look. But if you insist I will send another patch
> set that replaces divide and modulo with shifts and masks.
> 

I do insist.

	-hpa