From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v2 1/4] libxc/libxl: fill xc_hvm_build_args in libxl Date: Fri, 29 May 2015 13:29:18 +0100 Message-ID: <55685B9E.3000703@citrix.com> References: <1432899434-1534-1-git-send-email-wei.liu2@citrix.com> <1432899434-1534-2-git-send-email-wei.liu2@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YyJPm-0005wN-3C for xen-devel@lists.xenproject.org; Fri, 29 May 2015 12:29:30 +0000 In-Reply-To: <1432899434-1534-2-git-send-email-wei.liu2@citrix.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: Wei Liu , Xen-devel Cc: Boris Ostrovsky , "Chen, Tiejun" , Dario Faggioli , Ian Jackson , Ian Campbell List-Id: xen-devel@lists.xenproject.org On 29/05/15 12:37, Wei Liu wrote: > When building HVM guests, originally some fields of xc_hvm_build_args > are filled in xc_hvm_build (and buried in the wrong function), some are > set in libxl__build_hvm before passing xc_hvm_build_args to > xc_hvm_build. This is fragile. > > After examining the code in xc_hvm_build that sets those fields, we can > in fact move setting of mmio_start etc in libxl. This way we consolidate > memory layout setting in libxl. > > The setting of firmware data related fields is left in xc_hvm_build > because it depends on parsing ELF image. Those fields only point to > scratch data that doesn't affect memory layout. > > There should be no change in the generated guest memory layout. > > Signed-off-by: Wei Liu > Cc: Ian Campbell > Cc: Ian Jackson > --- > Cc: "Chen, Tiejun" > > This might affect your RMRR patch series. It should at least me noted that this is a semantic change in domain construction for all other toolstacks out there, an aid to the unlucky person forward porting something like Xapi and finding that a chunk of work is no longer performed by libxc. > > I once said xc_hvm_build would touch various xc_hvm_build_args fields > that would affect guest memory layout. It won't be that case anymore > with this patch. > --- > tools/libxc/xc_hvm_build_x86.c | 37 +++++++------------------------------ > tools/libxl/libxl_dom.c | 16 ++++++++++++++++ > 2 files changed, 23 insertions(+), 30 deletions(-) > > diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c > index e45ae4a..92422bf 100644 > --- a/tools/libxc/xc_hvm_build_x86.c > +++ b/tools/libxc/xc_hvm_build_x86.c > @@ -88,22 +88,14 @@ static int modules_init(struct xc_hvm_build_args *args, > return 0; > } > > -static void build_hvm_info(void *hvm_info_page, uint64_t mem_size, > - uint64_t mmio_start, uint64_t mmio_size, > +static void build_hvm_info(void *hvm_info_page, > struct xc_hvm_build_args *args) > { > struct hvm_info_table *hvm_info = (struct hvm_info_table *) > (((unsigned char *)hvm_info_page) + HVM_INFO_OFFSET); > - uint64_t lowmem_end = mem_size, highmem_end = 0; > uint8_t sum; > int i; > > - if ( lowmem_end > mmio_start ) > - { > - highmem_end = (1ull<<32) + (lowmem_end - mmio_start); > - lowmem_end = mmio_start; > - } > - > memset(hvm_info_page, 0, PAGE_SIZE); > > /* Fill in the header. */ > @@ -116,14 +108,10 @@ static void build_hvm_info(void *hvm_info_page, uint64_t mem_size, > memset(hvm_info->vcpu_online, 0xff, sizeof(hvm_info->vcpu_online)); > > /* Memory parameters. */ > - hvm_info->low_mem_pgend = lowmem_end >> PAGE_SHIFT; > - hvm_info->high_mem_pgend = highmem_end >> PAGE_SHIFT; > + hvm_info->low_mem_pgend = args->lowmem_end >> PAGE_SHIFT; > + hvm_info->high_mem_pgend = args->highmem_end >> PAGE_SHIFT; > hvm_info->reserved_mem_pgstart = ioreq_server_pfn(0); > > - args->lowmem_end = lowmem_end; > - args->highmem_end = highmem_end; > - args->mmio_start = mmio_start; > - > /* Finish with the checksum. */ > for ( i = 0, sum = 0; i < hvm_info->length; i++ ) > sum += ((uint8_t *)hvm_info)[i]; > @@ -251,8 +239,6 @@ static int setup_guest(xc_interface *xch, > xen_pfn_t *page_array = NULL; > unsigned long i, vmemid, nr_pages = args->mem_size >> PAGE_SHIFT; > unsigned long target_pages = args->mem_target >> PAGE_SHIFT; > - uint64_t mmio_start = (1ull << 32) - args->mmio_size; > - uint64_t mmio_size = args->mmio_size; > unsigned long entry_eip, cur_pages, cur_pfn; > void *hvm_info_page; > uint32_t *ident_pt; > @@ -344,8 +330,8 @@ static int setup_guest(xc_interface *xch, > > for ( i = 0; i < nr_pages; i++ ) > page_array[i] = i; > - for ( i = mmio_start >> PAGE_SHIFT; i < nr_pages; i++ ) > - page_array[i] += mmio_size >> PAGE_SHIFT; > + for ( i = args->mmio_start >> PAGE_SHIFT; i < nr_pages; i++ ) > + page_array[i] += args->mmio_size >> PAGE_SHIFT; > > /* > * Try to claim pages for early warning of insufficient memory available. > @@ -446,7 +432,7 @@ static int setup_guest(xc_interface *xch, > * range */ > !check_mmio_hole(cur_pfn << PAGE_SHIFT, > SUPERPAGE_1GB_NR_PFNS << PAGE_SHIFT, > - mmio_start, mmio_size) ) > + args->mmio_start, args->mmio_size) ) > { > long done; > unsigned long nr_extents = count >> SUPERPAGE_1GB_SHIFT; > @@ -545,7 +531,7 @@ static int setup_guest(xc_interface *xch, > xch, dom, PAGE_SIZE, PROT_READ | PROT_WRITE, > HVM_INFO_PFN)) == NULL ) > goto error_out; > - build_hvm_info(hvm_info_page, v_end, mmio_start, mmio_size, args); > + build_hvm_info(hvm_info_page, args); > munmap(hvm_info_page, PAGE_SIZE); > > /* Allocate and clear special pages. */ > @@ -661,12 +647,6 @@ int xc_hvm_build(xc_interface *xch, uint32_t domid, > if ( args.image_file_name == NULL ) > return -1; > > - if ( args.mem_target == 0 ) > - args.mem_target = args.mem_size; > - > - if ( args.mmio_size == 0 ) > - args.mmio_size = HVM_BELOW_4G_MMIO_LENGTH; > - > /* An HVM guest must be initialised with at least 2MB memory. */ > if ( args.mem_size < (2ull << 20) || args.mem_target < (2ull << 20) ) > return -1; > @@ -684,9 +664,6 @@ int xc_hvm_build(xc_interface *xch, uint32_t domid, > args.acpi_module.guest_addr_out; > hvm_args->smbios_module.guest_addr_out = > args.smbios_module.guest_addr_out; > - hvm_args->lowmem_end = args.lowmem_end; > - hvm_args->highmem_end = args.highmem_end; > - hvm_args->mmio_start = args.mmio_start; > } > > free(image); > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c > index a0c9850..dccc9ac 100644 > --- a/tools/libxl/libxl_dom.c > +++ b/tools/libxl/libxl_dom.c > @@ -920,6 +920,7 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid, > libxl_ctx *ctx = libxl__gc_owner(gc); > struct xc_hvm_build_args args = {}; > int ret, rc = ERROR_FAIL; > + uint64_t mmio_start, lowmem_end, highmem_end; > > memset(&args, 0, sizeof(struct xc_hvm_build_args)); > /* The params from the configuration file are in Mb, which are then > @@ -941,6 +942,21 @@ int libxl__build_hvm(libxl__gc *gc, uint32_t domid, > LOG(ERROR, "initializing domain firmware failed"); > goto out; > } > + if (args.mem_target == 0) > + args.mem_target = args.mem_size; > + if (args.mmio_size == 0) > + args.mmio_size = HVM_BELOW_4G_MMIO_LENGTH; > + lowmem_end = args.mem_size; > + highmem_end = 0; > + mmio_start = (1ull << 32) - args.mmio_size; > + if (lowmem_end > mmio_start) > + { > + highmem_end = (1ull << 32) + (lowmem_end - mmio_start); > + lowmem_end = mmio_start; > + } > + args.lowmem_end = lowmem_end; > + args.highmem_end = highmem_end; > + args.mmio_start = mmio_start; > > if (info->num_vnuma_nodes != 0) { > int i;