[PATCH 0/4] x86/nmi: NMI cleanups

[PATCH 0/4] x86/nmi: NMI cleanups

[Lai Jiangshan]

From: Lai Jiangshan <laijs@linux.alibaba.com>

These are very simple cleanups for NMI.  They make NMI code a bit
tidier.

Patch1-3 are picked from the patchset
https://lore.kernel.org/lkml/20211126101209.8613-1-jiangshanlai@gmail.com/
which coverts ASM code to C code.

Patch4 is new.

Lai Jiangshan (4):
  x86/entry: Make paranoid_exit() callable
  x86/entry: Call paranoid_exit() in asm_exc_nmi()
  x86/nmi: Use DEFINE_IDTENTRY_NMI for NMI handler
  x86/nmi: Convert nmi_cr2/nmi_dr7 to be func-local variable

 arch/x86/entry/entry_64.S | 52 ++++++++++++---------------------------
 arch/x86/kernel/nmi.c     | 16 ++++++------
 2 files changed, 24 insertions(+), 44 deletions(-)

-- 
2.19.1.6.gb485710b

[PATCH 1/4] x86/entry: Make paranoid_exit() callable

[Lai Jiangshan]

From: Lai Jiangshan <laijs@linux.alibaba.com>

Move the last JMP out of paranoid_exit() and make it callable.

It will allow asm_exc_nmi() to call it and avoid duplicated code.

No functional change intended.

Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 arch/x86/entry/entry_64.S | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 44dadea935f7..3dc3cec03425 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -439,7 +439,8 @@ SYM_CODE_START(\asmsym)
 
 	call	\cfunc
 
-	jmp	paranoid_exit
+	call	paranoid_exit
+	jmp	restore_regs_and_return_to_kernel
 
 	/* Switch to the regular task stack and use the noist entry point */
 .Lfrom_usermode_switch_stack_\@:
@@ -516,7 +517,8 @@ SYM_CODE_START(\asmsym)
 	 * identical to the stack in the IRET frame or the VC fall-back stack,
 	 * so it is definitely mapped even with PTI enabled.
 	 */
-	jmp	paranoid_exit
+	call	paranoid_exit
+	jmp	restore_regs_and_return_to_kernel
 
 	/* Switch to the regular task stack */
 .Lfrom_usermode_switch_stack_\@:
@@ -546,7 +548,8 @@ SYM_CODE_START(\asmsym)
 	movq	$-1, ORIG_RAX(%rsp)	/* no syscall to restart */
 	call	\cfunc
 
-	jmp	paranoid_exit
+	call	paranoid_exit
+	jmp	restore_regs_and_return_to_kernel
 
 _ASM_NOKPROBE(\asmsym)
 SYM_CODE_END(\asmsym)
@@ -935,7 +938,7 @@ SYM_CODE_END(paranoid_entry)
  *     Y        User space GSBASE, must be restored unconditionally
  */
 SYM_CODE_START_LOCAL(paranoid_exit)
-	UNWIND_HINT_REGS
+	UNWIND_HINT_REGS offset=8
 	/*
 	 * The order of operations is important. RESTORE_CR3 requires
 	 * kernel GSBASE.
@@ -951,16 +954,17 @@ SYM_CODE_START_LOCAL(paranoid_exit)
 
 	/* With FSGSBASE enabled, unconditionally restore GSBASE */
 	wrgsbase	%rbx
-	jmp		restore_regs_and_return_to_kernel
+	ret
 
 .Lparanoid_exit_checkgs:
 	/* On non-FSGSBASE systems, conditionally do SWAPGS */
 	testl		%ebx, %ebx
-	jnz		restore_regs_and_return_to_kernel
+	jnz		.Lparanoid_exit_done
 
 	/* We are returning to a context with user GSBASE */
 	swapgs
-	jmp		restore_regs_and_return_to_kernel
+.Lparanoid_exit_done:
+	ret
 SYM_CODE_END(paranoid_exit)
 
 /*
-- 
2.19.1.6.gb485710b

[PATCH 2/4] x86/entry: Call paranoid_exit() in asm_exc_nmi()

[Lai Jiangshan]

From: Lai Jiangshan <laijs@linux.alibaba.com>

The code between "call exc_nmi" and nmi_restore is as the same as
paranoid_exit(), so we can just use paranoid_exit() instead of the open
duplicated code.

No functional change intended.

Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 arch/x86/entry/entry_64.S | 34 +++++-----------------------------
 1 file changed, 5 insertions(+), 29 deletions(-)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 3dc3cec03425..47ff4abd87b5 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -920,8 +920,7 @@ SYM_CODE_END(paranoid_entry)
 
 /*
  * "Paranoid" exit path from exception stack.  This is invoked
- * only on return from non-NMI IST interrupts that came
- * from kernel space.
+ * only on return from IST interrupts that came from kernel space.
  *
  * We may be returning to very strange contexts (e.g. very early
  * in syscall entry), so checking for preemption here would
@@ -1354,11 +1353,7 @@ end_repeat_nmi:
 	pushq	$-1				/* ORIG_RAX: no syscall to restart */
 
 	/*
-	 * Use paranoid_entry to handle SWAPGS, but no need to use paranoid_exit
-	 * as we should not be calling schedule in NMI context.
-	 * Even with normal interrupts enabled. An NMI should not be
-	 * setting NEED_RESCHED or anything that normal interrupts and
-	 * exceptions might do.
+	 * Use paranoid_entry to handle SWAPGS and CR3.
 	 */
 	call	paranoid_entry
 	UNWIND_HINT_REGS
@@ -1367,31 +1362,12 @@ end_repeat_nmi:
 	movq	$-1, %rsi
 	call	exc_nmi
 
-	/* Always restore stashed CR3 value (see paranoid_entry) */
-	RESTORE_CR3 scratch_reg=%r15 save_reg=%r14
-
 	/*
-	 * The above invocation of paranoid_entry stored the GSBASE
-	 * related information in R/EBX depending on the availability
-	 * of FSGSBASE.
-	 *
-	 * If FSGSBASE is enabled, restore the saved GSBASE value
-	 * unconditionally, otherwise take the conditional SWAPGS path.
+	 * Use paranoid_exit to handle SWAPGS and CR3, but no need to use
+	 * restore_regs_and_return_to_kernel as we must handle nested NMI.
 	 */
-	ALTERNATIVE "jmp nmi_no_fsgsbase", "", X86_FEATURE_FSGSBASE
-
-	wrgsbase	%rbx
-	jmp	nmi_restore
-
-nmi_no_fsgsbase:
-	/* EBX == 0 -> invoke SWAPGS */
-	testl	%ebx, %ebx
-	jnz	nmi_restore
-
-nmi_swapgs:
-	swapgs
+	call	paranoid_exit
 
-nmi_restore:
 	POP_REGS
 
 	/*
-- 
2.19.1.6.gb485710b

[PATCH 3/4] x86/nmi: Use DEFINE_IDTENTRY_NMI for NMI handler

[Lai Jiangshan]

From: Lai Jiangshan <laijs@linux.alibaba.com>

DEFINE_IDTENTRY_NMI is introduced for nmi handler.  It is better to use it
which makes it clear that NMI is special handled.

Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 arch/x86/kernel/nmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 4bce802d25fb..44c3adb68282 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -473,7 +473,7 @@ static DEFINE_PER_CPU(enum nmi_states, nmi_state);
 static DEFINE_PER_CPU(unsigned long, nmi_cr2);
 static DEFINE_PER_CPU(unsigned long, nmi_dr7);
 
-DEFINE_IDTENTRY_RAW(exc_nmi)
+DEFINE_IDTENTRY_NMI(exc_nmi)
 {
 	irqentry_state_t irq_state;
 
-- 
2.19.1.6.gb485710b

[PATCH 4/4] x86/nmi: Convert nmi_cr2/nmi_dr7 to be func-local variable

[Lai Jiangshan]

From: Lai Jiangshan <laijs@linux.alibaba.com>

nmi_cr2 and nmi_dr7 are used inside a function (the outmost exc_nmi()),
they don't need to be percpu data.

Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
---
 arch/x86/kernel/nmi.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index 44c3adb68282..673eee5831db 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -470,12 +470,12 @@ enum nmi_states {
 	NMI_LATCHED,
 };
 static DEFINE_PER_CPU(enum nmi_states, nmi_state);
-static DEFINE_PER_CPU(unsigned long, nmi_cr2);
-static DEFINE_PER_CPU(unsigned long, nmi_dr7);
 
 DEFINE_IDTENTRY_NMI(exc_nmi)
 {
 	irqentry_state_t irq_state;
+	unsigned long nmi_cr2;
+	unsigned long nmi_dr7;
 
 	/*
 	 * Re-enable NMIs right here when running as an SEV-ES guest. This might
@@ -491,7 +491,7 @@ DEFINE_IDTENTRY_NMI(exc_nmi)
 		return;
 	}
 	this_cpu_write(nmi_state, NMI_EXECUTING);
-	this_cpu_write(nmi_cr2, read_cr2());
+	nmi_cr2 = read_cr2();
 nmi_restart:
 
 	/*
@@ -500,7 +500,7 @@ DEFINE_IDTENTRY_NMI(exc_nmi)
 	 */
 	sev_es_ist_enter(regs);
 
-	this_cpu_write(nmi_dr7, local_db_save());
+	nmi_dr7 = local_db_save();
 
 	irq_state = irqentry_nmi_enter(regs);
 
@@ -511,12 +511,12 @@ DEFINE_IDTENTRY_NMI(exc_nmi)
 
 	irqentry_nmi_exit(regs, irq_state);
 
-	local_db_restore(this_cpu_read(nmi_dr7));
+	local_db_restore(nmi_dr7);
 
 	sev_es_ist_exit();
 
-	if (unlikely(this_cpu_read(nmi_cr2) != read_cr2()))
-		write_cr2(this_cpu_read(nmi_cr2));
+	if (unlikely(nmi_cr2 != read_cr2()))
+		write_cr2(nmi_cr2);
 	if (this_cpu_dec_return(nmi_state))
 		goto nmi_restart;
 
-- 
2.19.1.6.gb485710b

Re: [PATCH 1/4] x86/entry: Make paranoid_exit() callable

[Borislav Petkov]

On Mon, Dec 13, 2021 at 11:03:37PM +0800, Lai Jiangshan wrote:
> From: Lai Jiangshan <laijs@linux.alibaba.com>
> 
> Move the last JMP out of paranoid_exit() and make it callable.
> 
> It will allow asm_exc_nmi() to call it and avoid duplicated code.
> 
> No functional change intended.
> 
> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> ---
>  arch/x86/entry/entry_64.S | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index 44dadea935f7..3dc3cec03425 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -439,7 +439,8 @@ SYM_CODE_START(\asmsym)
>  
>  	call	\cfunc
>  
> -	jmp	paranoid_exit
> +	call	paranoid_exit
> +	jmp	restore_regs_and_return_to_kernel

I guess but I don't like the glueing of the CALL to paranoid_exit with
the JMP to the restore_regs_and_return_to_kernel label. That reads
to me as, "if you're calling paranoid_exit() you must jump to the
restore_regs_and_return_to_kernel label but not always."

So I'm thinking you should leave the jump to that label inside
paranoid_exit() and use its %rbx argument to control when to jump to it
and when not.

I.e., not jump to it in the NMI case.

AFAICT, ofc. asm is always nasty to stare at.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/4] x86/entry: Make paranoid_exit() callable

[Lai Jiangshan]

On 2021/12/21 02:56, Borislav Petkov wrote:
> On Mon, Dec 13, 2021 at 11:03:37PM +0800, Lai Jiangshan wrote:
>> From: Lai Jiangshan <laijs@linux.alibaba.com>
>>
>> Move the last JMP out of paranoid_exit() and make it callable.
>>
>> It will allow asm_exc_nmi() to call it and avoid duplicated code.
>>
>> No functional change intended.
>>
>> Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
>> ---
>>   arch/x86/entry/entry_64.S | 18 +++++++++++-------
>>   1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
>> index 44dadea935f7..3dc3cec03425 100644
>> --- a/arch/x86/entry/entry_64.S
>> +++ b/arch/x86/entry/entry_64.S
>> @@ -439,7 +439,8 @@ SYM_CODE_START(\asmsym)
>>   
>>   	call	\cfunc
>>   
>> -	jmp	paranoid_exit
>> +	call	paranoid_exit
>> +	jmp	restore_regs_and_return_to_kernel
> 
> I guess but I don't like the glueing of the CALL to paranoid_exit with
> the JMP to the restore_regs_and_return_to_kernel label. That reads
> to me as, "if you're calling paranoid_exit() you must jump to the
> restore_regs_and_return_to_kernel label but not always."
> 
> So I'm thinking you should leave the jump to that label inside
> paranoid_exit() and use its %rbx argument to control when to jump to it
> and when not.

Hello

Thank you for reviewing it.

This patch is a part of the job converting some ASM code to C code.  The
changelog of it in the original big patchset reads:

   Allow paranoid_exit() to be re-written in C later and also allow
   asm_exc_nmi() to call it to avoid duplicated code.

And this smaller patchset doesn't include the work of converting ASM code,
so I removed "Allow paranoid_exit() to be re-written in C later" because I
thought "allowing asm_exc_nmi() to call it and avoiding duplicated code" is
enough to justify the value of this change.

When paranoid_exit() is ready to be converted to C, it can't have jump to
any label that is not in paranoid_exit() itself.

I'm sorry to not have put all of the intents in the changelog:

-It will allow asm_exc_nmi() to call it and avoid duplicated code.
+It will allow asm_exc_nmi() to call it and avoid duplicated code and allow
+future work to convert paranoid_exit() to C code.

Thanks
Lai

Re: [PATCH 1/4] x86/entry: Make paranoid_exit() callable

[Borislav Petkov]

On Tue, Dec 21, 2021 at 10:22:48AM +0800, Lai Jiangshan wrote:
> When paranoid_exit() is ready to be converted to C, it can't have jump to
> any label that is not in paranoid_exit() itself.

Then splitting out those 4 patches from the rest of the series was not
the right thing to do. Because how is a reviewer to know what your final
goal is without seeing it?

When I told you at the time that you could split the big patchset out, I
said:

"It might be even helpful if you could split it into more palatable
portions of maybe 10-ish patches each, if possible, and then send the
first portion, wait for review and only send the second portion after
the first has been applied, etc."

Maybe I should have explained what "if possible" means: if a subset can
exist on its own and is logically separate, then it should be split.
But, if, as in this case, it looks like introducing arbitrary changes
then I wouldn't do that.

IMNSVHO, ofc.

HTH.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette