Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 10 Oct 2018 18:37:30 +0200
From:   Borislav Petkov <bp@alien8.de>
To:     Andi Kleen <andi@firstfloor.org>
Cc:     peterz@infradead.org, x86@kernel.org, eranian@google.com,
        kan.liang@intel.com, linux-kernel@vger.kernel.org,
        Andi Kleen <ak@linux.intel.com>
Subject: Re: [PATCH v2 1/2] x86/cpufeature: Add facility to match microcode
 revisions
Message-ID: <20181010163730.GA6125@zn.tnic>
References: <20181010162608.23899-1-andi@firstfloor.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20181010162608.23899-1-andi@firstfloor.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Lemme paste some of tglx's review comments from last time.

On Wed, Oct 10, 2018 at 09:26:07AM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> For bug workarounds or checks it is useful to check for specific
> microcode versions. Add a new table format to check for steppings

s/versions/revisions/

> with min microcode revisions.
> 
> This does not change the existing x86_cpu_id because it's an ABI
> shared with modutils, and also has quite difference requirements,

s/difference/different/

> as in no wildcards, but everything has to be matched exactly.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
> v2:
> Remove all CPU match, only check boot cpu
> Move INTEL_MIN_UCODE macro to header.
> Minor cleanups.
> Remove max ucode and driver data
> ---
>  arch/x86/include/asm/cpu_device_id.h | 26 ++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/match.c          | 21 +++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/arch/x86/include/asm/cpu_device_id.h b/arch/x86/include/asm/cpu_device_id.h
> index baeba0567126..1b90bd1d0b95 100644
> --- a/arch/x86/include/asm/cpu_device_id.h
> +++ b/arch/x86/include/asm/cpu_device_id.h
> @@ -11,4 +11,30 @@
>  
>  extern const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match);
>  
> +/*
> + * Match specific microcodes

"What means microcodes or steppings? If you mean microcode revisions then
please spell it out and use it all over the place. steppings is confusing
at best as its associated to the CPU stepping."

> + *
> + * vendor/family/model/stepping must be all set.
> + * min_ucode is optional and can be 0.
> + */
> +
> +struct x86_ucode_id {
> +	u8 vendor;
> +	u8 family;
> +	u16 model;
> +	u16 stepping;

"Why u16? The corresponding members in cpuinfo_x86 are 8bit wide so why
wasting memory for these tables?"

> +	u32 min_ucode;
> +};
> +
> +#define INTEL_MIN_UCODE(mod, step, rev) {			\
> +	.vendor = X86_VENDOR_INTEL,				\
> +	.family = 6,						\
> +	.model = mod,						\
> +	.stepping = step,					\
> +	.min_ucode = rev,					\
> +}
> +
> +extern const struct x86_ucode_id *
> +x86_match_ucode(const struct x86_ucode_id *match);
> +
>  #endif
> diff --git a/arch/x86/kernel/cpu/match.c b/arch/x86/kernel/cpu/match.c
> index 3fed38812eea..ec8ee31699cd 100644
> --- a/arch/x86/kernel/cpu/match.c
> +++ b/arch/x86/kernel/cpu/match.c
> @@ -48,3 +48,24 @@ const struct x86_cpu_id *x86_match_cpu(const struct x86_cpu_id *match)
>  	return NULL;
>  }
>  EXPORT_SYMBOL(x86_match_cpu);
> +
> +const struct x86_ucode_id *x86_match_ucode(const struct x86_ucode_id *match)

s/ucode/microcode/

> +{
> +	struct cpuinfo_x86 *c = &boot_cpu_data;
> +	const struct x86_ucode_id *m;
> +
> +	for (m = match; m->vendor | m->family | m->model; m++) {
> +		if (c->x86_vendor != m->vendor)
> +			continue;
> +		if (c->x86 != m->family)
> +			continue;
> +		if (c->x86_model != m->model)
> +			continue;
> +		if (c->x86_stepping != m->stepping)
> +			continue;
> +		if (c->microcode < m->min_ucode)
> +			continue;
> +		return m;
> +	}
> +	return NULL;
> +}
> -- 
> 2.17.1
> 

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.