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

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

El 22/12/15 a les 0.45, 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.
> 
> Since the only operation related to allocating magic pages in that
> routine is allocation of HVMlite start info we can move everything
> else to a later point such as xc_dom_arch.start_info().
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
> I am not sure xc_dom_arch.start_info() is the right place neither since we
> still are doing things that have nothing to do with start_info.
>
>  tools/libxc/xc_dom_x86.c |   45 ++++++++++++++++++++++++++++++---------------
>  1 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index 3960875..f64079e 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -585,6 +585,32 @@ 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)
>  {
> +        struct xc_dom_seg seg;
> +        size_t start_info_size = sizeof(struct hvm_start_info);
> +
> +        if ( dom->device_model )
> +            return 0;
> +
> +        if ( dom->cmdline )
> +            start_info_size += ROUNDUP(strlen(dom->cmdline) + 1, 8);
> +        if ( dom->ramdisk_blob )
> +            /* Limited to one module. */
> +            start_info_size += sizeof( struct hvm_modlist_entry);
                                         ^ extra space.
> +
> +        if ( xc_dom_alloc_segment(dom, &seg, "HVMlite start info", 0,
> +                                  start_info_size) )
> +        {
> +            DOMPRINTF("Unable to reserve memory for the start info");
> +            return -1;
> +        }
> +
> +        dom->start_info_pfn = seg.pfn;
> +
> +        return 0;
> +}
> +
> +static int start_info_hvm(struct xc_dom_image *dom)
> +{
>      unsigned long i;
>      void *hvm_info_page;
>      uint32_t *ident_pt, domid = dom->guest_domid;
> @@ -636,7 +662,6 @@ 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;
> @@ -653,17 +678,9 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>          if ( dom->ramdisk_blob )
>              start_info_size += sizeof(*modlist); /* Limited to one module. */
>  
> -        rc = xc_dom_alloc_segment(dom, &seg, "HVMlite start info", 0,
> -                                  start_info_size);
> -        if ( rc != 0 )
> -        {
> -            DOMPRINTF("Unable to reserve memory for the start info");
> -            goto out;
> -        }
> -
>          start_page = xc_map_foreign_range(xch, domid, start_info_size,

IMHO, I would stash start_info_size somewhere inside xc_dom_image in
order to avoid calculating it twice.

Also, why is everything done inside of alloc_magic_pages_hvm moved to
start_info_hvm? AFAICT we should only need to move the code that fills
the hvm_start_info struct, but the rest of the code already present in
alloc_magic_pages_hvm could be left as-is.

Roger.