Re: [PATCH 11/13] x86/microcode/AMD: Unify load_ucode_amd_ap()

[Thomas Gleixner]

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