Re: [PATCH v2 1/4] libxc/libxl: fill xc_hvm_build_args in libxl

[Andrew Cooper <andrew.cooper3@citrix.com>]

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 <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
> Cc: "Chen, Tiejun" <tiejun.chen@intel.com>
>
> 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;