linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Paolo Bonzini <pbonzini@redhat.com>
To: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: x86@kernel.org, linux-kernel@vger.kernel.org,
	"Peter Zijlstra" <peterz@infradead.org>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Nick Desaulniers" <ndesaulniers@google.com>,
	"Arnd Bergmann" <arnd@arndb.de>, "Jann Horn" <jannh@google.com>,
	"Randy Dunlap" <rdunlap@infradead.org>,
	"Radim Krčmář" <rkrcmar@redhat.com>
Subject: Re: [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter()
Date: Mon, 15 Jul 2019 15:03:23 +0200	[thread overview]
Message-ID: <2172ac52-899b-a32a-cba7-c2e5f2bb784e@redhat.com> (raw)
In-Reply-To: <20190715123704.oke4pd4wguj5a7i3@treble>

On 15/07/19 14:37, Josh Poimboeuf wrote:
> On Mon, Jul 15, 2019 at 11:04:03AM +0200, Paolo Bonzini wrote:
>> On 15/07/19 02:36, Josh Poimboeuf wrote:
>>> With CONFIG_FRAME_POINTER, vmx_vmenter() needs to do frame pointer setup
>>> before calling kvm_spurious_fault().
>>>
>>> Fixes the following warning:
>>>
>>>   arch/x86/kvm/vmx/vmenter.o: warning: objtool: vmx_vmenter()+0x14: call without frame pointer save/setup
>>>
>>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>>
>> This is not enough, because the RSP value must match what is computed at
>> this place:
>>
>>         /* Adjust RSP to account for the CALL to vmx_vmenter(). */
>>         lea -WORD_SIZE(%_ASM_SP), %_ASM_ARG2
>>         call vmx_update_host_rsp
> 
> Ah, that is surprising :-)
> 
> And then there's this, which overwrites the frame pointer anyway:
> 
> 	mov VCPU_RBP(%_ASM_AX), %_ASM_BP
> 
> Would it make sense to remove the call to vmx_vmenter() altogether, and
> just either embed it in __vmx_vcpu_run(), or jmp back and forth to it
> from __vmx_vcpu_run()?

Unfortunately there's another use of it in nested_vmx_check_vmentry_hw.

The problem is that storing RSP (no matter if adjusted or not) needs a
scratch register.  And vmx_vmenter is exactly the part of the vmentry
that doesn't have any scratch registers available.

Therefore, you must arrange for the caller to store RSP.  You cannot for
example remove it from __vmx_vcpu_run and nested_vmx_check_vmentry_hw in
favor of vmx_vmenter. :(

>> Is this important since kvm_spurious_fault is just BUG()?
> 
> It's probably only important if you care about the stack trace for the
> BUG() case.  But BP is clobbered anyway so I guess it doesn't matter.

Yes, BP is the guest RBP at this point of the code.

Paolo

>> There is no macro currently to support CONFIG_DEBUG_BUGVERBOSE in
>> assembly code, but it's also fine if you just change the call to ud2.
> 
> That would be one way to make objtool happy.
> 


  reply	other threads:[~2019-07-15 13:03 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-15  0:36 [PATCH 00/22] x86, objtool: several fixes/improvements Josh Poimboeuf
2019-07-15  0:36 ` [PATCH 01/22] x86/paravirt: Fix callee-saved function ELF sizes Josh Poimboeuf
2019-07-15  4:58   ` Juergen Gross
2019-07-15 12:43     ` Josh Poimboeuf
2019-07-15  0:36 ` [PATCH 02/22] x86/kvm: Fix fastop function ELF metadata Josh Poimboeuf
2019-07-15  9:05   ` Paolo Bonzini
2019-07-15  0:36 ` [PATCH 03/22] x86/kvm: Fix frame pointer usage in vmx_vmenter() Josh Poimboeuf
2019-07-15  9:04   ` Paolo Bonzini
2019-07-15 12:37     ` Josh Poimboeuf
2019-07-15 13:03       ` Paolo Bonzini [this message]
2019-07-15 13:35         ` Josh Poimboeuf
2019-07-15 18:17           ` Paolo Bonzini
2019-07-15  0:36 ` [PATCH 04/22] x86/kvm: Don't call kvm_spurious_fault() from .fixup Josh Poimboeuf
2019-07-15  9:07   ` Paolo Bonzini
2019-07-15 12:40     ` Josh Poimboeuf
2019-07-15 13:05       ` Paolo Bonzini
2019-07-15 13:25         ` Josh Poimboeuf
2019-07-15 18:16           ` Paolo Bonzini
2019-07-15  0:37 ` [PATCH 05/22] x86/entry: Fix thunk function ELF sizes Josh Poimboeuf
2019-07-15  0:37 ` [PATCH 06/22] x86/head/64: Annotate start_cpu0() as non-callable Josh Poimboeuf
2019-07-15  0:37 ` [PATCH 07/22] x86/uaccess: Remove ELF function annotation from copy_user_handle_tail() Josh Poimboeuf
2019-07-16 18:16   ` Nick Desaulniers
2019-07-16 18:51     ` Peter Zijlstra
2019-07-15  0:37 ` [PATCH 08/22] x86/uaccess: Don't leak AC flag into fentry from mcsafe_handle_tail() Josh Poimboeuf
2019-07-15  0:37 ` [PATCH 09/22] x86/uaccess: Remove redundant CLACs in getuser/putuser error paths Josh Poimboeuf
2019-07-15  0:37 ` [PATCH 10/22] bpf: Disable GCC -fgcse optimization for ___bpf_prog_run() Josh Poimboeuf
2019-07-16 18:15   ` Nick Desaulniers
2019-07-16 23:02     ` Josh Poimboeuf
2019-07-15  0:37 ` [PATCH 11/22] objtool: Add mcsafe_handle_tail() to the uaccess safe list Josh Poimboeuf
2019-07-15  0:37 ` [PATCH 12/22] objtool: Track original function across branches Josh Poimboeuf
2019-07-15  0:37 ` [PATCH 13/22] objtool: Refactor function alias logic Josh Poimboeuf
2019-07-15  0:37 ` [PATCH 14/22] objtool: Warn on zero-length functions Josh Poimboeuf
2019-07-15  0:37 ` [PATCH 15/22] objtool: Change dead_end_function() to return boolean Josh Poimboeuf
2019-07-15  0:37 ` [PATCH 16/22] objtool: Do frame pointer check before dead end check Josh Poimboeuf
2019-07-15  0:37 ` [PATCH 17/22] objtool: Refactor sibling call detection logic Josh Poimboeuf
2019-07-15  0:37 ` [PATCH 18/22] objtool: Refactor jump table code Josh Poimboeuf
2019-07-15  9:38   ` Peter Zijlstra
2019-07-15  0:37 ` [PATCH 19/22] objtool: Support repeated uses of the same C jump table Josh Poimboeuf
2019-07-15  0:37 ` [PATCH 20/22] objtool: Fix seg fault on bad switch table entry Josh Poimboeuf
2019-07-15 17:24   ` Nick Desaulniers
2019-07-15 17:29     ` Josh Poimboeuf
2019-07-18 23:02       ` Nick Desaulniers
2019-07-15  0:37 ` [PATCH 21/22] objtool: convert insn type to enum Josh Poimboeuf
2019-07-15  0:37 ` [PATCH 22/22] objtool: Support conditional retpolines Josh Poimboeuf
2019-07-15  9:52 ` [PATCH 00/22] x86, objtool: several fixes/improvements Peter Zijlstra
2019-07-15 19:38 ` Josh Poimboeuf
2019-07-15 21:45   ` Nick Desaulniers
2019-07-16 23:17     ` Josh Poimboeuf
2019-07-18 22:26       ` Nick Desaulniers
2019-09-27 20:24         ` Nick Desaulniers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2172ac52-899b-a32a-cba7-c2e5f2bb784e@redhat.com \
    --to=pbonzini@redhat.com \
    --cc=arnd@arndb.de \
    --cc=jannh@google.com \
    --cc=jpoimboe@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ndesaulniers@google.com \
    --cc=peterz@infradead.org \
    --cc=rdunlap@infradead.org \
    --cc=rkrcmar@redhat.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).