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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Mar 16, 2016 at 02:01:38PM -0400, Boris Ostrovsky wrote:
> 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?

Yes, I will.

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

Yes, there are pointer to the same part of memory, with different types.
By droping start_page, I could replace few "start_page+sizeof(*start_info)"
by "start_info + 1" :), and have to use a cast for the line abrove.

-- 
Anthony PERARD

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