Re: [PATCH 1/7] Nested VMX patch 1 implements vmon and vmoff

[Avi Kivity <avi@redhat.com>]

On 12/10/2009 08:38 PM, oritw@il.ibm.com wrote:
> From: Orit Wasserman<oritw@il.ibm.com>
>
>    

Missing changelog entry.  Please use the format common to all kvm patches.


> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 3de0b37..3f63cdd 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -121,9 +121,6 @@ static int npt = 1;
>
>   module_param(npt, int, S_IRUGO);
>
> -static int nested = 1;
> -module_param(nested, int, S_IRUGO);
> -
>    

Separate moving 'nested' into a different patch.

> +struct __attribute__ ((__packed__)) level_state {
> +};
> +
> +struct nested_vmx {
> +	/* Has the level1 guest done vmxon? */
> +	bool vmxon;
> +	/* Level 1 state for switching to level 2 and back */
> +	struct level_state *l1_state;
>    

If this doesn't grow too large, can keep it as a member instead of a 
pointer.

> +};
> +
>
>   static inline struct vcpu_vmx *to_vmx(struct kvm_vcpu *vcpu)
> @@ -201,6 +214,7 @@ static struct kvm_vmx_segment_field {
>   static u64 host_efer;
>
>   static void ept_save_pdptrs(struct kvm_vcpu *vcpu);
> +static int create_l1_state(struct kvm_vcpu *vcpu);
>
>   /*
>    * Keep MSR_K6_STAR at the end, as setup_msrs() will try to optimize it
> @@ -961,6 +975,95 @@ static void guest_write_tsc(u64 guest_tsc, u64 host_tsc)
>   }
>
>   /*
> + * Handles msr read for nested virtualization
> + */
> +static int nested_vmx_get_msr(struct kvm_vcpu *vcpu, u32 msr_index,
> +			      u64 *pdata)
> +{
> +	u64 vmx_msr = 0;
> +
> +	switch (msr_index) {
> +	case MSR_IA32_FEATURE_CONTROL:
> +		*pdata = 0;
> +		break;
> +	case MSR_IA32_VMX_BASIC:
> +		*pdata = 0;
>    

Not needed.

> +		rdmsrl(MSR_IA32_VMX_BASIC, vmx_msr);
> +		*pdata = (vmx_msr&  0x00ffffcfffffffff);
>    

Please use symbolic constants.

> +		break;
> +	case MSR_IA32_VMX_PINBASED_CTLS:
> +		rdmsrl(MSR_IA32_VMX_PINBASED_CTLS, vmx_msr);
> +		*pdata = (PIN_BASED_EXT_INTR_MASK&  vmcs_config.pin_based_exec_ctrl) |
> +			(PIN_BASED_NMI_EXITING&  vmcs_config.pin_based_exec_ctrl) |
> +			(PIN_BASED_VIRTUAL_NMIS&  vmcs_config.pin_based_exec_ctrl);
>    

Don't understand.  You read vmx_msr and then use vmcs_config?

> +	case MSR_IA32_VMX_PROCBASED_CTLS:
> +	{
> +		u32 vmx_msr_high, vmx_msr_low;
> +		u32 control = CPU_BASED_HLT_EXITING |
> +#ifdef CONFIG_X86_64
> +			CPU_BASED_CR8_LOAD_EXITING |
> +			CPU_BASED_CR8_STORE_EXITING |
> +#endif
> +			CPU_BASED_CR3_LOAD_EXITING |
> +			CPU_BASED_CR3_STORE_EXITING |
> +			CPU_BASED_USE_IO_BITMAPS |
> +			CPU_BASED_MOV_DR_EXITING |
> +			CPU_BASED_USE_TSC_OFFSETING |
> +			CPU_BASED_INVLPG_EXITING |
> +			CPU_BASED_TPR_SHADOW |
> +			CPU_BASED_USE_MSR_BITMAPS |
> +			CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
> +
> +		rdmsr(MSR_IA32_VMX_PROCBASED_CTLS, vmx_msr_low, vmx_msr_high);
> +
> +		control&= vmx_msr_high; /* bit == 0 in high word ==>  must be zero */
> +		control |= vmx_msr_low;  /* bit == 1 in low word  ==>  must be one  */
> +
> +		*pdata = (CPU_BASED_HLT_EXITING&  control) |
> +#ifdef CONFIG_X86_64
> +			(CPU_BASED_CR8_LOAD_EXITING&  control) |
> +			(CPU_BASED_CR8_STORE_EXITING&  control) |
> +#endif
> +			(CPU_BASED_CR3_LOAD_EXITING&  control) |
> +			(CPU_BASED_CR3_STORE_EXITING&  control) |
> +			(CPU_BASED_USE_IO_BITMAPS&  control) |
> +			(CPU_BASED_MOV_DR_EXITING&  control) |
> +			(CPU_BASED_USE_TSC_OFFSETING&  control) |
> +			(CPU_BASED_INVLPG_EXITING&  control) ;
>    

What about the high word of the msr?  Will it always allow 0?

>
>   /*
> + * Writes msr value for nested virtualization
> + * Returns 0 on success, non-0 otherwise.
> + */
> +static int nested_vmx_set_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 data)
> +{
> +	switch (msr_index) {
> +	case MSR_IA32_FEATURE_CONTROL:
> +		if ((data&  (FEATURE_CONTROL_LOCKED |
> +			     FEATURE_CONTROL_VMXON_ENABLED))
> +		    != (FEATURE_CONTROL_LOCKED |
> +			FEATURE_CONTROL_VMXON_ENABLED))
> +			return 1;
> +		break;
>    

Need to trap if unsupported bits are set.

Need a way for userspace to write these msrs, so that live migration to 
an older kvm can work.  We do the same thing with cpuid - userspace sets 
cpuid to values that are common across the migration cluster.

> +static void free_l1_state(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_vmx *vmx = to_vmx(vcpu);
> +
> +	if (!vmx->nested.l1_state)
> +		return;
>    

Check isn't needed, kfree() likes NULLs.

> +
> +	kfree(vmx->nested.l1_state);
> +	vmx->nested.l1_state = NULL;
> +}
> +
> +
>
>    

>
>   struct kvm_shared_msrs_global {
> @@ -505,7 +509,7 @@ void kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
>   		return;
>   	}
>
> -	if (cr4&  X86_CR4_VMXE) {
> +	if (cr4&  X86_CR4_VMXE&&  !nested) {
>   		printk(KERN_DEBUG "set_cr4: #GP, setting VMXE\n");
>   		kvm_inject_gp(vcpu, 0);
>   		return;
>    

Some bits are required to be set when VMXE is enabled.

Please split the MSR changes into a separate patch.  Even cr4 is better 
on its own.

-- 
error compiling committee.c: too many arguments to function