linux-kernel - Re: [PATCH 11/13] x86/microcode/AMD: Unify load_ucode_amd

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.20.1701172214300.3645@nanos>
Date:   Tue, 17 Jan 2017 22:18:34 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Borislav Petkov <bp@...en8.de>
cc:     X86 ML <x86@...nel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 11/13] x86/microcode/AMD: Unify load_ucode_amd_ap()

On Tue, 17 Jan 2017, Thomas Gleixner wrote:

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

Reading further through the series I see that you remove all that cruft at
the end. Dammit, I never start reading books in the final chapter, maybe I
should do that with patch series :)

It might be nice nevertheless to have an initial patch which cleans up the
name of that 'cont' variable and that status thing, but it might not be
worth the trouble as you remove it at the end anyway, which makes a lot of
sense btw. Your call.

Thanks,

	tglx