Re: [Xen-devel] [PATCH v6 26/29] libxc/xen: introduce a start info structure for HVMlite guests

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.09.15 at 14:09, <roger.pau@xxxxxxxxxx> wrote:

First of all - I suppose it is intentional for this to not consider the Dom0
side (yet)?

> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -560,7 +560,70 @@ 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 = 0;
> +        size_t start_info_size = sizeof(*start_info);
> +
> +        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. */
> +
> +        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) +

Not knowing much about the tools interface used for allocation
above: Does that interface guarantee this shift (and another
one below) to not overflow?

> +                                ((xen_pfn_t)cmdline - (xen_pfn_t)start_info);

xen_pfn_t? Aren't these byte addresses?

> --- 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 PVH guests in %ebx.
> + */

This is a single line comment.

> +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 {
> +    uint32_t paddr;             /* Physical address of the module.           
> */
> +    uint32_t size;              /* Size of the module in bytes.              
> */
> +};

Iirc this already went back and forth, but - is this meant to be an
x86-specific interface? If not, should we really limit physical
addresses to 32 bits here?

Also this now sits inside a XEN_HAVE_PV_GUEST_ENTRY conditional
- is that intended?

Jan


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