Re: [PATCH 2/2] KVM: nVMX: invvpid handling improvements

From: "Radim Krčmář" <rkrcmar@redhat.com>
To: Jan Dakinevich <jan.dakinevich@gmail.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, kernellwp@gmail.com
Subject: Re: [PATCH 2/2] KVM: nVMX: invvpid handling improvements
Date: Fri, 21 Oct 2016 22:02:24 +0200	[thread overview]
Message-ID: <20161021200224.GC29131@potion> (raw)
In-Reply-To: <1476830705-19269-2-git-send-email-jan.dakinevich@gmail.com>

2016-10-19 01:45+0300, Jan Dakinevich:
>  - Expose all invalidation types to the L1
> 
>  - Reject invvpid instruction, if L1 passed zero vpid value to single
>    context invalidations
> 
> Signed-off-by: Jan Dakinevich <jan.dakinevich@gmail.com>
> ---
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> @@ -132,6 +132,11 @@
>  
>  #define VMX_MISC_EMULATED_PREEMPTION_TIMER_RATE 5
>  
> +#define VMX_VPID_EXTENT_ALL_MASK (VMX_VPID_EXTENT_INDIVIDUAL_ADDR_BIT |	\

SUPPORTED instead of ALL would be a better name.

> +	VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT |				\
> +	VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT |				\
> +	VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS_BIT)
> +
>  /*
>   * These 2 parameters are used to config the controls for Pause-Loop Exiting:
>   * ple_gap:    upper bound on the amount of time between two successive
> @@ -2838,8 +2843,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
>  	 */
>  	if (enable_vpid)
>  		vmx->nested.nested_vmx_vpid_caps = VMX_VPID_INVVPID_BIT |
> -				VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT |
> -				VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT;
> +			VMX_VPID_EXTENT_ALL_MASK;

I'd still support only type 2, because it is the only one we implement,
and type 1, because of buggy KVMs.

Are there some OSes that can't use single or all context invalidation,
so supporting more might benefit something?

>  	else
>  		vmx->nested.nested_vmx_vpid_caps = 0;
> @@ -7720,7 +7724,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
>  	vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
>  	type = kvm_register_readl(vcpu, (vmx_instruction_info >> 28) & 0xf);
>  
> -	types = (vmx->nested.nested_vmx_vpid_caps >> 8) & 0x7;
> +	types = (vmx->nested.nested_vmx_vpid_caps & VMX_VPID_EXTENT_ALL_MASK) >> 8;
>  
>  	if (!(types & (1UL << type))) {
>  		nested_vmx_failValid(vcpu,
> @@ -7742,17 +7746,24 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
>  	}
>  
>  	switch (type) {
> +	case VMX_VPID_EXTENT_INDIVIDUAL_ADDR:
>  	case VMX_VPID_EXTENT_SINGLE_CONTEXT:
> +	case VMX_VPID_EXTENT_SINGLE_CONTEXT_RETAINING_GLOBALS:
> +		if (!vpid) {
> +			nested_vmx_failValid(vcpu,
> +				VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
> +			skip_emulated_instruction(vcpu);
> +			return 1;

(Just break and share the code.)

> +		}
> +		/* fall through */
> +	case VMX_VPID_EXTENT_GLOBAL_CONTEXT:
>  		/*
> -		 * Old versions of KVM use the single-context version so we
> -		 * have to support it; just treat it the same as all-context.
> +		 * Treat any invvpid type as all-context invalidation
>  		 */
> -	case VMX_VPID_EXTENT_GLOBAL_CONTEXT:
>  		__vmx_flush_tlb(vcpu, to_vmx(vcpu)->nested.vpid02);
>  		nested_vmx_succeed(vcpu);
>  		break;
>  	default:
> -		/* Trap individual address invalidation invvpid calls */
>  		BUG_ON(1);

Yeah, this was wrong when this type was not actually supported.
BUG_ON() is not a good thing to do in any case, please change it into
WARN_ON_ONCE(), because we should never get into the default branch.

Thanks.