From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= Subject: Re: [PATCH v3 2/2] libxc: Defer initialization of start_page for HVM guests Date: Thu, 7 Jan 2016 12:43:21 +0100 Message-ID: <568E4F59.4080602@citrix.com> References: <1452110602-3570-1-git-send-email-boris.ostrovsky@oracle.com> <1452110602-3570-3-git-send-email-boris.ostrovsky@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1452110602-3570-3-git-send-email-boris.ostrovsky@oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Boris Ostrovsky , ian.jackson@eu.citrix.com, stefano.stabellini@eu.citrix.com, ian.campbell@citrix.com, wei.liu2@citrix.com Cc: jgross@suse.com, andrew.cooper3@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org El 06/01/16 a les 21.03, Boris Ostrovsky ha escrit: > With commit 8c45adec18e0 ("libxc: create unmapped initrd in domain > builder if supported") location of ramdisk may not be available to > HVMlite guests by the time alloc_magic_pages_hvm() is invoked if the > guest supports unmapped initrd. > > So let's move ramdisk info initialization (along with a few other > operations that are not directly related to allocating magic/special > pages) from alloc_magic_pages_hvm() to bootlate_hvm(). > > Signed-off-by: Boris Ostrovsky > Acked-by: Wei Liu > --- > tools/libxc/xc_dom_x86.c | 116 ++++++++++++++++++++++++++------------------- > 1 files changed, 67 insertions(+), 49 deletions(-) > > diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c > index b8d2904..e102bd2 100644 > --- a/tools/libxc/xc_dom_x86.c > +++ b/tools/libxc/xc_dom_x86.c > @@ -586,23 +586,12 @@ static void build_hvm_info(void *hvm_info_page, struct xc_dom_image *dom) > static int alloc_magic_pages_hvm(struct xc_dom_image *dom) > { > unsigned long i; > - void *hvm_info_page; > uint32_t *ident_pt, domid = dom->guest_domid; > int rc; > xen_pfn_t special_array[X86_HVM_NR_SPECIAL_PAGES]; > xen_pfn_t ioreq_server_array[NR_IOREQ_SERVER_PAGES]; > xc_interface *xch = dom->xch; > > - if ( dom->device_model ) > - { > - if ( (hvm_info_page = xc_map_foreign_range( > - xch, domid, PAGE_SIZE, PROT_READ | PROT_WRITE, > - HVM_INFO_PFN)) == NULL ) > - goto error_out; > - build_hvm_info(hvm_info_page, dom); > - munmap(hvm_info_page, PAGE_SIZE); > - } > - > /* Allocate and clear special pages. */ > for ( i = 0; i < X86_HVM_NR_SPECIAL_PAGES; i++ ) > special_array[i] = special_pfn(i); > @@ -637,12 +626,9 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) > if ( !dom->device_model ) > { > struct xc_dom_seg seg; > - struct hvm_start_info *start_info; > - char *cmdline; > struct hvm_modlist_entry *modlist; This can be removed if the conditional below is changed to: if ( dom->ramdisk_blob ) - start_info_size += sizeof(*modlist); + start_info_size += sizeof(struct hvm_modlist_entry); Because AFAICT the variable itself is not used for anything else. > - void *start_page; > size_t cmdline_size = 0; > - size_t start_info_size = sizeof(*start_info); > + size_t start_info_size = sizeof(struct hvm_start_info); > > if ( dom->cmdline ) > { > @@ -661,39 +647,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom) > goto out; > } > > - start_page = xc_map_foreign_range(xch, domid, start_info_size, > - PROT_READ | PROT_WRITE, > - seg.pfn); > - if ( start_page == NULL ) > - { > - DOMPRINTF("Unable to map HVM start info page"); > - goto error_out; > - } > - > - start_info = start_page; > - cmdline = start_page + sizeof(*start_info); > - modlist = start_page + sizeof(*start_info) + cmdline_size; > - > - if ( dom->cmdline ) > - { > - strncpy(cmdline, dom->cmdline, cmdline_size); > - start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) + > - ((uintptr_t)cmdline - (uintptr_t)start_info); > - } > - > - if ( dom->ramdisk_blob ) > - { > - modlist[0].paddr = dom->ramdisk_seg.vstart - dom->parms.virt_base; > - modlist[0].size = dom->ramdisk_seg.vend - dom->ramdisk_seg.vstart; > - start_info->modlist_paddr = (seg.pfn << PAGE_SHIFT) + > - ((uintptr_t)modlist - (uintptr_t)start_info); > - start_info->nr_modules = 1; > - } > - > - start_info->magic = HVM_START_MAGIC_VALUE; > - > - munmap(start_page, start_info_size); > - > dom->start_info_pfn = seg.pfn; > } > else > @@ -1783,7 +1736,72 @@ static int alloc_pgtables_hvm(struct xc_dom_image *dom) > > static int bootlate_hvm(struct xc_dom_image *dom) > { > - DOMPRINTF("%s: doing nothing", __func__); > + uint32_t domid = dom->guest_domid; > + xc_interface *xch = dom->xch; > + > + if ( !dom->device_model ) > + { > + struct hvm_start_info *start_info; > + size_t start_info_size = sizeof(*start_info); > + void *start_page; > + struct hvm_modlist_entry *modlist; > + size_t cmdline_size = 0; > + > + if ( dom->cmdline ) > + { > + cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8); > + start_info_size += cmdline_size; > + } > + if ( dom->ramdisk_blob ) > + start_info_size += sizeof(*modlist); /* Limited to one module. */ The size calculations are duplicated, could you either stash start_info_size into xc_dom_image, or simply do the memory allocation (xc_dom_alloc_segment) inside of bootlate_hvm? (I think the latter would be better if possible). Roger.