[PATCH] kvm: add proper frame pointer logic for vmx

[PATCH] kvm: add proper frame pointer logic for vmx

[Qian Cai]

compilation warning since v5.0-rc1,

arch/x86/kvm/vmx/vmx.o: warning: objtool: vmx_vcpu_run.part.17()+0x3171:
call without frame pointer save/setup

Fixes: 453eafbe65f (KVM: VMX: Move VM-Enter + VM-Exit handling to
non-inline sub-routines)
Signed-off-by: Qian Cai <cai@lca.pw>
---
 arch/x86/kvm/vmx/vmenter.S | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
index bcef2c7e9bc4..874dd3030dee 100644
--- a/arch/x86/kvm/vmx/vmenter.S
+++ b/arch/x86/kvm/vmx/vmenter.S
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 #include <linux/linkage.h>
 #include <asm/asm.h>
+#include <asm/frame.h>
 
 	.text
 
@@ -20,18 +21,22 @@
  */
 ENTRY(vmx_vmenter)
 	/* EFLAGS.ZF is set if VMCS.LAUNCHED == 0 */
+	FRAME_BEGIN
 	je 2f
 
 1:	vmresume
+	FRAME_END
 	ret
 
 2:	vmlaunch
+	FRAME_END
 	ret
 
 3:	cmpb $0, kvm_rebooting
 	jne 4f
 	call kvm_spurious_fault
-4:	ret
+4:	FRAME_END
+	ret
 
 	.pushsection .fixup, "ax"
 5:	jmp 3b
-- 
2.17.2 (Apple Git-113)

Re: [PATCH] kvm: add proper frame pointer logic for vmx

[Qian Cai]

On 1/15/19 1:44 AM, Qian Cai wrote:
> compilation warning since v5.0-rc1,
> 
> arch/x86/kvm/vmx/vmx.o: warning: objtool: vmx_vcpu_run.part.17()+0x3171:
> call without frame pointer save/setup
> 
> Fixes: 453eafbe65f (KVM: VMX: Move VM-Enter + VM-Exit handling to
> non-inline sub-routines)

Oops, wrong fix. Back to square one.

Re: [PATCH] kvm: add proper frame pointer logic for vmx

[Paolo Bonzini]

On 15/01/19 08:04, Qian Cai wrote:
> 
> 
> On 1/15/19 1:44 AM, Qian Cai wrote:
>> compilation warning since v5.0-rc1,
>>
>> arch/x86/kvm/vmx/vmx.o: warning: objtool: vmx_vcpu_run.part.17()+0x3171:
>> call without frame pointer save/setup
>>
>> Fixes: 453eafbe65f (KVM: VMX: Move VM-Enter + VM-Exit handling to
>> non-inline sub-routines)
> 
> Oops, wrong fix. Back to square one.
> 

Hmm, maybe like this:

diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
index bcef2c7e9bc4..33122fa9d4bd 100644
--- a/arch/x86/kvm/vmx/vmenter.S
+++ b/arch/x86/kvm/vmx/vmenter.S
@@ -26,19 +26,17 @@ ENTRY(vmx_vmenter)
 	ret

 2:	vmlaunch
+3:
 	ret

-3:	cmpb $0, kvm_rebooting
-	jne 4f
-	call kvm_spurious_fault
-4:	ret
-
 	.pushsection .fixup, "ax"
-5:	jmp 3b
+4:	cmpb $0, kvm_rebooting
+	jne 3b
+	jmp kvm_spurious_fault
 	.popsection

-	_ASM_EXTABLE(1b, 5b)
-	_ASM_EXTABLE(2b, 5b)
+	_ASM_EXTABLE(1b, 4b)
+	_ASM_EXTABLE(2b, 4b)

 ENDPROC(vmx_vmenter)


Paolo

Re: [PATCH] kvm: add proper frame pointer logic for vmx

[Sean Christopherson]

On Tue, Jan 15, 2019 at 08:13:22AM +0100, Paolo Bonzini wrote:
> On 15/01/19 08:04, Qian Cai wrote:
> > 
> > 
> > On 1/15/19 1:44 AM, Qian Cai wrote:
> >> compilation warning since v5.0-rc1,
> >>
> >> arch/x86/kvm/vmx/vmx.o: warning: objtool: vmx_vcpu_run.part.17()+0x3171:
> >> call without frame pointer save/setup

The warning is complaining about vmx_vcpu_run() in vmx.c, not vmenter.S.
The rule being "broken" is that a call is made without creating a stack
frame, and vmx_vmenter() obviously makes no calls.

E.g., manually running objtool check:

    $ tools/objtool/objtool check arch/x86/kvm/vmx/vmenter.o
    $ tools/objtool/objtool check arch/x86/kvm/vmx/vmx.o
    arch/x86/kvm/vmx/vmx.o: warning: objtool: vmx_vcpu_run.part.19()+0x83e: call without frame pointer save/setup

I put "broken" in quotes because AFAICT we're not actually violating the
rule.  From tools/objtool/Documentation/stack-validation.txt:

   If it's a GCC-compiled .c file, the error may be because the function
   uses an inline asm() statement which has a "call" instruction.  An
   asm() statement with a call instruction must declare the use of the
   stack pointer in its output operand.  On x86_64, this means adding
   the ASM_CALL_CONSTRAINT as an output constraint:

     asm volatile("call func" : ASM_CALL_CONSTRAINT);

   Otherwise the stack frame may not get created before the call.


The asm() blob that calls vmx_vmenter() uses ASM_CALL_CONSTRAINT, and
the resulting asm output generates a frame pointer, e.g. this is from
the vmx.o that objtool warns on:

Dump of assembler code for function vmx_vcpu_run:
   0x0000000000007440 <+0>:     e8 00 00 00 00  callq  0x7445 <vmx_vcpu_run+5>
   0x0000000000007445 <+5>:     55      push   %rbp
   0x0000000000007446 <+6>:     48 89 e5        mov    %rsp,%rbp


The warning only shows up in certain configs, e.g. I was only able to
reproduce this using the .config provided by lkp.  Even explicitly
enabling CONFIG_FRAME_POINTERS and CONFIG_STACK_VALIDATION didn't
trigger the warning using my usual config.

And all that being said, I'm pretty sure this isn't related to the call
to vmx_vmenter() at all, but rather is something that was exposed by
removing __noclone from vmx_vcpu_run().

E.g. I still get the warning if I comment out the call to vmx_vmenter,
it just shifts to something else (and continues to shift I comment out
more calls).  The warning goes away if I re-add __noclone, regardless
of whether or not commit 2bcbd406715d ("Revert "compiler-gcc: disable
-ftracer for __noclone functions"") is applied.

0x6659 is in vmx_vcpu_run (./arch/x86/include/asm/spec-ctrl.h:43).
38       * Avoids writing to the MSR if the content/bits are the same
39       */
40      static inline
41      void x86_spec_ctrl_restore_host(u64 guest_spec_ctrl, u64 guest_virt_spec_ctrl)
42      {
43              x86_virt_spec_ctrl(guest_spec_ctrl, guest_virt_spec_ctrl, false);
44      }
45
46      /* AMD specific Speculative Store Bypass MSR data */
47      extern u64 x86_amd_ls_cfg_base;

What's really baffling is that we explicitly tell objtool to not do stack
metadata validation on vmx_vcpu_run():

	STACK_FRAME_NON_STANDARD(vmx_vcpu_run);

As an aside, we might be able to remove STACK_FRAME_NON_STANDARD, assuming
we get this warning sorted out.

> >> Fixes: 453eafbe65f (KVM: VMX: Move VM-Enter + VM-Exit handling to
> >> non-inline sub-routines)
> > 
> > Oops, wrong fix. Back to square one.
> > 
> 
> Hmm, maybe like this:
> 
> diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> index bcef2c7e9bc4..33122fa9d4bd 100644
> --- a/arch/x86/kvm/vmx/vmenter.S
> +++ b/arch/x86/kvm/vmx/vmenter.S
> @@ -26,19 +26,17 @@ ENTRY(vmx_vmenter)
>  	ret
> 
>  2:	vmlaunch
> +3:
>  	ret
> 
> -3:	cmpb $0, kvm_rebooting
> -	jne 4f
> -	call kvm_spurious_fault
> -4:	ret
> -
>  	.pushsection .fixup, "ax"
> -5:	jmp 3b
> +4:	cmpb $0, kvm_rebooting
> +	jne 3b
> +	jmp kvm_spurious_fault
>  	.popsection
> 
> -	_ASM_EXTABLE(1b, 5b)
> -	_ASM_EXTABLE(2b, 5b)
> +	_ASM_EXTABLE(1b, 4b)
> +	_ASM_EXTABLE(2b, 4b)
> 
>  ENDPROC(vmx_vmenter)
> 
> 
> Paolo

Re: [PATCH] kvm: add proper frame pointer logic for vmx

[Qian Cai]

On 1/15/19 11:34 AM, Sean Christopherson wrote:
> On Tue, Jan 15, 2019 at 08:13:22AM +0100, Paolo Bonzini wrote:
>> On 15/01/19 08:04, Qian Cai wrote:
>>>
>>>
>>> On 1/15/19 1:44 AM, Qian Cai wrote:
>>>> compilation warning since v5.0-rc1,
>>>>
>>>> arch/x86/kvm/vmx/vmx.o: warning: objtool: vmx_vcpu_run.part.17()+0x3171:
>>>> call without frame pointer save/setup
> 
> The warning is complaining about vmx_vcpu_run() in vmx.c, not vmenter.S.
> The rule being "broken" is that a call is made without creating a stack
> frame, and vmx_vmenter() obviously makes no calls.
> 
> E.g., manually running objtool check:
> 
>     $ tools/objtool/objtool check arch/x86/kvm/vmx/vmenter.o
>     $ tools/objtool/objtool check arch/x86/kvm/vmx/vmx.o
>     arch/x86/kvm/vmx/vmx.o: warning: objtool: vmx_vcpu_run.part.19()+0x83e: call without frame pointer save/setup
> 
> I put "broken" in quotes because AFAICT we're not actually violating the
> rule.  From tools/objtool/Documentation/stack-validation.txt:
> 
>    If it's a GCC-compiled .c file, the error may be because the function
>    uses an inline asm() statement which has a "call" instruction.  An
>    asm() statement with a call instruction must declare the use of the
>    stack pointer in its output operand.  On x86_64, this means adding
>    the ASM_CALL_CONSTRAINT as an output constraint:
> 
>      asm volatile("call func" : ASM_CALL_CONSTRAINT);
> 
>    Otherwise the stack frame may not get created before the call.
> 
> 
> The asm() blob that calls vmx_vmenter() uses ASM_CALL_CONSTRAINT, and
> the resulting asm output generates a frame pointer, e.g. this is from
> the vmx.o that objtool warns on:
> 
> Dump of assembler code for function vmx_vcpu_run:
>    0x0000000000007440 <+0>:     e8 00 00 00 00  callq  0x7445 <vmx_vcpu_run+5>
>    0x0000000000007445 <+5>:     55      push   %rbp
>    0x0000000000007446 <+6>:     48 89 e5        mov    %rsp,%rbp
> 
> 
> The warning only shows up in certain configs, e.g. I was only able to
> reproduce this using the .config provided by lkp.  Even explicitly
> enabling CONFIG_FRAME_POINTERS and CONFIG_STACK_VALIDATION didn't
> trigger the warning using my usual config.
> 
> And all that being said, I'm pretty sure this isn't related to the call
> to vmx_vmenter() at all, but rather is something that was exposed by
> removing __noclone from vmx_vcpu_run().
> 
> E.g. I still get the warning if I comment out the call to vmx_vmenter,
> it just shifts to something else (and continues to shift I comment out
> more calls).  The warning goes away if I re-add __noclone, regardless
> of whether or not commit 2bcbd406715d ("Revert "compiler-gcc: disable
> -ftracer for __noclone functions"") is applied.

It complained the call right here at the end of vmx_vcpu_run().

"callq 19eb6" in __read_once_size() via atomic_read()

and then jump back to vmx_vcpu_run() again.

/root/linux-debug/arch/x86/kvm/vmx/vmx.c:6650
}
   19e94:       e8 00 00 00 00          callq  19e99 <vmx_vcpu_run.part.21+0x3159>

__read_once_size():
/root/linux-debug/./include/linux/compiler.h:191
   19e99:       48 c7 c7 00 00 00 00    mov    $0x0,%rdi
   19ea0:       e8 00 00 00 00          callq  19ea5 <vmx_vcpu_run.part.21+0x3165>
   19ea5:       e9 8b dd ff ff          jmpq   17c35 <vmx_vcpu_run.part.21+0xef5>
   19eaa:       48 8b bd 48 ff ff ff    mov    -0xb8(%rbp),%rdi
   19eb1:       e8 00 00 00 00          callq  19eb6 <vmx_vcpu_run.part.21+0x3176>
   19eb6:       e9 b8 df ff ff          jmpq   17e73 <vmx_vcpu_run.part.21+0x1133>

vmx_vcpu_run():
/root/linux-debug/arch/x86/kvm/vmx/vmx.c:6621
        vcpu->arch.regs_dirty = 0;
   19ebb:       48 89 f7                mov    %rsi,%rdi
   19ebe:       e8 00 00 00 00          callq  19ec3 <vmx_vcpu_run.part.21+0x3183>
   19ec3:       e9 f1 e0 ff ff          jmpq   17fb9 <vmx_vcpu_run.part.21+0x1279>

Re: [PATCH] kvm: add proper frame pointer logic for vmx

[Qian Cai]

On 1/15/19 2:13 AM, Paolo Bonzini wrote:
> Hmm, maybe like this:
> 
> diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> index bcef2c7e9bc4..33122fa9d4bd 100644
> --- a/arch/x86/kvm/vmx/vmenter.S
> +++ b/arch/x86/kvm/vmx/vmenter.S
> @@ -26,19 +26,17 @@ ENTRY(vmx_vmenter)
>  	ret
> 
>  2:	vmlaunch
> +3:
>  	ret
> 
> -3:	cmpb $0, kvm_rebooting
> -	jne 4f
> -	call kvm_spurious_fault
> -4:	ret
> -
>  	.pushsection .fixup, "ax"
> -5:	jmp 3b
> +4:	cmpb $0, kvm_rebooting
> +	jne 3b
> +	jmp kvm_spurious_fault
>  	.popsection
> 
> -	_ASM_EXTABLE(1b, 5b)
> -	_ASM_EXTABLE(2b, 5b)
> +	_ASM_EXTABLE(1b, 4b)
> +	_ASM_EXTABLE(2b, 4b)
> 
>  ENDPROC(vmx_vmenter)

No, that will not work. The problem is in vmx.o where I just sent another patch
for it.

I can see there are five options to solve it.

1) always inline vmx_vcpu_run()
2) always noinline vmx_vcpu_run()
3) add -fdiable-ipa-fnsplit option to Makefile for vmx.o
4) let STACK_FRAME_NON_STANDARD support part.* syntax.
5) trim-down vmx_vcpu_run() even more to not causing splitting by GCC.

Option 1) and 2) seems give away the decision for user with
CONFIG_CC_OPTIMIZE_FOR_(PERFORMANCE/SIZE).

Option 3) prevents other functions there for splitting for optimization.

Option 4) and 5) seems tricky to implement.

I am not more leaning to 3) as only other fuction will miss splitting is
vmx_segment_access_rights().

Re: [PATCH] kvm: add proper frame pointer logic for vmx

[Qian Cai]

On 1/15/19 11:43 AM, Qian Cai wrote:
> 
> 
> On 1/15/19 2:13 AM, Paolo Bonzini wrote:
>> Hmm, maybe like this:
>>
>> diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
>> index bcef2c7e9bc4..33122fa9d4bd 100644
>> --- a/arch/x86/kvm/vmx/vmenter.S
>> +++ b/arch/x86/kvm/vmx/vmenter.S
>> @@ -26,19 +26,17 @@ ENTRY(vmx_vmenter)
>>  	ret
>>
>>  2:	vmlaunch
>> +3:
>>  	ret
>>
>> -3:	cmpb $0, kvm_rebooting
>> -	jne 4f
>> -	call kvm_spurious_fault
>> -4:	ret
>> -
>>  	.pushsection .fixup, "ax"
>> -5:	jmp 3b
>> +4:	cmpb $0, kvm_rebooting
>> +	jne 3b
>> +	jmp kvm_spurious_fault
>>  	.popsection
>>
>> -	_ASM_EXTABLE(1b, 5b)
>> -	_ASM_EXTABLE(2b, 5b)
>> +	_ASM_EXTABLE(1b, 4b)
>> +	_ASM_EXTABLE(2b, 4b)
>>
>>  ENDPROC(vmx_vmenter)
> 
> No, that will not work. The problem is in vmx.o where I just sent another patch
> for it.
> 
> I can see there are five options to solve it.
> 
> 1) always inline vmx_vcpu_run()
> 2) always noinline vmx_vcpu_run()
> 3) add -fdiable-ipa-fnsplit option to Makefile for vmx.o
> 4) let STACK_FRAME_NON_STANDARD support part.* syntax.
> 5) trim-down vmx_vcpu_run() even more to not causing splitting by GCC.
> 
> Option 1) and 2) seems give away the decision for user with
> CONFIG_CC_OPTIMIZE_FOR_(PERFORMANCE/SIZE).
> 
> Option 3) prevents other functions there for splitting for optimization.
> 
> Option 4) and 5) seems tricky to implement.
> 
> I am not more leaning to 3) as only other fuction will miss splitting is
> vmx_segment_access_rights().
> 

Option 3) seems a bit tricky to implement too, as it needs to check for the
compiler version before to see if the option is available before put it to the
CFLAGS.

Re: [PATCH] kvm: add proper frame pointer logic for vmx

[Sean Christopherson]

On Tue, Jan 15, 2019 at 11:43:20AM -0500, Qian Cai wrote:
> 
> 
> On 1/15/19 2:13 AM, Paolo Bonzini wrote:
> > Hmm, maybe like this:
> > 
> > diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> > index bcef2c7e9bc4..33122fa9d4bd 100644
> > --- a/arch/x86/kvm/vmx/vmenter.S
> > +++ b/arch/x86/kvm/vmx/vmenter.S
> > @@ -26,19 +26,17 @@ ENTRY(vmx_vmenter)
> >  	ret
> > 
> >  2:	vmlaunch
> > +3:
> >  	ret
> > 
> > -3:	cmpb $0, kvm_rebooting
> > -	jne 4f
> > -	call kvm_spurious_fault
> > -4:	ret
> > -
> >  	.pushsection .fixup, "ax"
> > -5:	jmp 3b
> > +4:	cmpb $0, kvm_rebooting
> > +	jne 3b
> > +	jmp kvm_spurious_fault
> >  	.popsection
> > 
> > -	_ASM_EXTABLE(1b, 5b)
> > -	_ASM_EXTABLE(2b, 5b)
> > +	_ASM_EXTABLE(1b, 4b)
> > +	_ASM_EXTABLE(2b, 4b)
> > 
> >  ENDPROC(vmx_vmenter)
> 
> No, that will not work. The problem is in vmx.o where I just sent another patch
> for it.
> 
> I can see there are five options to solve it.
> 
> 1) always inline vmx_vcpu_run()
> 2) always noinline vmx_vcpu_run()
> 3) add -fdiable-ipa-fnsplit option to Makefile for vmx.o
> 4) let STACK_FRAME_NON_STANDARD support part.* syntax.

What is ".part." and where does it come from?  Searching for information
is futile, the term is too generic.

> 5) trim-down vmx_vcpu_run() even more to not causing splitting by GCC.
> 
> Option 1) and 2) seems give away the decision for user with
> CONFIG_CC_OPTIMIZE_FOR_(PERFORMANCE/SIZE).
> 
> Option 3) prevents other functions there for splitting for optimization.
> 
> Option 4) and 5) seems tricky to implement.
> 
> I am not more leaning to 3) as only other fuction will miss splitting is
> vmx_segment_access_rights().

Re: [PATCH] kvm: add proper frame pointer logic for vmx

[Sean Christopherson]

On Tue, Jan 15, 2019 at 09:48:45AM -0800, Sean Christopherson wrote:
> On Tue, Jan 15, 2019 at 11:43:20AM -0500, Qian Cai wrote:
> > 
> > 
> > On 1/15/19 2:13 AM, Paolo Bonzini wrote:
> > > Hmm, maybe like this:
> > > 
> > > diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> > > index bcef2c7e9bc4..33122fa9d4bd 100644
> > > --- a/arch/x86/kvm/vmx/vmenter.S
> > > +++ b/arch/x86/kvm/vmx/vmenter.S
> > > @@ -26,19 +26,17 @@ ENTRY(vmx_vmenter)
> > >  	ret
> > > 
> > >  2:	vmlaunch
> > > +3:
> > >  	ret
> > > 
> > > -3:	cmpb $0, kvm_rebooting
> > > -	jne 4f
> > > -	call kvm_spurious_fault
> > > -4:	ret
> > > -
> > >  	.pushsection .fixup, "ax"
> > > -5:	jmp 3b
> > > +4:	cmpb $0, kvm_rebooting
> > > +	jne 3b
> > > +	jmp kvm_spurious_fault
> > >  	.popsection
> > > 
> > > -	_ASM_EXTABLE(1b, 5b)
> > > -	_ASM_EXTABLE(2b, 5b)
> > > +	_ASM_EXTABLE(1b, 4b)
> > > +	_ASM_EXTABLE(2b, 4b)
> > > 
> > >  ENDPROC(vmx_vmenter)
> > 
> > No, that will not work. The problem is in vmx.o where I just sent another patch
> > for it.
> > 
> > I can see there are five options to solve it.
> > 
> > 1) always inline vmx_vcpu_run()
> > 2) always noinline vmx_vcpu_run()
> > 3) add -fdiable-ipa-fnsplit option to Makefile for vmx.o
> > 4) let STACK_FRAME_NON_STANDARD support part.* syntax.
> 
> What is ".part." and where does it come from?  Searching for information
> is futile, the term is too generic.

And never mind, my eyes glazed over -fdiable-ipa-fnsplit.

> > 5) trim-down vmx_vcpu_run() even more to not causing splitting by GCC.
> > 
> > Option 1) and 2) seems give away the decision for user with
> > CONFIG_CC_OPTIMIZE_FOR_(PERFORMANCE/SIZE).
> > 
> > Option 3) prevents other functions there for splitting for optimization.
> > 
> > Option 4) and 5) seems tricky to implement.
> > 
> > I am not more leaning to 3) as only other fuction will miss splitting is
> > vmx_segment_access_rights().

Re: [PATCH] kvm: add proper frame pointer logic for vmx

[Qian Cai]

On 1/15/19 12:49 PM, Sean Christopherson wrote:
> On Tue, Jan 15, 2019 at 09:48:45AM -0800, Sean Christopherson wrote:
>> On Tue, Jan 15, 2019 at 11:43:20AM -0500, Qian Cai wrote:
>>>
>>>
>>> On 1/15/19 2:13 AM, Paolo Bonzini wrote:
>>>> Hmm, maybe like this:
>>>>
>>>> diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
>>>> index bcef2c7e9bc4..33122fa9d4bd 100644
>>>> --- a/arch/x86/kvm/vmx/vmenter.S
>>>> +++ b/arch/x86/kvm/vmx/vmenter.S
>>>> @@ -26,19 +26,17 @@ ENTRY(vmx_vmenter)
>>>>  	ret
>>>>
>>>>  2:	vmlaunch
>>>> +3:
>>>>  	ret
>>>>
>>>> -3:	cmpb $0, kvm_rebooting
>>>> -	jne 4f
>>>> -	call kvm_spurious_fault
>>>> -4:	ret
>>>> -
>>>>  	.pushsection .fixup, "ax"
>>>> -5:	jmp 3b
>>>> +4:	cmpb $0, kvm_rebooting
>>>> +	jne 3b
>>>> +	jmp kvm_spurious_fault
>>>>  	.popsection
>>>>
>>>> -	_ASM_EXTABLE(1b, 5b)
>>>> -	_ASM_EXTABLE(2b, 5b)
>>>> +	_ASM_EXTABLE(1b, 4b)
>>>> +	_ASM_EXTABLE(2b, 4b)
>>>>
>>>>  ENDPROC(vmx_vmenter)
>>>
>>> No, that will not work. The problem is in vmx.o where I just sent another patch
>>> for it.
>>>
>>> I can see there are five options to solve it.
>>>
>>> 1) always inline vmx_vcpu_run()
>>> 2) always noinline vmx_vcpu_run()
>>> 3) add -fdiable-ipa-fnsplit option to Makefile for vmx.o
>>> 4) let STACK_FRAME_NON_STANDARD support part.* syntax.
>>
>> What is ".part." and where does it come from?  Searching for information
>> is futile, the term is too generic.
> 
> And never mind, my eyes glazed over -fdiable-ipa-fnsplit.

For example, this works too,

diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
index 69b3a7c30013..990dfc254e71 100644
--- a/arch/x86/kvm/Makefile
+++ b/arch/x86/kvm/Makefile
@@ -4,7 +4,7 @@ ccflags-y += -Iarch/x86/kvm

 CFLAGS_x86.o := -I.
 CFLAGS_svm.o := -I.
-CFLAGS_vmx.o := -I.
+CFLAGS_vmx.o := -I. -fdisable-tree-fnsplit

 KVM := ../../../virt/kvm

Re: [PATCH] kvm: add proper frame pointer logic for vmx

[Sean Christopherson]

+cc Josh

Josh,

The issue we're encountering is that vmx_vcpu_run() is marked with
STACK_FRAME_NON_STANDARD because %rbp is saved/modified/restored when
transitioning to/from the guest.  But vmx_vcpu_run() also happens to
be a giant blob of code that gcc will sometime split into separate
.part.* functions, which means that objtool's symbol lookup to whitelist
the function doesn't work.

On Tue, Jan 15, 2019 at 11:43:20AM -0500, Qian Cai wrote:
> 
> 
> On 1/15/19 2:13 AM, Paolo Bonzini wrote:
> > Hmm, maybe like this:
> > 
> > diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> > index bcef2c7e9bc4..33122fa9d4bd 100644
> > --- a/arch/x86/kvm/vmx/vmenter.S
> > +++ b/arch/x86/kvm/vmx/vmenter.S
> > @@ -26,19 +26,17 @@ ENTRY(vmx_vmenter)
> >  	ret
> > 
> >  2:	vmlaunch
> > +3:
> >  	ret
> > 
> > -3:	cmpb $0, kvm_rebooting
> > -	jne 4f
> > -	call kvm_spurious_fault
> > -4:	ret
> > -
> >  	.pushsection .fixup, "ax"
> > -5:	jmp 3b
> > +4:	cmpb $0, kvm_rebooting
> > +	jne 3b
> > +	jmp kvm_spurious_fault
> >  	.popsection
> > 
> > -	_ASM_EXTABLE(1b, 5b)
> > -	_ASM_EXTABLE(2b, 5b)
> > +	_ASM_EXTABLE(1b, 4b)
> > +	_ASM_EXTABLE(2b, 4b)
> > 
> >  ENDPROC(vmx_vmenter)
> 
> No, that will not work. The problem is in vmx.o where I just sent another patch
> for it.
> 
> I can see there are five options to solve it.
> 
> 1) always inline vmx_vcpu_run()
> 2) always noinline vmx_vcpu_run()
> 3) add -fdiable-ipa-fnsplit option to Makefile for vmx.o
> 4) let STACK_FRAME_NON_STANDARD support part.* syntax.
> 5) trim-down vmx_vcpu_run() even more to not causing splitting by GCC.
> 
> Option 1) and 2) seems give away the decision for user with
> CONFIG_CC_OPTIMIZE_FOR_(PERFORMANCE/SIZE).
> 
> Option 3) prevents other functions there for splitting for optimization.
> 
> Option 4) and 5) seems tricky to implement.
> 
> I am not more leaning to 3) as only other fuction will miss splitting is
> vmx_segment_access_rights().

Option 4) is the most correct, but "tricky" is an understatement.  Unless
Josh is willing to pick up the task it'll likely have to wait.

There's actually a few more options:

 6) Replace "pop %rbp" in the vmx_vmenter() asm blob with an open-coded
    equivalent, e.g. "mov [%rsp], %rbp; add $8, %rsp".  This runs an end-
    around on objtool since objtool explicitly keys off "pop %rbp" and NOT
    "mov ..., %rbp" (which is probably an objtool checking flaw?").

 7) Move the vmx_vmenter() asm blob and a few other lines of code into a
    separate helper, e.g. __vmx_vcpu_run(), and mark that as having a
    non-standard stack frame.

Option 6) is an ugly (but amusing) hack.

Arguably we should do option 7) regardless of whether or not objtool gets
updated to support fnsplit behavior, as it it allows objtool to do proper
stack checking on the bulk of vmx_vcpu_run().  And it's a pretty trivial
change.

I'll send a patch for option 7), which in theory should fix the warning
for all .configs.

Re: [PATCH] kvm: add proper frame pointer logic for vmx

[Sean Christopherson]

+cc Josh, for real this time.

On Tue, Jan 15, 2019 at 11:06:17AM -0800, Sean Christopherson wrote:
> +cc Josh
> 
> Josh,
> 
> The issue we're encountering is that vmx_vcpu_run() is marked with
> STACK_FRAME_NON_STANDARD because %rbp is saved/modified/restored when
> transitioning to/from the guest.  But vmx_vcpu_run() also happens to
> be a giant blob of code that gcc will sometime split into separate
> .part.* functions, which means that objtool's symbol lookup to whitelist
> the function doesn't work.
> 
> On Tue, Jan 15, 2019 at 11:43:20AM -0500, Qian Cai wrote:
> > 
> > 
> > On 1/15/19 2:13 AM, Paolo Bonzini wrote:
> > > Hmm, maybe like this:
> > > 
> > > diff --git a/arch/x86/kvm/vmx/vmenter.S b/arch/x86/kvm/vmx/vmenter.S
> > > index bcef2c7e9bc4..33122fa9d4bd 100644
> > > --- a/arch/x86/kvm/vmx/vmenter.S
> > > +++ b/arch/x86/kvm/vmx/vmenter.S
> > > @@ -26,19 +26,17 @@ ENTRY(vmx_vmenter)
> > >  	ret
> > > 
> > >  2:	vmlaunch
> > > +3:
> > >  	ret
> > > 
> > > -3:	cmpb $0, kvm_rebooting
> > > -	jne 4f
> > > -	call kvm_spurious_fault
> > > -4:	ret
> > > -
> > >  	.pushsection .fixup, "ax"
> > > -5:	jmp 3b
> > > +4:	cmpb $0, kvm_rebooting
> > > +	jne 3b
> > > +	jmp kvm_spurious_fault
> > >  	.popsection
> > > 
> > > -	_ASM_EXTABLE(1b, 5b)
> > > -	_ASM_EXTABLE(2b, 5b)
> > > +	_ASM_EXTABLE(1b, 4b)
> > > +	_ASM_EXTABLE(2b, 4b)
> > > 
> > >  ENDPROC(vmx_vmenter)
> > 
> > No, that will not work. The problem is in vmx.o where I just sent another patch
> > for it.
> > 
> > I can see there are five options to solve it.
> > 
> > 1) always inline vmx_vcpu_run()
> > 2) always noinline vmx_vcpu_run()
> > 3) add -fdiable-ipa-fnsplit option to Makefile for vmx.o
> > 4) let STACK_FRAME_NON_STANDARD support part.* syntax.
> > 5) trim-down vmx_vcpu_run() even more to not causing splitting by GCC.
> > 
> > Option 1) and 2) seems give away the decision for user with
> > CONFIG_CC_OPTIMIZE_FOR_(PERFORMANCE/SIZE).
> > 
> > Option 3) prevents other functions there for splitting for optimization.
> > 
> > Option 4) and 5) seems tricky to implement.
> > 
> > I am not more leaning to 3) as only other fuction will miss splitting is
> > vmx_segment_access_rights().
> 
> Option 4) is the most correct, but "tricky" is an understatement.  Unless
> Josh is willing to pick up the task it'll likely have to wait.
> 
> There's actually a few more options:
> 
>  6) Replace "pop %rbp" in the vmx_vmenter() asm blob with an open-coded
>     equivalent, e.g. "mov [%rsp], %rbp; add $8, %rsp".  This runs an end-
>     around on objtool since objtool explicitly keys off "pop %rbp" and NOT
>     "mov ..., %rbp" (which is probably an objtool checking flaw?").
> 
>  7) Move the vmx_vmenter() asm blob and a few other lines of code into a
>     separate helper, e.g. __vmx_vcpu_run(), and mark that as having a
>     non-standard stack frame.
> 
> Option 6) is an ugly (but amusing) hack.
> 
> Arguably we should do option 7) regardless of whether or not objtool gets
> updated to support fnsplit behavior, as it it allows objtool to do proper
> stack checking on the bulk of vmx_vcpu_run().  And it's a pretty trivial
> change.
> 
> I'll send a patch for option 7), which in theory should fix the warning
> for all .configs.

Re: [PATCH] kvm: add proper frame pointer logic for vmx

[Josh Poimboeuf]

On Tue, Jan 15, 2019 at 11:06:17AM -0800, Sean Christopherson wrote:
> > I can see there are five options to solve it.
> > 
> > 1) always inline vmx_vcpu_run()
> > 2) always noinline vmx_vcpu_run()
> > 3) add -fdiable-ipa-fnsplit option to Makefile for vmx.o
> > 4) let STACK_FRAME_NON_STANDARD support part.* syntax.
> > 5) trim-down vmx_vcpu_run() even more to not causing splitting by GCC.
> > 
> > Option 1) and 2) seems give away the decision for user with
> > CONFIG_CC_OPTIMIZE_FOR_(PERFORMANCE/SIZE).
> > 
> > Option 3) prevents other functions there for splitting for optimization.
> > 
> > Option 4) and 5) seems tricky to implement.
> > 
> > I am not more leaning to 3) as only other fuction will miss splitting is
> > vmx_segment_access_rights().
> 
> Option 4) is the most correct, but "tricky" is an understatement.  Unless
> Josh is willing to pick up the task it'll likely have to wait.
> 
> There's actually a few more options:
> 
>  6) Replace "pop %rbp" in the vmx_vmenter() asm blob with an open-coded
>     equivalent, e.g. "mov [%rsp], %rbp; add $8, %rsp".  This runs an end-
>     around on objtool since objtool explicitly keys off "pop %rbp" and NOT
>     "mov ..., %rbp" (which is probably an objtool checking flaw?").
> 
>  7) Move the vmx_vmenter() asm blob and a few other lines of code into a
>     separate helper, e.g. __vmx_vcpu_run(), and mark that as having a
>     non-standard stack frame.

Do you mean moving the asm blob to a .S file instead of inline asm?  If
so, I think that's definitely a good idea.  It would be a nice cleanup,
regardless of the objtool false positive.

That would allow vmx_vcpu_run() to be a "normal" C function which
objtool can validate (and also create ORC data for).  It would also
prevent future nasty GCC optimizations (which was why the __noclone was
needed in the first place).

And also, I *think* objtool would no longer warn in that case, because
there would no longer be any calls in the function after popping %rbp.
Though if I'm wrong about that, I'd be glad to help fix the warning one
way or another.

-- 
Josh

Re: [PATCH] kvm: add proper frame pointer logic for vmx

[Sean Christopherson]

On Tue, Jan 15, 2019 at 04:38:49PM -0600, Josh Poimboeuf wrote:
> On Tue, Jan 15, 2019 at 11:06:17AM -0800, Sean Christopherson wrote:
> > > I can see there are five options to solve it.
> > > 
> > > 1) always inline vmx_vcpu_run()
> > > 2) always noinline vmx_vcpu_run()
> > > 3) add -fdiable-ipa-fnsplit option to Makefile for vmx.o
> > > 4) let STACK_FRAME_NON_STANDARD support part.* syntax.
> > > 5) trim-down vmx_vcpu_run() even more to not causing splitting by GCC.
> > > 
> > > Option 1) and 2) seems give away the decision for user with
> > > CONFIG_CC_OPTIMIZE_FOR_(PERFORMANCE/SIZE).
> > > 
> > > Option 3) prevents other functions there for splitting for optimization.
> > > 
> > > Option 4) and 5) seems tricky to implement.
> > > 
> > > I am not more leaning to 3) as only other fuction will miss splitting is
> > > vmx_segment_access_rights().
> > 
> > Option 4) is the most correct, but "tricky" is an understatement.  Unless
> > Josh is willing to pick up the task it'll likely have to wait.
> > 
> > There's actually a few more options:
> > 
> >  6) Replace "pop %rbp" in the vmx_vmenter() asm blob with an open-coded
> >     equivalent, e.g. "mov [%rsp], %rbp; add $8, %rsp".  This runs an end-
> >     around on objtool since objtool explicitly keys off "pop %rbp" and NOT
> >     "mov ..., %rbp" (which is probably an objtool checking flaw?").
> > 
> >  7) Move the vmx_vmenter() asm blob and a few other lines of code into a
> >     separate helper, e.g. __vmx_vcpu_run(), and mark that as having a
> >     non-standard stack frame.
> 
> Do you mean moving the asm blob to a .S file instead of inline asm?  If
> so, I think that's definitely a good idea.  It would be a nice cleanup,
> regardless of the objtool false positive.

No, just moving the inline asm to a separate function.  Moving the entire
inline asm blob is annoying because it references a large number of struct
offsets and doesn't solve the fundamental problem (more on this later).

The VMLAUNCH and VMRESUME invocations themselves, i.e. the really nasty
bits, have already been moved to a .S file (by the commit that exposed
this warning).  That approach eliminates the worst of the conflicts with
compiler optimizations without having to deal with exposing the struct
offsets to asm.

> That would allow vmx_vcpu_run() to be a "normal" C function which
> objtool can validate (and also create ORC data for).  It would also
> prevent future nasty GCC optimizations (which was why the __noclone was
> needed in the first place).

Moving the inline asm to a separate function (on top of having a separate
.S file for VMLAUNCH/VMRESUME) accomplishes sort of the same thing, i.e.
vmx_vcpu_run() gets to be a normal function.  It's not as bulletproof
from a tooling perspective, but odds are pretty good that all usage of
STACK_FRAME_NON_STANDARD will break if the compiler manages to muck up
the inline asm wrapper function.

And having a dedicated function for VMLAUNCH/VMRESUME is actually nice
from a stack trace perspective.  The transition to/from the guest is by
far the most likely source of faults, i.e. a stack trace that originates
in vmx_vmenter() all but guarantees that a fault occurred on VM-Enter or
immediately after VM-Exit.

> And also, I *think* objtool would no longer warn in that case, because
> there would no longer be any calls in the function after popping %rbp.
> Though if I'm wrong about that, I'd be glad to help fix the warning one
> way or another.

In the vmx_vcpu_run() case, warning on calls after "pop %rbp" is actually
a false positive.  The POP restores the host's RBP, i.e. the stack frame,
meaning all calls after the POP are ok.  The window where stack traces
will go awry is between loading RBP with the guest's value and the POP to
restore the host's stack frame, i.e. in this case "mov <guest_rbp>, %rbp"
should trigger a warning irrespective of any calls.

I'm not saying it's actually worth updating objtool, rather that "fixing"
the KVM issue by moving the inline asm to a dedicated .S file doesn't
solve the fundamental problem that VM-Enter/VM-Exit needs to temporarily
corrupt RBP.

Re: [PATCH] kvm: add proper frame pointer logic for vmx

[Josh Poimboeuf]

On Tue, Jan 15, 2019 at 04:54:38PM -0800, Sean Christopherson wrote:
> On Tue, Jan 15, 2019 at 04:38:49PM -0600, Josh Poimboeuf wrote:
> > On Tue, Jan 15, 2019 at 11:06:17AM -0800, Sean Christopherson wrote:
> > > > I can see there are five options to solve it.
> > > > 
> > > > 1) always inline vmx_vcpu_run()
> > > > 2) always noinline vmx_vcpu_run()
> > > > 3) add -fdiable-ipa-fnsplit option to Makefile for vmx.o
> > > > 4) let STACK_FRAME_NON_STANDARD support part.* syntax.
> > > > 5) trim-down vmx_vcpu_run() even more to not causing splitting by GCC.
> > > > 
> > > > Option 1) and 2) seems give away the decision for user with
> > > > CONFIG_CC_OPTIMIZE_FOR_(PERFORMANCE/SIZE).
> > > > 
> > > > Option 3) prevents other functions there for splitting for optimization.
> > > > 
> > > > Option 4) and 5) seems tricky to implement.
> > > > 
> > > > I am not more leaning to 3) as only other fuction will miss splitting is
> > > > vmx_segment_access_rights().
> > > 
> > > Option 4) is the most correct, but "tricky" is an understatement.  Unless
> > > Josh is willing to pick up the task it'll likely have to wait.
> > > 
> > > There's actually a few more options:
> > > 
> > >  6) Replace "pop %rbp" in the vmx_vmenter() asm blob with an open-coded
> > >     equivalent, e.g. "mov [%rsp], %rbp; add $8, %rsp".  This runs an end-
> > >     around on objtool since objtool explicitly keys off "pop %rbp" and NOT
> > >     "mov ..., %rbp" (which is probably an objtool checking flaw?").
> > > 
> > >  7) Move the vmx_vmenter() asm blob and a few other lines of code into a
> > >     separate helper, e.g. __vmx_vcpu_run(), and mark that as having a
> > >     non-standard stack frame.
> > 
> > Do you mean moving the asm blob to a .S file instead of inline asm?  If
> > so, I think that's definitely a good idea.  It would be a nice cleanup,
> > regardless of the objtool false positive.
> 
> No, just moving the inline asm to a separate function.  Moving the entire
> inline asm blob is annoying because it references a large number of struct
> offsets and doesn't solve the fundamental problem (more on this later).
> 
> The VMLAUNCH and VMRESUME invocations themselves, i.e. the really nasty
> bits, have already been moved to a .S file (by the commit that exposed
> this warning).  That approach eliminates the worst of the conflicts with
> compiler optimizations without having to deal with exposing the struct
> offsets to asm.

Exposing all the struct offsets isn't a big deal -- that's what
asm-offsets.c is for.

On the other hand, having such a large inline asm block is fragile and
unholy IMO.  I wouldn't be surprised if there are more GCC problems
lurking.

> > That would allow vmx_vcpu_run() to be a "normal" C function which
> > objtool can validate (and also create ORC data for).  It would also
> > prevent future nasty GCC optimizations (which was why the __noclone was
> > needed in the first place).
> 
> Moving the inline asm to a separate function (on top of having a separate
> .S file for VMLAUNCH/VMRESUME) accomplishes sort of the same thing, i.e.
> vmx_vcpu_run() gets to be a normal function.  It's not as bulletproof
> from a tooling perspective, but odds are pretty good that all usage of
> STACK_FRAME_NON_STANDARD will break if the compiler manages to muck up
> the inline asm wrapper function.

I wouldn't bet on that.  Many/most optimizations don't change the symbol
name, in which case STACK_FRAME_NON_STANDARD would work just fine.

> And having a dedicated function for VMLAUNCH/VMRESUME is actually nice
> from a stack trace perspective.  The transition to/from the guest is by
> far the most likely source of faults, i.e. a stack trace that originates
> in vmx_vmenter() all but guarantees that a fault occurred on VM-Enter or
> immediately after VM-Exit.

But moving __vmx_vcpu_run() to a proper asm function doesn't prevent
that.

BTW, do the stack traces from this path even work with the ORC unwinder?
Since objtool doesn't annotate vmx_vcpu_run() (or now __vmx_vcpu_run),
that should break stack tracing and instead produce a "guess" stack
trace (with the question marks), where it prints all text addresses it
finds on the stack, instead of doing a proper stack trace.

Which would be another reason to move this code to proper asm.

> > And also, I *think* objtool would no longer warn in that case, because
> > there would no longer be any calls in the function after popping %rbp.
> > Though if I'm wrong about that, I'd be glad to help fix the warning one
> > way or another.
> 
> In the vmx_vcpu_run() case, warning on calls after "pop %rbp" is actually
> a false positive.  The POP restores the host's RBP, i.e. the stack frame,
> meaning all calls after the POP are ok.  The window where stack traces
> will go awry is between loading RBP with the guest's value and the POP to
> restore the host's stack frame, i.e. in this case "mov <guest_rbp>, %rbp"
> should trigger a warning irrespective of any calls.
> 
> I'm not saying it's actually worth updating objtool, rather that "fixing"
> the KVM issue by moving the inline asm to a dedicated .S file doesn't
> solve the fundamental problem that VM-Enter/VM-Exit needs to temporarily
> corrupt RBP.

I agree the objtool warning was a false positive, but in many cases
these false positives end up pointing out some convoluted code which
really should be cleaned up anyway.  That's what I'm proposing here.
Moving the function to proper asm would be so much cleaner.

-- 
Josh

Re: [PATCH] kvm: add proper frame pointer logic for vmx

[Sean Christopherson]

On Wed, Jan 16, 2019 at 08:56:59AM -0600, Josh Poimboeuf wrote:
> On Tue, Jan 15, 2019 at 04:54:38PM -0800, Sean Christopherson wrote:
> > On Tue, Jan 15, 2019 at 04:38:49PM -0600, Josh Poimboeuf wrote:
> > > On Tue, Jan 15, 2019 at 11:06:17AM -0800, Sean Christopherson wrote:
> > > > > I can see there are five options to solve it.
> > > > > 
> > > > > 1) always inline vmx_vcpu_run()
> > > > > 2) always noinline vmx_vcpu_run()
> > > > > 3) add -fdiable-ipa-fnsplit option to Makefile for vmx.o
> > > > > 4) let STACK_FRAME_NON_STANDARD support part.* syntax.
> > > > > 5) trim-down vmx_vcpu_run() even more to not causing splitting by GCC.
> > > > > 
> > > > > Option 1) and 2) seems give away the decision for user with
> > > > > CONFIG_CC_OPTIMIZE_FOR_(PERFORMANCE/SIZE).
> > > > > 
> > > > > Option 3) prevents other functions there for splitting for optimization.
> > > > > 
> > > > > Option 4) and 5) seems tricky to implement.
> > > > > 
> > > > > I am not more leaning to 3) as only other fuction will miss splitting is
> > > > > vmx_segment_access_rights().
> > > > 
> > > > Option 4) is the most correct, but "tricky" is an understatement.  Unless
> > > > Josh is willing to pick up the task it'll likely have to wait.
> > > > 
> > > > There's actually a few more options:
> > > > 
> > > >  6) Replace "pop %rbp" in the vmx_vmenter() asm blob with an open-coded
> > > >     equivalent, e.g. "mov [%rsp], %rbp; add $8, %rsp".  This runs an end-
> > > >     around on objtool since objtool explicitly keys off "pop %rbp" and NOT
> > > >     "mov ..., %rbp" (which is probably an objtool checking flaw?").
> > > > 
> > > >  7) Move the vmx_vmenter() asm blob and a few other lines of code into a
> > > >     separate helper, e.g. __vmx_vcpu_run(), and mark that as having a
> > > >     non-standard stack frame.
> > > 
> > > Do you mean moving the asm blob to a .S file instead of inline asm?  If
> > > so, I think that's definitely a good idea.  It would be a nice cleanup,
> > > regardless of the objtool false positive.
> > 
> > No, just moving the inline asm to a separate function.  Moving the entire
> > inline asm blob is annoying because it references a large number of struct
> > offsets and doesn't solve the fundamental problem (more on this later).
> > 
> > The VMLAUNCH and VMRESUME invocations themselves, i.e. the really nasty
> > bits, have already been moved to a .S file (by the commit that exposed
> > this warning).  That approach eliminates the worst of the conflicts with
> > compiler optimizations without having to deal with exposing the struct
> > offsets to asm.
> 
> Exposing all the struct offsets isn't a big deal -- that's what
> asm-offsets.c is for.

The struct in question, vcpu_vmx, is "private" to the VMX code and KVM
can technically be compiled as an external module, i.e. the struct layout
could change without asm-offsets.c being recompiled.

I have an idea to avoid this hiccup, but even if it works the end result
may still be fairly ugly.  Regardless, moving the code to proper asm is
far too big of a change for v5.0.

> On the other hand, having such a large inline asm block is fragile and
> unholy IMO.  I wouldn't be surprised if there are more GCC problems
> lurking.

Unholy is a perfect description.
 
> > > That would allow vmx_vcpu_run() to be a "normal" C function which
> > > objtool can validate (and also create ORC data for).  It would also
> > > prevent future nasty GCC optimizations (which was why the __noclone was
> > > needed in the first place).
> > 
> > Moving the inline asm to a separate function (on top of having a separate
> > .S file for VMLAUNCH/VMRESUME) accomplishes sort of the same thing, i.e.
> > vmx_vcpu_run() gets to be a normal function.  It's not as bulletproof
> > from a tooling perspective, but odds are pretty good that all usage of
> > STACK_FRAME_NON_STANDARD will break if the compiler manages to muck up
> > the inline asm wrapper function.
> 
> I wouldn't bet on that.  Many/most optimizations don't change the symbol
> name, in which case STACK_FRAME_NON_STANDARD would work just fine.

By "muck up" I meant break STACK_FRAME_NON_STANDARD, i.e. if gcc breaks
objtool for __vmx_vcpu_run() then it'll likely break objtool across the
board.

> > And having a dedicated function for VMLAUNCH/VMRESUME is actually nice
> > from a stack trace perspective.  The transition to/from the guest is by
> > far the most likely source of faults, i.e. a stack trace that originates
> > in vmx_vmenter() all but guarantees that a fault occurred on VM-Enter or
> > immediately after VM-Exit.
> 
> But moving __vmx_vcpu_run() to a proper asm function doesn't prevent
> that.

Yeah, I was trying to say that the "improved debugability" motivation for
eliminating the inline asm is basically nullified by vmx_vmenter(), but
it came out a bit backwards.

> BTW, do the stack traces from this path even work with the ORC unwinder?
> Since objtool doesn't annotate vmx_vcpu_run() (or now __vmx_vcpu_run),
> that should break stack tracing and instead produce a "guess" stack
> trace (with the question marks), where it prints all text addresses it
> finds on the stack, instead of doing a proper stack trace.

It produce guess traces.

> Which would be another reason to move this code to proper asm.

Eh, not really.  In practice it doesn't matter because there is literally
a single path for reaching __vmx_vcpu_run().  And if this changes we'll
need to revisit the VM-Enter code because our VMCS.HOST_RSP optimizations
depend on host's %rsp being identical for every call (for a VM/process).

There is a second path for vmx_vmenter(), nested_vmx_check_vmentry_hw(),
but it also has a single invocation path and the unwinder gets us to the
caller of vmx_vmenter(), so again in practice not having a full stack
trace doesn't affect debugging.

> > > And also, I *think* objtool would no longer warn in that case, because
> > > there would no longer be any calls in the function after popping %rbp.
> > > Though if I'm wrong about that, I'd be glad to help fix the warning one
> > > way or another.
> > 
> > In the vmx_vcpu_run() case, warning on calls after "pop %rbp" is actually
> > a false positive.  The POP restores the host's RBP, i.e. the stack frame,
> > meaning all calls after the POP are ok.  The window where stack traces
> > will go awry is between loading RBP with the guest's value and the POP to
> > restore the host's stack frame, i.e. in this case "mov <guest_rbp>, %rbp"
> > should trigger a warning irrespective of any calls.
> > 
> > I'm not saying it's actually worth updating objtool, rather that "fixing"
> > the KVM issue by moving the inline asm to a dedicated .S file doesn't
> > solve the fundamental problem that VM-Enter/VM-Exit needs to temporarily
> > corrupt RBP.
> 
> I agree the objtool warning was a false positive, but in many cases
> these false positives end up pointing out some convoluted code which
> really should be cleaned up anyway.  That's what I'm proposing here.
> Moving the function to proper asm would be so much cleaner.

It'd definitely be prettier, but I think the low level transition code
will always be convoluted :)

All that being said, I do agree that eliminating the inline asm would be
a welcome change, just not for v5.0.  I'll play around with the code and
see what I can come up with.