Re: [PATCH 2/3] x86/entry/64: Use the TSS sp2 slot for rsp_scratch

[Linus Torvalds]

On Fri, Aug 31, 2018 at 3:21 PM Andy Lutomirski <luto@xxxxxxxxxx> wrote:
>
>  #ifdef CONFIG_X86_64
>  # define cpu_current_top_of_stack (cpu_tss_rw + TSS_sp1)
> +# define rsp_scratch (cpu_tss_rw + TSS_sp2)
>  #endif

Ugh. The above gets used by *assembler* code. I was really confused by how this:


> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -59,8 +59,6 @@
>  #include <asm/unistd_32_ia32.h>
>  #endif
>
> -__visible DEFINE_PER_CPU(unsigned long, rsp_scratch);
> -

could continue to work despite the accesses to "rsp_scratch" still
remaining in the asm files.

Can yu humor me, and just not do something quite that subtle. I must
have missed this the first time around.

Please get rid of the define, and just make the asm code spell out
what it actually does.

We already do that for TSS_sp0 for the normal case:

      movq    PER_CPU_VAR(cpu_tss_rw + TSS_sp0), %rsp

so I think this should just change

-     movq    %rsp, PER_CPU_VAR(rsp_scratch)
+     movq    %rsp, PER_CPU_VAR(cpu_tss_rw + TSS_sp2)

instead of having that subtle rsp_scratch thing.

And honestly, I think we should strive to do the same thing with
cpu_current_top_of_stack. There at least the #define currently makes
sense (because on 32-bit, it's actually a percpu variable, on 64-bit
it's that sp1 field).

But wouldn't it be nice to just unify 32-bit and 64-bit in this
respect, and get rid of that subtle difference?

But regardless of whether we eventually do that kind of unification
change, the cpu_current_top_of_stack #define has a _reason_ for it as
things stand now.

The new rsp_scratch thing does not. Just spell out what you're doing.

                  Linus