From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 1/7] Nested VMX patch 1 implements vmon and vmoff Date: Wed, 16 Dec 2009 15:34:07 +0200 Message-ID: <4B28E1CF.6070606@redhat.com> References: <1260470309-7166-1-git-send-email-oritw@il.ibm.com> <1260470309-7166-2-git-send-email-oritw@il.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, benami@il.ibm.com, abelg@il.ibm.com, muli@il.ibm.com, aliguori@us.ibm.com, mdday@us.ibm.com To: oritw@il.ibm.com Return-path: Received: from mx1.redhat.com ([209.132.183.28]:47259 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751025AbZLPNeN (ORCPT ); Wed, 16 Dec 2009 08:34:13 -0500 In-Reply-To: <1260470309-7166-2-git-send-email-oritw@il.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: On 12/10/2009 08:38 PM, oritw@il.ibm.com wrote: > From: Orit Wasserman > > 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