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

[PATCH] 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.

Since the only operation related to allocating magic pages in that
routine is allocation of HVMlite start info we can move everything
else to a later point such as xc_dom_arch.start_info().

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
I am not sure xc_dom_arch.start_info() is the right place neither since we
still are doing things that have nothing to do with start_info.

 tools/libxc/xc_dom_x86.c |   45 ++++++++++++++++++++++++++++++---------------
 1 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 3960875..f64079e 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -585,6 +585,32 @@ 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)
 {
+        struct xc_dom_seg seg;
+        size_t start_info_size = sizeof(struct hvm_start_info);
+
+        if ( dom->device_model )
+            return 0;
+
+        if ( dom->cmdline )
+            start_info_size += ROUNDUP(strlen(dom->cmdline) + 1, 8);
+        if ( dom->ramdisk_blob )
+            /* Limited to one module. */
+            start_info_size += sizeof( struct hvm_modlist_entry);
+
+        if ( xc_dom_alloc_segment(dom, &seg, "HVMlite start info", 0,
+                                  start_info_size) )
+        {
+            DOMPRINTF("Unable to reserve memory for the start info");
+            return -1;
+        }
+
+        dom->start_info_pfn = seg.pfn;
+
+        return 0;
+}
+
+static int start_info_hvm(struct xc_dom_image *dom)
+{
     unsigned long i;
     void *hvm_info_page;
     uint32_t *ident_pt, domid = dom->guest_domid;
@@ -636,7 +662,6 @@ 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;
@@ -653,17 +678,9 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
         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);
+                                          dom->start_info_pfn);
         if ( start_page == NULL )
         {
             DOMPRINTF("Unable to map HVM start info page");
@@ -678,7 +695,7 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
         {
             strncpy(cmdline, dom->cmdline, MAX_GUEST_CMDLINE);
             cmdline[MAX_GUEST_CMDLINE - 1] = '\0';
-            start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) +
+            start_info->cmdline_paddr = (dom->start_info_pfn << PAGE_SHIFT) +
                                 ((uintptr_t)cmdline - (uintptr_t)start_info);
         }
 
@@ -686,7 +703,7 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
         {
             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) +
+            start_info->modlist_paddr = (dom->start_info_pfn << PAGE_SHIFT) +
                                 ((uintptr_t)modlist - (uintptr_t)start_info);
             start_info->nr_modules = 1;
         }
@@ -694,8 +711,6 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
         start_info->magic = HVM_START_MAGIC_VALUE;
 
         munmap(start_page, start_info_size);
-
-        dom->start_info_pfn = seg.pfn;
     }
     else
     {
@@ -1845,7 +1860,7 @@ static struct xc_dom_arch xc_hvm_32 = {
     .alloc_magic_pages = alloc_magic_pages_hvm,
     .alloc_pgtables = alloc_pgtables_hvm,
     .setup_pgtables = NULL,
-    .start_info = NULL,
+    .start_info = start_info_hvm,
     .shared_info = NULL,
     .vcpu = vcpu_hvm,
     .meminit = meminit_hvm,
-- 
1.7.1

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

[Roger Pau Monné]

El 22/12/15 a les 0.45, 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.
> 
> Since the only operation related to allocating magic pages in that
> routine is allocation of HVMlite start info we can move everything
> else to a later point such as xc_dom_arch.start_info().
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
> I am not sure xc_dom_arch.start_info() is the right place neither since we
> still are doing things that have nothing to do with start_info.
>
>  tools/libxc/xc_dom_x86.c |   45 ++++++++++++++++++++++++++++++---------------
>  1 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index 3960875..f64079e 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -585,6 +585,32 @@ 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)
>  {
> +        struct xc_dom_seg seg;
> +        size_t start_info_size = sizeof(struct hvm_start_info);
> +
> +        if ( dom->device_model )
> +            return 0;
> +
> +        if ( dom->cmdline )
> +            start_info_size += ROUNDUP(strlen(dom->cmdline) + 1, 8);
> +        if ( dom->ramdisk_blob )
> +            /* Limited to one module. */
> +            start_info_size += sizeof( struct hvm_modlist_entry);
                                         ^ extra space.
> +
> +        if ( xc_dom_alloc_segment(dom, &seg, "HVMlite start info", 0,
> +                                  start_info_size) )
> +        {
> +            DOMPRINTF("Unable to reserve memory for the start info");
> +            return -1;
> +        }
> +
> +        dom->start_info_pfn = seg.pfn;
> +
> +        return 0;
> +}
> +
> +static int start_info_hvm(struct xc_dom_image *dom)
> +{
>      unsigned long i;
>      void *hvm_info_page;
>      uint32_t *ident_pt, domid = dom->guest_domid;
> @@ -636,7 +662,6 @@ 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;
> @@ -653,17 +678,9 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>          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,

IMHO, I would stash start_info_size somewhere inside xc_dom_image in
order to avoid calculating it twice.

Also, why is everything done inside of alloc_magic_pages_hvm moved to
start_info_hvm? AFAICT we should only need to move the code that fills
the hvm_start_info struct, but the rest of the code already present in
alloc_magic_pages_hvm could be left as-is.

Roger.

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

[Boris Ostrovsky]

On 12/22/2015 04:42 AM, Roger Pau Monné wrote:
> El 22/12/15 a les 0.45, 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.
>>
>> Since the only operation related to allocating magic pages in that
>> routine is allocation of HVMlite start info we can move everything
>> else to a later point such as xc_dom_arch.start_info().
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>> I am not sure xc_dom_arch.start_info() is the right place neither since we
>> still are doing things that have nothing to do with start_info.
>>
>>   tools/libxc/xc_dom_x86.c |   45 ++++++++++++++++++++++++++++++---------------
>>   1 files changed, 30 insertions(+), 15 deletions(-)
>>
>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>> index 3960875..f64079e 100644
>> --- a/tools/libxc/xc_dom_x86.c
>> +++ b/tools/libxc/xc_dom_x86.c
>> @@ -585,6 +585,32 @@ 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)
>>   {
>> +        struct xc_dom_seg seg;
>> +        size_t start_info_size = sizeof(struct hvm_start_info);
>> +
>> +        if ( dom->device_model )
>> +            return 0;
>> +
>> +        if ( dom->cmdline )
>> +            start_info_size += ROUNDUP(strlen(dom->cmdline) + 1, 8);
>> +        if ( dom->ramdisk_blob )
>> +            /* Limited to one module. */
>> +            start_info_size += sizeof( struct hvm_modlist_entry);
>                                           ^ extra space.
>> +
>> +        if ( xc_dom_alloc_segment(dom, &seg, "HVMlite start info", 0,
>> +                                  start_info_size) )
>> +        {
>> +            DOMPRINTF("Unable to reserve memory for the start info");
>> +            return -1;
>> +        }
>> +
>> +        dom->start_info_pfn = seg.pfn;
>> +
>> +        return 0;
>> +}
>> +
>> +static int start_info_hvm(struct xc_dom_image *dom)
>> +{
>>       unsigned long i;
>>       void *hvm_info_page;
>>       uint32_t *ident_pt, domid = dom->guest_domid;
>> @@ -636,7 +662,6 @@ 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;
>> @@ -653,17 +678,9 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>>           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,
> IMHO, I would stash start_info_size somewhere inside xc_dom_image in
> order to avoid calculating it twice.

Yes, I should do this.

>
> Also, why is everything done inside of alloc_magic_pages_hvm moved to
> start_info_hvm? AFAICT we should only need to move the code that fills
> the hvm_start_info struct, but the rest of the code already present in
> alloc_magic_pages_hvm could be left as-is.

That's what I was referring to in the commit message (and in the comment 
below it): most, if not all, of the stuff that alloc_magic_pages_hvm() 
currently does really has nothing to do with magic pages allocation. 
E.g. hvm_params settting, special pages initialization, etc. So I 
figured I could move all of this to a later point. But, as I said in the 
comment, I am not convinced that start_info() is the right place either.


-boris

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

[Roger Pau Monné]

El 22/12/15 a les 15.12, Boris Ostrovsky ha escrit:
> On 12/22/2015 04:42 AM, Roger Pau Monné wrote:
>> Also, why is everything done inside of alloc_magic_pages_hvm moved to
>> start_info_hvm? AFAICT we should only need to move the code that fills
>> the hvm_start_info struct, but the rest of the code already present in
>> alloc_magic_pages_hvm could be left as-is.
> 
> That's what I was referring to in the commit message (and in the comment
> below it): most, if not all, of the stuff that alloc_magic_pages_hvm()
> currently does really has nothing to do with magic pages allocation.
> E.g. hvm_params settting, special pages initialization, etc. So I
> figured I could move all of this to a later point. But, as I said in the
> comment, I am not convinced that start_info() is the right place either.

Some of it has to do with magic pages IMHO (although I have to admit
this is probably a question of taste), for example it makes sense from
my PoV to allocate the hvm_info_page, the special pages (xenstore,
console...), the ioreq server pages and possibly the EPT identity map.
Moving the start_info stuff into it's own function also makes sense.

Roger.

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

[Boris Ostrovsky]

On 12/22/2015 09:36 AM, Roger Pau Monné wrote:
> El 22/12/15 a les 15.12, Boris Ostrovsky ha escrit:
>> On 12/22/2015 04:42 AM, Roger Pau Monné wrote:
>>> Also, why is everything done inside of alloc_magic_pages_hvm moved to
>>> start_info_hvm? AFAICT we should only need to move the code that fills
>>> the hvm_start_info struct, but the rest of the code already present in
>>> alloc_magic_pages_hvm could be left as-is.
>> That's what I was referring to in the commit message (and in the comment
>> below it): most, if not all, of the stuff that alloc_magic_pages_hvm()
>> currently does really has nothing to do with magic pages allocation.
>> E.g. hvm_params settting, special pages initialization, etc. So I
>> figured I could move all of this to a later point. But, as I said in the
>> comment, I am not convinced that start_info() is the right place either.
> Some of it has to do with magic pages IMHO (although I have to admit
> this is probably a question of taste), for example it makes sense from
> my PoV to allocate the hvm_info_page, the special pages (xenstore,
> console...), the ioreq server pages and possibly the EPT identity map.
> Moving the start_info stuff into it's own function also makes sense.

Since there are many ways to separate this I'll wait for maintainers to 
express their preference.

(I don't know who is around now and I will disappear too on Friday until 
January so this may have to wait until then).

-boris

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

[Ian Campbell]

On Tue, 2015-12-22 at 09:48 -0500, Boris Ostrovsky wrote:
> On 12/22/2015 09:36 AM, Roger Pau Monné wrote:
> > El 22/12/15 a les 15.12, Boris Ostrovsky ha escrit:
> > > On 12/22/2015 04:42 AM, Roger Pau Monné wrote:
> > > > Also, why is everything done inside of alloc_magic_pages_hvm moved
> > > > to
> > > > start_info_hvm? AFAICT we should only need to move the code that
> > > > fills
> > > > the hvm_start_info struct, but the rest of the code already present
> > > > in
> > > > alloc_magic_pages_hvm could be left as-is.
> > > That's what I was referring to in the commit message (and in the
> > > comment
> > > below it): most, if not all, of the stuff that
> > > alloc_magic_pages_hvm()
> > > currently does really has nothing to do with magic pages allocation.
> > > E.g. hvm_params settting, special pages initialization, etc. So I
> > > figured I could move all of this to a later point. But, as I said in
> > > the
> > > comment, I am not convinced that start_info() is the right place
> > > either.
> > Some of it has to do with magic pages IMHO (although I have to admit
> > this is probably a question of taste), for example it makes sense from
> > my PoV to allocate the hvm_info_page, the special pages (xenstore,
> > console...), the ioreq server pages and possibly the EPT identity map.
> > Moving the start_info stuff into it's own function also makes sense.
> 
> Since there are many ways to separate this I'll wait for maintainers to 
> express their preference.

So AIUI the issue here is that alloc_magic_pages_hvm has become something
of a dumping ground for stuff that needs to (or can) happen at this
particular point in time, some of which is certainly not "magic pages" and
others for which it is debatable.

One particular piece of this is the start_info which is not a magic page
either and which as it happens needs to be done later. (Much later it
seems?)

As part of fixing that you essentially moved the "dumping ground" from
alloc_magic_pages_hvm to start_info_hvm (which is a new callback on an
existing hook), since, why not.

Looking at xc_dom_boot_image (the caller of the start_info hook) I see that
it also calls, not long after the start_info hook, other hooks such as
->bootlate ("/* misc x86 stuff */").

So if all the random stuff which this patch moves from ->alloc_magic to
->start_info can really be safely moved like that (which I didn't check)
I'd expect it could equally well move to ->bootlate (the only thing in the
middle is setup_hypercall_page() and xc_dom_log_memory_footprint(), and
that stuff does seem much more in keeping with "misc x86 stuff" than
->start_info or ->alloc_magic.

Maybe the xc_dom_log_memory_footprint call would need to move after the
->bootlate, that doesn't seem like the end of the world.

Of course if there is functionality which seems worthy of its own new
callback or which would be best placed in some other existing callback I'm
sure that would be fine too.

> 
> (I don't know who is around now and I will disappear too on Friday until 
> January so this may have to wait until then).
> 
> -boris

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

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

[Boris Ostrovsky]

On 01/05/2016 09:06 AM, Ian Campbell wrote:
> On Tue, 2015-12-22 at 09:48 -0500, Boris Ostrovsky wrote:
>> On 12/22/2015 09:36 AM, Roger Pau Monné wrote:
>>> El 22/12/15 a les 15.12, Boris Ostrovsky ha escrit:
>>>> On 12/22/2015 04:42 AM, Roger Pau Monné wrote:
>>>>> Also, why is everything done inside of alloc_magic_pages_hvm moved
>>>>> to
>>>>> start_info_hvm? AFAICT we should only need to move the code that
>>>>> fills
>>>>> the hvm_start_info struct, but the rest of the code already present
>>>>> in
>>>>> alloc_magic_pages_hvm could be left as-is.
>>>> That's what I was referring to in the commit message (and in the
>>>> comment
>>>> below it): most, if not all, of the stuff that
>>>> alloc_magic_pages_hvm()
>>>> currently does really has nothing to do with magic pages allocation.
>>>> E.g. hvm_params settting, special pages initialization, etc. So I
>>>> figured I could move all of this to a later point. But, as I said in
>>>> the
>>>> comment, I am not convinced that start_info() is the right place
>>>> either.
>>> Some of it has to do with magic pages IMHO (although I have to admit
>>> this is probably a question of taste), for example it makes sense from
>>> my PoV to allocate the hvm_info_page, the special pages (xenstore,
>>> console...), the ioreq server pages and possibly the EPT identity map.
>>> Moving the start_info stuff into it's own function also makes sense.
>> Since there are many ways to separate this I'll wait for maintainers to
>> express their preference.
> So AIUI the issue here is that alloc_magic_pages_hvm has become something
> of a dumping ground for stuff that needs to (or can) happen at this
> particular point in time, some of which is certainly not "magic pages" and
> others for which it is debatable.
>
> One particular piece of this is the start_info which is not a magic page
> either and which as it happens needs to be done later. (Much later it
> seems?)
>
> As part of fixing that you essentially moved the "dumping ground" from
> alloc_magic_pages_hvm to start_info_hvm (which is a new callback on an
> existing hook), since, why not.
>
> Looking at xc_dom_boot_image (the caller of the start_info hook) I see that
> it also calls, not long after the start_info hook, other hooks such as
> ->bootlate ("/* misc x86 stuff */").
>
> So if all the random stuff which this patch moves from ->alloc_magic to
> ->start_info can really be safely moved like that (which I didn't check)
> I'd expect it could equally well move to ->bootlate (the only thing in the
> middle is setup_hypercall_page() and xc_dom_log_memory_footprint(), and
> that stuff does seem much more in keeping with "misc x86 stuff" than
> ->start_info or ->alloc_magic.
>
> Maybe the xc_dom_log_memory_footprint call would need to move after the
> ->bootlate, that doesn't seem like the end of the world.
>
> Of course if there is functionality which seems worthy of its own new
> callback or which would be best placed in some other existing callback I'm
> sure that would be fine too.

I didn't notice bootlate --- it looks to me like a place where we'd do 
things we couldn't do earlier, which is exactly what I was trying to 
achieve.

So I'll use it (and see if I can keep some of the things Roger thought 
are magic pages-related in alloc_magic_pages_hvm()).

-boris

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