From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 3/7] Nested VMX patch 3 implements vmptrld and vmptrst Date: Wed, 16 Dec 2009 16:32:31 +0200 Message-ID: <4B28EF7F.7080204@redhat.com> References: <1260470309-7166-1-git-send-email-oritw@il.ibm.com> <1260470309-7166-2-git-send-email-oritw@il.ibm.com> <1260470309-7166-3-git-send-email-oritw@il.ibm.com> <1260470309-7166-4-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]:10883 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756784AbZLPOci (ORCPT ); Wed, 16 Dec 2009 09:32:38 -0500 In-Reply-To: <1260470309-7166-4-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: > > + > + > struct __attribute__ ((__packed__)) level_state { > /* Has the level1 guest done vmclear? */ > bool vmclear; > + > + u64 io_bitmap_a; > + u64 io_bitmap_b; > + u64 msr_bitmap; > + > + bool first_launch; > }; > Please keep things naturally aligned. > /* > @@ -122,6 +255,8 @@ struct nested_vmx { > gpa_t current_vmptr; > /* Level 1 state for switching to level 2 and back */ > struct level_state *l1_state; > + /* Level 1 shadow vmcs for switching to level 2 and back */ > + struct shadow_vmcs *l1_shadow_vmcs; > /* list of vmcs for each l2 guest created by l1 */ > struct list_head l2_vmcs_list; > /* l2 page corresponding to the current vmcs set by l1 */ > @@ -187,10 +322,7 @@ static struct page *nested_get_page(struct kvm_vcpu *vcpu, > { > struct page *vmcs_page = NULL; > > - down_read(¤t->mm->mmap_sem); > vmcs_page = gfn_to_page(vcpu->kvm, vmcs_addr>> PAGE_SHIFT); > - up_read(¤t->mm->mmap_sem); > Fold this into the patch that introduced the problem. > - > if (is_error_page(vmcs_page)) { > printk(KERN_ERR "%s error allocating page 0x%llx\n", > __func__, vmcs_addr); > @@ -832,13 +964,14 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu) > > if (per_cpu(current_vmcs, cpu) != vmx->vmcs) { > u8 error; > - > per_cpu(current_vmcs, cpu) = vmx->vmcs; > + > Please avoid pointless whitespace changes. > asm volatile (__ex(ASM_VMX_VMPTRLD_RAX) "; setna %0" > : "=g"(error) : "a"(&phys_addr), "m"(phys_addr) > : "cc"); > + > if (error) > - printk(KERN_ERR "kvm: vmptrld %p/%llx fail\n", > + printk(KERN_ERR "kvm: vmptrld %p/%llx failed\n", > vmx->vmcs, phys_addr); > Fold. > + > static int create_l1_state(struct kvm_vcpu *vcpu) > { > struct vcpu_vmx *vmx = to_vmx(vcpu); > @@ -1441,10 +1587,75 @@ static int create_l1_state(struct kvm_vcpu *vcpu) > } else > return 0; > > + vmx->nested.l1_shadow_vmcs = kzalloc(PAGE_SIZE, GFP_KERNEL); > + if (!vmx->nested.l1_shadow_vmcs) { > + printk(KERN_INFO "%s could not allocate memory for l1_shadow vmcs\n", > + __func__); > + kfree(vmx->nested.l1_state); > + return -ENOMEM; > + } > + > INIT_LIST_HEAD(&(vmx->nested.l2_vmcs_list)); > return 0; > } > > +static struct vmcs *alloc_vmcs(void); > +int create_l2_state(struct kvm_vcpu *vcpu) > +{ > + struct vcpu_vmx *vmx = to_vmx(vcpu); > + struct vmcs *l2_vmcs; > + > + if (!nested_map_current(vcpu)) { > + printk(KERN_ERR "%s error mapping level 2 page", __func__); > + return -ENOMEM; > + } > + > + l2_vmcs = nested_get_current_vmcs(vcpu); > + if (!l2_vmcs) { > + struct nested_vmcs_list *new_l2_guest = > + (struct nested_vmcs_list *) > + kmalloc(sizeof(struct nested_vmcs_list), GFP_KERNEL); > + > + if (!new_l2_guest) { > + printk(KERN_ERR "%s error could not allocate memory for a new l2 guest list item", > + __func__); > + nested_unmap_current(vcpu); > + return -ENOMEM; > + } > Can the list grow without bounds? > + > + l2_vmcs = alloc_vmcs(); > + > + if (!l2_vmcs) { > + printk(KERN_ERR "%s error could not allocate memory for l2_vmcs", > + __func__); > + kfree(new_l2_guest); > + nested_unmap_current(vcpu); > + return -ENOMEM; > + } > + > + new_l2_guest->vmcs_addr = vmx->nested.current_vmptr; > + new_l2_guest->l2_vmcs = l2_vmcs; > + list_add(&(new_l2_guest->list),&(vmx->nested.l2_vmcs_list)); > + } > + > + if (cpu_has_vmx_msr_bitmap()) > + vmx->nested.current_l2_page->l2_state.msr_bitmap = > + vmcs_read64(MSR_BITMAP); > + else > + vmx->nested.current_l2_page->l2_state.msr_bitmap = 0; > + > + vmx->nested.current_l2_page->l2_state.io_bitmap_a = > + vmcs_read64(IO_BITMAP_A); > + vmx->nested.current_l2_page->l2_state.io_bitmap_b = > + vmcs_read64(IO_BITMAP_B); > Don't understand why these reads are needed. > + > + vmx->nested.current_l2_page->l2_state.first_launch = true; > + > + nested_unmap_current(vcpu); > + > + return 0; > +} > + > > @@ -3633,8 +3849,9 @@ static int handle_vmon(struct kvm_vcpu *vcpu) > return 1; > } > > - if (create_l1_state(vcpu)) { > - printk(KERN_ERR "%s create_l1_state failed\n", __func__); > + r = create_l1_state(vcpu); > + if (r) { > + printk(KERN_ERR "%s create_l1_state failed: %d\n", __func__, r); > Move this to the original patch. > kvm_queue_exception(vcpu, UD_VECTOR); > return 1; > } > @@ -3645,6 +3862,63 @@ static int handle_vmon(struct kvm_vcpu *vcpu) > return 1; > } > > +static int handle_vmptrld(struct kvm_vcpu *vcpu) > +{ > + struct vcpu_vmx *vmx = to_vmx(vcpu); > + u64 guest_vmcs_addr; > + gva_t vmcs_gva; > + unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION); > + u32 vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO); > + int r = 0; > + > + if (!nested_vmx_check_permission(vcpu)) > + return 1; > + > + vmcs_gva = get_vmx_mem_address(vcpu, exit_qualification, > + vmx_instruction_info); > + > + if (read_guest_vmcs_gpa(vcpu, vmcs_gva,&guest_vmcs_addr)) > + return 1; > + > + if (vmx->nested.current_vmptr != guest_vmcs_addr) { > + vmx->nested.current_vmptr = guest_vmcs_addr; > + r = create_l2_state(vcpu); > + if (r) { > + printk(KERN_ERR "%s create_l2_state failed: %d\n", > + __func__, r); > + return 1; > + } > + } > + > + clear_rflags_cf_zf(vcpu); > + skip_emulated_instruction(vcpu); > + return 1; > No set_rflags() on error? -- error compiling committee.c: too many arguments to function