On Mon, Oct 11, 2021 at 1:23 PM Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
> >
