Re: [patch 1/3] x86/cpu: Remove segment load from switch_to_new_gdt()

Linus Torvalds
2022-07-18T18:43:43Z

So I appreciate the added big comments in this code, but looking at this patch:

On Mon, Jul 18, 2022 at 10:52 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> +/**
> + * switch_to_new_gdt - Switch form early GDT to the direct one
> + * @cpu: The CPU number for which this is invoked
> + *
> + * Invoked during early boot to switch from early GDT and early per CPU
> + * (%fs on 32bit, GS_BASE on 64bit) to the direct GDT and the runtime per
> + * CPU area.
> */
> void switch_to_new_gdt(int cpu)
> {
> - /* Load the original GDT */
> load_direct_gdt(cpu);
> - /* Reload the per-cpu base */
> - load_percpu_segment(cpu);
> +
> + /*
> + * No need to load the %gs (%fs for 32bit) segment. It is already
> + * correct, %gs is 0 on 64bit and %fs is __KERNEL_PERCPU on 32 bit.
> + *
> + * Writing %gs on 64bit would zero GSBASE which would make any per
> + * CPU operation up to the point of the wrmsrl() fault.
> + *
> + * 64bit requires to point GSBASE to the new offset. Until the
> + * wrmsrl() happens the early mapping is still valid. That means
> + * the GSBASE update will lose any prior per CPU data which was
> + * not copied over in setup_per_cpu_areas().
> + *
> + * For secondary CPUs this is not a problem because they start
> + * already with the direct GDT and the real GSBASE. This invocation
> + * is pointless and will be removed in a subsequent step.
> + */
> + if (IS_ENABLED(CONFIG_X86_64))
> + wrmsrl(MSR_GS_BASE, cpu_kernelmode_gs_base(cpu));
> }

... while those comments are nice and all, I do think this retains the
basic insanity of having "switch_to_new_gdt()" do magical things on
x86-64 that don't really match the name.

So honestly, I'd be happier of that whole

if (IS_ENABLED(CONFIG_X86_64))
wrmsrl(MSR_GS_BASE, cpu_kernelmode_gs_base(cpu));

was migrated to the callers instead. There aren't *that* many callers.

I expect that it is then quite possible that several of the call-sites
would go "GS_BASE is already correct here, I can remove this".

But even if every single caller keeps that wrmsrl() around, at least
it wouldn't be hidden behind a function call that has a name that
implies something completely different is happening.

And no, I don't care *that* deeply, so this is just a suggestion.

But wouldn't it be nice if this function was actually named by what it
does, rather than by what it used to do back in the i386 days when the
GDT affected the segment bases?

Linus