[PATCH v2] xen: prevent PVH Dom0 from having pages with more than one ref

[PATCH v2] xen: prevent PVH Dom0 from having pages with more than one ref

[Roger Pau Monne]

With the current implementation of the Dom0 domain builder a PVH Dom0
ends up with pages that have 2 references, which means they are not
freed when Dom0 balloons memory down. This leads to those pages
not being freed in guest_remove_page, so they are leaked and the
memory accounting between Xen and Dom0 differs, because Dom0 OS
actually thinks they are freed, but d->tot_pages is not decreased.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Mukesh Rathor <mukesh.rathor@oracle.com>
---
Changes since v1:
 - Fix coding style.
 - Fix a second occurrence of the same issue.
 - Use paging_mode_translate instead of is_pvh_domain.
---
 xen/arch/x86/domain_build.c |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
index ba42fc9..842ed5f 100644
--- a/xen/arch/x86/domain_build.c
+++ b/xen/arch/x86/domain_build.c
@@ -1137,10 +1137,13 @@ int __init construct_dom0(
                                     L1_PROT : COMPAT_L1_PROT));
         l1tab++;
 
-        page = mfn_to_page(mfn);
-        if ( (page->u.inuse.type_info == 0) &&
-             !get_page_and_type(page, d, PGT_writable_page) )
-            BUG();
+        if ( !paging_mode_translate(d) )
+        {
+            page = mfn_to_page(mfn);
+            if ( (page->u.inuse.type_info == 0) &&
+                 !get_page_and_type(page, d, PGT_writable_page) )
+                BUG();
+        }
     }
 
     if ( is_pv_32on64_domain(d) )
@@ -1300,9 +1303,10 @@ int __init construct_dom0(
         if ( get_gpfn_from_mfn(mfn) >= count )
         {
             BUG_ON(is_pv_32bit_domain(d));
-            if ( !page->u.inuse.type_info &&
-                 !get_page_and_type(page, d, PGT_writable_page) )
-                BUG();
+            if ( !paging_mode_translate(d) )
+                if ( !page->u.inuse.type_info &&
+                     !get_page_and_type(page, d, PGT_writable_page) )
+                    BUG();
 
             dom0_update_physmap(d, pfn, mfn, vphysmap_start);
             ++pfn;
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH v2] xen: prevent PVH Dom0 from having pages with more than one ref

[Andrew Cooper]

On 13/06/14 12:43, Roger Pau Monne wrote:
> With the current implementation of the Dom0 domain builder a PVH Dom0
> ends up with pages that have 2 references, which means they are not
> freed when Dom0 balloons memory down. This leads to those pages
> not being freed in guest_remove_page, so they are leaked and the
> memory accounting between Xen and Dom0 differs, because Dom0 OS
> actually thinks they are freed, but d->tot_pages is not decreased.

Please can you explain the cause of the problem, and state that it is a
difference between PV and HVM memory management.

>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
> Changes since v1:
>  - Fix coding style.
>  - Fix a second occurrence of the same issue.
>  - Use paging_mode_translate instead of is_pvh_domain.
> ---
>  xen/arch/x86/domain_build.c |   18 +++++++++++-------
>  1 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/domain_build.c b/xen/arch/x86/domain_build.c
> index ba42fc9..842ed5f 100644
> --- a/xen/arch/x86/domain_build.c
> +++ b/xen/arch/x86/domain_build.c
> @@ -1137,10 +1137,13 @@ int __init construct_dom0(
>                                      L1_PROT : COMPAT_L1_PROT));
>          l1tab++;
>  
> -        page = mfn_to_page(mfn);
> -        if ( (page->u.inuse.type_info == 0) &&
> -             !get_page_and_type(page, d, PGT_writable_page) )
> -            BUG();
> +        if ( !paging_mode_translate(d) )
> +        {
> +            page = mfn_to_page(mfn);
> +            if ( (page->u.inuse.type_info == 0) &&
> +                 !get_page_and_type(page, d, PGT_writable_page) )
> +                BUG();
> +        }
>      }
>  
>      if ( is_pv_32on64_domain(d) )
> @@ -1300,9 +1303,10 @@ int __init construct_dom0(
>          if ( get_gpfn_from_mfn(mfn) >= count )
>          {
>              BUG_ON(is_pv_32bit_domain(d));
> -            if ( !page->u.inuse.type_info &&
> -                 !get_page_and_type(page, d, PGT_writable_page) )
> -                BUG();
> +            if ( !paging_mode_translate(d) )
> +                if ( !page->u.inuse.type_info &&
> +                     !get_page_and_type(page, d, PGT_writable_page) )
> +                    BUG();
>  

These two ifs should be joined, which looks as if it will reduce the
delta to 1 line.

~Andrew

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

Re: [PATCH v2] xen: prevent PVH Dom0 from having pages with more than one ref

[Roger Pau Monné]

On 13/06/14 13:48, Andrew Cooper wrote:
> On 13/06/14 12:43, Roger Pau Monne wrote:
>> With the current implementation of the Dom0 domain builder a PVH Dom0
>> ends up with pages that have 2 references, which means they are not
>> freed when Dom0 balloons memory down. This leads to those pages
>> not being freed in guest_remove_page, so they are leaked and the
>> memory accounting between Xen and Dom0 differs, because Dom0 OS
>> actually thinks they are freed, but d->tot_pages is not decreased.
> 
> Please can you explain the cause of the problem, and state that it is a
> difference between PV and HVM memory management.

What about the following commit message:

xen: prevent PVH Dom0 from having pages with more than one ref

On PV guests a reference is taken when a page gets added to the page
tables, which makes pages added to the page tables have two references,
but this is not suitable for PVH that doesn't use the PVMMU. In the PVH
case only one reference has to be taken or else the page would not be
freed when the memory of the domain is decreased.

Roger.

Re: [PATCH v2] xen: prevent PVH Dom0 from having pages with more than one ref

[Mukesh Rathor]

On Fri, 13 Jun 2014 13:43:55 +0200
Roger Pau Monne <roger.pau@citrix.com> wrote:

> With the current implementation of the Dom0 domain builder a PVH Dom0
> ends up with pages that have 2 references, which means they are not
> freed when Dom0 balloons memory down. This leads to those pages
> not being freed in guest_remove_page, so they are leaked and the
> memory accounting between Xen and Dom0 differs, because Dom0 OS
> actually thinks they are freed, but d->tot_pages is not decreased.

Ah, right. good catch. I agree with the changes...

thanks,
Mukesh