| Message ID | 20211103183232.1213761-3-vipinsh@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add wrapper to read GPR of INVPCID, INVVPID, and INVEPT | expand |
On Wed, Nov 03, 2021, Vipin Sharma wrote: > This check will be done in switch statement of kvm_handle_invpcid(), Please make the changelog a stand on its own, i.e. don't rely on the shortlog for context. > used by both VMX and SVM. It also removes (type > 3) check. Use imperative mood, i.e. state what you're doing as a "command", don't refer to the patch from a third-person point of view. The changelog also needs to call out that, unlike INVVPID and INVEPT, INVPCID is not explicitly documented as checking the "type" before reading the operand from memory. I.e. there's a subtle, undocumented functional change in this patch. Something like: Handle #GP on INVCPID due to an invalid type in the common switch statement instead of relying on callers to manually verify the type is valid. Unlike INVVPID and INVPET, INVPCID is not explicitly documented as checking the type before reading the operand from memory, so deferring the type validity check until after that point is architecturally allowed. For the code: Reviewed-by: Sean Christopherson <seanjc@google.com>
On Wed, Nov 3, 2021 at 1:00 PM Sean Christopherson <seanjc@google.com> wrote: > > On Wed, Nov 03, 2021, Vipin Sharma wrote: > > This check will be done in switch statement of kvm_handle_invpcid(), > > Please make the changelog a stand on its own, i.e. don't rely on the shortlog > for context. > > > used by both VMX and SVM. It also removes (type > 3) check. > > Use imperative mood, i.e. state what you're doing as a "command", don't refer to > the patch from a third-person point of view. > > The changelog also needs to call out that, unlike INVVPID and INVEPT, INVPCID is > not explicitly documented as checking the "type" before reading the operand from > memory. I.e. there's a subtle, undocumented functional change in this patch. > > Something like: > > Handle #GP on INVCPID due to an invalid type in the common switch statement > instead of relying on callers to manually verify the type is valid. Unlike > INVVPID and INVPET, INVPCID is not explicitly documented as checking the type > before reading the operand from memory, so deferring the type validity check > until after that point is architecturally allowed. > Thanks. I will update it and send out v3. > For the code: > > Reviewed-by: Sean Christopherson <seanjc@google.com>
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index 21bb81710e0f..ccbf96876ec6 100644 --- a/arch/x86/kvm/svm/svm.c +++ b/arch/x86/kvm/svm/svm.c @@ -3119,11 +3119,6 @@ static int invpcid_interception(struct kvm_vcpu *vcpu) type = svm->vmcb->control.exit_info_2; gva = svm->vmcb->control.exit_info_1; - if (type > 3) { - kvm_inject_gp(vcpu, 0); - return 1; - } - return kvm_handle_invpcid(vcpu, type, gva); } diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index e41d207e3298..a3bb9854f4d2 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -5505,11 +5505,6 @@ static int handle_invpcid(struct kvm_vcpu *vcpu) gpr_index = vmx_get_instr_info_reg2(vmx_instruction_info); type = kvm_register_read(vcpu, gpr_index); - if (type > 3) { - kvm_inject_gp(vcpu, 0); - return 1; - } - /* According to the Intel instruction reference, the memory operand * is read even if it isn't needed (e.g., for type==all) */ diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index ac83d873d65b..134585027e92 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -12443,7 +12443,8 @@ int kvm_handle_invpcid(struct kvm_vcpu *vcpu, unsigned long type, gva_t gva) return kvm_skip_emulated_instruction(vcpu); default: - BUG(); /* We have already checked above that type <= 3 */ + kvm_inject_gp(vcpu, 0); + return 1; } } EXPORT_SYMBOL_GPL(kvm_handle_invpcid);
This check will be done in switch statement of kvm_handle_invpcid(), used by both VMX and SVM. It also removes (type > 3) check. Signed-off-by: Vipin Sharma <vipinsh@google.com> --- arch/x86/kvm/svm/svm.c | 5 ----- arch/x86/kvm/vmx/vmx.c | 5 ----- arch/x86/kvm/x86.c | 3 ++- 3 files changed, 2 insertions(+), 11 deletions(-)