From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 3/5] Nested VMX patch 3 implements vmptrld and vmptrst Date: Tue, 20 Oct 2009 13:24:33 +0900 Message-ID: <4ADD3B81.2050202@redhat.com> References: <1255617706-13564-1-git-send-email-oritw@il.ibm.com> <1255617706-13564-2-git-send-email-oritw@il.ibm.com> <1255617706-13564-3-git-send-email-oritw@il.ibm.com> <1255617706-13564-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]:36839 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750795AbZJTEYr (ORCPT ); Tue, 20 Oct 2009 00:24:47 -0400 In-Reply-To: <1255617706-13564-4-git-send-email-oritw@il.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: On 10/15/2009 11:41 PM, oritw@il.ibm.com wrote: > > + > +struct __attribute__ ((__packed__)) shadow_vmcs { > Since this is in guest memory, we need it packed so the binary format is preserved across migration. Please add a comment so it isn't changed (at least without changing the revision_id). vmclear state should be here, that will help multiguest support. > > struct nested_vmx { > /* Has the level1 guest done vmxon? */ > bool vmxon; > - > + /* What is the location of the vmcs l1 keeps for l2? (in level1 gpa) */ > + u64 vmptr; > Need to expose it for live migration. > /* > * Level 2 state : includes vmcs,registers and > * a copy of vmcs12 for vmread/vmwrite > */ > struct level_state *l2_state; > + /* Level 1 state for switching to level 2 and back */ > + struct level_state *l1_state; > This creates a ton of duplication. Some of the data is completely unnecessary, for example we can recalculate cr0 from HOST_CR0 and GUEST_CR0. > + > +static int vmptrld(struct kvm_vcpu *vcpu, > + u64 phys_addr) > +{ > + u8 error; > + > + asm volatile (__ex(ASM_VMX_VMPTRLD_RAX) "; setna %0" > + : "=g"(error) : "a"(&phys_addr), "m"(phys_addr) > + : "cc"); > + if (error) { > + printk(KERN_ERR "kvm: %s vmptrld %llx failed\n", > + __func__, phys_addr); > + return 1; > + } > + > + return 0; > +} > + > /* > * Switches to specified vcpu, until a matching vcpu_put(), but assumes > * vcpu mutex is already taken. > @@ -736,15 +923,8 @@ 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; > - 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", > - vmx->vmcs, phys_addr); > + vmptrld(vcpu, phys_addr); > } > This part of the patch is no longer needed. > + if (cpu_has_vmx_msr_bitmap()) > + vmx->nested.l2_state->msr_bitmap = vmcs_read64(MSR_BITMAP); > + else > + vmx->nested.l2_state->msr_bitmap = 0; > + > + vmx->nested.l2_state->io_bitmap_a = vmcs_read64(IO_BITMAP_A); > + vmx->nested.l2_state->io_bitmap_b = vmcs_read64(IO_BITMAP_B); > + > This no longer works, since we don't load the guest vmcs. > +int kvm_read_guest_virt(gva_t addr, void *val, unsigned int bytes, > + struct kvm_vcpu *vcpu); > Isn't this in a header somewhere? > + > +int read_guest_vmcs_gpa(struct kvm_vcpu *vcpu, u64 *gentry) > +{ > + > + int r = 0; > + > + r = kvm_read_guest_virt(vcpu->arch.regs[VCPU_REGS_RAX], gentry, > + sizeof(u64), vcpu); > + if (r) { > + printk(KERN_ERR "%s cannot read guest vmcs addr %lx : %d\n", > + __func__, vcpu->arch.regs[VCPU_REGS_RAX], r); > + return r; > + } > + > + if (!IS_ALIGNED(*gentry, PAGE_SIZE)) { > + printk(KERN_DEBUG "%s addr %llx not aligned\n", > + __func__, *gentry); > + return 1; > + } > + > + return 0; > +} > + > Should go through the emulator to evaluate arguments. > +static int handle_vmptrld(struct kvm_vcpu *vcpu) > +{ > + struct vcpu_vmx *vmx = to_vmx(vcpu); > + struct page *vmcs_page; > + u64 guest_vmcs_addr; > + > + if (!nested_vmx_check_permission(vcpu)) > + return 1; > + > + if (read_guest_vmcs_gpa(vcpu,&guest_vmcs_addr)) > + return 1; > + > + if (create_l1_state(vcpu)) { > + printk(KERN_ERR "%s create_l1_state failed\n", __func__); > + return 1; > + } > + > + if (create_l2_state(vcpu)) { > + printk(KERN_ERR "%s create_l2_state failed\n", __func__); > + return 1; > + } > return errors here, so we see the problem. > + > +static int handle_vmptrst(struct kvm_vcpu *vcpu) > +{ > + int r = 0; > + > + if (!nested_vmx_check_permission(vcpu)) > + return 1; > + > + r = kvm_write_guest_virt(vcpu->arch.regs[VCPU_REGS_RAX], > + (void *)&to_vmx(vcpu)->nested.vmptr, > + sizeof(u64), vcpu); > Emulator again. > +void save_vmcs(struct shadow_vmcs *dst) > +{ > > + dst->io_bitmap_a = vmcs_read64(IO_BITMAP_A); > + dst->io_bitmap_b = vmcs_read64(IO_BITMAP_B); > These (and many others) can never change due to a nested guest running, so no need to save them. > + dst->virtual_apic_page_addr = vmcs_read64(VIRTUAL_APIC_PAGE_ADDR); > In general, you need to translate host physical addresses to guest physical addresses. > + dst->apic_access_addr = vmcs_read64(APIC_ACCESS_ADDR); > + if (enable_ept) > + dst->ept_pointer = vmcs_read64(EPT_POINTER); > + > Not all hosts support these features. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.