[PATCH v3 0/2] HVMlite start_info initialization fixes

[PATCH v3 0/2] HVMlite start_info initialization fixes

[Boris Ostrovsky]

v3:
* Dropped test for MAX_GUEST_CMDLINE, use strncpy(.., cmdline_size)
  instead of strcpy() in patch 1
* Dropped comment about dom->cmdline size having been tested in
  alloc_magic_pages_hvm() in patch 2

Boris Ostrovsky (2):
  libxc: Don't write terminating NULL character to command string
  libxc: Defer initialization of start_page for HVM guests

 tools/libxc/xc_dom_x86.c |  117 ++++++++++++++++++++++++++--------------------
 1 files changed, 67 insertions(+), 50 deletions(-)

[PATCH v3 1/2] libxc: Don't write terminating NULL character to command string

[Boris Ostrovsky]

When copying boot command string for HVMlite guests we explicitly write
'\0' at MAX_GUEST_CMDLINE offset. Unless the string is close to
MAX_GUEST_CMDLINE in length this write will end up in the wrong place,
beyond the end of the mapped range.

We don't need to limit the size of command string to some arbitrary
number. Any size that can be successfully allocated and mapped is valid
and so the string is guaranteed to be NULL-terminated (since we use
strlen, which needs terminating '\0', to calculate allocation size).

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 tools/libxc/xc_dom_x86.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 3960875..b8d2904 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -676,8 +676,7 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
 
         if ( dom->cmdline )
         {
-            strncpy(cmdline, dom->cmdline, MAX_GUEST_CMDLINE);
-            cmdline[MAX_GUEST_CMDLINE - 1] = '\0';
+            strncpy(cmdline, dom->cmdline, cmdline_size);
             start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) +
                                 ((uintptr_t)cmdline - (uintptr_t)start_info);
         }
-- 
1.7.1

Re: [PATCH v3 1/2] libxc: Don't write terminating NULL character to command string

[Wei Liu]

On Wed, Jan 06, 2016 at 03:03:21PM -0500, Boris Ostrovsky wrote:
> When copying boot command string for HVMlite guests we explicitly write
> '\0' at MAX_GUEST_CMDLINE offset. Unless the string is close to
> MAX_GUEST_CMDLINE in length this write will end up in the wrong place,
> beyond the end of the mapped range.
> 
> We don't need to limit the size of command string to some arbitrary
> number. Any size that can be successfully allocated and mapped is valid
> and so the string is guaranteed to be NULL-terminated (since we use
> strlen, which needs terminating '\0', to calculate allocation size).
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
>  tools/libxc/xc_dom_x86.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index 3960875..b8d2904 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -676,8 +676,7 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>  
>          if ( dom->cmdline )
>          {
> -            strncpy(cmdline, dom->cmdline, MAX_GUEST_CMDLINE);
> -            cmdline[MAX_GUEST_CMDLINE - 1] = '\0';
> +            strncpy(cmdline, dom->cmdline, cmdline_size);
>              start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) +
>                                  ((uintptr_t)cmdline - (uintptr_t)start_info);
>          }
> -- 
> 1.7.1
> 

Re: [PATCH v3 1/2] libxc: Don't write terminating NULL character to command string

[Ian Campbell]

On Thu, 2016-01-07 at 11:19 +0000, Wei Liu wrote:
> On Wed, Jan 06, 2016 at 03:03:21PM -0500, Boris Ostrovsky wrote:
> > When copying boot command string for HVMlite guests we explicitly write
> > '\0' at MAX_GUEST_CMDLINE offset. Unless the string is close to
> > MAX_GUEST_CMDLINE in length this write will end up in the wrong place,
> > beyond the end of the mapped range.
> > 
> > We don't need to limit the size of command string to some arbitrary
> > number. Any size that can be successfully allocated and mapped is valid
> > and so the string is guaranteed to be NULL-terminated (since we use
> > strlen, which needs terminating '\0', to calculate allocation size).
> > 
> > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Applied.

Roger commented on #2 so I didn't take that, but this seemed to standalone.

[PATCH v3 2/2] libxc: Defer initialization of start_page for HVM guests

[Boris Ostrovsky]

With commit 8c45adec18e0 ("libxc: create unmapped initrd in domain
builder if supported") location of ramdisk may not be available to
HVMlite guests by the time alloc_magic_pages_hvm() is invoked if the
guest supports unmapped initrd.

So let's move ramdisk info initialization (along with a few other
operations that are not directly related to allocating magic/special
pages) from alloc_magic_pages_hvm() to bootlate_hvm().

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxc/xc_dom_x86.c |  116 ++++++++++++++++++++++++++-------------------
 1 files changed, 67 insertions(+), 49 deletions(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index b8d2904..e102bd2 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -586,23 +586,12 @@ static void build_hvm_info(void *hvm_info_page, struct xc_dom_image *dom)
 static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
 {
     unsigned long i;
-    void *hvm_info_page;
     uint32_t *ident_pt, domid = dom->guest_domid;
     int rc;
     xen_pfn_t special_array[X86_HVM_NR_SPECIAL_PAGES];
     xen_pfn_t ioreq_server_array[NR_IOREQ_SERVER_PAGES];
     xc_interface *xch = dom->xch;
 
-    if ( dom->device_model )
-    {
-        if ( (hvm_info_page = xc_map_foreign_range(
-                  xch, domid, PAGE_SIZE, PROT_READ | PROT_WRITE,
-                  HVM_INFO_PFN)) == NULL )
-            goto error_out;
-        build_hvm_info(hvm_info_page, dom);
-        munmap(hvm_info_page, PAGE_SIZE);
-    }
-
     /* Allocate and clear special pages. */
     for ( i = 0; i < X86_HVM_NR_SPECIAL_PAGES; i++ )
         special_array[i] = special_pfn(i);
@@ -637,12 +626,9 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
     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);
+        size_t start_info_size = sizeof(struct hvm_start_info);
 
         if ( dom->cmdline )
         {
@@ -661,39 +647,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
             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, cmdline_size);
-            start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) +
-                                ((uintptr_t)cmdline - (uintptr_t)start_info);
-        }
-
-        if ( dom->ramdisk_blob )
-        {
-            modlist[0].paddr = dom->ramdisk_seg.vstart - dom->parms.virt_base;
-            modlist[0].size = dom->ramdisk_seg.vend - dom->ramdisk_seg.vstart;
-            start_info->modlist_paddr = (seg.pfn << PAGE_SHIFT) +
-                                ((uintptr_t)modlist - (uintptr_t)start_info);
-            start_info->nr_modules = 1;
-        }
-
-        start_info->magic = HVM_START_MAGIC_VALUE;
-
-        munmap(start_page, start_info_size);
-
         dom->start_info_pfn = seg.pfn;
     }
     else
@@ -1783,7 +1736,72 @@ static int alloc_pgtables_hvm(struct xc_dom_image *dom)
 
 static int bootlate_hvm(struct xc_dom_image *dom)
 {
-    DOMPRINTF("%s: doing nothing", __func__);
+    uint32_t domid = dom->guest_domid;
+    xc_interface *xch = dom->xch;
+
+    if ( !dom->device_model )
+    {
+        struct hvm_start_info *start_info;
+        size_t start_info_size = sizeof(*start_info);
+        void *start_page;
+        struct hvm_modlist_entry *modlist;
+        size_t cmdline_size = 0;
+
+        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. */
+
+        start_page = xc_map_foreign_range(xch, domid, start_info_size,
+                                          PROT_READ | PROT_WRITE,
+                                          dom->start_info_pfn);
+        if ( start_page == NULL )
+        {
+            DOMPRINTF("Unable to map HVM start info page");
+            return -1;
+        }
+
+        start_info = start_page;
+
+        if ( dom->cmdline )
+        {
+            char *cmdline = start_page + sizeof(*start_info);
+
+            strncpy(cmdline, dom->cmdline, cmdline_size);
+            start_info->cmdline_paddr = (dom->start_info_pfn << PAGE_SHIFT) +
+                                ((uintptr_t)cmdline - (uintptr_t)start_info);
+        }
+
+        if ( dom->ramdisk_blob )
+        {
+            modlist = start_page + sizeof(*start_info) + cmdline_size;
+
+            modlist[0].paddr = dom->ramdisk_seg.vstart - dom->parms.virt_base;
+            modlist[0].size = dom->ramdisk_seg.vend - dom->ramdisk_seg.vstart;
+            start_info->modlist_paddr = (dom->start_info_pfn << PAGE_SHIFT) +
+                                ((uintptr_t)modlist - (uintptr_t)start_info);
+            start_info->nr_modules = 1;
+        }
+
+        start_info->magic = HVM_START_MAGIC_VALUE;
+
+        munmap(start_page, start_info_size);
+    }
+    else
+    {
+        void *hvm_info_page;
+
+        if ( (hvm_info_page = xc_map_foreign_range(
+                  xch, domid, PAGE_SIZE, PROT_READ | PROT_WRITE,
+                  HVM_INFO_PFN)) == NULL )
+            return -1;
+        build_hvm_info(hvm_info_page, dom);
+        munmap(hvm_info_page, PAGE_SIZE);
+    }
+
     return 0;
 }
 
-- 
1.7.1

Re: [PATCH v3 2/2] libxc: Defer initialization of start_page for HVM guests

[Roger Pau Monné]

El 06/01/16 a les 21.03, Boris Ostrovsky ha escrit:
> With commit 8c45adec18e0 ("libxc: create unmapped initrd in domain
> builder if supported") location of ramdisk may not be available to
> HVMlite guests by the time alloc_magic_pages_hvm() is invoked if the
> guest supports unmapped initrd.
> 
> So let's move ramdisk info initialization (along with a few other
> operations that are not directly related to allocating magic/special
> pages) from alloc_magic_pages_hvm() to bootlate_hvm().
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxc/xc_dom_x86.c |  116 ++++++++++++++++++++++++++-------------------
>  1 files changed, 67 insertions(+), 49 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index b8d2904..e102bd2 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -586,23 +586,12 @@ static void build_hvm_info(void *hvm_info_page, struct xc_dom_image *dom)
>  static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>  {
>      unsigned long i;
> -    void *hvm_info_page;
>      uint32_t *ident_pt, domid = dom->guest_domid;
>      int rc;
>      xen_pfn_t special_array[X86_HVM_NR_SPECIAL_PAGES];
>      xen_pfn_t ioreq_server_array[NR_IOREQ_SERVER_PAGES];
>      xc_interface *xch = dom->xch;
>  
> -    if ( dom->device_model )
> -    {
> -        if ( (hvm_info_page = xc_map_foreign_range(
> -                  xch, domid, PAGE_SIZE, PROT_READ | PROT_WRITE,
> -                  HVM_INFO_PFN)) == NULL )
> -            goto error_out;
> -        build_hvm_info(hvm_info_page, dom);
> -        munmap(hvm_info_page, PAGE_SIZE);
> -    }
> -
>      /* Allocate and clear special pages. */
>      for ( i = 0; i < X86_HVM_NR_SPECIAL_PAGES; i++ )
>          special_array[i] = special_pfn(i);
> @@ -637,12 +626,9 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>      if ( !dom->device_model )
>      {
>          struct xc_dom_seg seg;
> -        struct hvm_start_info *start_info;
> -        char *cmdline;
>          struct hvm_modlist_entry *modlist;

This can be removed if the conditional below is changed to:

        if ( dom->ramdisk_blob )
-           start_info_size += sizeof(*modlist);
+           start_info_size += sizeof(struct hvm_modlist_entry);

Because AFAICT the variable itself is not used for anything else.

> -        void *start_page;
>          size_t cmdline_size = 0;
> -        size_t start_info_size = sizeof(*start_info);
> +        size_t start_info_size = sizeof(struct hvm_start_info);
>  
>          if ( dom->cmdline )
>          {
> @@ -661,39 +647,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>              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, cmdline_size);
> -            start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) +
> -                                ((uintptr_t)cmdline - (uintptr_t)start_info);
> -        }
> -
> -        if ( dom->ramdisk_blob )
> -        {
> -            modlist[0].paddr = dom->ramdisk_seg.vstart - dom->parms.virt_base;
> -            modlist[0].size = dom->ramdisk_seg.vend - dom->ramdisk_seg.vstart;
> -            start_info->modlist_paddr = (seg.pfn << PAGE_SHIFT) +
> -                                ((uintptr_t)modlist - (uintptr_t)start_info);
> -            start_info->nr_modules = 1;
> -        }
> -
> -        start_info->magic = HVM_START_MAGIC_VALUE;
> -
> -        munmap(start_page, start_info_size);
> -
>          dom->start_info_pfn = seg.pfn;
>      }
>      else
> @@ -1783,7 +1736,72 @@ static int alloc_pgtables_hvm(struct xc_dom_image *dom)
>  
>  static int bootlate_hvm(struct xc_dom_image *dom)
>  {
> -    DOMPRINTF("%s: doing nothing", __func__);
> +    uint32_t domid = dom->guest_domid;
> +    xc_interface *xch = dom->xch;
> +
> +    if ( !dom->device_model )
> +    {
> +        struct hvm_start_info *start_info;
> +        size_t start_info_size = sizeof(*start_info);
> +        void *start_page;
> +        struct hvm_modlist_entry *modlist;
> +        size_t cmdline_size = 0;
> +
> +        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. */

The size calculations are duplicated, could you either stash
start_info_size into xc_dom_image, or simply do the memory allocation
(xc_dom_alloc_segment) inside of bootlate_hvm? (I think the latter would
be better if possible).

Roger.

Re: [PATCH v3 2/2] libxc: Defer initialization of start_page for HVM guests

[Boris Ostrovsky]

On 01/07/2016 06:43 AM, Roger Pau Monné wrote:
> El 06/01/16 a les 21.03, Boris Ostrovsky ha escrit:
>>   
>>   static int bootlate_hvm(struct xc_dom_image *dom)
>>   {
>> -    DOMPRINTF("%s: doing nothing", __func__);
>> +    uint32_t domid = dom->guest_domid;
>> +    xc_interface *xch = dom->xch;
>> +
>> +    if ( !dom->device_model )
>> +    {
>> +        struct hvm_start_info *start_info;
>> +        size_t start_info_size = sizeof(*start_info);
>> +        void *start_page;
>> +        struct hvm_modlist_entry *modlist;
>> +        size_t cmdline_size = 0;
>> +
>> +        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. */
> The size calculations are duplicated, could you either stash
> start_info_size into xc_dom_image, or simply do the memory allocation
> (xc_dom_alloc_segment) inside of bootlate_hvm? (I think the latter would
> be better if possible).

I didn't want to do the first because we'd use that information (two 
pieces --- we need to to know both the size of the extra chunk and where 
modlist starts) only once and it's not on a critical path. You can, of 
course, argue that we increase text size.

The problem with the second approach is that while it does seem to work 
I don't know whether we can delay allocations until bootlate(): notice 
how we print dom->virt_alloc_end in xc_dom_build_image() which to me 
indicates some "finality" as far as allocations for domain are concerned.


-boris

Re: [PATCH v3 2/2] libxc: Defer initialization of start_page for HVM guests

[Roger Pau Monné]

El 07/01/16 a les 15.47, Boris Ostrovsky ha escrit:
> On 01/07/2016 06:43 AM, Roger Pau Monné wrote:
>> El 06/01/16 a les 21.03, Boris Ostrovsky ha escrit:
>>>     static int bootlate_hvm(struct xc_dom_image *dom)
>>>   {
>>> -    DOMPRINTF("%s: doing nothing", __func__);
>>> +    uint32_t domid = dom->guest_domid;
>>> +    xc_interface *xch = dom->xch;
>>> +
>>> +    if ( !dom->device_model )
>>> +    {
>>> +        struct hvm_start_info *start_info;
>>> +        size_t start_info_size = sizeof(*start_info);
>>> +        void *start_page;
>>> +        struct hvm_modlist_entry *modlist;
>>> +        size_t cmdline_size = 0;
>>> +
>>> +        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. */
>> The size calculations are duplicated, could you either stash
>> start_info_size into xc_dom_image, or simply do the memory allocation
>> (xc_dom_alloc_segment) inside of bootlate_hvm? (I think the latter would
>> be better if possible).
> 
> I didn't want to do the first because we'd use that information (two
> pieces --- we need to to know both the size of the extra chunk and where
> modlist starts) only once and it's not on a critical path. You can, of
> course, argue that we increase text size.

It's just that I don't want to get them out of synch, and that's what
usually happens when you perform the same calculations in two different
places, one might get out of synch with the other.

> The problem with the second approach is that while it does seem to work
> I don't know whether we can delay allocations until bootlate(): notice
> how we print dom->virt_alloc_end in xc_dom_build_image() which to me
> indicates some "finality" as far as allocations for domain are concerned.

For PV guests it probably matters, because we have to build the page
tables and the p2m, for HVMlite guests I don't think it matters at all
(or at least I don't see any obvious reason).

Another third option is to place the code that performs the size
calculations inside of a function that's called by both (bootlate_hvm
and alloc_magic_pages_hvm).

Roger.

Re: [PATCH v3 2/2] libxc: Defer initialization of start_page for HVM guests

[Ian Campbell]

On Thu, 2016-01-07 at 17:54 +0100, Roger Pau Monné wrote:
> El 07/01/16 a les 15.47, Boris Ostrovsky ha escrit:
> > On 01/07/2016 06:43 AM, Roger Pau Monné wrote:
> > > El 06/01/16 a les 21.03, Boris Ostrovsky ha escrit:
> > > >     static int bootlate_hvm(struct xc_dom_image *dom)
> > > >   {
> > > > -    DOMPRINTF("%s: doing nothing", __func__);
> > > > +    uint32_t domid = dom->guest_domid;
> > > > +    xc_interface *xch = dom->xch;
> > > > +
> > > > +    if ( !dom->device_model )
> > > > +    {
> > > > +        struct hvm_start_info *start_info;
> > > > +        size_t start_info_size = sizeof(*start_info);
> > > > +        void *start_page;
> > > > +        struct hvm_modlist_entry *modlist;
> > > > +        size_t cmdline_size = 0;
> > > > +
> > > > +        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. */
> > > The size calculations are duplicated, could you either stash
> > > start_info_size into xc_dom_image, or simply do the memory allocation
> > > (xc_dom_alloc_segment) inside of bootlate_hvm? (I think the latter
> > > would
> > > be better if possible).
> > 
> > I didn't want to do the first because we'd use that information (two
> > pieces --- we need to to know both the size of the extra chunk and
> > where
> > modlist starts) only once and it's not on a critical path. You can, of
> > course, argue that we increase text size.
> 
> It's just that I don't want to get them out of synch, and that's what
> usually happens when you perform the same calculations in two different
> places, one might get out of synch with the other.
> 
> > The problem with the second approach is that while it does seem to work
> > I don't know whether we can delay allocations until bootlate(): notice
> > how we print dom->virt_alloc_end in xc_dom_build_image() which to me
> > indicates some "finality" as far as allocations for domain are
> > concerned.
> 
> For PV guests it probably matters, because we have to build the page
> tables and the p2m, for HVMlite guests I don't think it matters at all
> (or at least I don't see any obvious reason).
> 
> Another third option is to place the code that performs the size
> calculations inside of a function that's called by both (bootlate_hvm
> and alloc_magic_pages_hvm).

The call to xc_dom_alloc_segment initialises a xc_dom_seg, which in this
case alloc_magic just throws away. If the location/size of that segment is
required elsewhere then the normal approach would be to add it to the
handle struct (cf dom->{kernel,ramdisk}_seg et al) and to consume it in the
other places.

My concerns would be the size calculation and the pointer figuring out
getting out of sync or the size of one of the inputs changing between the
size calculation and the use (i.e. by a future patch moving something
around). So the place which figures out the pointers would also need to
bounds check against the mapped region's size.

Ian.


> 
> Roger.
> 

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

Re: [PATCH v3 2/2] libxc: Defer initialization of start_page for HVM guests

[Boris Ostrovsky]

On 01/07/2016 12:06 PM, Ian Campbell wrote:
> On Thu, 2016-01-07 at 17:54 +0100, Roger Pau Monné wrote:
>> El 07/01/16 a les 15.47, Boris Ostrovsky ha escrit:
>>> On 01/07/2016 06:43 AM, Roger Pau Monné wrote:
>>>> El 06/01/16 a les 21.03, Boris Ostrovsky ha escrit:
>>>>>      static int bootlate_hvm(struct xc_dom_image *dom)
>>>>>    {
>>>>> -    DOMPRINTF("%s: doing nothing", __func__);
>>>>> +    uint32_t domid = dom->guest_domid;
>>>>> +    xc_interface *xch = dom->xch;
>>>>> +
>>>>> +    if ( !dom->device_model )
>>>>> +    {
>>>>> +        struct hvm_start_info *start_info;
>>>>> +        size_t start_info_size = sizeof(*start_info);
>>>>> +        void *start_page;
>>>>> +        struct hvm_modlist_entry *modlist;
>>>>> +        size_t cmdline_size = 0;
>>>>> +
>>>>> +        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. */
>>>> The size calculations are duplicated, could you either stash
>>>> start_info_size into xc_dom_image, or simply do the memory allocation
>>>> (xc_dom_alloc_segment) inside of bootlate_hvm? (I think the latter
>>>> would
>>>> be better if possible).
>>> I didn't want to do the first because we'd use that information (two
>>> pieces --- we need to to know both the size of the extra chunk and
>>> where
>>> modlist starts) only once and it's not on a critical path. You can, of
>>> course, argue that we increase text size.
>> It's just that I don't want to get them out of synch, and that's what
>> usually happens when you perform the same calculations in two different
>> places, one might get out of synch with the other.
>>
>>> The problem with the second approach is that while it does seem to work
>>> I don't know whether we can delay allocations until bootlate(): notice
>>> how we print dom->virt_alloc_end in xc_dom_build_image() which to me
>>> indicates some "finality" as far as allocations for domain are
>>> concerned.
>> For PV guests it probably matters, because we have to build the page
>> tables and the p2m, for HVMlite guests I don't think it matters at all
>> (or at least I don't see any obvious reason).
>>
>> Another third option is to place the code that performs the size
>> calculations inside of a function that's called by both (bootlate_hvm
>> and alloc_magic_pages_hvm).
> The call to xc_dom_alloc_segment initialises a xc_dom_seg, which in this
> case alloc_magic just throws away. If the location/size of that segment is
> required elsewhere then the normal approach would be to add it to the
> handle struct (cf dom->{kernel,ramdisk}_seg et al) and to consume it in the
> other places.

xc_dom_alloc_segment() also updates domain's pfn_alloc_end and virt_alloc_end, which is what I was thinking about (i.e. that updating those values bootlate() time may be too late).


>
> My concerns would be the size calculation and the pointer figuring out
> getting out of sync or the size of one of the inputs changing between the
> size calculation and the use (i.e. by a future patch moving something
> around). So the place which figures out the pointers would also need to
> bounds check against the mapped region's size.

So then I think the safest is to stash the value of cmdline length in 
xc_dom_image (and use strncpy).

For modlist we only have a single module (ramdisk) and I just realized 
that we can actually use ramdisk_seg. In fact, it looks like we are 
setting it up and then never use it for HVMlite.

-boris

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

Re: [PATCH v3 2/2] libxc: Defer initialization of start_page for HVM guests

[Ian Campbell]

On Thu, 2016-01-07 at 12:33 -0500, Boris Ostrovsky wrote:
> On 01/07/2016 12:06 PM, Ian Campbell wrote:
> > On Thu, 2016-01-07 at 17:54 +0100, Roger Pau Monné wrote:
> > > El 07/01/16 a les 15.47, Boris Ostrovsky ha escrit:
> > > > On 01/07/2016 06:43 AM, Roger Pau Monné wrote:
> > > > > El 06/01/16 a les 21.03, Boris Ostrovsky ha escrit:
> > > > > >      static int bootlate_hvm(struct xc_dom_image *dom)
> > > > > >    {
> > > > > > -    DOMPRINTF("%s: doing nothing", __func__);
> > > > > > +    uint32_t domid = dom->guest_domid;
> > > > > > +    xc_interface *xch = dom->xch;
> > > > > > +
> > > > > > +    if ( !dom->device_model )
> > > > > > +    {
> > > > > > +        struct hvm_start_info *start_info;
> > > > > > +        size_t start_info_size = sizeof(*start_info);
> > > > > > +        void *start_page;
> > > > > > +        struct hvm_modlist_entry *modlist;
> > > > > > +        size_t cmdline_size = 0;
> > > > > > +
> > > > > > +        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. */
> > > > > The size calculations are duplicated, could you either stash
> > > > > start_info_size into xc_dom_image, or simply do the memory
> > > > > allocation
> > > > > (xc_dom_alloc_segment) inside of bootlate_hvm? (I think the
> > > > > latter
> > > > > would
> > > > > be better if possible).
> > > > I didn't want to do the first because we'd use that information
> > > > (two
> > > > pieces --- we need to to know both the size of the extra chunk and
> > > > where
> > > > modlist starts) only once and it's not on a critical path. You can,
> > > > of
> > > > course, argue that we increase text size.
> > > It's just that I don't want to get them out of synch, and that's what
> > > usually happens when you perform the same calculations in two
> > > different
> > > places, one might get out of synch with the other.
> > > 
> > > > The problem with the second approach is that while it does seem to
> > > > work
> > > > I don't know whether we can delay allocations until bootlate():
> > > > notice
> > > > how we print dom->virt_alloc_end in xc_dom_build_image() which to
> > > > me
> > > > indicates some "finality" as far as allocations for domain are
> > > > concerned.
> > > For PV guests it probably matters, because we have to build the page
> > > tables and the p2m, for HVMlite guests I don't think it matters at
> > > all
> > > (or at least I don't see any obvious reason).
> > > 
> > > Another third option is to place the code that performs the size
> > > calculations inside of a function that's called by both (bootlate_hvm
> > > and alloc_magic_pages_hvm).
> > The call to xc_dom_alloc_segment initialises a xc_dom_seg, which in
> > this
> > case alloc_magic just throws away. If the location/size of that segment
> > is
> > required elsewhere then the normal approach would be to add it to the
> > handle struct (cf dom->{kernel,ramdisk}_seg et al) and to consume it in
> > the
> > other places.
> 
> xc_dom_alloc_segment() also updates domain's pfn_alloc_end and
> virt_alloc_end, which is what I was thinking about (i.e. that updating
> those values bootlate() time may be too late).

I was assuming the reason for calculating the size twice was
that xc_dom_alloc_segment() was called in the earlier hook, which is why I
mentioned stacking the xc_dom_seg.

Ian.

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

Re: [PATCH v3 2/2] libxc: Defer initialization of start_page for HVM guests

[Roger Pau Monné]

El 06/01/16 a les 21.03, Boris Ostrovsky ha escrit:
> With commit 8c45adec18e0 ("libxc: create unmapped initrd in domain
> builder if supported") location of ramdisk may not be available to
> HVMlite guests by the time alloc_magic_pages_hvm() is invoked if the
> guest supports unmapped initrd.
> 
> So let's move ramdisk info initialization (along with a few other
> operations that are not directly related to allocating magic/special
> pages) from alloc_magic_pages_hvm() to bootlate_hvm().
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxc/xc_dom_x86.c |  116 ++++++++++++++++++++++++++-------------------
>  1 files changed, 67 insertions(+), 49 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index b8d2904..e102bd2 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -586,23 +586,12 @@ static void build_hvm_info(void *hvm_info_page, struct xc_dom_image *dom)
>  static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>  {
>      unsigned long i;
> -    void *hvm_info_page;
>      uint32_t *ident_pt, domid = dom->guest_domid;
>      int rc;
>      xen_pfn_t special_array[X86_HVM_NR_SPECIAL_PAGES];
>      xen_pfn_t ioreq_server_array[NR_IOREQ_SERVER_PAGES];
>      xc_interface *xch = dom->xch;
>  
> -    if ( dom->device_model )
> -    {
> -        if ( (hvm_info_page = xc_map_foreign_range(
> -                  xch, domid, PAGE_SIZE, PROT_READ | PROT_WRITE,
> -                  HVM_INFO_PFN)) == NULL )
> -            goto error_out;
> -        build_hvm_info(hvm_info_page, dom);
> -        munmap(hvm_info_page, PAGE_SIZE);
> -    }
> -
>      /* Allocate and clear special pages. */
>      for ( i = 0; i < X86_HVM_NR_SPECIAL_PAGES; i++ )
>          special_array[i] = special_pfn(i);
> @@ -637,12 +626,9 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>      if ( !dom->device_model )
>      {
>          struct xc_dom_seg seg;
> -        struct hvm_start_info *start_info;
> -        char *cmdline;
>          struct hvm_modlist_entry *modlist;

This can be removed if the conditional below is changed to:

        if ( dom->ramdisk_blob )
-           start_info_size += sizeof(*modlist);
+           start_info_size += sizeof(struct hvm_modlist_entry);

Because AFAICT the variable itself is not used for anything else.

> -        void *start_page;
>          size_t cmdline_size = 0;
> -        size_t start_info_size = sizeof(*start_info);
> +        size_t start_info_size = sizeof(struct hvm_start_info);
>  
>          if ( dom->cmdline )
>          {
> @@ -661,39 +647,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>              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, cmdline_size);
> -            start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) +
> -                                ((uintptr_t)cmdline - (uintptr_t)start_info);
> -        }
> -
> -        if ( dom->ramdisk_blob )
> -        {
> -            modlist[0].paddr = dom->ramdisk_seg.vstart - dom->parms.virt_base;
> -            modlist[0].size = dom->ramdisk_seg.vend - dom->ramdisk_seg.vstart;
> -            start_info->modlist_paddr = (seg.pfn << PAGE_SHIFT) +
> -                                ((uintptr_t)modlist - (uintptr_t)start_info);
> -            start_info->nr_modules = 1;
> -        }
> -
> -        start_info->magic = HVM_START_MAGIC_VALUE;
> -
> -        munmap(start_page, start_info_size);
> -
>          dom->start_info_pfn = seg.pfn;
>      }
>      else
> @@ -1783,7 +1736,72 @@ static int alloc_pgtables_hvm(struct xc_dom_image *dom)
>  
>  static int bootlate_hvm(struct xc_dom_image *dom)
>  {
> -    DOMPRINTF("%s: doing nothing", __func__);
> +    uint32_t domid = dom->guest_domid;
> +    xc_interface *xch = dom->xch;
> +
> +    if ( !dom->device_model )
> +    {
> +        struct hvm_start_info *start_info;
> +        size_t start_info_size = sizeof(*start_info);
> +        void *start_page;
> +        struct hvm_modlist_entry *modlist;
> +        size_t cmdline_size = 0;
> +
> +        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. */

The size calculations are duplicated, could you either stash
start_info_size into xc_dom_image, or simply do the memory allocation
(xc_dom_alloc_segment) inside of bootlate_hvm? (I think the latter would
be better if possible).

Roger.