|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-4.10 4/5] tools/dombuilder: Fix asymmetry when setting up console and xenstore rings
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@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.
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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |