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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel