| Message ID | 20211103205911.1253463-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: > Handle #GP on INVPCID due to an invalid type in the common switch > statement instead of relying on the callers (VMX and SVM) to manually > validate the type. > > Unlike INVVPID and INVEPT, INVPCID is not explicitly documented to check > the type before reading the operand from memory, so deferring the > type validity check until after that point is architecturally allowed. > > Signed-off-by: Vipin Sharma <vipinsh@google.com> > --- For future reference, a R-b that comes with qualifiers can be carried so long as the issues raised by the reviewer are addressed. Obviously it can be somewhat subjective, but common sense usually goes a long ways, and most reviewers won't be too grumpy about mistakes so long as you had good intentions and remedy any mistakes. And if you're in doubt, you can always add a blurb in the cover letter or ignored part of the patch to explicitly confirm that it was ok to add the tag, e.g. "Sean, I added your Reviewed-by in patch 02 after fixing the changelog, let me know if that's not what you intended". Thanks! Reviewed-by: Sean Christopherson <seanjc@google.com>
On Wed, Nov 3, 2021 at 4:20 PM Sean Christopherson <seanjc@google.com> wrote: > > On Wed, Nov 03, 2021, Vipin Sharma wrote: > > Handle #GP on INVPCID due to an invalid type in the common switch > > statement instead of relying on the callers (VMX and SVM) to manually > > validate the type. > > > > Unlike INVVPID and INVEPT, INVPCID is not explicitly documented to check > > the type before reading the operand from memory, so deferring the > > type validity check until after that point is architecturally allowed. > > > > Signed-off-by: Vipin Sharma <vipinsh@google.com> > > --- > > For future reference, a R-b that comes with qualifiers can be carried so long as > the issues raised by the reviewer are addressed. Obviously it can be somewhat > subjective, but common sense usually goes a long ways, and most reviewers won't > be too grumpy about mistakes so long as you had good intentions and remedy any > mistakes. And if you're in doubt, you can always add a blurb in the cover letter > or ignored part of the patch to explicitly confirm that it was ok to add the tag, > e.g. "Sean, I added your Reviewed-by in patch 02 after fixing the changelog, let > me know if that's not what you intended". > > Thanks! > > Reviewed-by: Sean Christopherson <seanjc@google.com> I was not sure if I can add R-b as it was only for the code and not changelog. Good to know that I can ask such things in the cover letter or the ignored part of the patch. Thanks Vipin
On Wed, Nov 03, 2021, Vipin Sharma wrote: > On Wed, Nov 3, 2021 at 4:20 PM Sean Christopherson <seanjc@google.com> wrote: > > > > On Wed, Nov 03, 2021, Vipin Sharma wrote: > > > Handle #GP on INVPCID due to an invalid type in the common switch > > > statement instead of relying on the callers (VMX and SVM) to manually > > > validate the type. > > > > > > Unlike INVVPID and INVEPT, INVPCID is not explicitly documented to check > > > the type before reading the operand from memory, so deferring the > > > type validity check until after that point is architecturally allowed. > > > > > > Signed-off-by: Vipin Sharma <vipinsh@google.com> > > > --- > > > > For future reference, a R-b that comes with qualifiers can be carried so long as > > the issues raised by the reviewer are addressed. Obviously it can be somewhat > > subjective, but common sense usually goes a long ways, and most reviewers won't > > be too grumpy about mistakes so long as you had good intentions and remedy any > > mistakes. And if you're in doubt, you can always add a blurb in the cover letter > > or ignored part of the patch to explicitly confirm that it was ok to add the tag, > > e.g. "Sean, I added your Reviewed-by in patch 02 after fixing the changelog, let > > me know if that's not what you intended". > > > > Thanks! > > > > Reviewed-by: Sean Christopherson <seanjc@google.com> > > I was not sure if I can add R-b as it was only for the code and not > changelog. Good to know that I can ask such things in the cover letter > or the ignored part of the patch. Ah, that's my bad, that was indeed a very confusing way to phrase my contingent review.
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);
Handle #GP on INVPCID due to an invalid type in the common switch statement instead of relying on the callers (VMX and SVM) to manually validate the type. Unlike INVVPID and INVEPT, INVPCID is not explicitly documented to check the type before reading the operand from memory, so deferring the type validity check until after that point is architecturally allowed. 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(-)