Re: [PATCH 3/9] KVM: x86: add pcommit support

[Paolo Bonzini <pbonzini@redhat.com>]

On 21/08/2015 06:50, Xiao Guangrong wrote:
> Pass PCOMMIT CPU feature to guest to enable PCOMMIT instruction
> 
> Currently we do not catch pcommit instruction for L1 guest and
> allow L1 to catch this instruction for L2
> 
> The specification locates at:
> https://software.intel.com/sites/default/files/managed/0d/53/319433-022.pdf
> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>  arch/x86/include/asm/vmx.h      |  2 +-
>  arch/x86/include/uapi/asm/vmx.h |  4 +++-
>  arch/x86/kvm/cpuid.c            |  2 +-
>  arch/x86/kvm/cpuid.h            |  8 ++++++++
>  arch/x86/kvm/vmx.c              | 29 ++++++++++++++++++++++++-----
>  5 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index 9299ae5..e2ad08a 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -72,7 +72,7 @@
>  #define SECONDARY_EXEC_SHADOW_VMCS              0x00004000
>  #define SECONDARY_EXEC_ENABLE_PML               0x00020000
>  #define SECONDARY_EXEC_XSAVES			0x00100000
> -
> +#define SECONDARY_EXEC_PCOMMIT			0x00200000
>  
>  #define PIN_BASED_EXT_INTR_MASK                 0x00000001
>  #define PIN_BASED_NMI_EXITING                   0x00000008
> diff --git a/arch/x86/include/uapi/asm/vmx.h b/arch/x86/include/uapi/asm/vmx.h
> index 37fee27..5b15d94 100644
> --- a/arch/x86/include/uapi/asm/vmx.h
> +++ b/arch/x86/include/uapi/asm/vmx.h
> @@ -78,6 +78,7 @@
>  #define EXIT_REASON_PML_FULL            62
>  #define EXIT_REASON_XSAVES              63
>  #define EXIT_REASON_XRSTORS             64
> +#define EXIT_REASON_PCOMMIT             65
>  
>  #define VMX_EXIT_REASONS \
>  	{ EXIT_REASON_EXCEPTION_NMI,         "EXCEPTION_NMI" }, \
> @@ -126,7 +127,8 @@
>  	{ EXIT_REASON_INVVPID,               "INVVPID" }, \
>  	{ EXIT_REASON_INVPCID,               "INVPCID" }, \
>  	{ EXIT_REASON_XSAVES,                "XSAVES" }, \
> -	{ EXIT_REASON_XRSTORS,               "XRSTORS" }
> +	{ EXIT_REASON_XRSTORS,               "XRSTORS" }, \
> +	{ EXIT_REASON_PCOMMIT,               "PCOMMIT" }
>  
>  #define VMX_ABORT_SAVE_GUEST_MSR_FAIL        1
>  #define VMX_ABORT_LOAD_HOST_MSR_FAIL         4
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 962fc7d..faeb0b3 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -348,7 +348,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
>  		F(FSGSBASE) | F(BMI1) | F(HLE) | F(AVX2) | F(SMEP) |
>  		F(BMI2) | F(ERMS) | f_invpcid | F(RTM) | f_mpx | F(RDSEED) |
>  		F(ADX) | F(SMAP) | F(AVX512F) | F(AVX512PF) | F(AVX512ER) |
> -		F(AVX512CD) | F(CLFLUSHOPT) | F(CLWB);
> +		F(AVX512CD) | F(CLFLUSHOPT) | F(CLWB) | F(PCOMMIT);
>  
>  	/* cpuid 0xD.1.eax */
>  	const u32 kvm_supported_word10_x86_features =
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index dd05b9c..aed7bfe 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -133,4 +133,12 @@ static inline bool guest_cpuid_has_mpx(struct kvm_vcpu *vcpu)
>  	best = kvm_find_cpuid_entry(vcpu, 7, 0);
>  	return best && (best->ebx & bit(X86_FEATURE_MPX));
>  }
> +
> +static inline bool guest_cpuid_has_pcommit(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_cpuid_entry2 *best;
> +
> +	best = kvm_find_cpuid_entry(vcpu, 7, 0);
> +	return best && (best->ebx & bit(X86_FEATURE_PCOMMIT));
> +}
>  #endif
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 4cf25b9..b526c61 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2474,7 +2474,8 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
>  		SECONDARY_EXEC_APIC_REGISTER_VIRT |
>  		SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
>  		SECONDARY_EXEC_WBINVD_EXITING |
> -		SECONDARY_EXEC_XSAVES;
> +		SECONDARY_EXEC_XSAVES |
> +		SECONDARY_EXEC_PCOMMIT;
>  
>  	if (enable_ept) {
>  		/* nested EPT: emulate EPT also to L1 */
> @@ -3015,7 +3016,8 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
>  			SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
>  			SECONDARY_EXEC_SHADOW_VMCS |
>  			SECONDARY_EXEC_XSAVES |
> -			SECONDARY_EXEC_ENABLE_PML;
> +			SECONDARY_EXEC_ENABLE_PML |
> +			SECONDARY_EXEC_PCOMMIT;
>  		if (adjust_vmx_controls(min2, opt2,
>  					MSR_IA32_VMX_PROCBASED_CTLS2,
>  					&_cpu_based_2nd_exec_control) < 0)
> @@ -4570,6 +4572,9 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
>  	/* PML is enabled/disabled in creating/destorying vcpu */
>  	exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
>  
> +	/* Currently, we allow L1 guest to directly run pcommit instruction. */
> +	exec_control &= ~SECONDARY_EXEC_PCOMMIT;
> +
>  	return exec_control;
>  }
>  
> @@ -4613,10 +4618,9 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
>  
>  	vmcs_write32(CPU_BASED_VM_EXEC_CONTROL, vmx_exec_control(vmx));
>  
> -	if (cpu_has_secondary_exec_ctrls()) {
> +	if (cpu_has_secondary_exec_ctrls())
>  		vmcs_write32(SECONDARY_VM_EXEC_CONTROL,
>  				vmx_secondary_exec_control(vmx));
> -	}
>  
>  	if (vmx_cpu_uses_apicv(&vmx->vcpu)) {
>  		vmcs_write64(EOI_EXIT_BITMAP0, 0);
> @@ -7208,6 +7212,13 @@ static int handle_pml_full(struct kvm_vcpu *vcpu)
>  	return 1;
>  }
>  
> +static int handle_pcommit(struct kvm_vcpu *vcpu)
> +{
> +	/* we never catch pcommit instruct for L1 guest. */
> +	BUG();

Please WARN instead.

> +	return 1;
> +}
> +
>  /*
>   * The exit handlers return 1 if the exit was handled fully and guest execution
>   * may resume.  Otherwise they set the kvm_run parameter to indicate what needs
> @@ -7258,6 +7269,7 @@ static int (*const kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
>  	[EXIT_REASON_XSAVES]                  = handle_xsaves,
>  	[EXIT_REASON_XRSTORS]                 = handle_xrstors,
>  	[EXIT_REASON_PML_FULL]		      = handle_pml_full,
> +	[EXIT_REASON_PCOMMIT]                 = handle_pcommit,
>  };
>  
>  static const int kvm_vmx_max_exit_handlers =
> @@ -7559,6 +7571,8 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
>  		 * the XSS exit bitmap in vmcs12.
>  		 */
>  		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_XSAVES);
> +	case EXIT_REASON_PCOMMIT:
> +		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_PCOMMIT);
>  	default:
>  		return true;
>  	}
> @@ -8688,6 +8702,10 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
>  		if (best)
>  			best->ebx &= ~bit(X86_FEATURE_INVPCID);
>  	}
> +
> +	if (!guest_cpuid_has_pcommit(vcpu) && nested)
> +		vmx->nested.nested_vmx_secondary_ctls_high &=
> +			~SECONDARY_EXEC_PCOMMIT;

Why is this needed?

Paolo

>  }
>  
>  static void vmx_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
> @@ -9301,7 +9319,8 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>  		exec_control &= ~(SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
>  				  SECONDARY_EXEC_RDTSCP |
>  				  SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |
> -				  SECONDARY_EXEC_APIC_REGISTER_VIRT);
> +				  SECONDARY_EXEC_APIC_REGISTER_VIRT |
> +				  SECONDARY_EXEC_PCOMMIT);
>  		if (nested_cpu_has(vmcs12,
>  				CPU_BASED_ACTIVATE_SECONDARY_CONTROLS))
>  			exec_control |= vmcs12->secondary_vm_exec_control;
>