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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> ---
>  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.

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