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

[Andy Lutomirski]

On Sat, Sep 1, 2018 at 9:33 AM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> 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.

Done for v2.

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

Yes.  But ugh, the way that thing has worked has changed so many times
on 32-bit and 64-bit that I've lost track a little bit.  I'll put it
on my long list of things to clean up.