Re: [PATCH v5 26/28] libxc/xen: introduce a start info structure for HVMlite guests

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

On 21/08/15 17:53, Roger Pau Monne wrote:
> This structure contains the physical address of the command line, as well as
> the physical address of the list of loaded modules. The physical address of
> this structure is passed to the guest at boot time in the %ebx register.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>

This looks much nicer than the previous method.  A few comments.

> ---
>  tools/libxc/xc_dom_x86.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-
>  xen/include/public/xen.h | 19 +++++++++++++++
>  2 files changed, 78 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index fb71717..3b40ba1 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -560,7 +560,62 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>      xc_hvm_param_set(xch, domid, HVM_PARAM_SHARING_RING_PFN,
>                       special_pfn(SPECIALPAGE_SHARING));
>  
> -    if ( dom->device_model )
> +    if ( !dom->device_model )
> +    {
> +        struct xc_dom_seg seg;
> +        struct hvm_start_info *start_info;
> +        char *cmdline;
> +        struct hvm_modlist_entry *modlist;
> +        void *start_page;
> +        size_t cmdline_size = dom->cmdline ? strlen(dom->cmdline) + 1 : 0;
> +        size_t start_info_size = sizeof(*start_info) + cmdline_size +

I would round cmdline_size up to the nearest 8, to align the modlist.

> +                                 dom->ramdisk_blob ? sizeof(*modlist)

sizeof(*modlist) * nr_entries.

This currently works because there is only ever one extra blob, but will
be wrong in the future when we want to pass other blobs.  (I have some
plans to fix the current requirements to build seabios into hvmloader,
and make it possible to pass things like NVRAM blobs.)

>  : 0;

I would suggest:

size_t start_info_size = sizeof(*start_info);
if ( dom->cmdline )
    start_info_size += ROUNDUP(strlen(dom->cmdline) + 1, 8);
if ( dom->ramdisk_blob )
    start_info_size += sizeof(*modlist); /* Only one module at the
moment. */

As a clearer way of expressing the logic.

> +
> +        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,
> +                                          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, MAX_GUEST_CMDLINE);
> +            cmdline[MAX_GUEST_CMDLINE - 1] = '\0';
> +            start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) +
> +                                        sizeof(*start_info);

As the layout starts to get more complicated, this style of calculation
would be cleaner as

(seg.pfn << PAGE_SHIFT) + (cmdline - 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) +
> +                                        sizeof(*start_info) + cmdline_size;
> +            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
>      {
>          /*
>           * Allocate and clear additional ioreq server pages. The default
> @@ -916,6 +971,9 @@ static int vcpu_hvm(struct xc_dom_image *dom)
>      /* Set the IP. */
>      bsp_ctx.cpu.rip = dom->parms.phys_entry;
>  
> +    if ( dom->start_info_pfn )
> +        bsp_ctx.cpu.rbx = dom->start_info_pfn << PAGE_SHIFT;
> +
>      /* Set the end descriptor. */
>      bsp_ctx.end_d.typecode = HVM_SAVE_CODE(END);
>      bsp_ctx.end_d.instance = 0;
> diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> index ff5547e..ca6a066 100644
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -784,6 +784,25 @@ struct start_info {
>  };
>  typedef struct start_info start_info_t;
>  
> +/*
> + * Start of day structure passed to HVMlite guests in %ebx.

As we are planning to rename HVMlite to PVH, I would avoid the use of
HVMlite in committed code.

Also as this is HVM specific, might it be better in public/hvm?  I guess
that depends on whether ARM is likely to use it.

~Andrew

> + */
> +struct hvm_start_info {
> +#define HVM_START_MAGIC_VALUE 0x336ec578
> +    uint32_t magic;             /* Contains the magic value 0x336ec578       */
> +                                /* ("xEn3" with the 0x80 bit of the "E" set).*/
> +    uint32_t flags;             /* SIF_xxx flags.                            */
> +    uint32_t cmdline_paddr;     /* Physical address of the command line.     */
> +    uint32_t nr_modules;        /* Number of modules passed to the kernel.   */
> +    uint32_t modlist_paddr;     /* Physical address of an array of           */
> +                                /* hvm_modlist_entry.                        */
> +};
> +
> +struct hvm_modlist_entry {
> +    uint64_t paddr;             /* Physical address of the module.           */
> +    uint64_t size;              /* Size of the module in bytes.              */
> +};
> +
>  /* New console union for dom0 introduced in 0x00030203. */
>  #if __XEN_INTERFACE_VERSION__ < 0x00030203
>  #define console_mfn    console.domU.mfn


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel