Re: [PATCH v4 02/14] libxc: Prepare a start info structure for hvmloader

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 03/15/2016 08:18 PM, Konrad Rzeszutek Wilk wrote:
> On Mon, Mar 14, 2016 at 05:55:37PM +0000, Anthony PERARD wrote:
>


>> @@ -624,8 +628,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>>   
>>       if ( !dom->device_model )
>>       {
>> -        size_t start_info_size = sizeof(struct hvm_start_info);
>> -
>>           if ( dom->cmdline )
>>           {
>>               dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8);
>> @@ -635,17 +637,26 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>>           /* Limited to one module. */
>>           if ( dom->ramdisk_blob )
>>               start_info_size += sizeof(struct hvm_modlist_entry);
>> -
>> -        rc = xc_dom_alloc_segment(dom, &dom->start_info_seg,
>> -                                  "HVMlite start info", 0, start_info_size);
>> -        if ( rc != 0 )
>> -        {
>> -            DOMPRINTF("Unable to reserve memory for the start info");
>> -            goto out;
>> -        }
>>       }
>>       else
>>       {
>> +        start_info_size +=
>> +            sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;
>> +        /* Add extra space to write modules name */
>> +        start_info_size +=
>> +            HVMLOADER_MODULE_NAME_SIZE * HVMLOADER_MODULE_MAX_COUNT;
> What about \0 ? Ah, the strncpy we use adds \0 byte. But it would be nice
> to mention that somewhere. Perhaps mention:
>
> The HVMLOADER_MODULE_NAME_SIZE accounts for NUL byte?
>
>> +    }
>> +
>> +    rc = xc_dom_alloc_segment(dom, &dom->start_info_seg,
>> +                              "HVMlite start info", 0, start_info_size);
>> +    if ( rc != 0 )
>> +    {
>> +        DOMPRINTF("Unable to reserve memory for the start info");
>> +        goto out;
>> +    }
>> +
>> +    if ( dom->device_model )
>> +    {


Can you fold this into the 'else' clause above and move 
xc_dom_alloc_segment() down?


>> +
>>   static int bootlate_hvm(struct xc_dom_image *dom)
>>   {
>>       uint32_t domid = dom->guest_domid;
>>       xc_interface *xch = dom->xch;
>> +    struct hvm_start_info *start_info;
>> +    size_t start_info_size;
>> +    void *start_page;
>> +    struct hvm_modlist_entry *modlist;
>>   
>> -    if ( !dom->device_model )
>> -    {
>> -        struct hvm_start_info *start_info;
>> -        size_t start_info_size;
>> -        void *start_page;
>> -
>> -        start_info_size = sizeof(*start_info) + dom->cmdline_size;
>> -        if ( dom->ramdisk_blob )
>> -            start_info_size += sizeof(struct hvm_modlist_entry);
>> +    start_info_size = sizeof(*start_info) + dom->cmdline_size;
>> +    if ( dom->ramdisk_blob )
>> +        start_info_size += sizeof(struct hvm_modlist_entry);
>>   
>> -        if ( start_info_size >
>> -             dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) )
>> -        {
>> -            DOMPRINTF("Trying to map beyond start_info_seg");
>> -            return -1;
>> -        }
>> +    if ( start_info_size >
>> +         dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) )
>> +    {
>> +        DOMPRINTF("Trying to map beyond start_info_seg");
>> +        return -1;
>> +    }
>>   
>> -        start_page = xc_map_foreign_range(xch, domid, start_info_size,
>> -                                          PROT_READ | PROT_WRITE,
>> -                                          dom->start_info_seg.pfn);
>> -        if ( start_page == NULL )
>> -        {
>> -            DOMPRINTF("Unable to map HVM start info page");
>> -            return -1;
>> -        }
>> +    start_page = xc_map_foreign_range(xch, domid, start_info_size,
>> +                                      PROT_READ | PROT_WRITE,
>> +                                      dom->start_info_seg.pfn);
>> +    if ( start_page == NULL )
>> +    {
>> +        DOMPRINTF("Unable to map HVM start info page");
>> +        return -1;
>> +    }
>>   
>> -        start_info = start_page;
>> +    start_info = start_page;
>> +    modlist = start_page + sizeof(*start_info) + dom->cmdline_size;

I think we can drop start_page and use start_info only. They are the 
same, aren't they?

-boris



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