From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v2 3/4] libxc: rework vnuma bits in setup_guest Date: Fri, 29 May 2015 11:02:16 -0400 Message-ID: <55687F78.8080802@oracle.com> References: <1432899434-1534-1-git-send-email-wei.liu2@citrix.com> <1432899434-1534-4-git-send-email-wei.liu2@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YyLnn-0000oV-Tb for xen-devel@lists.xenproject.org; Fri, 29 May 2015 15:02:28 +0000 In-Reply-To: <1432899434-1534-4-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: Dario Faggioli , Ian Jackson , Ian Campbell List-Id: xen-devel@lists.xenproject.org On 05/29/2015 07:37 AM, Wei Liu wrote: > Make the setup process similar to PV counterpart. That is, to allocate a > P2M array that covers the whole memory range and start from there. This > is clearer than using an array with no holes in it. > > Also the dummy layout should take MMIO hole into consideration. We might > end up having two vmemranges in the dummy layout. > > Signed-off-by: Wei Liu > Cc: Ian Campbell > Cc: Ian Jackson > --- > v2: only generate 1 vnode in dummy layout > --- > tools/libxc/xc_hvm_build_x86.c | 62 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 47 insertions(+), 15 deletions(-) > > diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c > index df4b7ed..b3855e0 100644 > --- a/tools/libxc/xc_hvm_build_x86.c > +++ b/tools/libxc/xc_hvm_build_x86.c > @@ -238,6 +238,7 @@ 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 p2m_size; > unsigned long target_pages = args->mem_target >> PAGE_SHIFT; > unsigned long entry_eip, cur_pages, cur_pfn; > void *hvm_info_page; > @@ -254,8 +255,8 @@ static int setup_guest(xc_interface *xch, > xen_pfn_t special_array[NR_SPECIAL_PAGES]; > xen_pfn_t ioreq_server_array[NR_IOREQ_SERVER_PAGES]; > uint64_t total_pages; > - xen_vmemrange_t dummy_vmemrange; > - unsigned int dummy_vnode_to_pnode; > + xen_vmemrange_t dummy_vmemrange[2]; > + unsigned int dummy_vnode_to_pnode[1]; > > memset(&elf, 0, sizeof(elf)); > if ( elf_init(&elf, image, image_size) != 0 ) > @@ -275,17 +276,35 @@ static int setup_guest(xc_interface *xch, > > if ( args->nr_vmemranges == 0 ) > { > - /* Build dummy vnode information */ > - dummy_vmemrange.start = 0; > - dummy_vmemrange.end = args->mem_size; > - dummy_vmemrange.flags = 0; > - dummy_vmemrange.nid = 0; > + /* Build dummy vnode information > + * > + * Guest physical address space layout: > + * [0, hole_start) [hole_start, 4G) [4G, highmem_end) > + * > + * Of course if there is no high memory, the second vmemrange > + * has no effect on the actual result. > + */ > + > + dummy_vmemrange[0].start = 0; > + dummy_vmemrange[0].end = args->lowmem_end; > + dummy_vmemrange[0].flags = 0; > + dummy_vmemrange[0].nid = 0; > args->nr_vmemranges = 1; > - args->vmemranges = &dummy_vmemrange; > > - dummy_vnode_to_pnode = XC_NUMA_NO_NODE; > + if ( args->highmem_end > (1ULL << 32) ) > + { > + dummy_vmemrange[1].start = 1ULL << 32; > + dummy_vmemrange[1].end = args->highmem_end; > + dummy_vmemrange[1].flags = 0; > + dummy_vmemrange[1].nid = 0; > + > + args->nr_vmemranges++; > + } > + > + dummy_vnode_to_pnode[0] = XC_NUMA_NO_NODE; > args->nr_vnodes = 1; > - args->vnode_to_pnode = &dummy_vnode_to_pnode; > + args->vmemranges = dummy_vmemrange; > + args->vnode_to_pnode = dummy_vnode_to_pnode; I should have asked last time: we are setting args->vmemranges and args->vnode_to_pnode to point to something on the stack. Is it safe to assume that these pointers will never be used outside of setup_guest()? (and if it is, should they be set to NULL before returning, just in case?) I also had a very cursory look at their usage and I think I see in some cases memory is allocated (and presumably then freed), but I am not sure whether this on some other code path. -boris > } > else > { > @@ -297,9 +316,15 @@ static int setup_guest(xc_interface *xch, > } > > total_pages = 0; > + p2m_size = 0; > for ( i = 0; i < args->nr_vmemranges; i++ ) > + { > total_pages += ((args->vmemranges[i].end - args->vmemranges[i].start) > >> PAGE_SHIFT); > + p2m_size = p2m_size > (args->vmemranges[i].end >> PAGE_SHIFT) ? > + p2m_size : (args->vmemranges[i].end >> PAGE_SHIFT); > + } > + > if ( total_pages != (args->mem_size >> PAGE_SHIFT) ) > { > PERROR("vNUMA memory pages mismatch (0x%"PRIx64" != 0x%"PRIx64")", > @@ -325,16 +350,23 @@ static int setup_guest(xc_interface *xch, > DPRINTF(" TOTAL: %016"PRIx64"->%016"PRIx64"\n", v_start, v_end); > DPRINTF(" ENTRY: %016"PRIx64"\n", elf_uval(&elf, elf.ehdr, e_entry)); > > - if ( (page_array = malloc(nr_pages * sizeof(xen_pfn_t))) == NULL ) > + if ( (page_array = malloc(p2m_size * sizeof(xen_pfn_t))) == NULL ) > { > PERROR("Could not allocate memory."); > goto error_out; > } > > - for ( i = 0; i < nr_pages; i++ ) > - page_array[i] = i; > - for ( i = args->mmio_start >> PAGE_SHIFT; i < nr_pages; i++ ) > - page_array[i] += args->mmio_size >> PAGE_SHIFT; > + for ( i = 0; i < p2m_size; i++ ) > + page_array[i] = ((xen_pfn_t)-1); > + for ( vmemid = 0; vmemid < args->nr_vmemranges; vmemid++ ) > + { > + uint64_t pfn; > + > + for ( pfn = args->vmemranges[vmemid].start >> PAGE_SHIFT; > + pfn < args->vmemranges[vmemid].end >> PAGE_SHIFT; > + pfn++ ) > + page_array[pfn] = pfn; > + } > > /* > * Try to claim pages for early warning of insufficient memory available.