From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 5/24] Introduce vmcs12: a VMCS structure for L1 Date: Mon, 14 Jun 2010 11:33:33 +0300 Message-ID: <4C15E95D.9000300@redhat.com> References: <1276431753-nyh@il.ibm.com> <201006131225.o5DCP79H012922@rice.haifa.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 To: "Nadav Har'El" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:6561 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754461Ab0FNIdh (ORCPT ); Mon, 14 Jun 2010 04:33:37 -0400 In-Reply-To: <201006131225.o5DCP79H012922@rice.haifa.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: On 06/13/2010 03:25 PM, Nadav Har'El wrote: > An implementation of VMX needs to define a VMCS structure. This structure > is kept in guest memory, but is opaque to the guest (who can only read or > write it with VMX instructions). > > This patch starts to define the VMCS structure which our nested VMX > implementation will present to L1. We call it "vmcs12", as it is the VMCS > that L1 keeps for its L2 guests. > > This patch also adds the notion (as required by the VMX spec) of the "current > VMCS", and finally includes utility functions for mapping the guest-allocated > VMCSs in host memory. > > +#define VMCS12_REVISION 0x11e57ed0 > Where did this number come from? It's not from real hardware, yes? > + > +/* > + * struct vmcs12 describes the state that our guest hypervisor (L1) keeps for a > + * single nested guest (L2), hence the name vmcs12. Any VMX implementation has > + * a VMCS structure (which is opaque to the guest), and vmcs12 is our emulated > + * VMX's VMCS. This structure is stored in guest memory specified by VMPTRLD, > + * and accessed by the guest using VMREAD/VMWRITE/VMCLEAR instructions. More > + * than one of these structures may exist, if L1 runs multiple L2 guests. > + * nested_vmx_run() will use the data here to build a VMCS for the underlying > + * hardware which will be used to run L2. > + * This structure is packed in order to preseve the binary content after live > + * migration. If there are changes in the content or layout, VMCS12_REVISION > + * must be changed. > + */ > +struct __attribute__ ((__packed__)) vmcs12 { > __packed is a convenient define for this. > + /* According to the Intel spec, a VMCS region must start with the > + * following two fields. Then follow implementation-specific data. > + */ > + u32 revision_id; > + u32 abort; > +}; > Note that this structure becomes an ABI, it cannot change except in a backward compatible way due to the need for live migration. So I'd like a documentation patch that adds a description of the content to Documentation/kvm/. It can be as simple as listing the structure definition. > > +static struct page *nested_get_page(struct kvm_vcpu *vcpu, u64 vmcs_addr) > +{ > + struct page *vmcs_page = > + gfn_to_page(vcpu->kvm, vmcs_addr>> PAGE_SHIFT); > + > + if (is_error_page(vmcs_page)) { > + printk(KERN_ERR "%s error allocating page 0x%llx\n", > + __func__, vmcs_addr); > Those printks can be used by a malicious guest to span the host logs. Please wrap them with something that is conditional on a debug flag. I'm not sure what we need to do with vmcs that is not in RAM. It may simplify things to return the error_page to the caller and set KVM_REQ_TRIPLE_FAULT, so we don't have to deal with error handling later on. > + kvm_release_page_clean(vmcs_page); > + return NULL; > + } > + return vmcs_page; > +} > + > +static void nested_unmap_current(struct kvm_vcpu *vcpu) > +{ > + struct page *page; > + struct vcpu_vmx *vmx = to_vmx(vcpu); > + > + if (!vmx->nested.current_l2_page) { > + printk(KERN_INFO "Shadow vmcs already unmapped\n"); > + BUG_ON(1); > + return; > + } > + > + page = kmap_atomic_to_page(vmx->nested.current_l2_page); > + > + kunmap_atomic(vmx->nested.current_l2_page, KM_USER0); > + > + kvm_release_page_dirty(page); > Do we always dirty the page? I guess it is no big deal even if we don't. -- error compiling committee.c: too many arguments to function