From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Julian Stecklina <jsteckli@amazon.de>
Cc: kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
Julian Stecklina <js@alien8.de>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/3] kvm, vmx: move CR2 context switch out of assembly path
Date: Mon, 29 Oct 2018 10:14:50 -0700 [thread overview]
Message-ID: <20181029171448.GA27762@linux.intel.com> (raw)
In-Reply-To: <74fa3809ea293cc05d37b1449b16e08480c4ddbd.1540822350.git.jsteckli@amazon.de>
On Mon, Oct 29, 2018 at 04:40:42PM +0100, Julian Stecklina wrote:
> The VM entry/exit path is a giant inline assembly statement. Simplify it
> by doing CR2 context switching in plain C. Move CR2 restore behind IBRS
> clearing, so we reduce the amount of code we execute with IBRS on.
I think it's worth documenting two things in the changelog:
- Using {read,write}_cr2() means KVM will use pv_mmu_ops instead of
open coding native_{read,write}_cr2().
- The CR2 code has been done in assembly since KVM's genesis[1],
which predates the addition of the paravirt ops[2], i.e. KVM isn't
deliberately avoiding the paravirt ops.
The above info makes it trivially easy to review this patch from a
correctness standpoint. With that:
Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
[1] Commit 6aa8b732ca01 ("[PATCH] kvm: userspace interface")
[2] Commit d3561b7fa0fb ("[PATCH] paravirt: header and stubs for paravirtualisation")
> Signed-off-by: Julian Stecklina <jsteckli@amazon.de>
> Reviewed-by: Jan H. Schönherr <jschoenh@amazon.de>
> Reviewed-by: Konrad Jan Miller <kjm@amazon.de>
> Reviewed-by: Jim Mattson <jmattson@google.com>
> ---
> arch/x86/kvm/vmx.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index ccc6a01..a6e5a5c 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11212,6 +11212,9 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
> evmcs_rsp = static_branch_unlikely(&enable_evmcs) ?
> (unsigned long)¤t_evmcs->host_rsp : 0;
>
> + if (read_cr2() != vcpu->arch.cr2)
> + write_cr2(vcpu->arch.cr2);
> +
> if (static_branch_unlikely(&vmx_l1d_should_flush))
> vmx_l1d_flush(vcpu);
>
> @@ -11231,13 +11234,6 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
> "2: \n\t"
> __ex("vmwrite %%" _ASM_SP ", %%" _ASM_DX) "\n\t"
> "1: \n\t"
> - /* Reload cr2 if changed */
> - "mov %c[cr2](%0), %%" _ASM_AX " \n\t"
> - "mov %%cr2, %%" _ASM_DX " \n\t"
> - "cmp %%" _ASM_AX ", %%" _ASM_DX " \n\t"
> - "je 3f \n\t"
> - "mov %%" _ASM_AX", %%cr2 \n\t"
> - "3: \n\t"
> /* Check if vmlaunch of vmresume is needed */
> "cmpl $0, %c[launched](%0) \n\t"
> /* Load guest registers. Don't clobber flags. */
> @@ -11298,8 +11294,6 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
> "xor %%r14d, %%r14d \n\t"
> "xor %%r15d, %%r15d \n\t"
> #endif
> - "mov %%cr2, %%" _ASM_AX " \n\t"
> - "mov %%" _ASM_AX ", %c[cr2](%0) \n\t"
>
> "xor %%eax, %%eax \n\t"
> "xor %%ebx, %%ebx \n\t"
> @@ -11331,7 +11325,6 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
> [r14]"i"(offsetof(struct vcpu_vmx, vcpu.arch.regs[VCPU_REGS_R14])),
> [r15]"i"(offsetof(struct vcpu_vmx, vcpu.arch.regs[VCPU_REGS_R15])),
> #endif
> - [cr2]"i"(offsetof(struct vcpu_vmx, vcpu.arch.cr2)),
> [wordsize]"i"(sizeof(ulong))
> : "cc", "memory"
> #ifdef CONFIG_X86_64
> @@ -11365,6 +11358,8 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
> /* Eliminate branch target predictions from guest mode */
> vmexit_fill_RSB();
>
> + vcpu->arch.cr2 = read_cr2();
> +
> /* All fields are clean at this point */
> if (static_branch_unlikely(&enable_evmcs))
> current_evmcs->hv_clean_fields |=
> --
> 2.7.4
>
prev parent reply other threads:[~2018-10-29 17:14 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-29 15:40 [PATCH v2 1/3] kvm, vmx: move CR2 context switch out of assembly path Julian Stecklina
2018-10-29 15:40 ` [PATCH v2 2/3] kvm, vmx: move register clearing " Julian Stecklina
2018-10-29 17:26 ` Sean Christopherson
2018-11-01 13:40 ` Stecklina, Julian
2018-10-29 15:40 ` [PATCH v2 3/3] kvm, vmx: fix __invvpid style Julian Stecklina
2018-10-29 17:14 ` Sean Christopherson [this message]
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=20181029171448.GA27762@linux.intel.com \
--to=sean.j.christopherson@intel.com \
--cc=js@alien8.de \
--cc=jsteckli@amazon.de \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pbonzini@redhat.com \
/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).