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
>
