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

["Radim Krčmář" <rkrcmar@redhat.com>]

2016-10-28 07:00+0300, Jan Dakinevich:
> From: Jan Dakinevich <jan.dakinevich@gmail.com>
> 
>  - 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>
> ---
>  arch/x86/kvm/vmx.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> @@ -132,6 +132,12 @@
>  
>  #define VMX_MISC_EMULATED_PREEMPTION_TIMER_RATE 5
>  
> +#define VMX_VPID_EXTENT_SUPPORTED_MASK		\
> +	(VMX_VPID_EXTENT_INDIVIDUAL_ADDR_BIT |	\
> +	VMX_VPID_EXTENT_SINGLE_CONTEXT_BIT |	\
> +	VMX_VPID_EXTENT_GLOBAL_CONTEXT_BIT |	\
> +	VMX_VPID_EXTENT_SINGLE_NON_GLOBAL_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 +2844,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_SUPPORTED_MASK;
>  	else
>  		vmx->nested.nested_vmx_vpid_caps = 0;
>  
> @@ -7720,7 +7725,8 @@ 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_SUPPORTED_MASK) >> 8;
>  
>  	if (!(types & (1UL << type))) {

Sorry for the late review.

This condition changed in 4.9-rc2, with 85c856b39b47 ("kvm: nVMX: Fix
kernel panics induced by illegal INVEPT/INVVPID types").

I applied the patch to kvm/queue without any changes as I think it
didn't affect this patch.

>  		nested_vmx_failValid(vcpu,
> @@ -7742,21 +7748,27 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
>  	}
>  
>  	switch (type) {
> +	case VMX_VPID_EXTENT_INDIVIDUAL_ADDR:
>  	case VMX_VPID_EXTENT_SINGLE_CONTEXT:
> -		/*
> -		 * Old versions of KVM use the single-context version so we
> -		 * have to support it; just treat it the same as all-context.
> -		 */
> +	case VMX_VPID_EXTENT_SINGLE_NON_GLOBAL:
> +		if (!vpid) {
> +			nested_vmx_failValid(vcpu,
> +				VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
> +			skip_emulated_instruction(vcpu);
> +			return 1;

In v1, I said here:

  (Just break and share the code.)

by the code I meant

  skip_emulated_instruction(vcpu);
  return 1;

> +		}
> +		break;
>  	case VMX_VPID_EXTENT_ALL_CONTEXT:
> -		__vmx_flush_tlb(vcpu, to_vmx(vcpu)->nested.vpid02);
> -		nested_vmx_succeed(vcpu);
>  		break;

as I actually prefer v1, which had here:

  		 __vmx_flush_tlb(vcpu, vmx->nested.vpid02);
  		 nested_vmx_succeed(vcpu);
  		 break;

>  	default:
> -		/* Trap individual address invalidation invvpid calls */
> -		BUG_ON(1);
> -		break;
> +		WARN_ON_ONCE(1);
> +		skip_emulated_instruction(vcpu);
> +		return 1;

You could also omit the skip and return here ...

>  	}
>  
> +	__vmx_flush_tlb(vcpu, vmx->nested.vpid02);
> +	nested_vmx_succeed(vcpu);
> +

... if this block was in the switch.
Do you wish to change the code?

Thanks.

>  	skip_emulated_instruction(vcpu);
>  	return 1;
>  }