* [PATCH] KVM: VMX: Add a wrapper for reading INVPCID/INVEPT/INVVPID type
@ 2021-10-11 19:46 Vipin Sharma
2021-10-11 20:23 ` Sean Christopherson
0 siblings, 1 reply; 7+ messages in thread
From: Vipin Sharma @ 2021-10-11 19:46 UTC (permalink / raw)
To: pbonzini, seanjc, dmatlack; +Cc: kvm, linux-kernel, Vipin Sharma
Add a common helper function to read invalidation type specified by a
trapped INVPCID/INVEPT/INVVPID instruction.
Add a symbol constant for max INVPCID type.
No functional change intended.
Signed-off-by: Vipin Sharma <vipinsh@google.com>
---
arch/x86/include/asm/invpcid.h | 1 +
arch/x86/kvm/vmx/nested.c | 4 ++--
arch/x86/kvm/vmx/vmx.c | 4 ++--
arch/x86/kvm/vmx/vmx.h | 12 ++++++++++++
4 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/invpcid.h b/arch/x86/include/asm/invpcid.h
index 734482afbf81..b5ac26784c1b 100644
--- a/arch/x86/include/asm/invpcid.h
+++ b/arch/x86/include/asm/invpcid.h
@@ -21,6 +21,7 @@ static inline void __invpcid(unsigned long pcid, unsigned long addr,
#define INVPCID_TYPE_SINGLE_CTXT 1
#define INVPCID_TYPE_ALL_INCL_GLOBAL 2
#define INVPCID_TYPE_ALL_NON_GLOBAL 3
+#define INVPCID_TYPE_MAX 3
/* Flush all mappings for a given pcid and addr, not including globals. */
static inline void invpcid_flush_one(unsigned long pcid,
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index af1bbb73430a..f0605a99e0fc 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5392,7 +5392,7 @@ 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);
+ type = vmx_read_invalidation_type(vcpu, vmx_instruction_info);
types = (vmx->nested.msrs.ept_caps >> VMX_EPT_EXTENT_SHIFT) & 6;
@@ -5472,7 +5472,7 @@ 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);
+ type = vmx_read_invalidation_type(vcpu, vmx_instruction_info);
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 1c8b2b6e7ed9..77f72a41dde3 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5502,9 +5502,9 @@ 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);
+ type = vmx_read_invalidation_type(vcpu, vmx_instruction_info);
- if (type > 3) {
+ if (type > INVPCID_TYPE_MAX) {
kvm_inject_gp(vcpu, 0);
return 1;
}
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 592217fd7d92..eeafcce57df7 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -522,4 +522,16 @@ static inline bool vmx_guest_state_valid(struct kvm_vcpu *vcpu)
void dump_vmcs(struct kvm_vcpu *vcpu);
+/*
+ * When handling a VM-exit for one of INVPCID, INVEPT or INVVPID, read the type
+ * of invalidation specified by the instruction.
+ */
+static inline unsigned long vmx_read_invalidation_type(struct kvm_vcpu *vcpu,
+ u32 vmx_instr_info)
+{
+ u32 vmx_instr_reg2 = (vmx_instr_info >> 28) & 0xf;
+
+ return kvm_register_read(vcpu, vmx_instr_reg2);
+}
+
#endif /* __KVM_X86_VMX_H */
--
2.33.0.882.g93a45727a2-goog
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: VMX: Add a wrapper for reading INVPCID/INVEPT/INVVPID type
2021-10-11 19:46 [PATCH] KVM: VMX: Add a wrapper for reading INVPCID/INVEPT/INVVPID type Vipin Sharma
@ 2021-10-11 20:23 ` Sean Christopherson
2021-10-11 20:51 ` Jim Mattson
0 siblings, 1 reply; 7+ messages in thread
From: Sean Christopherson @ 2021-10-11 20:23 UTC (permalink / raw)
To: Vipin Sharma; +Cc: pbonzini, dmatlack, kvm, linux-kernel
On Mon, Oct 11, 2021, Vipin Sharma wrote:
> Add a common helper function to read invalidation type specified by a
> trapped INVPCID/INVEPT/INVVPID instruction.
>
> Add a symbol constant for max INVPCID type.
>
> No functional change intended.
>
> Signed-off-by: Vipin Sharma <vipinsh@google.com>
> ---
> arch/x86/include/asm/invpcid.h | 1 +
> arch/x86/kvm/vmx/nested.c | 4 ++--
> arch/x86/kvm/vmx/vmx.c | 4 ++--
> arch/x86/kvm/vmx/vmx.h | 12 ++++++++++++
> 4 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/invpcid.h b/arch/x86/include/asm/invpcid.h
> index 734482afbf81..b5ac26784c1b 100644
> --- a/arch/x86/include/asm/invpcid.h
> +++ b/arch/x86/include/asm/invpcid.h
> @@ -21,6 +21,7 @@ static inline void __invpcid(unsigned long pcid, unsigned long addr,
> #define INVPCID_TYPE_SINGLE_CTXT 1
> #define INVPCID_TYPE_ALL_INCL_GLOBAL 2
> #define INVPCID_TYPE_ALL_NON_GLOBAL 3
> +#define INVPCID_TYPE_MAX 3
...
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 1c8b2b6e7ed9..77f72a41dde3 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -5502,9 +5502,9 @@ 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);
> + type = vmx_read_invalidation_type(vcpu, vmx_instruction_info);
I would prefer to keep the register read visibile in this code so that it's
obvious what exactly is being read. With this approach, it's not clear that KVM
is reading a GPR vs. VMCS vs. simply extracting "type" from "vmx_instruction_info".
And it's not just the INV* instruction, VMREAD and VMWRITE use the same encoding.
The hardest part is coming up with a name :-) Maybe do the usually-ill-advised
approach of following the SDM verbatim? Reg2 is common to all flavors, so this?
gpr_index = vmx_get_instr_info_reg2(vmx_instruction_info);
type = kvm_register_read(vcpu, gpr_index);
>
> - if (type > 3) {
> + if (type > INVPCID_TYPE_MAX) {
Hrm, I don't love this because it's not auto-updating in the unlikely chance that
a new type is added. I definitely don't like open coding '3' either. What about
going with a verbose option of
if (type != INVPCID_TYPE_INDIV_ADDR &&
type != INVPCID_TYPE_SINGLE_CTXT &&
type != INVPCID_TYPE_ALL_INCL_GLOBAL &&
type != INVPCID_TYPE_ALL_NON_GLOBAL) {
kvm_inject_gp(vcpu, 0);
return 1;
}
It's kind of gross, but gcc10 is smart enought to coalesce those all into a single
CMP <reg>, 3; JA <#GP>, i.e. the resulting binary is identical.
> kvm_inject_gp(vcpu, 0);
> return 1;
> }
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 592217fd7d92..eeafcce57df7 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -522,4 +522,16 @@ static inline bool vmx_guest_state_valid(struct kvm_vcpu *vcpu)
>
> void dump_vmcs(struct kvm_vcpu *vcpu);
>
> +/*
> + * When handling a VM-exit for one of INVPCID, INVEPT or INVVPID, read the type
> + * of invalidation specified by the instruction.
> + */
> +static inline unsigned long vmx_read_invalidation_type(struct kvm_vcpu *vcpu,
> + u32 vmx_instr_info)
> +{
> + u32 vmx_instr_reg2 = (vmx_instr_info >> 28) & 0xf;
> +
> + return kvm_register_read(vcpu, vmx_instr_reg2);
> +}
> +
> #endif /* __KVM_X86_VMX_H */
> --
> 2.33.0.882.g93a45727a2-goog
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: VMX: Add a wrapper for reading INVPCID/INVEPT/INVVPID type
2021-10-11 20:23 ` Sean Christopherson
@ 2021-10-11 20:51 ` Jim Mattson
2021-10-14 16:54 ` Sean Christopherson
0 siblings, 1 reply; 7+ messages in thread
From: Jim Mattson @ 2021-10-11 20:51 UTC (permalink / raw)
To: Sean Christopherson; +Cc: Vipin Sharma, pbonzini, dmatlack, kvm, linux-kernel
On Mon, Oct 11, 2021 at 1:23 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Oct 11, 2021, Vipin Sharma wrote:
> > Add a common helper function to read invalidation type specified by a
> > trapped INVPCID/INVEPT/INVVPID instruction.
> >
> > Add a symbol constant for max INVPCID type.
> >
> > No functional change intended.
> >
> > Signed-off-by: Vipin Sharma <vipinsh@google.com>
> > ---
> > arch/x86/include/asm/invpcid.h | 1 +
> > arch/x86/kvm/vmx/nested.c | 4 ++--
> > arch/x86/kvm/vmx/vmx.c | 4 ++--
> > arch/x86/kvm/vmx/vmx.h | 12 ++++++++++++
> > 4 files changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/invpcid.h b/arch/x86/include/asm/invpcid.h
> > index 734482afbf81..b5ac26784c1b 100644
> > --- a/arch/x86/include/asm/invpcid.h
> > +++ b/arch/x86/include/asm/invpcid.h
> > @@ -21,6 +21,7 @@ static inline void __invpcid(unsigned long pcid, unsigned long addr,
> > #define INVPCID_TYPE_SINGLE_CTXT 1
> > #define INVPCID_TYPE_ALL_INCL_GLOBAL 2
> > #define INVPCID_TYPE_ALL_NON_GLOBAL 3
> > +#define INVPCID_TYPE_MAX 3
>
> ...
>
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 1c8b2b6e7ed9..77f72a41dde3 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -5502,9 +5502,9 @@ 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);
> > + type = vmx_read_invalidation_type(vcpu, vmx_instruction_info);
>
> I would prefer to keep the register read visibile in this code so that it's
> obvious what exactly is being read. With this approach, it's not clear that KVM
> is reading a GPR vs. VMCS vs. simply extracting "type" from "vmx_instruction_info".
>
> And it's not just the INV* instruction, VMREAD and VMWRITE use the same encoding.
>
> The hardest part is coming up with a name :-) Maybe do the usually-ill-advised
> approach of following the SDM verbatim? Reg2 is common to all flavors, so this?
>
> gpr_index = vmx_get_instr_info_reg2(vmx_instruction_info);
> type = kvm_register_read(vcpu, gpr_index);
>
> >
> > - if (type > 3) {
> > + if (type > INVPCID_TYPE_MAX) {
>
> Hrm, I don't love this because it's not auto-updating in the unlikely chance that
> a new type is added. I definitely don't like open coding '3' either. What about
> going with a verbose option of
>
> if (type != INVPCID_TYPE_INDIV_ADDR &&
> type != INVPCID_TYPE_SINGLE_CTXT &&
> type != INVPCID_TYPE_ALL_INCL_GLOBAL &&
> type != INVPCID_TYPE_ALL_NON_GLOBAL) {
> kvm_inject_gp(vcpu, 0);
> return 1;
> }
Better, perhaps, to introduce a new function, valid_invpcid_type(),
and squirrel away the ugliness there?
> It's kind of gross, but gcc10 is smart enought to coalesce those all into a single
> CMP <reg>, 3; JA <#GP>, i.e. the resulting binary is identical.
>
> > kvm_inject_gp(vcpu, 0);
> > return 1;
> > }
> > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > index 592217fd7d92..eeafcce57df7 100644
> > --- a/arch/x86/kvm/vmx/vmx.h
> > +++ b/arch/x86/kvm/vmx/vmx.h
> > @@ -522,4 +522,16 @@ static inline bool vmx_guest_state_valid(struct kvm_vcpu *vcpu)
> >
> > void dump_vmcs(struct kvm_vcpu *vcpu);
> >
> > +/*
> > + * When handling a VM-exit for one of INVPCID, INVEPT or INVVPID, read the type
> > + * of invalidation specified by the instruction.
> > + */
> > +static inline unsigned long vmx_read_invalidation_type(struct kvm_vcpu *vcpu,
> > + u32 vmx_instr_info)
> > +{
> > + u32 vmx_instr_reg2 = (vmx_instr_info >> 28) & 0xf;
> > +
> > + return kvm_register_read(vcpu, vmx_instr_reg2);
> > +}
> > +
> > #endif /* __KVM_X86_VMX_H */
> > --
> > 2.33.0.882.g93a45727a2-goog
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: VMX: Add a wrapper for reading INVPCID/INVEPT/INVVPID type
2021-10-11 20:51 ` Jim Mattson
@ 2021-10-14 16:54 ` Sean Christopherson
2021-10-14 17:05 ` Jim Mattson
0 siblings, 1 reply; 7+ messages in thread
From: Sean Christopherson @ 2021-10-14 16:54 UTC (permalink / raw)
To: Jim Mattson; +Cc: Vipin Sharma, pbonzini, dmatlack, kvm, linux-kernel
On Mon, Oct 11, 2021, Jim Mattson wrote:
> On Mon, Oct 11, 2021 at 1:23 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Mon, Oct 11, 2021, Vipin Sharma wrote:
> > > - if (type > 3) {
> > > + if (type > INVPCID_TYPE_MAX) {
> >
> > Hrm, I don't love this because it's not auto-updating in the unlikely chance that
> > a new type is added. I definitely don't like open coding '3' either. What about
> > going with a verbose option of
> >
> > if (type != INVPCID_TYPE_INDIV_ADDR &&
> > type != INVPCID_TYPE_SINGLE_CTXT &&
> > type != INVPCID_TYPE_ALL_INCL_GLOBAL &&
> > type != INVPCID_TYPE_ALL_NON_GLOBAL) {
> > kvm_inject_gp(vcpu, 0);
> > return 1;
> > }
>
> Better, perhaps, to introduce a new function, valid_invpcid_type(),
> and squirrel away the ugliness there?
Oh, yeah, definitely. I missed that SVM's invpcid_interception() has the same
open-coded check.
Alternatively, could we handle the invalid type in the main switch statement? I
don't see anything in the SDM or APM that architecturally _requires_ the type be
checked before reading the INVPCID descriptor. Hardware may operate that way,
but that's uArch specific behavior unless there's explicit documentation.
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 89077160d463..c8aade2e2a20 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 1c8b2b6e7ed9..ad2e794d4cb2 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5504,11 +5504,6 @@ 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);
- 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 d9273f536f9d..a3657db6daf9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -12382,7 +12382,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);
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: VMX: Add a wrapper for reading INVPCID/INVEPT/INVVPID type
2021-10-14 16:54 ` Sean Christopherson
@ 2021-10-14 17:05 ` Jim Mattson
2021-11-02 18:12 ` Vipin Sharma
0 siblings, 1 reply; 7+ messages in thread
From: Jim Mattson @ 2021-10-14 17:05 UTC (permalink / raw)
To: Sean Christopherson; +Cc: Vipin Sharma, pbonzini, dmatlack, kvm, linux-kernel
On Thu, Oct 14, 2021 at 9:54 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Oct 11, 2021, Jim Mattson wrote:
> > On Mon, Oct 11, 2021 at 1:23 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Mon, Oct 11, 2021, Vipin Sharma wrote:
> > > > - if (type > 3) {
> > > > + if (type > INVPCID_TYPE_MAX) {
> > >
> > > Hrm, I don't love this because it's not auto-updating in the unlikely chance that
> > > a new type is added. I definitely don't like open coding '3' either. What about
> > > going with a verbose option of
> > >
> > > if (type != INVPCID_TYPE_INDIV_ADDR &&
> > > type != INVPCID_TYPE_SINGLE_CTXT &&
> > > type != INVPCID_TYPE_ALL_INCL_GLOBAL &&
> > > type != INVPCID_TYPE_ALL_NON_GLOBAL) {
> > > kvm_inject_gp(vcpu, 0);
> > > return 1;
> > > }
> >
> > Better, perhaps, to introduce a new function, valid_invpcid_type(),
> > and squirrel away the ugliness there?
>
> Oh, yeah, definitely. I missed that SVM's invpcid_interception() has the same
> open-coded check.
>
> Alternatively, could we handle the invalid type in the main switch statement? I
> don't see anything in the SDM or APM that architecturally _requires_ the type be
> checked before reading the INVPCID descriptor. Hardware may operate that way,
> but that's uArch specific behavior unless there's explicit documentation.
Right. INVVPID and INVEPT are explicitly documented to check the type
first, but INVPCID is not.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: VMX: Add a wrapper for reading INVPCID/INVEPT/INVVPID type
2021-10-14 17:05 ` Jim Mattson
@ 2021-11-02 18:12 ` Vipin Sharma
2021-11-02 19:19 ` Sean Christopherson
0 siblings, 1 reply; 7+ messages in thread
From: Vipin Sharma @ 2021-11-02 18:12 UTC (permalink / raw)
To: Jim Mattson, Sean Christopherson; +Cc: pbonzini, dmatlack, kvm, linux-kernel
Sorry for the late reply.
On Thu, Oct 14, 2021 at 10:05 AM Jim Mattson <jmattson@google.com> wrote:
>
> On Thu, Oct 14, 2021 at 9:54 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Mon, Oct 11, 2021, Jim Mattson wrote:
> > > On Mon, Oct 11, 2021 at 1:23 PM Sean Christopherson <seanjc@google.com> wrote:
> > > >
> > > > On Mon, Oct 11, 2021, Vipin Sharma wrote:
> > > > > - if (type > 3) {
> > > > > + if (type > INVPCID_TYPE_MAX) {
> > > >
> > > > Hrm, I don't love this because it's not auto-updating in the unlikely chance that
> > > > a new type is added. I definitely don't like open coding '3' either. What about
> > > > going with a verbose option of
> > > >
> > > > if (type != INVPCID_TYPE_INDIV_ADDR &&
> > > > type != INVPCID_TYPE_SINGLE_CTXT &&
> > > > type != INVPCID_TYPE_ALL_INCL_GLOBAL &&
> > > > type != INVPCID_TYPE_ALL_NON_GLOBAL) {
> > > > kvm_inject_gp(vcpu, 0);
> > > > return 1;
> > > > }
> > >
> > > Better, perhaps, to introduce a new function, valid_invpcid_type(),
> > > and squirrel away the ugliness there?
> >
I might not have understood your auto-updating concern correctly, can
I change these macros to an enum like:
enum INVPCID_TYPE {
INVPCID_TYPE_INDIV_ADDR,
INVPCID_TYPE_SINGLE_CTXT,
INVPCID_TYPE_ALL_INCL_GLOBAL,
INVPCID_TYPE_ALL_NON_GLOBAL,
INVPCID_TYPE_MAX,
};
My check in the condition will be then "if (type >= INVPCID_TYPE_MAX) {}"
This way if there is a new type added, max will be auto updated. Will
this answers your concern?
> > Oh, yeah, definitely. I missed that SVM's invpcid_interception() has the same
> > open-coded check.
> >
> > Alternatively, could we handle the invalid type in the main switch statement? I
> > don't see anything in the SDM or APM that architecturally _requires_ the type be
> > checked before reading the INVPCID descriptor. Hardware may operate that way,
> > but that's uArch specific behavior unless there's explicit documentation.
>
> Right. INVVPID and INVEPT are explicitly documented to check the type
> first, but INVPCID is not.
It seems to me that I can move type > 3 check to kvm_handle_invpcid()
switch statement. I can replace BUG() in that switch statement with
kvm_inject_gp for the default case, I won't even need INVPCID_TYPE_MAX
in this case.
If you are fine with this approach then I will send out a patch where
invalid type is handled in kvm_handle_invpcid() switch statement.
Thanks
Vipin
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] KVM: VMX: Add a wrapper for reading INVPCID/INVEPT/INVVPID type
2021-11-02 18:12 ` Vipin Sharma
@ 2021-11-02 19:19 ` Sean Christopherson
0 siblings, 0 replies; 7+ messages in thread
From: Sean Christopherson @ 2021-11-02 19:19 UTC (permalink / raw)
To: Vipin Sharma; +Cc: Jim Mattson, pbonzini, dmatlack, kvm, linux-kernel
On Tue, Nov 02, 2021, Vipin Sharma wrote:
> Sorry for the late reply.
>
> On Thu, Oct 14, 2021 at 10:05 AM Jim Mattson <jmattson@google.com> wrote:
> >
> > On Thu, Oct 14, 2021 at 9:54 AM Sean Christopherson <seanjc@google.com> wrote:
> > > Oh, yeah, definitely. I missed that SVM's invpcid_interception() has the same
> > > open-coded check.
> > >
> > > Alternatively, could we handle the invalid type in the main switch statement? I
> > > don't see anything in the SDM or APM that architecturally _requires_ the type be
> > > checked before reading the INVPCID descriptor. Hardware may operate that way,
> > > but that's uArch specific behavior unless there's explicit documentation.
> >
> > Right. INVVPID and INVEPT are explicitly documented to check the type
> > first, but INVPCID is not.
>
> It seems to me that I can move type > 3 check to kvm_handle_invpcid()
> switch statement. I can replace BUG() in that switch statement with
> kvm_inject_gp for the default case, I won't even need INVPCID_TYPE_MAX
> in this case.
Yep.
> If you are fine with this approach then I will send out a patch where
> invalid type is handled in kvm_handle_invpcid() switch statement.
This has my vote.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-11-02 19:19 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-11 19:46 [PATCH] KVM: VMX: Add a wrapper for reading INVPCID/INVEPT/INVVPID type Vipin Sharma
2021-10-11 20:23 ` Sean Christopherson
2021-10-11 20:51 ` Jim Mattson
2021-10-14 16:54 ` Sean Christopherson
2021-10-14 17:05 ` Jim Mattson
2021-11-02 18:12 ` Vipin Sharma
2021-11-02 19:19 ` Sean Christopherson
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).