Re: [PATCH v2 3/4] libxc: rework vnuma bits in setup_guest

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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 <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
> 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.