Re: [Xen-devel] [PATCH for-4.10 4/5] tools/dombuilder: Fix asymmetry when setting up console and xenstore rings

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

On Thu, Oct 05, 2017 at 06:23:43PM +0000, Andrew Cooper wrote:
> libxl always uses xc_dom_gnttab_init(), which internally calls
> xc_dom_gnttab{_hvm,}_seed() to set up the grants point at the console and
> xenstore rings.  For HVM guests, libxl then asks Xen for the information set
> up previously, and calls xc_dom_gnttab_hvm_seed() a second time, which is
> wasteful.  ARM construction expects libxl to have set up
> dom->{console,xenstore}_evtchn earlier, so only actually functions because of
> this second call.
> 
> Rationalise everything and make it consistent for all guests.
> 
>  1) Users of the domain builder are expected to provide
>     dom->{console,xenstore}_{evtchn,domid} unconditionally.  This is checked
>     by setting invalid values in xc_dom_allocate(), and checking in
>     xc_dom_boot_image().
> 
>  2) For x86 HVM and ARM guests, the event channels are given to Xen at the
>     same time as the ring gfns.  ARM already did this, but x86 is updated to
>     match.  x86 PV already provides this information in the start_info page.
> 
>  3) Libxl is updated to drop all relevant functionality from
>     hvm_build_set_params(), and behave consistently with PV guests when it
>     comes to the handling of dom->{console,xenstore}_{evtchn,domid,gfn}.
> 
> This removes several redundant hypercalls (including a foreign mapping) from
> the x86 HVM and ARM construction paths.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

LGTM, just one nit:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> ---
>  tools/libxc/include/xc_dom.h      | 12 ++++++++----
>  tools/libxc/xc_dom_arm.c          |  2 +-
>  tools/libxc/xc_dom_boot.c         | 36 ++++++++++++++++++++++++++++++++++++
>  tools/libxc/xc_dom_compat_linux.c |  2 ++
>  tools/libxc/xc_dom_core.c         |  5 +++++
>  tools/libxc/xc_dom_x86.c          |  4 ++++
>  tools/libxl/libxl_dom.c           | 28 ++++++++++------------------
>  tools/libxl/libxl_internal.h      |  1 -
>  8 files changed, 66 insertions(+), 24 deletions(-)
> 
> diff --git a/tools/libxc/include/xc_dom.h b/tools/libxc/include/xc_dom.h
> index 80b4fbd..790869b 100644
> --- a/tools/libxc/include/xc_dom.h
> +++ b/tools/libxc/include/xc_dom.h
> @@ -20,6 +20,8 @@
>  #include <xenguest.h>
>  
>  #define INVALID_PFN ((xen_pfn_t)-1)
> +#define INVALID_EVTCHN (~0u)
> +#define INVALID_DOMID  (-1)

Both xl and libxl already have an INVALID_DOMID, maybe it would be
time to place this in a public header.

Oh, I see that at the end of the patch you remove the one from libxl,
so nm.

>  #define X86_HVM_NR_SPECIAL_PAGES    8
>  #define X86_HVM_END_SPECIAL_REGION  0xff000u
>  
> @@ -104,10 +106,16 @@ struct xc_dom_image {
>       * Details for the toolstack-prepared rings.
>       *
>       * *_gfn fields are allocated by the domain builder.
> +     * *_{evtchn,domid} fields must be provided by the caller.
>       */
>      xen_pfn_t console_gfn;
>      xen_pfn_t xenstore_gfn;
>  
> +    unsigned int console_evtchn;
> +    unsigned int xenstore_evtchn;
> +    domid_t console_domid;
> +    domid_t xenstore_domid;
> +
>      /*
>       * initrd parameters as specified in start_info page
>       * Depending on capabilities of the booted kernel this may be a virtual
> @@ -165,10 +173,6 @@ struct xc_dom_image {
>  
>      /* misc xen domain config stuff */
>      unsigned long flags;
> -    unsigned int console_evtchn;
> -    unsigned int xenstore_evtchn;
> -    domid_t console_domid;
> -    domid_t xenstore_domid;
>      xen_pfn_t shared_info_mfn;
>  
>      xc_interface *xch;
> diff --git a/tools/libxc/xc_dom_arm.c b/tools/libxc/xc_dom_arm.c
> index c7aa44a..d668df1 100644
> --- a/tools/libxc/xc_dom_arm.c
> +++ b/tools/libxc/xc_dom_arm.c
> @@ -99,7 +99,7 @@ static int alloc_magic_pages(struct xc_dom_image *dom)
>              dom->xenstore_gfn);
>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_MONITOR_RING_PFN,
>              base + MEMACCESS_PFN_OFFSET);
> -    /* allocated by toolstack */
> +
>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_CONSOLE_EVTCHN,
>              dom->console_evtchn);
>      xc_hvm_param_set(dom->xch, dom->guest_domid, HVM_PARAM_STORE_EVTCHN,
> diff --git a/tools/libxc/xc_dom_boot.c b/tools/libxc/xc_dom_boot.c
> index a84a95e..8d4fefa 100644
> --- a/tools/libxc/xc_dom_boot.c
> +++ b/tools/libxc/xc_dom_boot.c
> @@ -163,6 +163,39 @@ void *xc_dom_boot_domU_map(struct xc_dom_image *dom, 
> xen_pfn_t pfn,
>      return ptr;
>  }
>  
> +static int xc_dom_check_required_fields(struct xc_dom_image *dom)
> +{
> +    int rc = 0;
> +
> +    if ( dom->console_evtchn == INVALID_EVTCHN )
> +    {
> +        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
> +                     "%s: Caller didn't set dom->console_evtchn", __func__);
> +        rc = -1;
> +    }
> +    if ( dom->console_domid == INVALID_DOMID )
> +    {
> +        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
> +                     "%s: Caller didn't set dom->console_domid", __func__);
> +        rc = -1;
> +    }
> +
> +    if ( dom->xenstore_evtchn == INVALID_EVTCHN )
> +    {
> +        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
> +                     "%s: Caller didn't set dom->xenstore_evtchn", __func__);
> +        rc = -1;
> +    }
> +    if ( dom->xenstore_domid == INVALID_DOMID )
> +    {
> +        xc_dom_panic(dom->xch, XC_INVALID_PARAM,
> +                     "%s: Caller didn't set dom->xenstore_domid", __func__);
> +        rc = -1;
> +    }

if ( rc != 0 )
    errno = EINVAL;

Thanks Roger.

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