Re: [tip:x86/asm] x86/asm/entry/64: Rename 'old_rsp' to 'rsp_scratch'

Re: [tip:x86/asm] x86/asm/entry/64: Rename 'old_rsp' to 'rsp_scratch'

[Andy Lutomirski]

On Tue, Mar 17, 2015 at 9:43 AM, tip-bot for Ingo Molnar
<tipbot@zytor.com> wrote:
> Commit-ID:  c38e503804b0402c510f82437069f7769fa0cea9
> Gitweb:     http://git.kernel.org/tip/c38e503804b0402c510f82437069f7769fa0cea9
> Author:     Ingo Molnar <mingo@kernel.org>
> AuthorDate: Tue, 17 Mar 2015 14:42:59 +0100
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Tue, 17 Mar 2015 16:01:42 +0100
>
> x86/asm/entry/64: Rename 'old_rsp' to 'rsp_scratch'
>
> Make clear that the usage of PER_CPU(old_rsp) is purely temporary,
> by renaming it to 'rsp_scratch'.

Thanks.  This is much better.

--Andy

>
> Cc: Alexei Starovoitov <ast@plumgrid.com>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Denys Vlasenko <dvlasenk@redhat.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Will Drewry <wad@chromium.org>
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  arch/x86/kernel/entry_64.S   | 10 +++++-----
>  arch/x86/kernel/process_64.c |  2 +-
>  arch/x86/xen/xen-asm_64.S    |  8 ++++----
>  3 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
> index d287f78..0c91256 100644
> --- a/arch/x86/kernel/entry_64.S
> +++ b/arch/x86/kernel/entry_64.S
> @@ -237,16 +237,16 @@ ENTRY(system_call)
>  GLOBAL(system_call_after_swapgs)
>
>         /*
> -        * We use 'old_rsp' as a scratch register, hence this block must execute
> +        * We use 'rsp_scratch' as a scratch register, hence this block must execute
>          * atomically in the face of possible interrupt-driven task preemption,
> -        * so we can enable interrupts only after we're done with using old_rsp:
> +        * so we can enable interrupts only after we're done with using rsp_scratch:
>          */
> -       movq    %rsp,PER_CPU_VAR(old_rsp)
> +       movq    %rsp,PER_CPU_VAR(rsp_scratch)
>         /* kernel_stack is set so that 5 slots (iret frame) are preallocated */
>         movq    PER_CPU_VAR(kernel_stack),%rsp
>         ALLOC_PT_GPREGS_ON_STACK 8              /* +8: space for orig_ax */
>         movq    %rcx,RIP(%rsp)
> -       movq    PER_CPU_VAR(old_rsp),%rcx
> +       movq    PER_CPU_VAR(rsp_scratch),%rcx
>         movq    %r11,EFLAGS(%rsp)
>         movq    %rcx,RSP(%rsp)
>         /*
> @@ -657,7 +657,7 @@ common_interrupt:
>         ASM_CLAC
>         addq $-0x80,(%rsp)              /* Adjust vector to [-256,-1] range */
>         interrupt do_IRQ
> -       /* 0(%rsp): old_rsp */
> +       /* 0(%rsp): rsp_scratch */
>  ret_from_intr:
>         DISABLE_INTERRUPTS(CLBR_NONE)
>         TRACE_IRQS_OFF
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 14df2be..97f5658 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -52,7 +52,7 @@
>
>  asmlinkage extern void ret_from_fork(void);
>
> -__visible DEFINE_PER_CPU(unsigned long, old_rsp);
> +__visible DEFINE_PER_CPU(unsigned long, rsp_scratch);
>
>  /* Prints also some state that isn't saved in the pt_regs */
>  void __show_regs(struct pt_regs *regs, int all)
> diff --git a/arch/x86/xen/xen-asm_64.S b/arch/x86/xen/xen-asm_64.S
> index 53adefd..985fc3e 100644
> --- a/arch/x86/xen/xen-asm_64.S
> +++ b/arch/x86/xen/xen-asm_64.S
> @@ -68,11 +68,11 @@ ENTRY(xen_sysret64)
>          * We're already on the usermode stack at this point, but
>          * still with the kernel gs, so we can easily switch back
>          */
> -       movq %rsp, PER_CPU_VAR(old_rsp)
> +       movq %rsp, PER_CPU_VAR(rsp_scratch)
>         movq PER_CPU_VAR(kernel_stack), %rsp
>
>         pushq $__USER_DS
> -       pushq PER_CPU_VAR(old_rsp)
> +       pushq PER_CPU_VAR(rsp_scratch)
>         pushq %r11
>         pushq $__USER_CS
>         pushq %rcx
> @@ -87,11 +87,11 @@ ENTRY(xen_sysret32)
>          * We're already on the usermode stack at this point, but
>          * still with the kernel gs, so we can easily switch back
>          */
> -       movq %rsp, PER_CPU_VAR(old_rsp)
> +       movq %rsp, PER_CPU_VAR(rsp_scratch)
>         movq PER_CPU_VAR(kernel_stack), %rsp
>
>         pushq $__USER32_DS
> -       pushq PER_CPU_VAR(old_rsp)
> +       pushq PER_CPU_VAR(rsp_scratch)
>         pushq %r11
>         pushq $__USER32_CS
>         pushq %rcx



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [tip:x86/asm] x86/asm/entry/64: Rename 'old_rsp' to 'rsp_scratch'

[Denys Vlasenko]

On 03/17/2015 05:43 PM, tip-bot for Ingo Molnar wrote:
> Commit-ID:  c38e503804b0402c510f82437069f7769fa0cea9
> Gitweb:     http://git.kernel.org/tip/c38e503804b0402c510f82437069f7769fa0cea9
> Author:     Ingo Molnar <mingo@kernel.org>
> AuthorDate: Tue, 17 Mar 2015 14:42:59 +0100
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Tue, 17 Mar 2015 16:01:42 +0100
> 
> x86/asm/entry/64: Rename 'old_rsp' to 'rsp_scratch'
> 
> Make clear that the usage of PER_CPU(old_rsp) is purely temporary,
> by renaming it to 'rsp_scratch'.
...
> @@ -657,7 +657,7 @@ common_interrupt:
>  	ASM_CLAC
>  	addq $-0x80,(%rsp)		/* Adjust vector to [-256,-1] range */
>  	interrupt do_IRQ
> -	/* 0(%rsp): old_rsp */
> +	/* 0(%rsp): rsp_scratch */
>  ret_from_intr:

Here the "old_rsp" word in comment did not refer to old_rsp variable.

Rather, it literally means "old %rsp register value (before we possibly
switched to per-CPU interrupt stack)".

The stack switch is hidden in "interrupt do_IRQ" macro invocation.

I think here "old_rsp" needs to be replaced by "old rsp" or "old %rsp".

Re: [tip:x86/asm] x86/asm/entry/64: Rename 'old_rsp' to 'rsp_scratch'

[Ingo Molnar]

* Denys Vlasenko <dvlasenk@redhat.com> wrote:

> On 03/17/2015 05:43 PM, tip-bot for Ingo Molnar wrote:
> > Commit-ID:  c38e503804b0402c510f82437069f7769fa0cea9
> > Gitweb:     http://git.kernel.org/tip/c38e503804b0402c510f82437069f7769fa0cea9
> > Author:     Ingo Molnar <mingo@kernel.org>
> > AuthorDate: Tue, 17 Mar 2015 14:42:59 +0100
> > Committer:  Ingo Molnar <mingo@kernel.org>
> > CommitDate: Tue, 17 Mar 2015 16:01:42 +0100
> > 
> > x86/asm/entry/64: Rename 'old_rsp' to 'rsp_scratch'
> > 
> > Make clear that the usage of PER_CPU(old_rsp) is purely temporary,
> > by renaming it to 'rsp_scratch'.
> ...
> > @@ -657,7 +657,7 @@ common_interrupt:
> >  	ASM_CLAC
> >  	addq $-0x80,(%rsp)		/* Adjust vector to [-256,-1] range */
> >  	interrupt do_IRQ
> > -	/* 0(%rsp): old_rsp */
> > +	/* 0(%rsp): rsp_scratch */
> >  ret_from_intr:
> 
> Here the "old_rsp" word in comment did not refer to old_rsp variable.
> 
> Rather, it literally means "old %rsp register value (before we possibly
> switched to per-CPU interrupt stack)".
> 
> The stack switch is hidden in "interrupt do_IRQ" macro invocation.
> 
> I think here "old_rsp" needs to be replaced by "old rsp" or "old %rsp".

Indeed - and I applied your comment-fix patch.

Thanks,

	Ingo