| Message ID | 20211011194615.2955791-1-vipinsh@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: VMX: Add a wrapper for reading INVPCID/INVEPT/INVVPID type | expand |
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 >
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 > >
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);
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.
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
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.
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 */
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(-)