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

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

On 06/10/17 11:17, Roger Pau Monné wrote:
> 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@citrix.com>
> LGTM, just one nit:
>
> Reviewed-by: Roger Pau Monné <roger.pau@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/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.

It turns out that Clang objects to this particular constant.

https://travis-ci.org/andyhhp/xen/jobs/283828942

I've folded a change to use (~0), which was what libxl used.

>
>>  #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;

Will do.

~Andrew

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