From: Sean Christopherson <seanjc@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [PATCH 2/3] KVM: x86: move kvm_inject_gp up from kvm_handle_invpcid to callers
Date: Tue, 2 Feb 2021 09:38:26 -0800 [thread overview]
Message-ID: <YBmOEt6YezYWtTjQ@google.com> (raw)
In-Reply-To: <20210202165141.88275-3-pbonzini@redhat.com>
On Tue, Feb 02, 2021, Paolo Bonzini wrote:
> Push the injection of #GP up to the callers, so that they can just use
> kvm_complete_insn_gp.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
...
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 08568c47337c..edbeb162012b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11375,7 +11375,6 @@ int kvm_handle_invpcid(struct kvm_vcpu *vcpu, unsigned long type, gva_t gva)
> return kvm_handle_memory_failure(vcpu, r, &e);
This is broken. kvm_handle_memory_failure() injects a #PF and returns 1, which
means an error here will trigger #PF->#GP->#DF.
>
> if (operand.pcid >> 12 != 0) {
> - kvm_inject_gp(vcpu, 0);
> return 1;
> }
Curly braces can be dropped.
> @@ -11385,15 +11384,13 @@ int kvm_handle_invpcid(struct kvm_vcpu *vcpu, unsigned long type, gva_t gva)
> case INVPCID_TYPE_INDIV_ADDR:
> if ((!pcid_enabled && (operand.pcid != 0)) ||
> is_noncanonical_address(operand.gla, vcpu)) {
> - kvm_inject_gp(vcpu, 0);
> return 1;
> }
> kvm_mmu_invpcid_gva(vcpu, operand.gla, operand.pcid);
> - return kvm_skip_emulated_instruction(vcpu);
> + return 0;
>
> case INVPCID_TYPE_SINGLE_CTXT:
> if (!pcid_enabled && (operand.pcid != 0)) {
> - kvm_inject_gp(vcpu, 0);
> return 1;
> }
Same here.
>
> @@ -11414,7 +11411,7 @@ int kvm_handle_invpcid(struct kvm_vcpu *vcpu, unsigned long type, gva_t gva)
> * resync will happen anyway before switching to any other CR3.
> */
>
> - return kvm_skip_emulated_instruction(vcpu);
> + return 0;
>
> case INVPCID_TYPE_ALL_NON_GLOBAL:
> /*
> @@ -11427,7 +11424,7 @@ int kvm_handle_invpcid(struct kvm_vcpu *vcpu, unsigned long type, gva_t gva)
> fallthrough;
> case INVPCID_TYPE_ALL_INCL_GLOBAL:
> kvm_mmu_unload(vcpu);
> - return kvm_skip_emulated_instruction(vcpu);
> + return 0;
>
> default:
> BUG(); /* We have already checked above that type <= 3 */
> --
> 2.26.2
IMO, this isn't an improvement. For flows that can't easily be consolidated to
x86.c, e.g. CRs (and DRs?), I agree it makes sense to use kvm_complete_insn_gp(),
but this feels forced.
What about a pure refactoring of kvm_handle_invpcid() to get a similar effect?
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ef630f8d8bd2..b9f57f9f2422 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11363,28 +11363,23 @@ int kvm_handle_invpcid(struct kvm_vcpu *vcpu, unsigned long type, gva_t gva)
if (r != X86EMUL_CONTINUE)
return kvm_handle_memory_failure(vcpu, r, &e);
- if (operand.pcid >> 12 != 0) {
- kvm_inject_gp(vcpu, 0);
- return 1;
- }
+ if (operand.pcid >> 12 != 0)
+ goto inject_gp;
pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE);
switch (type) {
case INVPCID_TYPE_INDIV_ADDR:
if ((!pcid_enabled && (operand.pcid != 0)) ||
- is_noncanonical_address(operand.gla, vcpu)) {
- kvm_inject_gp(vcpu, 0);
- return 1;
- }
+ is_noncanonical_address(operand.gla, vcpu))
+ goto inject_gp;
+
kvm_mmu_invpcid_gva(vcpu, operand.gla, operand.pcid);
- return kvm_skip_emulated_instruction(vcpu);
+ break;
case INVPCID_TYPE_SINGLE_CTXT:
- if (!pcid_enabled && (operand.pcid != 0)) {
- kvm_inject_gp(vcpu, 0);
- return 1;
- }
+ if (!pcid_enabled && (operand.pcid != 0))
+ goto inject_gp;
if (kvm_get_active_pcid(vcpu) == operand.pcid) {
kvm_mmu_sync_roots(vcpu);
@@ -11402,8 +11397,7 @@ int kvm_handle_invpcid(struct kvm_vcpu *vcpu, unsigned long type, gva_t gva)
* given PCID, then nothing needs to be done here because a
* resync will happen anyway before switching to any other CR3.
*/
-
- return kvm_skip_emulated_instruction(vcpu);
+ break;
case INVPCID_TYPE_ALL_NON_GLOBAL:
/*
@@ -11416,11 +11410,17 @@ int kvm_handle_invpcid(struct kvm_vcpu *vcpu, unsigned long type, gva_t gva)
fallthrough;
case INVPCID_TYPE_ALL_INCL_GLOBAL:
kvm_mmu_unload(vcpu);
- return kvm_skip_emulated_instruction(vcpu);
+ break;
default:
BUG(); /* We have already checked above that type <= 3 */
}
+
+ return kvm_skip_emulated_instruction(vcpu);
+
+inject_gp:
+ kvm_inject_gp(vcpu, 0);
+ return 1;
}
EXPORT_SYMBOL_GPL(kvm_handle_invpcid);
next prev parent reply other threads:[~2021-02-02 17:42 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-02 16:51 [PATCH 0/3] use kvm_complete_insn_gp more Paolo Bonzini
2021-02-02 16:51 ` [PATCH 1/3] KVM: x86: move kvm_inject_gp up from kvm_set_xcr to callers Paolo Bonzini
2021-02-02 17:19 ` Sean Christopherson
2021-02-02 17:24 ` Paolo Bonzini
2021-02-02 16:51 ` [PATCH 2/3] KVM: x86: move kvm_inject_gp up from kvm_handle_invpcid " Paolo Bonzini
2021-02-02 17:38 ` Sean Christopherson [this message]
2021-02-02 17:44 ` Paolo Bonzini
2021-02-02 16:51 ` [PATCH 3/3] KVM: x86: move kvm_inject_gp up from kvm_set_dr " Paolo Bonzini
2021-02-02 18:17 ` Sean Christopherson
2021-02-03 9:24 ` Paolo Bonzini
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=YBmOEt6YezYWtTjQ@google.com \
--to=seanjc@google.com \
--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).