linux-kernel - Re: [patch V3 21/30] x86/microcode: Add per CPU result state

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20230927112800.GBZRQRwFUWs0bKTP7Q@fat_crate.local>
Date:   Wed, 27 Sep 2023 13:28:00 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
        "Chang S. Bae" <chang.seok.bae@...el.com>,
        Arjan van de Ven <arjan@...ux.intel.com>,
        Nikolay Borisov <nik.borisov@...e.com>
Subject: Re: [patch V3 21/30] x86/microcode: Add per CPU result state

On Tue, Sep 26, 2023 at 11:09:01AM +0200, Thomas Gleixner wrote:
> That starts to get silly. The struct is used only in the microcode realm
> and nothing which is globally visible. ucode is a pretty obvious and
> established shortcut. But so what....

Ok, which prefix do you propose?

"microcode_", "ucode_"?

And I chose "microcode_" a while back and planned on converting stuff
gradually when touching the code and not do solely a renaming patch.

All I'm saying is, we should be consistent.

> > You could do
> >
> > static DEFINE_PER_CPU(struct microcode_ctrl, ucode_ctrl);
> >
> > so that the naming is different too.
> 
> And that solves what?

I find it somewhat confusing when the variable name is called the same
name as the struct and I try to have the struct names be more expressive
than the variables of the same type.

But not a big deal.

> > No need for "ucode_" prefixes to static functions.
> 
> What's the problem with that prefix? The function name clearly says what
> this is doing.

Giving proper prefixes only to the externally visible functions is,
I think, a nice way of showing what is what. The static, internally used
symbols, OTOH, don't need a prefix and when you look at the name, you know
immediately whether it is a static symbol or an externally visible and
potentially used by other things. We do that already for other code,
like global variables, for example.

> Nope, because stop_machine_cpuslocked() does not usefully accumulate
> results from all involved CPUs. But it can return errors related to the
> invocation itself, which is a completely different story.

Ah, I see what you mean:

" * RETURNS:
 * -ENOENT if @fn(@arg) was not executed at all because all cpus in
 * @cpumask were offline; otherwise, 0 if all executions of @fn
 * returned 0, any non zero return value if any returned non zero."

So we have to return 0 here. Oh well.

> That's why ucode_ctrl.result is per CPU and has to be evaluated
> separately.

Right.

> >> +	/* Analyze the results */
> >> +	for_each_cpu_and(cpu, cpu_present_mask, &cpus_booted_once_mask) {
> >> +		switch (per_cpu(ucode_ctrl.result, cpu)) {
> >> +		case UCODE_UPDATED:	updated++; break;
> >> +		case UCODE_TIMEOUT:	timedout++; break;
> >> +		case UCODE_OK:		siblings++; break;
> >> +		default:		failed++; break;
> >> +		}
> >
> > Align vertically.
> 
> Align what?

		switch (per_cpu(ucode_ctrl.result, cpu)) {
		case UCODE_UPDATED:	updated++;	break;
		case UCODE_TIMEOUT:	timedout++;	break;
		case UCODE_OK:		siblings++;	break;
		default:		failed++;	break;

But meh, it's ok either way.

> and setup_cpus() then tells what?

See above. I think there's a merit in distinguishing the symbol scope
based on the naming only but I'm sure you have an opinion... :-)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette