From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751216AbdAQU6H (ORCPT <rfc822;w@1wt.eu>);
        Tue, 17 Jan 2017 15:58:07 -0500
Received: from Galois.linutronix.de ([146.0.238.70]:46419 "EHLO
        Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1750893AbdAQU6G (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 17 Jan 2017 15:58:06 -0500
Date: Tue, 17 Jan 2017 21:58:00 +0100 (CET)
From: Thomas Gleixner <tglx@linutronix.de>
To: Borislav Petkov <bp@alien8.de>
cc: X86 ML <x86@kernel.org>, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 11/13] x86/microcode/AMD: Unify load_ucode_amd_ap()
In-Reply-To: <20170117173734.14251-12-bp@alien8.de>
Message-ID: <alpine.DEB.2.20.1701172145470.3645@nanos>
References: <20170117173734.14251-1-bp@alien8.de> <20170117173734.14251-12-bp@alien8.de>
User-Agent: Alpine 2.20 (DEB 67 2015-01-07)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, 17 Jan 2017, Borislav Petkov wrote:
>  void load_ucode_amd_ap(unsigned int cpuid_1_eax)
>  {
>  	struct equiv_cpu_entry *eq;
>  	struct microcode_amd *mc;
> +	struct cont_desc *desc;
>  	u16 eq_id;
>  
> +	if (IS_ENABLED(CONFIG_X86_32)) {
> +		mc   = (struct microcode_amd *)__pa_nodebug(amd_ucode_patch);
> +		desc = (struct cont_desc *)__pa_nodebug(&cont);
> +	} else {
> +		mc   = (struct microcode_amd *)amd_ucode_patch;
> +		desc = &cont;

Bah! Now I realize that 'cont' is not a local variable as I assumed when
looking at the other patch. 'cont' is a pretty bad name for a (file) global
variable. Can we please use a more obvious name ?

While at it please make that thing static as there cant be a user outside
of that file.

> +	}
> +
>  	/* First AP hasn't cached it yet, go through the blob. */
> -	if (!cont.data) {
> -		struct cpio_data cp;
> +	if (!desc->data) {
> +		struct cpio_data cp = { };
>  
> -		if (cont.size == -1)
> +		if (desc->size == -1)
>  			return;

I'm not really fond of abusing the size member for this. And that '-1'
seems to have different meanings depending on other members. Really not
intuitive.

Please introduce a proper state member which tells what the descriptor
struct contains, i.e. EMPTY, VALID, INVALID ....

Thanks,

	tglx