Re: [PATCH] KVM: VMX: Add a wrapper for reading INVPCID/INVEPT/INVVPID type

[Sean Christopherson]

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;
	}

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
>