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

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

El 21/09/15 a les 17.53, Jan Beulich ha escrit:
>>>> On 04.09.15 at 14:09, <roger.pau@citrix.com> wrote:
> 
> First of all - I suppose it is intentional for this to not consider the Dom0
> side (yet)?

Yes, let's leave Dom0 for a later patch series please, this is already
big enough.

>> --- 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?

I can add a check to make sure pfn are always below the 4GB boundary.

>> +                                ((xen_pfn_t)cmdline - (xen_pfn_t)start_info);
> 
> xen_pfn_t? Aren't these byte addresses?

Right, these are not pfns.

>> --- 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.

Ack.

>> +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?

That's what I though initially (x86 only), and nobody on the ARM side
expressed interest in using it there. Ian, Stefano, do you foresee this
being used for ARM also?

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

That's hard to tell, I would consider a guest/kernel using this
structure a PV guest (PVH), but I'm not sure if we can make a clear cut
here.

Roger.