Patchwork [for-4.10,2/5] tools/dombuilder: Remove clear_page() from xc_dom_boot.c

login
register
mail settings
Submitter Andrew Cooper
Date Oct. 5, 2017, 6:23 p.m.
Message ID <1507227824-11744-3-git-send-email-andrew.cooper3@citrix.com>
Download mbox | patch
Permalink /patch/353881/
State New
Headers show

Comments

Andrew Cooper - Oct. 5, 2017, 6:23 p.m.
pfn 0 is a legitimate (albeit unlikely) frame to use for translated domains,
so skipping it is wrong.  (This behaviour appears to exists simply to cover
the fact that zero is the default value of an uninitialised field in dom.)

ARM already clears the frames at the point that the pfns are allocated,
meaning that the added clear_page() is wasteful.  Alter x86 to match ARM and
clear the page when it is allocated.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 tools/libxc/xc_dom_arm.c  |  3 ++-
 tools/libxc/xc_dom_boot.c | 26 --------------------------
 tools/libxc/xc_dom_x86.c  |  8 ++++++++
 3 files changed, 10 insertions(+), 27 deletions(-)
Roger Pau Monne - Oct. 6, 2017, 9:35 a.m.
On Thu, Oct 05, 2017 at 06:23:40PM +0000, Andrew Cooper wrote:
> pfn 0 is a legitimate (albeit unlikely) frame to use for translated domains,
> so skipping it is wrong.  (This behaviour appears to exists simply to cover
> the fact that zero is the default value of an uninitialised field in dom.)
> 
> ARM already clears the frames at the point that the pfns are allocated,
> meaning that the added clear_page() is wasteful.  Alter x86 to match ARM and
> clear the page when it is allocated.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  tools/libxc/xc_dom_arm.c  |  3 ++-
>  tools/libxc/xc_dom_boot.c | 26 --------------------------
>  tools/libxc/xc_dom_x86.c  |  8 ++++++++
>  3 files changed, 10 insertions(+), 27 deletions(-)
> 
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index 98200ae..2aeb287 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -91,7 +91,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>      xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
>      xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
>      xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET);
> -    xc_clear_domain_page(dom->xch, dom->guest_domid, base + VUART_PFN_OFFSET);
> +    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn);
> +

This seems kind of unrelated to the patch itself, not that it's wrong.
IMHO it should be split into a separate patch.

>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
>              dom->console_pfn);
>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
> index 8a376d0..ce3c22e 100644
> --- a/tools/libxc/xc_dom_boot.c
> +++ b/tools/libxc/xc_dom_boot.c
> @@ -62,25 +62,6 @@ static int setup_hypercall_page(struct xc_dom_image *dom)
>      return rc;
>  }
>  
> -static int clear_page(struct xc_dom_image *dom, xen_pfn_t pfn)
> -{
> -    xen_pfn_t dst;
> -    int rc;
> -
> -    if ( pfn == 0 )
> -        return 0;
> -
> -    dst = xc_dom_p2m(dom, pfn);
> -    DOMPRINTF("%s: pfn 0x%" PRIpfn ", mfn 0x%" PRIpfn "",
> -              __FUNCTION__, pfn, dst);
> -    rc = xc_clear_domain_page(dom->xch, dom->guest_domid, dst);
> -    if ( rc != 0 )
> -        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
> -                     "%s: xc_clear_domain_page failed (pfn 0x%" PRIpfn
> -                     ", rc=%d)", __FUNCTION__, pfn, rc);
> -    return rc;
> -}
> -
>  
>  /* ------------------------------------------------------------------------ */
>  
> @@ -222,13 +203,6 @@ int xc_dom_boot_image(struct xc_dom_image *dom)
>          if ( (rc = dom->arch_hooks->setup_pgtables(dom)) != 0 )
>              return rc;
>  
> -    if ( (rc = clear_page(dom, dom->console_pfn)) != 0 )
> -        return rc;
> -    if ( (rc = clear_page(dom, dom->xenstore_pfn)) != 0 )
> -        return rc;
> -    if ( (rc = clear_page(dom, dom->vuart_gfn)) != 0 )
> -        return rc;
> -
>      /* start info page */
>      if ( dom->arch_hooks->start_info )
>          dom->arch_hooks->start_info(dom);
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index 885ca1b..0c80b59 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -543,10 +543,14 @@ static int alloc_magic_pages_pv(struct xc_dom_image *dom)
>      dom->xenstore_pfn = xc_dom_alloc_page(dom, "xenstore");
>      if ( dom->xenstore_pfn == INVALID_PFN )
>          return -1;
> +    xc_clear_domain_page(dom->xch, dom->guest_domid,
> +                         xc_dom_p2m(dom, dom->xenstore_pfn));

The start info page doesn't need to be cleared because it's re-written
anyway with the data I guess.

Thanks, Roger.
Andrew Cooper - Oct. 6, 2017, 9:51 a.m.
On 06/10/17 10:35, Roger Pau Monné wrote:
> On Thu, Oct 05, 2017 at 06:23:40PM +0000, Andrew Cooper wrote:
>> pfn 0 is a legitimate (albeit unlikely) frame to use for translated domains,
>> so skipping it is wrong.  (This behaviour appears to exists simply to cover
>> the fact that zero is the default value of an uninitialised field in dom.)
>>
>> ARM already clears the frames at the point that the pfns are allocated,
>> meaning that the added clear_page() is wasteful.  Alter x86 to match ARM and
>> clear the page when it is allocated.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>>  tools/libxc/xc_dom_arm.c  |  3 ++-
>>  tools/libxc/xc_dom_boot.c | 26 --------------------------
>>  tools/libxc/xc_dom_x86.c  |  8 ++++++++
>>  3 files changed, 10 insertions(+), 27 deletions(-)
>>
>> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
>> index 98200ae..2aeb287 100644
>> --- a/tools/libxc/xc_dom_arm.c
>> +++ b/tools/libxc/xc_dom_arm.c
>> @@ -91,7 +91,8 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>>      xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
>>      xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
>>      xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET);
>> -    xc_clear_domain_page(dom->xch, dom->guest_domid, base + VUART_PFN_OFFSET);
>> +    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn);
>> +
> This seems kind of unrelated to the patch itself, not that it's wrong.
> IMHO it should be split into a separate patch.

It is related.  It covers the fact that I remove a clear_page() of
dom->vuart_gfn below, and its context confirms that the console and
xenstore rings are cleared on ARM as well.

>
>>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
>>              dom->console_pfn);
>>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
>> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
>> index 8a376d0..ce3c22e 100644
>> --- a/tools/libxc/xc_dom_boot.c
>> +++ b/tools/libxc/xc_dom_boot.c
>> @@ -62,25 +62,6 @@ static int setup_hypercall_page(struct xc_dom_image *dom)
>>      return rc;
>>  }
>>  
>> -static int clear_page(struct xc_dom_image *dom, xen_pfn_t pfn)
>> -{
>> -    xen_pfn_t dst;
>> -    int rc;
>> -
>> -    if ( pfn == 0 )
>> -        return 0;
>> -
>> -    dst = xc_dom_p2m(dom, pfn);
>> -    DOMPRINTF("%s: pfn 0x%" PRIpfn ", mfn 0x%" PRIpfn "",
>> -              __FUNCTION__, pfn, dst);
>> -    rc = xc_clear_domain_page(dom->xch, dom->guest_domid, dst);
>> -    if ( rc != 0 )
>> -        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
>> -                     "%s: xc_clear_domain_page failed (pfn 0x%" PRIpfn
>> -                     ", rc=%d)", __FUNCTION__, pfn, rc);
>> -    return rc;
>> -}
>> -
>>  
>>  /* ------------------------------------------------------------------------ */
>>  
>> @@ -222,13 +203,6 @@ int xc_dom_boot_image(struct xc_dom_image *dom)
>>          if ( (rc = dom->arch_hooks->setup_pgtables(dom)) != 0 )
>>              return rc;
>>  
>> -    if ( (rc = clear_page(dom, dom->console_pfn)) != 0 )
>> -        return rc;
>> -    if ( (rc = clear_page(dom, dom->xenstore_pfn)) != 0 )
>> -        return rc;
>> -    if ( (rc = clear_page(dom, dom->vuart_gfn)) != 0 )
>> -        return rc;
>> -
>>      /* start info page */
>>      if ( dom->arch_hooks->start_info )
>>          dom->arch_hooks->start_info(dom);
>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>> index 885ca1b..0c80b59 100644
>> --- a/tools/libxc/xc_dom_x86.c
>> +++ b/tools/libxc/xc_dom_x86.c
>> @@ -543,10 +543,14 @@ static int alloc_magic_pages_pv(struct xc_dom_image *dom)
>>      dom->xenstore_pfn = xc_dom_alloc_page(dom, "xenstore");
>>      if ( dom->xenstore_pfn == INVALID_PFN )
>>          return -1;
>> +    xc_clear_domain_page(dom->xch, dom->guest_domid,
>> +                         xc_dom_p2m(dom, dom->xenstore_pfn));
> The start info page doesn't need to be cleared because it's re-written
> anyway with the data I guess.

In general, guests do get zeroed RAM to begin with, but there are
certain cases where this doesn't happen (mainly by a warm reboot and
using no-bootscrub).

The rings are critical to zero, as the ring indices need to start at
zero for them to function, but yes - the start info page does get fully
written with data.

~Andrew
Wei Liu - Oct. 6, 2017, 5:28 p.m.
On Thu, Oct 05, 2017 at 07:23:40PM +0100, Andrew Cooper wrote:
> pfn 0 is a legitimate (albeit unlikely) frame to use for translated domains,
> so skipping it is wrong.  (This behaviour appears to exists simply to cover
> the fact that zero is the default value of an uninitialised field in dom.)
> 
> ARM already clears the frames at the point that the pfns are allocated,
> meaning that the added clear_page() is wasteful.  Alter x86 to match ARM and
> clear the page when it is allocated.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Patch

diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
index 98200ae..2aeb287 100644
--- a/tools/libxc/xc_dom_arm.c
+++ b/tools/libxc/xc_dom_arm.c
@@ -91,7 +91,8 @@  static int alloc_magic_pages(struct xc_dom_image *dom)
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
     xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
     xc_clear_domain_page(dom->xch, dom->guest_domid, base + MEMACCESS_PFN_OFFSET);
-    xc_clear_domain_page(dom->xch, dom->guest_domid, base + VUART_PFN_OFFSET);
+    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->vuart_gfn);
+
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_PFN,
             dom->console_pfn);
     xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_PFN,
diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
index 8a376d0..ce3c22e 100644
--- a/tools/libxc/xc_dom_boot.c
+++ b/tools/libxc/xc_dom_boot.c
@@ -62,25 +62,6 @@  static int setup_hypercall_page(struct xc_dom_image *dom)
     return rc;
 }
 
-static int clear_page(struct xc_dom_image *dom, xen_pfn_t pfn)
-{
-    xen_pfn_t dst;
-    int rc;
-
-    if ( pfn == 0 )
-        return 0;
-
-    dst = xc_dom_p2m(dom, pfn);
-    DOMPRINTF("%s: pfn 0x%" PRIpfn ", mfn 0x%" PRIpfn "",
-              __FUNCTION__, pfn, dst);
-    rc = xc_clear_domain_page(dom->xch, dom->guest_domid, dst);
-    if ( rc != 0 )
-        xc_dom_panic(dom->xch, XC_INTERNAL_ERROR,
-                     "%s: xc_clear_domain_page failed (pfn 0x%" PRIpfn
-                     ", rc=%d)", __FUNCTION__, pfn, rc);
-    return rc;
-}
-
 
 /* ------------------------------------------------------------------------ */
 
@@ -222,13 +203,6 @@  int xc_dom_boot_image(struct xc_dom_image *dom)
         if ( (rc = dom->arch_hooks->setup_pgtables(dom)) != 0 )
             return rc;
 
-    if ( (rc = clear_page(dom, dom->console_pfn)) != 0 )
-        return rc;
-    if ( (rc = clear_page(dom, dom->xenstore_pfn)) != 0 )
-        return rc;
-    if ( (rc = clear_page(dom, dom->vuart_gfn)) != 0 )
-        return rc;
-
     /* start info page */
     if ( dom->arch_hooks->start_info )
         dom->arch_hooks->start_info(dom);
diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 885ca1b..0c80b59 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -543,10 +543,14 @@  static int alloc_magic_pages_pv(struct xc_dom_image *dom)
     dom->xenstore_pfn = xc_dom_alloc_page(dom, "xenstore");
     if ( dom->xenstore_pfn == INVALID_PFN )
         return -1;
+    xc_clear_domain_page(dom->xch, dom->guest_domid,
+                         xc_dom_p2m(dom, dom->xenstore_pfn));
 
     dom->console_pfn = xc_dom_alloc_page(dom, "console");
     if ( dom->console_pfn == INVALID_PFN )
         return -1;
+    xc_clear_domain_page(dom->xch, dom->guest_domid,
+                         xc_dom_p2m(dom, dom->console_pfn));
 
     dom->alloc_bootstack = 1;
 
@@ -696,7 +700,11 @@  static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
                      special_pfn(SPECIALPAGE_IDENT_PT) << PAGE_SHIFT);
 
     dom->console_pfn = special_pfn(SPECIALPAGE_CONSOLE);
+    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->console_pfn);
+
     dom->xenstore_pfn = special_pfn(SPECIALPAGE_XENSTORE);
+    xc_clear_domain_page(dom->xch, dom->guest_domid, dom->xenstore_pfn);
+
     dom->parms.virt_hypercall = -1;
 
     rc = 0;