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

[Borislav Petkov]

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