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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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

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