public inbox for kvm@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
@ 2023-08-02  9:11 Nikunj A Dadhania
  2023-08-02 14:02 ` Sean Christopherson
                   ` (3 more replies)
  0 siblings, 4 replies; 17+ messages in thread
From: Nikunj A Dadhania @ 2023-08-02  9:11 UTC (permalink / raw)
  To: kvm, Sean Christopherson
  Cc: Nikunj A Dadhania, Josh Poimboeuf, Peter Zijlstra, Paolo Bonzini,
	Randy Dunlap, Tom Lendacky, Ravi Bangoria

commit 7f4b5cde2409 ("kvm: Disable objtool frame pointer checking for
vmenter.S") had added the vmenter.o file to the exception list.

objtool gives the following warnings in the newer kernel builds:

  arch/x86/kvm/kvm-amd.o: warning: objtool: __svm_vcpu_run+0x17d: BP used as a scratch register
  arch/x86/kvm/kvm-amd.o: warning: objtool: __svm_sev_es_vcpu_run+0x72: BP used as a scratch register

As kvm-amd.o is a link time object, skipping the kvm-amd.o is not possible
as per the objtool documentation, better to skip the offending functions.

Functions __svm_vcpu_run() and __svm_sev_es_vcpu_run() saves and restores
RBP. Below is the snippet:

    SYM_FUNC_START(__svm_vcpu_run)
        push %_ASM_BP
    <…>
        pop %_ASM_BP
        RET

Add exceptions to skip both these functions. Remove the
OBJECT_FILES_NON_STANDARD for vmenter.o

Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Sean Christopherson <seanjc@google.com>
Reported-by: Ravi Bangoria <ravi.bangoria@amd.com>
Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
---
 arch/x86/kvm/Makefile      | 4 ----
 arch/x86/kvm/svm/vmenter.S | 2 ++
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
index 80e3fe184d17..0c5c2f090e93 100644
--- a/arch/x86/kvm/Makefile
+++ b/arch/x86/kvm/Makefile
@@ -3,10 +3,6 @@
 ccflags-y += -I $(srctree)/arch/x86/kvm
 ccflags-$(CONFIG_KVM_WERROR) += -Werror
 
-ifeq ($(CONFIG_FRAME_POINTER),y)
-OBJECT_FILES_NON_STANDARD_vmenter.o := y
-endif
-
 include $(srctree)/virt/kvm/Makefile.kvm
 
 kvm-y			+= x86.o emulate.o i8259.o irq.o lapic.o \
diff --git a/arch/x86/kvm/svm/vmenter.S b/arch/x86/kvm/svm/vmenter.S
index 8e8295e774f0..8fd37d661c33 100644
--- a/arch/x86/kvm/svm/vmenter.S
+++ b/arch/x86/kvm/svm/vmenter.S
@@ -289,6 +289,7 @@ SYM_FUNC_START(__svm_vcpu_run)
 	_ASM_EXTABLE(7b, 70b)
 
 SYM_FUNC_END(__svm_vcpu_run)
+STACK_FRAME_NON_STANDARD(__svm_vcpu_run)
 
 /**
  * __svm_sev_es_vcpu_run - Run a SEV-ES vCPU via a transition to SVM guest mode
@@ -388,3 +389,4 @@ SYM_FUNC_START(__svm_sev_es_vcpu_run)
 	_ASM_EXTABLE(1b, 3b)
 
 SYM_FUNC_END(__svm_sev_es_vcpu_run)
+STACK_FRAME_NON_STANDARD_FP(__svm_sev_es_vcpu_run)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
  2023-08-02  9:11 [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o Nikunj A Dadhania
@ 2023-08-02 14:02 ` Sean Christopherson
  2023-08-03  6:25   ` Nikunj A. Dadhania
  2023-08-03 12:06 ` Peter Zijlstra
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 17+ messages in thread
From: Sean Christopherson @ 2023-08-02 14:02 UTC (permalink / raw)
  To: Nikunj A Dadhania
  Cc: kvm, Josh Poimboeuf, Peter Zijlstra, Paolo Bonzini, Randy Dunlap,
	Tom Lendacky, Ravi Bangoria

On Wed, Aug 02, 2023, Nikunj A Dadhania wrote:
> objtool gives the following warnings in the newer kernel builds:

Define "newer".  As in, exactly what commit is causing problems?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
  2023-08-02 14:02 ` Sean Christopherson
@ 2023-08-03  6:25   ` Nikunj A. Dadhania
  0 siblings, 0 replies; 17+ messages in thread
From: Nikunj A. Dadhania @ 2023-08-03  6:25 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: kvm, Josh Poimboeuf, Peter Zijlstra, Paolo Bonzini, Randy Dunlap,
	Tom Lendacky, Ravi Bangoria, Arnd Bergmann

On 8/2/2023 7:32 PM, Sean Christopherson wrote:
> On Wed, Aug 02, 2023, Nikunj A Dadhania wrote:
>> objtool gives the following warnings in the newer kernel builds:
> 
> Define "newer".  As in, exactly what commit is causing problems?

Not that new it seems, I have tried till v5.19 and the warning is there but 
with a different signature. Do you want me to further bisect it ?

arch/x86/kvm/kvm-amd.o: warning: objtool: .altinstr_replacement+0x4d: call without frame pointer save/setup
arch/x86/kvm/kvm-amd.o: warning: objtool: .altinstr_replacement+0x57: call without frame pointer save/setup

kvm-amd.o warnings were also reported in the below thread:
https://lore.kernel.org/lkml/9698eff1-9680-4f0a-94de-590eaa923e94@app.fastmail.com/

Regards,
Nikunj


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
  2023-08-02  9:11 [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o Nikunj A Dadhania
  2023-08-02 14:02 ` Sean Christopherson
@ 2023-08-03 12:06 ` Peter Zijlstra
  2023-08-03 18:06   ` Paolo Bonzini
  2023-08-04 11:14 ` Peter Zijlstra
  2023-08-12  0:51 ` kernel test robot
  3 siblings, 1 reply; 17+ messages in thread
From: Peter Zijlstra @ 2023-08-03 12:06 UTC (permalink / raw)
  To: Nikunj A Dadhania
  Cc: kvm, Sean Christopherson, Josh Poimboeuf, Paolo Bonzini,
	Randy Dunlap, Tom Lendacky, Ravi Bangoria

On Wed, Aug 02, 2023 at 02:41:07PM +0530, Nikunj A Dadhania wrote:
> commit 7f4b5cde2409 ("kvm: Disable objtool frame pointer checking for
> vmenter.S") had added the vmenter.o file to the exception list.
> 
> objtool gives the following warnings in the newer kernel builds:
> 
>   arch/x86/kvm/kvm-amd.o: warning: objtool: __svm_vcpu_run+0x17d: BP used as a scratch register
>   arch/x86/kvm/kvm-amd.o: warning: objtool: __svm_sev_es_vcpu_run+0x72: BP used as a scratch register
> 
> As kvm-amd.o is a link time object, skipping the kvm-amd.o is not possible
> as per the objtool documentation, better to skip the offending functions.
> 
> Functions __svm_vcpu_run() and __svm_sev_es_vcpu_run() saves and restores
> RBP. Below is the snippet:
> 
>     SYM_FUNC_START(__svm_vcpu_run)
>         push %_ASM_BP
>     <…>
>         pop %_ASM_BP
>         RET
> 
> Add exceptions to skip both these functions. Remove the
> OBJECT_FILES_NON_STANDARD for vmenter.o
> 
> Cc: Josh Poimboeuf <jpoimboe@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Sean Christopherson <seanjc@google.com>
> Reported-by: Ravi Bangoria <ravi.bangoria@amd.com>
> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
> ---
>  arch/x86/kvm/Makefile      | 4 ----
>  arch/x86/kvm/svm/vmenter.S | 2 ++
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
> index 80e3fe184d17..0c5c2f090e93 100644
> --- a/arch/x86/kvm/Makefile
> +++ b/arch/x86/kvm/Makefile
> @@ -3,10 +3,6 @@
>  ccflags-y += -I $(srctree)/arch/x86/kvm
>  ccflags-$(CONFIG_KVM_WERROR) += -Werror
>  
> -ifeq ($(CONFIG_FRAME_POINTER),y)
> -OBJECT_FILES_NON_STANDARD_vmenter.o := y
> -endif
> -
>  include $(srctree)/virt/kvm/Makefile.kvm
>  
>  kvm-y			+= x86.o emulate.o i8259.o irq.o lapic.o \
> diff --git a/arch/x86/kvm/svm/vmenter.S b/arch/x86/kvm/svm/vmenter.S
> index 8e8295e774f0..8fd37d661c33 100644
> --- a/arch/x86/kvm/svm/vmenter.S
> +++ b/arch/x86/kvm/svm/vmenter.S
> @@ -289,6 +289,7 @@ SYM_FUNC_START(__svm_vcpu_run)
>  	_ASM_EXTABLE(7b, 70b)
>  
>  SYM_FUNC_END(__svm_vcpu_run)
> +STACK_FRAME_NON_STANDARD(__svm_vcpu_run)
>  
>  /**
>   * __svm_sev_es_vcpu_run - Run a SEV-ES vCPU via a transition to SVM guest mode
> @@ -388,3 +389,4 @@ SYM_FUNC_START(__svm_sev_es_vcpu_run)
>  	_ASM_EXTABLE(1b, 3b)
>  
>  SYM_FUNC_END(__svm_sev_es_vcpu_run)
> +STACK_FRAME_NON_STANDARD_FP(__svm_sev_es_vcpu_run)

Urgh... no, no, this is all broken.

By marking them with STACK_FRAME_NON_STANDARD you will get no ORC data
at all, and then you also violate the normal framepointer calling
convention.

This means that if you need to unwind here you're up a creek without no
paddles on.

Objtool complains for a reason, your changelog does not provide a
counter argument for that reason.

Hardware/firmware interfaces that require one to violate basic
calling conventions are horrible crap.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
  2023-08-03 12:06 ` Peter Zijlstra
@ 2023-08-03 18:06   ` Paolo Bonzini
  2023-08-03 19:07     ` Peter Zijlstra
  0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2023-08-03 18:06 UTC (permalink / raw)
  To: Peter Zijlstra, Nikunj A Dadhania
  Cc: kvm, Sean Christopherson, Josh Poimboeuf, Randy Dunlap,
	Tom Lendacky, Ravi Bangoria

On 8/3/23 14:06, Peter Zijlstra wrote:
> 
> By marking them with STACK_FRAME_NON_STANDARD you will get no ORC data
> at all, and then you also violate the normal framepointer calling
> convention.
> 
> This means that if you need to unwind here you're up a creek without no
> paddles on.

The only weird thing that can happen is ud2 instructions that are 
executed in case the vmload/vmrun/vmsave instructions causes a #GP, from 
the exception handler.

If I understand correctly those ud2 would use ORC information to show 
the backtrace, but even then the frame pointer should be correct.  Of 
these instructions, vmrun is the only one that runs with wrong %rbp; and 
it is unlikely or even impossible that a #GP happens at vmrun, because 
the same operand has been used for a vmload ten instructions before. 
The only time I saw that #GP it was due to a processor errata, but it 
happened consistently on the vmload.

So if frame pointer unwinding can be used in the absence of ORC, Nikunj 
patch should not break anything.

Paolo


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
  2023-08-03 18:06   ` Paolo Bonzini
@ 2023-08-03 19:07     ` Peter Zijlstra
  2023-08-04  3:25       ` Nikunj A. Dadhania
  2023-08-04 10:20       ` Paolo Bonzini
  0 siblings, 2 replies; 17+ messages in thread
From: Peter Zijlstra @ 2023-08-03 19:07 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Nikunj A Dadhania, kvm, Sean Christopherson, Josh Poimboeuf,
	Randy Dunlap, Tom Lendacky, Ravi Bangoria

On Thu, Aug 03, 2023 at 08:06:20PM +0200, Paolo Bonzini wrote:
> On 8/3/23 14:06, Peter Zijlstra wrote:
> > 
> > By marking them with STACK_FRAME_NON_STANDARD you will get no ORC data
> > at all, and then you also violate the normal framepointer calling
> > convention.
> > 
> > This means that if you need to unwind here you're up a creek without no
> > paddles on.
> 
> The only weird thing that can happen is ud2 instructions that are executed
> in case the vmload/vmrun/vmsave instructions causes a #GP, from the
> exception handler.

This code is ran with GIF disabled, so NMIs are not in the books, right?
Does GIF block #MC ?

> If I understand correctly those ud2 would use ORC information to show the
> backtrace, but even then the frame pointer should be correct.  Of these
> instructions, vmrun is the only one that runs with wrong %rbp; and it is
> unlikely or even impossible that a #GP happens at vmrun, because the same
> operand has been used for a vmload ten instructions before. The only time I
> saw that #GP it was due to a processor errata, but it happened consistently
> on the vmload.
> 
> So if frame pointer unwinding can be used in the absence of ORC, Nikunj
> patch should not break anything.

But framepointer unwinds rely on BP, and that is clobbered per the
objtool complaint.

Also, if you look at the makefile hunk that's being replaced, that was
conditional on CONFIG_FRAMEPOINTS, while the annotation that's being
added is not. I think objtool won't complain for ORC builds, only for
framepoints builds.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
  2023-08-03 19:07     ` Peter Zijlstra
@ 2023-08-04  3:25       ` Nikunj A. Dadhania
  2023-08-04 10:20       ` Paolo Bonzini
  1 sibling, 0 replies; 17+ messages in thread
From: Nikunj A. Dadhania @ 2023-08-04  3:25 UTC (permalink / raw)
  To: Peter Zijlstra, Paolo Bonzini
  Cc: kvm, Sean Christopherson, Josh Poimboeuf, Randy Dunlap,
	Tom Lendacky, Ravi Bangoria

On 8/4/2023 12:37 AM, Peter Zijlstra wrote:
> On Thu, Aug 03, 2023 at 08:06:20PM +0200, Paolo Bonzini wrote:
>> On 8/3/23 14:06, Peter Zijlstra wrote:
>>>
>>> By marking them with STACK_FRAME_NON_STANDARD you will get no ORC data
>>> at all, and then you also violate the normal framepointer calling
>>> convention.
>>>
>>> This means that if you need to unwind here you're up a creek without no
>>> paddles on.
>>
>> The only weird thing that can happen is ud2 instructions that are executed
>> in case the vmload/vmrun/vmsave instructions causes a #GP, from the
>> exception handler.
> 
> This code is ran with GIF disabled, so NMIs are not in the books, right?
> Does GIF block #MC ?
> 
>> If I understand correctly those ud2 would use ORC information to show the
>> backtrace, but even then the frame pointer should be correct.  Of these
>> instructions, vmrun is the only one that runs with wrong %rbp; and it is
>> unlikely or even impossible that a #GP happens at vmrun, because the same
>> operand has been used for a vmload ten instructions before. The only time I
>> saw that #GP it was due to a processor errata, but it happened consistently
>> on the vmload.
>>
>> So if frame pointer unwinding can be used in the absence of ORC, Nikunj
>> patch should not break anything.
> 
> But framepointer unwinds rely on BP, and that is clobbered per the
> objtool complaint.
> 
> Also, if you look at the makefile hunk that's being replaced, that was
> conditional on CONFIG_FRAMEPOINTS, while the annotation that's being
> added is not. 

Even with the hunk present with CONFIG_FRAME_POINTER=y, I was getting 
the warning. That is why I had added the STACK_FRAME_NON_STANDARD and
removed the hunk in makefile.

Do you recommend using the below instead ?

#ifdef CONFIG_FRAME_POINTER
STACK_FRAME_NON_STANDARD()
#endif

Regards
Nikunj


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
  2023-08-03 19:07     ` Peter Zijlstra
  2023-08-04  3:25       ` Nikunj A. Dadhania
@ 2023-08-04 10:20       ` Paolo Bonzini
  2023-08-04 20:48         ` Peter Zijlstra
  1 sibling, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2023-08-04 10:20 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Nikunj A Dadhania, kvm, Sean Christopherson, Josh Poimboeuf,
	Randy Dunlap, Tom Lendacky, Ravi Bangoria

On 8/3/23 21:07, Peter Zijlstra wrote:
>> The only weird thing that can happen is ud2 instructions that are executed
>> in case the vmload/vmrun/vmsave instructions causes a #GP, from the
>> exception handler.
> 
> This code is ran with GIF disabled, so NMIs are not in the books, right?

Yep.

> Does GIF block #MC ?

No, #MC is an exception not an interrupt.

>> So if frame pointer unwinding can be used in the absence of ORC, Nikunj
>> patch should not break anything.
>
> But framepointer unwinds rely on BP, and that is clobbered per the
> objtool complaint.

It's not clobbered in a part that will cause unwinding; we can further
restrict the part to a handful of instructions (and add a mov %rsp, %rbp
at the top, see untested patch after signature).

I think the chance of this failure is similar or lower to the chance of
a memory failure that hits the exception handler code itself.

Paolo

diff --git a/arch/x86/kvm/svm/vmenter.S b/arch/x86/kvm/svm/vmenter.S
index 8e8295e774f0..58fab5e0f7ae 100644
--- a/arch/x86/kvm/svm/vmenter.S
+++ b/arch/x86/kvm/svm/vmenter.S
@@ -99,6 +99,8 @@
   */
  SYM_FUNC_START(__svm_vcpu_run)
  	push %_ASM_BP
+	mov %_ASM_SP, %_ASM_BP
+
  #ifdef CONFIG_X86_64
  	push %r15
  	push %r14
@@ -121,7 +123,8 @@ SYM_FUNC_START(__svm_vcpu_run)
  	/* Needed to restore access to percpu variables.  */
  	__ASM_SIZE(push) PER_CPU_VAR(svm_data + SD_save_area_pa)
  
-	/* Finally save @svm. */
+	/* Finally save frame pointer and @svm. */
+	push %_ASM_BP
  	push %_ASM_ARG1
  
  .ifnc _ASM_ARG1, _ASM_DI
@@ -153,7 +156,6 @@ SYM_FUNC_START(__svm_vcpu_run)
  	mov VCPU_RCX(%_ASM_DI), %_ASM_CX
  	mov VCPU_RDX(%_ASM_DI), %_ASM_DX
  	mov VCPU_RBX(%_ASM_DI), %_ASM_BX
-	mov VCPU_RBP(%_ASM_DI), %_ASM_BP
  	mov VCPU_RSI(%_ASM_DI), %_ASM_SI
  #ifdef CONFIG_X86_64
  	mov VCPU_R8 (%_ASM_DI),  %r8
@@ -165,6 +167,7 @@ SYM_FUNC_START(__svm_vcpu_run)
  	mov VCPU_R14(%_ASM_DI), %r14
  	mov VCPU_R15(%_ASM_DI), %r15
  #endif
+	mov VCPU_RBP(%_ASM_DI), %_ASM_BP
  	mov VCPU_RDI(%_ASM_DI), %_ASM_DI
  
  	/* Enter guest mode */
@@ -177,11 +180,15 @@ SYM_FUNC_START(__svm_vcpu_run)
  	/* Pop @svm to RAX while it's the only available register. */
  	pop %_ASM_AX
  
-	/* Save all guest registers.  */
+	/*
+	 * Save all guest registers. Pop the frame pointer as soon as possible
+	 * to enable unwinding.
+	 */
+	mov %_ASM_BP,   VCPU_RBP(%_ASM_AX)
+	pop %_ASM_BP
  	mov %_ASM_CX,   VCPU_RCX(%_ASM_AX)
  	mov %_ASM_DX,   VCPU_RDX(%_ASM_AX)
  	mov %_ASM_BX,   VCPU_RBX(%_ASM_AX)
-	mov %_ASM_BP,   VCPU_RBP(%_ASM_AX)
  	mov %_ASM_SI,   VCPU_RSI(%_ASM_AX)
  	mov %_ASM_DI,   VCPU_RDI(%_ASM_AX)
  #ifdef CONFIG_X86_64
@@ -297,6 +304,7 @@ SYM_FUNC_END(__svm_vcpu_run)
   */
  SYM_FUNC_START(__svm_sev_es_vcpu_run)
  	push %_ASM_BP
+	mov %_ASM_SP, %_ASM_BP
  #ifdef CONFIG_X86_64
  	push %r15
  	push %r14
@@ -316,7 +324,8 @@ SYM_FUNC_START(__svm_sev_es_vcpu_run)
  	/* Accessed directly from the stack in RESTORE_HOST_SPEC_CTRL.  */
  	push %_ASM_ARG2
  
-	/* Save @svm. */
+	/* Save frame pointer and @svm. */
+	push %_ASM_BP
  	push %_ASM_ARG1
  
  .ifnc _ASM_ARG1, _ASM_DI
@@ -341,8 +350,12 @@ SYM_FUNC_START(__svm_sev_es_vcpu_run)
  
  2:	cli
  
-	/* Pop @svm to RDI, guest registers have been saved already. */
+	/*
+	 * Guest registers have been saved already.
+	 * Pop @svm to RDI and restore the frame pointer to allow unwinding.
+	 */
  	pop %_ASM_DI
+	pop %_ASM_BP
  
  #ifdef CONFIG_RETPOLINE
  	/* IMPORTANT: Stuff the RSB immediately after VM-Exit, before RET! */


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
  2023-08-02  9:11 [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o Nikunj A Dadhania
  2023-08-02 14:02 ` Sean Christopherson
  2023-08-03 12:06 ` Peter Zijlstra
@ 2023-08-04 11:14 ` Peter Zijlstra
  2023-08-04 12:40   ` Nikunj A. Dadhania
  2023-08-12  0:51 ` kernel test robot
  3 siblings, 1 reply; 17+ messages in thread
From: Peter Zijlstra @ 2023-08-04 11:14 UTC (permalink / raw)
  To: Nikunj A Dadhania
  Cc: kvm, Sean Christopherson, Josh Poimboeuf, Paolo Bonzini,
	Randy Dunlap, Tom Lendacky, Ravi Bangoria

On Wed, Aug 02, 2023 at 02:41:07PM +0530, Nikunj A Dadhania wrote:
> commit 7f4b5cde2409 ("kvm: Disable objtool frame pointer checking for
> vmenter.S") had added the vmenter.o file to the exception list.
> 
> objtool gives the following warnings in the newer kernel builds:
> 
>   arch/x86/kvm/kvm-amd.o: warning: objtool: __svm_vcpu_run+0x17d: BP used as a scratch register
>   arch/x86/kvm/kvm-amd.o: warning: objtool: __svm_sev_es_vcpu_run+0x72: BP used as a scratch register
> 

I wanted to poke around a little, but can't reproduce this.

I took x86_64-defconfig + KVM=m + KVM_AMD=m + UNWIND_FRAME_POINTER=y but
objtool won't complain :/ What actual .config trips this?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
  2023-08-04 11:14 ` Peter Zijlstra
@ 2023-08-04 12:40   ` Nikunj A. Dadhania
  2023-08-04 20:42     ` Peter Zijlstra
  0 siblings, 1 reply; 17+ messages in thread
From: Nikunj A. Dadhania @ 2023-08-04 12:40 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: kvm, Sean Christopherson, Josh Poimboeuf, Paolo Bonzini,
	Randy Dunlap, Tom Lendacky, Ravi Bangoria

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

On 8/4/2023 4:44 PM, Peter Zijlstra wrote:
> On Wed, Aug 02, 2023 at 02:41:07PM +0530, Nikunj A Dadhania wrote:
>> commit 7f4b5cde2409 ("kvm: Disable objtool frame pointer checking for
>> vmenter.S") had added the vmenter.o file to the exception list.
>>
>> objtool gives the following warnings in the newer kernel builds:
>>
>>   arch/x86/kvm/kvm-amd.o: warning: objtool: __svm_vcpu_run+0x17d: BP used as a scratch register
>>   arch/x86/kvm/kvm-amd.o: warning: objtool: __svm_sev_es_vcpu_run+0x72: BP used as a scratch register
>>
> 
> I wanted to poke around a little, but can't reproduce this.
> 
> I took x86_64-defconfig + KVM=m + KVM_AMD=m + UNWIND_FRAME_POINTER=y but
> objtool won't complain :/ What actual .config trips this?

I have attached the config.

Regards,
Nikunj

[-- Attachment #2: config.gz --]
[-- Type: application/x-gzip, Size: 38483 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
  2023-08-04 12:40   ` Nikunj A. Dadhania
@ 2023-08-04 20:42     ` Peter Zijlstra
  0 siblings, 0 replies; 17+ messages in thread
From: Peter Zijlstra @ 2023-08-04 20:42 UTC (permalink / raw)
  To: Nikunj A. Dadhania
  Cc: kvm, Sean Christopherson, Josh Poimboeuf, Paolo Bonzini,
	Randy Dunlap, Tom Lendacky, Ravi Bangoria

On Fri, Aug 04, 2023 at 06:10:19PM +0530, Nikunj A. Dadhania wrote:
> On 8/4/2023 4:44 PM, Peter Zijlstra wrote:
> > On Wed, Aug 02, 2023 at 02:41:07PM +0530, Nikunj A Dadhania wrote:
> >> commit 7f4b5cde2409 ("kvm: Disable objtool frame pointer checking for
> >> vmenter.S") had added the vmenter.o file to the exception list.
> >>
> >> objtool gives the following warnings in the newer kernel builds:
> >>
> >>   arch/x86/kvm/kvm-amd.o: warning: objtool: __svm_vcpu_run+0x17d: BP used as a scratch register
> >>   arch/x86/kvm/kvm-amd.o: warning: objtool: __svm_sev_es_vcpu_run+0x72: BP used as a scratch register
> >>
> > 
> > I wanted to poke around a little, but can't reproduce this.
> > 
> > I took x86_64-defconfig + KVM=m + KVM_AMD=m + UNWIND_FRAME_POINTER=y but
> > objtool won't complain :/ What actual .config trips this?
> 
> I have attached the config.

Thanks, that does reproduce, although now I'm left wondering what
exactly makes it go beep. Also, URGH, I thought the build was broken
until I noticed it had BTF on :/

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
  2023-08-04 10:20       ` Paolo Bonzini
@ 2023-08-04 20:48         ` Peter Zijlstra
  2023-08-04 23:19           ` Josh Poimboeuf
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Zijlstra @ 2023-08-04 20:48 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Nikunj A Dadhania, kvm, Sean Christopherson, Josh Poimboeuf,
	Randy Dunlap, Tom Lendacky, Ravi Bangoria

On Fri, Aug 04, 2023 at 12:20:05PM +0200, Paolo Bonzini wrote:
> It's not clobbered in a part that will cause unwinding; we can further
> restrict the part to a handful of instructions (and add a mov %rsp, %rbp
> at the top, see untested patch after signature).
> 
> I think the chance of this failure is similar or lower to the chance of
> a memory failure that hits the exception handler code itself.

Yes, that's very helpful, the below is your patch with a few extra
hints and a comment. This seems to cure things.

Specifically, your change is needed to put UNWIND_HINT_RESTORE before we
go CALL things (like entry_ibpb), otherwise objtool gets upset we CALL
without having a valid framepointer.

Josh, this look ok to you?

---
 arch/x86/kvm/Makefile      |  4 ----
 arch/x86/kvm/svm/vmenter.S | 38 ++++++++++++++++++++++++++++++++------
 2 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
index 80e3fe184d17..0c5c2f090e93 100644
--- a/arch/x86/kvm/Makefile
+++ b/arch/x86/kvm/Makefile
@@ -3,10 +3,6 @@
 ccflags-y += -I $(srctree)/arch/x86/kvm
 ccflags-$(CONFIG_KVM_WERROR) += -Werror
 
-ifeq ($(CONFIG_FRAME_POINTER),y)
-OBJECT_FILES_NON_STANDARD_vmenter.o := y
-endif
-
 include $(srctree)/virt/kvm/Makefile.kvm
 
 kvm-y			+= x86.o emulate.o i8259.o irq.o lapic.o \
diff --git a/arch/x86/kvm/svm/vmenter.S b/arch/x86/kvm/svm/vmenter.S
index 8e8295e774f0..99b9be9a56c3 100644
--- a/arch/x86/kvm/svm/vmenter.S
+++ b/arch/x86/kvm/svm/vmenter.S
@@ -99,6 +99,8 @@
  */
 SYM_FUNC_START(__svm_vcpu_run)
 	push %_ASM_BP
+	mov %_ASM_SP, %_ASM_BP
+
 #ifdef CONFIG_X86_64
 	push %r15
 	push %r14
@@ -121,7 +123,18 @@ SYM_FUNC_START(__svm_vcpu_run)
 	/* Needed to restore access to percpu variables.  */
 	__ASM_SIZE(push) PER_CPU_VAR(svm_data + SD_save_area_pa)
 
-	/* Finally save @svm. */
+	/*
+	 * Finally save frame pointer and @svm.
+	 *
+	 * Clobbering BP here is mostly ok since GIF will block NMIs and with
+	 * the exception of #MC and the kvm_rebooting _ASM_EXTABLE()s below
+	 * nothing untoward will happen until BP is restored.
+	 *
+	 * The kvm_rebooting exceptions should not want to unwind stack, and
+	 * while #MV might want to unwind stack, it is ultimately fatal.
+	 */
+	UNWIND_HINT_SAVE
+	push %_ASM_BP
 	push %_ASM_ARG1
 
 .ifnc _ASM_ARG1, _ASM_DI
@@ -153,7 +166,6 @@ SYM_FUNC_START(__svm_vcpu_run)
 	mov VCPU_RCX(%_ASM_DI), %_ASM_CX
 	mov VCPU_RDX(%_ASM_DI), %_ASM_DX
 	mov VCPU_RBX(%_ASM_DI), %_ASM_BX
-	mov VCPU_RBP(%_ASM_DI), %_ASM_BP
 	mov VCPU_RSI(%_ASM_DI), %_ASM_SI
 #ifdef CONFIG_X86_64
 	mov VCPU_R8 (%_ASM_DI),  %r8
@@ -165,6 +177,7 @@ SYM_FUNC_START(__svm_vcpu_run)
 	mov VCPU_R14(%_ASM_DI), %r14
 	mov VCPU_R15(%_ASM_DI), %r15
 #endif
+	mov VCPU_RBP(%_ASM_DI), %_ASM_BP
 	mov VCPU_RDI(%_ASM_DI), %_ASM_DI
 
 	/* Enter guest mode */
@@ -177,11 +190,16 @@ SYM_FUNC_START(__svm_vcpu_run)
 	/* Pop @svm to RAX while it's the only available register. */
 	pop %_ASM_AX
 
-	/* Save all guest registers.  */
+	/*
+	 * Save all guest registers. Pop the frame pointer as soon as possible
+	 * to enable unwinding.
+	 */
+	mov %_ASM_BP,   VCPU_RBP(%_ASM_AX)
+	pop %_ASM_BP
+	UNWIND_HINT_RESTORE
 	mov %_ASM_CX,   VCPU_RCX(%_ASM_AX)
 	mov %_ASM_DX,   VCPU_RDX(%_ASM_AX)
 	mov %_ASM_BX,   VCPU_RBX(%_ASM_AX)
-	mov %_ASM_BP,   VCPU_RBP(%_ASM_AX)
 	mov %_ASM_SI,   VCPU_RSI(%_ASM_AX)
 	mov %_ASM_DI,   VCPU_RDI(%_ASM_AX)
 #ifdef CONFIG_X86_64
@@ -297,6 +315,7 @@ SYM_FUNC_END(__svm_vcpu_run)
  */
 SYM_FUNC_START(__svm_sev_es_vcpu_run)
 	push %_ASM_BP
+	mov %_ASM_SP, %_ASM_BP
 #ifdef CONFIG_X86_64
 	push %r15
 	push %r14
@@ -316,7 +335,9 @@ SYM_FUNC_START(__svm_sev_es_vcpu_run)
 	/* Accessed directly from the stack in RESTORE_HOST_SPEC_CTRL.  */
 	push %_ASM_ARG2
 
-	/* Save @svm. */
+	/* Save frame pointer and @svm. */
+	UNWIND_HINT_SAVE
+	push %_ASM_BP
 	push %_ASM_ARG1
 
 .ifnc _ASM_ARG1, _ASM_DI
@@ -341,8 +362,13 @@ SYM_FUNC_START(__svm_sev_es_vcpu_run)
 
 2:	cli
 
-	/* Pop @svm to RDI, guest registers have been saved already. */
+	/*
+	 * Guest registers have been saved already.
+	 * Pop @svm to RDI and restore the frame pointer to allow unwinding.
+	 */
 	pop %_ASM_DI
+	pop %_ASM_BP
+	UNWIND_HINT_RESTORE
 
 #ifdef CONFIG_RETPOLINE
 	/* IMPORTANT: Stuff the RSB immediately after VM-Exit, before RET! */

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
  2023-08-04 20:48         ` Peter Zijlstra
@ 2023-08-04 23:19           ` Josh Poimboeuf
  2023-08-05  0:55             ` Peter Zijlstra
  0 siblings, 1 reply; 17+ messages in thread
From: Josh Poimboeuf @ 2023-08-04 23:19 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Paolo Bonzini, Nikunj A Dadhania, kvm, Sean Christopherson,
	Josh Poimboeuf, Randy Dunlap, Tom Lendacky, Ravi Bangoria

On Fri, Aug 04, 2023 at 10:48:40PM +0200, Peter Zijlstra wrote:
> On Fri, Aug 04, 2023 at 12:20:05PM +0200, Paolo Bonzini wrote:
> > It's not clobbered in a part that will cause unwinding; we can further
> > restrict the part to a handful of instructions (and add a mov %rsp, %rbp
> > at the top, see untested patch after signature).
> > 
> > I think the chance of this failure is similar or lower to the chance of
> > a memory failure that hits the exception handler code itself.
> 
> Yes, that's very helpful, the below is your patch with a few extra
> hints and a comment. This seems to cure things.
> 
> Specifically, your change is needed to put UNWIND_HINT_RESTORE before we
> go CALL things (like entry_ibpb), otherwise objtool gets upset we CALL
> without having a valid framepointer.
> 
> Josh, this look ok to you?

Looks mostly right, except this now creates an unnecessary gap in
unwinding coverage for the ORC unwinder.  So it's better to put the
FP-specific changes behind CONFIG_FRAME_POINTER:

diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
index 80e3fe184d17..0c5c2f090e93 100644
--- a/arch/x86/kvm/Makefile
+++ b/arch/x86/kvm/Makefile
@@ -3,10 +3,6 @@
 ccflags-y += -I $(srctree)/arch/x86/kvm
 ccflags-$(CONFIG_KVM_WERROR) += -Werror
 
-ifeq ($(CONFIG_FRAME_POINTER),y)
-OBJECT_FILES_NON_STANDARD_vmenter.o := y
-endif
-
 include $(srctree)/virt/kvm/Makefile.kvm
 
 kvm-y			+= x86.o emulate.o i8259.o irq.o lapic.o \
diff --git a/arch/x86/kvm/svm/vmenter.S b/arch/x86/kvm/svm/vmenter.S
index 8e8295e774f0..51f6851b1ae5 100644
--- a/arch/x86/kvm/svm/vmenter.S
+++ b/arch/x86/kvm/svm/vmenter.S
@@ -99,6 +99,9 @@
  */
 SYM_FUNC_START(__svm_vcpu_run)
 	push %_ASM_BP
+#ifdef CONFIG_FRAME_POINTER
+	mov %_ASM_SP, %_ASM_BP
+#endif
 #ifdef CONFIG_X86_64
 	push %r15
 	push %r14
@@ -121,7 +124,20 @@ SYM_FUNC_START(__svm_vcpu_run)
 	/* Needed to restore access to percpu variables.  */
 	__ASM_SIZE(push) PER_CPU_VAR(svm_data + SD_save_area_pa)
 
-	/* Finally save @svm. */
+	/*
+	 * Finally save frame pointer and @svm.
+	 *
+	 * Clobbering BP here is mostly ok since GIF will block NMIs and with
+	 * the exception of #MC and the kvm_rebooting _ASM_EXTABLE()s below
+	 * nothing untoward will happen until BP is restored.
+	 *
+	 * The kvm_rebooting exceptions should not want to unwind stack, and
+	 * while #MV might want to unwind stack, it is ultimately fatal.
+	 */
+#ifdef CONFIG_FRAME_POINTER
+	UNWIND_HINT_SAVE
+	push %_ASM_BP
+#endif
 	push %_ASM_ARG1
 
 .ifnc _ASM_ARG1, _ASM_DI
@@ -153,7 +169,6 @@ SYM_FUNC_START(__svm_vcpu_run)
 	mov VCPU_RCX(%_ASM_DI), %_ASM_CX
 	mov VCPU_RDX(%_ASM_DI), %_ASM_DX
 	mov VCPU_RBX(%_ASM_DI), %_ASM_BX
-	mov VCPU_RBP(%_ASM_DI), %_ASM_BP
 	mov VCPU_RSI(%_ASM_DI), %_ASM_SI
 #ifdef CONFIG_X86_64
 	mov VCPU_R8 (%_ASM_DI),  %r8
@@ -165,6 +180,7 @@ SYM_FUNC_START(__svm_vcpu_run)
 	mov VCPU_R14(%_ASM_DI), %r14
 	mov VCPU_R15(%_ASM_DI), %r15
 #endif
+	mov VCPU_RBP(%_ASM_DI), %_ASM_BP
 	mov VCPU_RDI(%_ASM_DI), %_ASM_DI
 
 	/* Enter guest mode */
@@ -177,11 +193,18 @@ SYM_FUNC_START(__svm_vcpu_run)
 	/* Pop @svm to RAX while it's the only available register. */
 	pop %_ASM_AX
 
-	/* Save all guest registers.  */
+	/*
+	 * Save all guest registers. Pop the frame pointer as soon as possible
+	 * to enable unwinding.
+	 */
+	mov %_ASM_BP,   VCPU_RBP(%_ASM_AX)
+#ifdef CONFIG_FRAME_POINTER
+	pop %_ASM_BP
+	UNWIND_HINT_RESTORE
+#endif
 	mov %_ASM_CX,   VCPU_RCX(%_ASM_AX)
 	mov %_ASM_DX,   VCPU_RDX(%_ASM_AX)
 	mov %_ASM_BX,   VCPU_RBX(%_ASM_AX)
-	mov %_ASM_BP,   VCPU_RBP(%_ASM_AX)
 	mov %_ASM_SI,   VCPU_RSI(%_ASM_AX)
 	mov %_ASM_DI,   VCPU_RDI(%_ASM_AX)
 #ifdef CONFIG_X86_64
@@ -297,6 +320,9 @@ SYM_FUNC_END(__svm_vcpu_run)
  */
 SYM_FUNC_START(__svm_sev_es_vcpu_run)
 	push %_ASM_BP
+#ifdef CONFIG_FRAME_POINTER
+	mov %_ASM_SP, %_ASM_BP
+#endif
 #ifdef CONFIG_X86_64
 	push %r15
 	push %r14
@@ -316,7 +342,11 @@ SYM_FUNC_START(__svm_sev_es_vcpu_run)
 	/* Accessed directly from the stack in RESTORE_HOST_SPEC_CTRL.  */
 	push %_ASM_ARG2
 
-	/* Save @svm. */
+	/* Save frame pointer and @svm. */
+#ifdef CONFIG_FRAME_POINTER
+	UNWIND_HINT_SAVE
+	push %_ASM_BP
+#endif
 	push %_ASM_ARG1
 
 .ifnc _ASM_ARG1, _ASM_DI
@@ -341,8 +371,15 @@ SYM_FUNC_START(__svm_sev_es_vcpu_run)
 
 2:	cli
 
-	/* Pop @svm to RDI, guest registers have been saved already. */
+	/*
+	 * Guest registers have been saved already.
+	 * Pop @svm to RDI and restore the frame pointer to allow unwinding.
+	 */
 	pop %_ASM_DI
+#ifdef CONFIG_FRAME_POINTER
+	pop %_ASM_BP
+	UNWIND_HINT_RESTORE
+#endif
 
 #ifdef CONFIG_RETPOLINE
 	/* IMPORTANT: Stuff the RSB immediately after VM-Exit, before RET! */

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
  2023-08-04 23:19           ` Josh Poimboeuf
@ 2023-08-05  0:55             ` Peter Zijlstra
  2023-08-10 14:17               ` Paolo Bonzini
  0 siblings, 1 reply; 17+ messages in thread
From: Peter Zijlstra @ 2023-08-05  0:55 UTC (permalink / raw)
  To: Josh Poimboeuf
  Cc: Paolo Bonzini, Nikunj A Dadhania, kvm, Sean Christopherson,
	Josh Poimboeuf, Randy Dunlap, Tom Lendacky, Ravi Bangoria

On Fri, Aug 04, 2023 at 06:19:54PM -0500, Josh Poimboeuf wrote:

> Looks mostly right, except this now creates an unnecessary gap in
> unwinding coverage for the ORC unwinder.  So it's better to put the
> FP-specific changes behind CONFIG_FRAME_POINTER:

Fair enough.

> diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
> index 80e3fe184d17..0c5c2f090e93 100644
> --- a/arch/x86/kvm/Makefile
> +++ b/arch/x86/kvm/Makefile
> @@ -3,10 +3,6 @@
>  ccflags-y += -I $(srctree)/arch/x86/kvm
>  ccflags-$(CONFIG_KVM_WERROR) += -Werror
>  
> -ifeq ($(CONFIG_FRAME_POINTER),y)
> -OBJECT_FILES_NON_STANDARD_vmenter.o := y
> -endif
> -
>  include $(srctree)/virt/kvm/Makefile.kvm
>  
>  kvm-y			+= x86.o emulate.o i8259.o irq.o lapic.o \
> diff --git a/arch/x86/kvm/svm/vmenter.S b/arch/x86/kvm/svm/vmenter.S
> index 8e8295e774f0..51f6851b1ae5 100644
> --- a/arch/x86/kvm/svm/vmenter.S
> +++ b/arch/x86/kvm/svm/vmenter.S
> @@ -99,6 +99,9 @@
>   */
>  SYM_FUNC_START(__svm_vcpu_run)
>  	push %_ASM_BP
> +#ifdef CONFIG_FRAME_POINTER
> +	mov %_ASM_SP, %_ASM_BP
> +#endif
>  #ifdef CONFIG_X86_64
>  	push %r15
>  	push %r14
> @@ -121,7 +124,20 @@ SYM_FUNC_START(__svm_vcpu_run)
>  	/* Needed to restore access to percpu variables.  */
>  	__ASM_SIZE(push) PER_CPU_VAR(svm_data + SD_save_area_pa)
>  
> -	/* Finally save @svm. */
> +	/*
> +	 * Finally save frame pointer and @svm.
> +	 *
> +	 * Clobbering BP here is mostly ok since GIF will block NMIs and with
> +	 * the exception of #MC and the kvm_rebooting _ASM_EXTABLE()s below
> +	 * nothing untoward will happen until BP is restored.
> +	 *
> +	 * The kvm_rebooting exceptions should not want to unwind stack, and
> +	 * while #MV might want to unwind stack, it is ultimately fatal.
> +	 */

Aside from me not being able to type #MC, I did realize that the
kvm_reboot exception will go outside noinstr code and can hit
tracing/instrumentation and do unwinds from there.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
  2023-08-05  0:55             ` Peter Zijlstra
@ 2023-08-10 14:17               ` Paolo Bonzini
  2023-08-10 21:14                 ` Peter Zijlstra
  0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2023-08-10 14:17 UTC (permalink / raw)
  To: Peter Zijlstra, Josh Poimboeuf
  Cc: Nikunj A Dadhania, kvm, Sean Christopherson, Josh Poimboeuf,
	Randy Dunlap, Tom Lendacky, Ravi Bangoria

On 8/5/23 02:55, Peter Zijlstra wrote:
>> +	 * Clobbering BP here is mostly ok since GIF will block NMIs and with
>> +	 * the exception of #MC and the kvm_rebooting _ASM_EXTABLE()s below
>> +	 * nothing untoward will happen until BP is restored.
>> +	 *
>> +	 * The kvm_rebooting exceptions should not want to unwind stack, and
>> +	 * while #MV might want to unwind stack, it is ultimately fatal.
>> +	 */
> Aside from me not being able to type #MC, I did realize that the
> kvm_reboot exception will go outside noinstr code and can hit
> tracing/instrumentation and do unwinds from there.

Asynchronously disabling SVM requires an IPI, so kvm_rebooting cannot 
change within CLGI/STGI.   We can check it after CLGI instead of waiting 
for a #GP:

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 956726d867aa..e3755f5eaf81 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4074,7 +4074,10 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct 
kvm_vcpu *vcpu)
  	if (!static_cpu_has(X86_FEATURE_V_SPEC_CTRL))
  		x86_spec_ctrl_set_guest(svm->virt_spec_ctrl);

-	svm_vcpu_enter_exit(vcpu, spec_ctrl_intercepted);
+	if (unlikely(kvm_rebooting))
+		svm->vmcb->control.exit_code = SVM_EXIT_PAUSE;
+	else
+		svm_vcpu_enter_exit(vcpu, spec_ctrl_intercepted);

  	if (!static_cpu_has(X86_FEATURE_V_SPEC_CTRL))
  		x86_spec_ctrl_restore_host(svm->virt_spec_ctrl);
diff --git a/arch/x86/kvm/svm/vmenter.S b/arch/x86/kvm/svm/vmenter.S
index 8e8295e774f0..34641b3a6823 100644
--- a/arch/x86/kvm/svm/vmenter.S
+++ b/arch/x86/kvm/svm/vmenter.S
@@ -270,23 +270,12 @@ SYM_FUNC_START(__svm_vcpu_run)
  	RESTORE_GUEST_SPEC_CTRL_BODY
  	RESTORE_HOST_SPEC_CTRL_BODY

-10:	cmpb $0, kvm_rebooting
-	jne 2b
-	ud2
-30:	cmpb $0, kvm_rebooting
-	jne 4b
-	ud2
-50:	cmpb $0, kvm_rebooting
-	jne 6b
-	ud2
-70:	cmpb $0, kvm_rebooting
-	jne 8b
-	ud2
+10:	ud2

  	_ASM_EXTABLE(1b, 10b)
-	_ASM_EXTABLE(3b, 30b)
-	_ASM_EXTABLE(5b, 50b)
-	_ASM_EXTABLE(7b, 70b)
+	_ASM_EXTABLE(3b, 10b)
+	_ASM_EXTABLE(5b, 10b)
+	_ASM_EXTABLE(7b, 10b)

  SYM_FUNC_END(__svm_vcpu_run)


Paolo


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
  2023-08-10 14:17               ` Paolo Bonzini
@ 2023-08-10 21:14                 ` Peter Zijlstra
  0 siblings, 0 replies; 17+ messages in thread
From: Peter Zijlstra @ 2023-08-10 21:14 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Josh Poimboeuf, Nikunj A Dadhania, kvm, Sean Christopherson,
	Josh Poimboeuf, Randy Dunlap, Tom Lendacky, Ravi Bangoria

On Thu, Aug 10, 2023 at 04:17:41PM +0200, Paolo Bonzini wrote:
> On 8/5/23 02:55, Peter Zijlstra wrote:
> > > +	 * Clobbering BP here is mostly ok since GIF will block NMIs and with
> > > +	 * the exception of #MC and the kvm_rebooting _ASM_EXTABLE()s below
> > > +	 * nothing untoward will happen until BP is restored.
> > > +	 *
> > > +	 * The kvm_rebooting exceptions should not want to unwind stack, and
> > > +	 * while #MV might want to unwind stack, it is ultimately fatal.
> > > +	 */
> > Aside from me not being able to type #MC, I did realize that the
> > kvm_reboot exception will go outside noinstr code and can hit
> > tracing/instrumentation and do unwinds from there.
> 
> Asynchronously disabling SVM requires an IPI, so kvm_rebooting cannot change
> within CLGI/STGI.   We can check it after CLGI instead of waiting for a #GP:

Seems fair; thanks!

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 956726d867aa..e3755f5eaf81 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4074,7 +4074,10 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct
> kvm_vcpu *vcpu)
>  	if (!static_cpu_has(X86_FEATURE_V_SPEC_CTRL))
>  		x86_spec_ctrl_set_guest(svm->virt_spec_ctrl);
> 
> -	svm_vcpu_enter_exit(vcpu, spec_ctrl_intercepted);
> +	if (unlikely(kvm_rebooting))
> +		svm->vmcb->control.exit_code = SVM_EXIT_PAUSE;
> +	else
> +		svm_vcpu_enter_exit(vcpu, spec_ctrl_intercepted);
> 
>  	if (!static_cpu_has(X86_FEATURE_V_SPEC_CTRL))
>  		x86_spec_ctrl_restore_host(svm->virt_spec_ctrl);
> diff --git a/arch/x86/kvm/svm/vmenter.S b/arch/x86/kvm/svm/vmenter.S
> index 8e8295e774f0..34641b3a6823 100644
> --- a/arch/x86/kvm/svm/vmenter.S
> +++ b/arch/x86/kvm/svm/vmenter.S
> @@ -270,23 +270,12 @@ SYM_FUNC_START(__svm_vcpu_run)
>  	RESTORE_GUEST_SPEC_CTRL_BODY
>  	RESTORE_HOST_SPEC_CTRL_BODY
> 
> -10:	cmpb $0, kvm_rebooting
> -	jne 2b
> -	ud2
> -30:	cmpb $0, kvm_rebooting
> -	jne 4b
> -	ud2
> -50:	cmpb $0, kvm_rebooting
> -	jne 6b
> -	ud2
> -70:	cmpb $0, kvm_rebooting
> -	jne 8b
> -	ud2
> +10:	ud2
> 
>  	_ASM_EXTABLE(1b, 10b)
> -	_ASM_EXTABLE(3b, 30b)
> -	_ASM_EXTABLE(5b, 50b)
> -	_ASM_EXTABLE(7b, 70b)
> +	_ASM_EXTABLE(3b, 10b)
> +	_ASM_EXTABLE(5b, 10b)
> +	_ASM_EXTABLE(7b, 10b)
> 
>  SYM_FUNC_END(__svm_vcpu_run)
> 
> 
> Paolo
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
  2023-08-02  9:11 [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o Nikunj A Dadhania
                   ` (2 preceding siblings ...)
  2023-08-04 11:14 ` Peter Zijlstra
@ 2023-08-12  0:51 ` kernel test robot
  3 siblings, 0 replies; 17+ messages in thread
From: kernel test robot @ 2023-08-12  0:51 UTC (permalink / raw)
  To: Nikunj A Dadhania, kvm, Sean Christopherson
  Cc: llvm, oe-kbuild-all, Nikunj A Dadhania, Josh Poimboeuf,
	Peter Zijlstra, Paolo Bonzini, Randy Dunlap, Tom Lendacky,
	Ravi Bangoria

Hi Nikunj,

kernel test robot noticed the following build errors:

[auto build test ERROR on kvm/queue]
[also build test ERROR on mst-vhost/linux-next linus/master v6.5-rc5 next-20230809]
[cannot apply to kvm/linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Nikunj-A-Dadhania/KVM-SVM-Add-exception-to-disable-objtool-warning-for-kvm-amd-o/20230802-171219
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
patch link:    https://lore.kernel.org/r/20230802091107.1160320-1-nikunj%40amd.com
patch subject: [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o
config: i386-randconfig-i013-20230812 (https://download.01.org/0day-ci/archive/20230812/202308120845.gkEMVH5a-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce: (https://download.01.org/0day-ci/archive/20230812/202308120845.gkEMVH5a-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308120845.gkEMVH5a-lkp@intel.com/

All errors (new ones prefixed by >>):

>> arch/x86/kvm/svm/vmenter.S:392:1: error: invalid instruction mnemonic 'stack_frame_non_standard_fp'
   STACK_FRAME_NON_STANDARD_FP(__svm_sev_es_vcpu_run)
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/stack_frame_non_standard_fp +392 arch/x86/kvm/svm/vmenter.S

   352	
   353		/* Clobbers RAX, RCX, RDX.  */
   354		RESTORE_HOST_SPEC_CTRL
   355	
   356		/*
   357		 * Mitigate RETBleed for AMD/Hygon Zen uarch. RET should be
   358		 * untrained as soon as we exit the VM and are back to the
   359		 * kernel. This should be done before re-enabling interrupts
   360		 * because interrupt handlers won't sanitize RET if the return is
   361		 * from the kernel.
   362		 */
   363		UNTRAIN_RET
   364	
   365		/* "Pop" @spec_ctrl_intercepted.  */
   366		pop %_ASM_BX
   367	
   368		pop %_ASM_BX
   369	
   370	#ifdef CONFIG_X86_64
   371		pop %r12
   372		pop %r13
   373		pop %r14
   374		pop %r15
   375	#else
   376		pop %esi
   377		pop %edi
   378	#endif
   379		pop %_ASM_BP
   380		RET
   381	
   382		RESTORE_GUEST_SPEC_CTRL_BODY
   383		RESTORE_HOST_SPEC_CTRL_BODY
   384	
   385	3:	cmpb $0, kvm_rebooting
   386		jne 2b
   387		ud2
   388	
   389		_ASM_EXTABLE(1b, 3b)
   390	
   391	SYM_FUNC_END(__svm_sev_es_vcpu_run)
 > 392	STACK_FRAME_NON_STANDARD_FP(__svm_sev_es_vcpu_run)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2023-08-12  0:52 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-02  9:11 [PATCH] KVM: SVM: Add exception to disable objtool warning for kvm-amd.o Nikunj A Dadhania
2023-08-02 14:02 ` Sean Christopherson
2023-08-03  6:25   ` Nikunj A. Dadhania
2023-08-03 12:06 ` Peter Zijlstra
2023-08-03 18:06   ` Paolo Bonzini
2023-08-03 19:07     ` Peter Zijlstra
2023-08-04  3:25       ` Nikunj A. Dadhania
2023-08-04 10:20       ` Paolo Bonzini
2023-08-04 20:48         ` Peter Zijlstra
2023-08-04 23:19           ` Josh Poimboeuf
2023-08-05  0:55             ` Peter Zijlstra
2023-08-10 14:17               ` Paolo Bonzini
2023-08-10 21:14                 ` Peter Zijlstra
2023-08-04 11:14 ` Peter Zijlstra
2023-08-04 12:40   ` Nikunj A. Dadhania
2023-08-04 20:42     ` Peter Zijlstra
2023-08-12  0:51 ` kernel test robot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox