[PATCH] libxc: fix claim mode when creating HVM guest

[PATCH] libxc: fix claim mode when creating HVM guest

[Wei Liu]

The original code is wrong because:
* claim mode wants to know the total number of pages needed while
  original code provides the additional number of pages needed.
* if pod is enabled memory will already be allocated by the time we try
  to claim memory.

So the fix would be:
* move claim mode before actual memory allocation.
* pass the right number of pages to hypervisor.

The "right number of pages" should be number of pages of target memory
minus VGA_HOLE_SIZE, regardless of whether PoD is enabled.

This fixes bug #32.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Konrad Wilk <konrad.wilk@oracle.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
WRT 4.4 release: this patch should be accpeted, otherwise PoD + claim
mode is complete broken. If this patch is deemed too complicated, we
should flip the switch to disable claim mode by default for 4.4.
---
 tools/libxc/xc_hvm_build_x86.c |   36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
index 77bd365..dd3b522 100644
--- a/tools/libxc/xc_hvm_build_x86.c
+++ b/tools/libxc/xc_hvm_build_x86.c
@@ -49,6 +49,8 @@
 #define NR_SPECIAL_PAGES     8
 #define special_pfn(x) (0xff000u - NR_SPECIAL_PAGES + (x))
 
+#define VGA_HOLE_SIZE (0x20)
+
 static int modules_init(struct xc_hvm_build_args *args,
                         uint64_t vend, struct elf_binary *elf,
                         uint64_t *mstart_out, uint64_t *mend_out)
@@ -302,14 +304,31 @@ static int setup_guest(xc_interface *xch,
     for ( i = mmio_start >> PAGE_SHIFT; i < nr_pages; i++ )
         page_array[i] += mmio_size >> PAGE_SHIFT;
 
+    /*
+     * Try to claim pages for early warning of insufficient memory available.
+     * This should go before xc_domain_set_pod_target, becuase that function
+     * actually allocates memory for the guest. Claiming after memory has been
+     * allocated is pointless.
+     */
+    if ( claim_enabled ) {
+        rc = xc_domain_claim_pages(xch, dom, target_pages - VGA_HOLE_SIZE);
+        if ( rc != 0 )
+        {
+            PERROR("Could not allocate memory for HVM guest as we cannot claim memory!");
+            goto error_out;
+        }
+    }
+
     if ( pod_mode )
     {
         /*
-         * Subtract 0x20 from target_pages for the VGA "hole".  Xen will
-         * adjust the PoD cache size so that domain tot_pages will be
-         * target_pages - 0x20 after this call.
+         * Subtract VGA_HOLE_SIZE from target_pages for the VGA
+         * "hole".  Xen will adjust the PoD cache size so that domain
+         * tot_pages will be target_pages - VGA_HOLE_SIZE after
+         * this call.
          */
-        rc = xc_domain_set_pod_target(xch, dom, target_pages - 0x20,
+        rc = xc_domain_set_pod_target(xch, dom,
+                                      target_pages - VGA_HOLE_SIZE,
                                       NULL, NULL, NULL);
         if ( rc != 0 )
         {
@@ -333,15 +352,6 @@ static int setup_guest(xc_interface *xch,
     cur_pages = 0xc0;
     stat_normal_pages = 0xc0;
 
-    /* try to claim pages for early warning of insufficient memory available */
-    if ( claim_enabled ) {
-        rc = xc_domain_claim_pages(xch, dom, nr_pages - cur_pages);
-        if ( rc != 0 )
-        {
-            PERROR("Could not allocate memory for HVM guest as we cannot claim memory!");
-            goto error_out;
-        }
-    }
     while ( (rc == 0) && (nr_pages > cur_pages) )
     {
         /* Clip count to maximum 1GB extent. */
-- 
1.7.10.4

Re: [PATCH] libxc: fix claim mode when creating HVM guest

[George Dunlap]

On 01/27/2014 05:53 PM, Wei Liu wrote:
> The original code is wrong because:
> * claim mode wants to know the total number of pages needed while
>    original code provides the additional number of pages needed.
> * if pod is enabled memory will already be allocated by the time we try
>    to claim memory.
>
> So the fix would be:
> * move claim mode before actual memory allocation.
> * pass the right number of pages to hypervisor.
>
> The "right number of pages" should be number of pages of target memory
> minus VGA_HOLE_SIZE, regardless of whether PoD is enabled.
>
> This fixes bug #32.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Konrad Wilk <konrad.wilk@oracle.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>

Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>

> ---
> WRT 4.4 release: this patch should be accpeted, otherwise PoD + claim
> mode is complete broken. If this patch is deemed too complicated, we
> should flip the switch to disable claim mode by default for 4.4.

I think a more reasonable mitigation strategy would simply be to ignore 
claim mode when constructing a domain that uses PoD.

I'm inclined to take this one.  Since claim mode is on by default, the 
currently-working path should get exercised well before the release to 
shake out any bugs.  The other path doesn't work at all currently 
(AFAICT) unless you disable claim mode -- which is still available as a 
work-around, even if there is a bug in this patch.

I'll wait a day or two for others to speak up before giving it a formal 
ack, just in case.

  -George

Re: [PATCH] libxc: fix claim mode when creating HVM guest

[Konrad Rzeszutek Wilk]

On Mon, Jan 27, 2014 at 07:09:45PM +0000, George Dunlap wrote:
> On 01/27/2014 05:53 PM, Wei Liu wrote:
> >The original code is wrong because:
> >* claim mode wants to know the total number of pages needed while
> >   original code provides the additional number of pages needed.
> >* if pod is enabled memory will already be allocated by the time we try
> >   to claim memory.
> >
> >So the fix would be:
> >* move claim mode before actual memory allocation.
> >* pass the right number of pages to hypervisor.
> >
> >The "right number of pages" should be number of pages of target memory
> >minus VGA_HOLE_SIZE, regardless of whether PoD is enabled.
> >
> >This fixes bug #32.
> >
> >Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> >Cc: Konrad Wilk <konrad.wilk@oracle.com>
> >Cc: George Dunlap <george.dunlap@eu.citrix.com>
> >Cc: Ian Campbell <ian.campbell@citrix.com>
> >Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

and tomorrow I should be able to test it out as well.
> 
> >---
> >WRT 4.4 release: this patch should be accpeted, otherwise PoD + claim
> >mode is complete broken. If this patch is deemed too complicated, we
> >should flip the switch to disable claim mode by default for 4.4.
> 
> I think a more reasonable mitigation strategy would simply be to
> ignore claim mode when constructing a domain that uses PoD.
> 
> I'm inclined to take this one.  Since claim mode is on by default,
> the currently-working path should get exercised well before the
> release to shake out any bugs.  The other path doesn't work at all
> currently (AFAICT) unless you disable claim mode -- which is still
> available as a work-around, even if there is a bug in this patch.
> 
> I'll wait a day or two for others to speak up before giving it a
> formal ack, just in case.
> 
>  -George

Re: [PATCH] libxc: fix claim mode when creating HVM guest

[Ian Campbell]

On Mon, 2014-01-27 at 19:09 +0000, George Dunlap wrote:
> On 01/27/2014 05:53 PM, Wei Liu wrote:
> > The original code is wrong because:
> > * claim mode wants to know the total number of pages needed while
> >    original code provides the additional number of pages needed.
> > * if pod is enabled memory will already be allocated by the time we try
> >    to claim memory.
> >
> > So the fix would be:
> > * move claim mode before actual memory allocation.
> > * pass the right number of pages to hypervisor.
> >
> > The "right number of pages" should be number of pages of target memory
> > minus VGA_HOLE_SIZE, regardless of whether PoD is enabled.
> >
> > This fixes bug #32.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > Cc: Konrad Wilk <konrad.wilk@oracle.com>
> > Cc: George Dunlap <george.dunlap@eu.citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Reviewed-by: George Dunlap <george.dunlap@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> 
> > ---
> > WRT 4.4 release: this patch should be accpeted, otherwise PoD + claim
> > mode is complete broken. If this patch is deemed too complicated, we
> > should flip the switch to disable claim mode by default for 4.4.
> 
> I think a more reasonable mitigation strategy would simply be to ignore 
> claim mode when constructing a domain that uses PoD.
> 
> I'm inclined to take this one.  Since claim mode is on by default, the 
> currently-working path should get exercised well before the release to 
> shake out any bugs.  The other path doesn't work at all currently 
> (AFAICT) unless you disable claim mode -- which is still available as a 
> work-around, even if there is a bug in this patch.
> 
> I'll wait a day or two for others to speak up before giving it a formal 
> ack, just in case.

I agree with this plan.

Ian.

Re: [PATCH] libxc: fix claim mode when creating HVM guest

[Konrad Rzeszutek Wilk]

On Mon, Jan 27, 2014 at 05:53:38PM +0000, Wei Liu wrote:
> The original code is wrong because:
> * claim mode wants to know the total number of pages needed while
>   original code provides the additional number of pages needed.
> * if pod is enabled memory will already be allocated by the time we try
>   to claim memory.
> 
> So the fix would be:
> * move claim mode before actual memory allocation.
> * pass the right number of pages to hypervisor.
> 
> The "right number of pages" should be number of pages of target memory
> minus VGA_HOLE_SIZE, regardless of whether PoD is enabled.
> 
> This fixes bug #32.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Konrad Wilk <konrad.wilk@oracle.com>

And also 'Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>'

Thank you!
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
> WRT 4.4 release: this patch should be accpeted, otherwise PoD + claim
> mode is complete broken. If this patch is deemed too complicated, we
> should flip the switch to disable claim mode by default for 4.4.
> ---
>  tools/libxc/xc_hvm_build_x86.c |   36 +++++++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
> index 77bd365..dd3b522 100644
> --- a/tools/libxc/xc_hvm_build_x86.c
> +++ b/tools/libxc/xc_hvm_build_x86.c
> @@ -49,6 +49,8 @@
>  #define NR_SPECIAL_PAGES     8
>  #define special_pfn(x) (0xff000u - NR_SPECIAL_PAGES + (x))
>  
> +#define VGA_HOLE_SIZE (0x20)
> +
>  static int modules_init(struct xc_hvm_build_args *args,
>                          uint64_t vend, struct elf_binary *elf,
>                          uint64_t *mstart_out, uint64_t *mend_out)
> @@ -302,14 +304,31 @@ static int setup_guest(xc_interface *xch,
>      for ( i = mmio_start >> PAGE_SHIFT; i < nr_pages; i++ )
>          page_array[i] += mmio_size >> PAGE_SHIFT;
>  
> +    /*
> +     * Try to claim pages for early warning of insufficient memory available.
> +     * This should go before xc_domain_set_pod_target, becuase that function
> +     * actually allocates memory for the guest. Claiming after memory has been
> +     * allocated is pointless.
> +     */
> +    if ( claim_enabled ) {
> +        rc = xc_domain_claim_pages(xch, dom, target_pages - VGA_HOLE_SIZE);
> +        if ( rc != 0 )
> +        {
> +            PERROR("Could not allocate memory for HVM guest as we cannot claim memory!");
> +            goto error_out;
> +        }
> +    }
> +
>      if ( pod_mode )
>      {
>          /*
> -         * Subtract 0x20 from target_pages for the VGA "hole".  Xen will
> -         * adjust the PoD cache size so that domain tot_pages will be
> -         * target_pages - 0x20 after this call.
> +         * Subtract VGA_HOLE_SIZE from target_pages for the VGA
> +         * "hole".  Xen will adjust the PoD cache size so that domain
> +         * tot_pages will be target_pages - VGA_HOLE_SIZE after
> +         * this call.
>           */
> -        rc = xc_domain_set_pod_target(xch, dom, target_pages - 0x20,
> +        rc = xc_domain_set_pod_target(xch, dom,
> +                                      target_pages - VGA_HOLE_SIZE,
>                                        NULL, NULL, NULL);
>          if ( rc != 0 )
>          {
> @@ -333,15 +352,6 @@ static int setup_guest(xc_interface *xch,
>      cur_pages = 0xc0;
>      stat_normal_pages = 0xc0;
>  
> -    /* try to claim pages for early warning of insufficient memory available */
> -    if ( claim_enabled ) {
> -        rc = xc_domain_claim_pages(xch, dom, nr_pages - cur_pages);
> -        if ( rc != 0 )
> -        {
> -            PERROR("Could not allocate memory for HVM guest as we cannot claim memory!");
> -            goto error_out;
> -        }
> -    }
>      while ( (rc == 0) && (nr_pages > cur_pages) )
>      {
>          /* Clip count to maximum 1GB extent. */
> -- 
> 1.7.10.4
> 

Re: [PATCH] libxc: fix claim mode when creating HVM guest

[George Dunlap]

On 01/29/2014 06:33 PM, Konrad Rzeszutek Wilk wrote:
> On Mon, Jan 27, 2014 at 05:53:38PM +0000, Wei Liu wrote:
>> The original code is wrong because:
>> * claim mode wants to know the total number of pages needed while
>>    original code provides the additional number of pages needed.
>> * if pod is enabled memory will already be allocated by the time we try
>>    to claim memory.
>>
>> So the fix would be:
>> * move claim mode before actual memory allocation.
>> * pass the right number of pages to hypervisor.
>>
>> The "right number of pages" should be number of pages of target memory
>> minus VGA_HOLE_SIZE, regardless of whether PoD is enabled.
>>
>> This fixes bug #32.
>>
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>> Cc: Konrad Wilk <konrad.wilk@oracle.com>
>
> And also 'Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>'

Right, well nobody has shouted, so:

Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>

>
> Thank you!
>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> ---
>> WRT 4.4 release: this patch should be accpeted, otherwise PoD + claim
>> mode is complete broken. If this patch is deemed too complicated, we
>> should flip the switch to disable claim mode by default for 4.4.
>> ---
>>   tools/libxc/xc_hvm_build_x86.c |   36 +++++++++++++++++++++++-------------
>>   1 file changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/tools/libxc/xc_hvm_build_x86.c b/tools/libxc/xc_hvm_build_x86.c
>> index 77bd365..dd3b522 100644
>> --- a/tools/libxc/xc_hvm_build_x86.c
>> +++ b/tools/libxc/xc_hvm_build_x86.c
>> @@ -49,6 +49,8 @@
>>   #define NR_SPECIAL_PAGES     8
>>   #define special_pfn(x) (0xff000u - NR_SPECIAL_PAGES + (x))
>>
>> +#define VGA_HOLE_SIZE (0x20)
>> +
>>   static int modules_init(struct xc_hvm_build_args *args,
>>                           uint64_t vend, struct elf_binary *elf,
>>                           uint64_t *mstart_out, uint64_t *mend_out)
>> @@ -302,14 +304,31 @@ static int setup_guest(xc_interface *xch,
>>       for ( i = mmio_start >> PAGE_SHIFT; i < nr_pages; i++ )
>>           page_array[i] += mmio_size >> PAGE_SHIFT;
>>
>> +    /*
>> +     * Try to claim pages for early warning of insufficient memory available.
>> +     * This should go before xc_domain_set_pod_target, becuase that function
>> +     * actually allocates memory for the guest. Claiming after memory has been
>> +     * allocated is pointless.
>> +     */
>> +    if ( claim_enabled ) {
>> +        rc = xc_domain_claim_pages(xch, dom, target_pages - VGA_HOLE_SIZE);
>> +        if ( rc != 0 )
>> +        {
>> +            PERROR("Could not allocate memory for HVM guest as we cannot claim memory!");
>> +            goto error_out;
>> +        }
>> +    }
>> +
>>       if ( pod_mode )
>>       {
>>           /*
>> -         * Subtract 0x20 from target_pages for the VGA "hole".  Xen will
>> -         * adjust the PoD cache size so that domain tot_pages will be
>> -         * target_pages - 0x20 after this call.
>> +         * Subtract VGA_HOLE_SIZE from target_pages for the VGA
>> +         * "hole".  Xen will adjust the PoD cache size so that domain
>> +         * tot_pages will be target_pages - VGA_HOLE_SIZE after
>> +         * this call.
>>            */
>> -        rc = xc_domain_set_pod_target(xch, dom, target_pages - 0x20,
>> +        rc = xc_domain_set_pod_target(xch, dom,
>> +                                      target_pages - VGA_HOLE_SIZE,
>>                                         NULL, NULL, NULL);
>>           if ( rc != 0 )
>>           {
>> @@ -333,15 +352,6 @@ static int setup_guest(xc_interface *xch,
>>       cur_pages = 0xc0;
>>       stat_normal_pages = 0xc0;
>>
>> -    /* try to claim pages for early warning of insufficient memory available */
>> -    if ( claim_enabled ) {
>> -        rc = xc_domain_claim_pages(xch, dom, nr_pages - cur_pages);
>> -        if ( rc != 0 )
>> -        {
>> -            PERROR("Could not allocate memory for HVM guest as we cannot claim memory!");
>> -            goto error_out;
>> -        }
>> -    }
>>       while ( (rc == 0) && (nr_pages > cur_pages) )
>>       {
>>           /* Clip count to maximum 1GB extent. */
>> --
>> 1.7.10.4
>>

Re: [PATCH] libxc: fix claim mode when creating HVM guest

[Ian Campbell]

On Thu, 2014-01-30 at 14:38 +0000, George Dunlap wrote:
> On 01/29/2014 06:33 PM, Konrad Rzeszutek Wilk wrote:
> > On Mon, Jan 27, 2014 at 05:53:38PM +0000, Wei Liu wrote:
> >> The original code is wrong because:
> >> * claim mode wants to know the total number of pages needed while
> >>    original code provides the additional number of pages needed.
> >> * if pod is enabled memory will already be allocated by the time we try
> >>    to claim memory.
> >>
> >> So the fix would be:
> >> * move claim mode before actual memory allocation.
> >> * pass the right number of pages to hypervisor.
> >>
> >> The "right number of pages" should be number of pages of target memory
> >> minus VGA_HOLE_SIZE, regardless of whether PoD is enabled.
> >>
> >> This fixes bug #32.
> >>
> >> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> >> Cc: Konrad Wilk <konrad.wilk@oracle.com>
> >
> > And also 'Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>'
> 
> Right, well nobody has shouted, so:
> 
> Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Applied. Thanks all.

Re: [PATCH] libxc: fix claim mode when creating HVM guest

[Ian Campbell]

close 32
thanks

> > >> This fixes bug #32.

Closed.

Processed: Re: [PATCH] libxc: fix claim mode when creating HVM guest

[xen]

Processing commands for xen@bugs.xenproject.org:

> close 32
Closing bug #32
> thanks
Finished processing.

Modified/created Bugs:
 - 32: http://bugs.xenproject.org/xen/bug/32

---
Xen Hypervisor Bug Tracker
See http://wiki.xen.org/wiki/Reporting_Bugs_against_Xen for information on reporting bugs
Contact xen-bugs-owner@bugs.xenproject.org with any infrastructure issues