Re: [PATCH v3 2/2] libxc: Defer initialization of start_page for HVM guests

["Roger Pau Monné" <roger.pau@citrix.com>]

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 <boris.ostrovsky@oracle.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  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.