[PATCH v3 0/2] Add wrapper to read GPR of INVPCID, INVVPID, and INVEPT

[PATCH v3 0/2] Add wrapper to read GPR of INVPCID, INVVPID, and INVEPT

[Vipin Sharma]

Hello,

v3 is similar to v2 except that the commit message of "PATCH v3 2/2" is now
clearer and detailed.

VMX code to handle INVPCID, INVVPID, and INVEPT read the same GPR index
in VM exit info. This patch series improves that handling by adding a
common wrapper function for them.

Patch 2 makes a sublte change in INVPCID type check. Unlike INVVPID and
INVEPT, INVPCID is not explicitly documented to check the type before
reading the operand from memory. So, this patch moves INVPCID type check
to the common switch statement instead of VMX and SVM validating it
individually.

Changes in v3:
- Patch 2's commit message is more detailed now.

Changes in v2:
- Keeping the register read visible in the functions.
- Removed INVPCID type check hardcoding and moved error condition to common 
  function.

[v2] https://lore.kernel.org/lkml/20211103183232.1213761-1-vipinsh@google.com/
[v1] https://lore.kernel.org/lkml/20211011194615.2955791-1-vipinsh@google.com/

Vipin Sharma (2):
  KVM: VMX: Add a wrapper to read index of GPR for INVPCID, INVVPID, and
    INVEPT
  KVM: Move INVPCID type check from vmx and svm to the common
    kvm_handle_invpcid()

 arch/x86/kvm/svm/svm.c    |  5 -----
 arch/x86/kvm/vmx/nested.c | 10 ++++++----
 arch/x86/kvm/vmx/vmx.c    |  9 +++------
 arch/x86/kvm/vmx/vmx.h    |  5 +++++
 arch/x86/kvm/x86.c        |  3 ++-
 5 files changed, 16 insertions(+), 16 deletions(-)

-- 
2.33.1.1089.g2158813163f-goog

[PATCH v3 1/2] KVM: VMX: Add a wrapper to read index of GPR for INVPCID, INVVPID, and INVEPT

[Vipin Sharma]

handle_invept(), handle_invvpid(), handle_invpcid() read the same reg2
on VM exit. Move them to a common wrapper function.

Signed-off-by: Vipin Sharma <vipinsh@google.com>
---
 arch/x86/kvm/vmx/nested.c | 10 ++++++----
 arch/x86/kvm/vmx/vmx.c    |  4 +++-
 arch/x86/kvm/vmx/vmx.h    |  5 +++++
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index b4ee5e9f9e20..f73d4e31dd99 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5379,7 +5379,7 @@ static int handle_invept(struct kvm_vcpu *vcpu)
 	struct {
 		u64 eptp, gpa;
 	} operand;
-	int i, r;
+	int i, r, gpr_index;
 
 	if (!(vmx->nested.msrs.secondary_ctls_high &
 	      SECONDARY_EXEC_ENABLE_EPT) ||
@@ -5392,7 +5392,8 @@ static int handle_invept(struct kvm_vcpu *vcpu)
 		return 1;
 
 	vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
-	type = kvm_register_read(vcpu, (vmx_instruction_info >> 28) & 0xf);
+	gpr_index = vmx_get_instr_info_reg2(vmx_instruction_info);
+	type = kvm_register_read(vcpu, gpr_index);
 
 	types = (vmx->nested.msrs.ept_caps >> VMX_EPT_EXTENT_SHIFT) & 6;
 
@@ -5459,7 +5460,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
 		u64 gla;
 	} operand;
 	u16 vpid02;
-	int r;
+	int r, gpr_index;
 
 	if (!(vmx->nested.msrs.secondary_ctls_high &
 	      SECONDARY_EXEC_ENABLE_VPID) ||
@@ -5472,7 +5473,8 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
 		return 1;
 
 	vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
-	type = kvm_register_read(vcpu, (vmx_instruction_info >> 28) & 0xf);
+	gpr_index = vmx_get_instr_info_reg2(vmx_instruction_info);
+	type = kvm_register_read(vcpu, gpr_index);
 
 	types = (vmx->nested.msrs.vpid_caps &
 			VMX_VPID_EXTENT_SUPPORTED_MASK) >> 8;
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 71f54d85f104..e41d207e3298 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5494,6 +5494,7 @@ static int handle_invpcid(struct kvm_vcpu *vcpu)
 		u64 pcid;
 		u64 gla;
 	} operand;
+	int gpr_index;
 
 	if (!guest_cpuid_has(vcpu, X86_FEATURE_INVPCID)) {
 		kvm_queue_exception(vcpu, UD_VECTOR);
@@ -5501,7 +5502,8 @@ static int handle_invpcid(struct kvm_vcpu *vcpu)
 	}
 
 	vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
-	type = kvm_register_read(vcpu, (vmx_instruction_info >> 28) & 0xf);
+	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);
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index e7db42e3b0ce..95c9bca45cdd 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -522,4 +522,9 @@ static inline bool vmx_guest_state_valid(struct kvm_vcpu *vcpu)
 
 void dump_vmcs(struct kvm_vcpu *vcpu);
 
+static inline int vmx_get_instr_info_reg2(u32 vmx_instr_info)
+{
+	return (vmx_instr_info >> 28) & 0xf;
+}
+
 #endif /* __KVM_X86_VMX_H */
-- 
2.33.1.1089.g2158813163f-goog

[PATCH v3 2/2] KVM: Move INVPCID type check from vmx and svm to the common kvm_handle_invpcid()

[Vipin Sharma]

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(-)

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);
-- 
2.33.1.1089.g2158813163f-goog

Re: [PATCH v3 2/2] KVM: Move INVPCID type check from vmx and svm to the common kvm_handle_invpcid()

[Sean Christopherson]

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>

Re: [PATCH v3 2/2] KVM: Move INVPCID type check from vmx and svm to the common kvm_handle_invpcid()

[Vipin Sharma]

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

Re: [PATCH v3 2/2] KVM: Move INVPCID type check from vmx and svm to the common kvm_handle_invpcid()

[Sean Christopherson]

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.

Re: [PATCH v3 0/2] Add wrapper to read GPR of INVPCID, INVVPID, and INVEPT

[Sean Christopherson]

On Wed, Nov 03, 2021, Vipin Sharma wrote:
> Hello,
> 
> v3 is similar to v2 except that the commit message of "PATCH v3 2/2" is now
> clearer and detailed.

Heh, please wait at _minimum_ one day before spinning a new version.  I know it's
a bit weird/silly for such a small series, but even in this case I replied to the
previous version because I circled back to the "series" while waiting for a build
to complete.  For small series and/or single patches, unless there's a reason for
urgency, it's polite to wait a few days between versions to give folks a reasonable
chance to weigh in before getting hit with a new version.

Re: [PATCH v3 0/2] Add wrapper to read GPR of INVPCID, INVVPID, and INVEPT

[Vipin Sharma]

On Wed, Nov 3, 2021 at 4:08 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Nov 03, 2021, Vipin Sharma wrote:
> > Hello,
> >
> > v3 is similar to v2 except that the commit message of "PATCH v3 2/2" is now
> > clearer and detailed.
>
> Heh, please wait at _minimum_ one day before spinning a new version.  I know it's
> a bit weird/silly for such a small series, but even in this case I replied to the
> previous version because I circled back to the "series" while waiting for a build
> to complete.  For small series and/or single patches, unless there's a reason for
> urgency, it's polite to wait a few days between versions to give folks a reasonable
> chance to weigh in before getting hit with a new version.

I am sorry about this. I will keep this in mind for any future work.
Thanks for the advice, I appreciate it.