From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [RFC PATCH V3 06/22] Define extended event channel registration interface Date: Thu, 28 Feb 2013 12:32:40 +0000 Message-ID: <512F4E68.2000707@citrix.com> References: <1361975655-22295-1-git-send-email-wei.liu2@citrix.com> <1361975655-22295-7-git-send-email-wei.liu2@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1361975655-22295-7-git-send-email-wei.liu2@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: "Keir (Xen.org)" , Ian Campbell , "jbeulich@suse.com" , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 27/02/13 14:33, Wei Liu wrote: > This interface allows user to query supported event channel types. If we need > to disable a specific ABI in the future, we just need to remove the dead code > and clear corresponding feature bit. > > Signed-off-by: Wei Liu > --- > xen/include/public/event_channel.h | 44 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/xen/include/public/event_channel.h b/xen/include/public/event_channel.h > index 07ff321..dff3364 100644 > --- a/xen/include/public/event_channel.h > +++ b/xen/include/public/event_channel.h > @@ -71,6 +71,7 @@ > #define EVTCHNOP_bind_vcpu 8 > #define EVTCHNOP_unmask 9 > #define EVTCHNOP_reset 10 > +#define EVTCHNOP_register_extended 11 > /* ` } */ > > typedef uint32_t evtchn_port_t; > @@ -258,6 +259,49 @@ struct evtchn_reset { > typedef struct evtchn_reset evtchn_reset_t; > > /* > + * EVTCHNOP_register_extended: Register extended event channel > + * NOTES: > + * 1. Currently only 3-level is supported. > + * 2. Should fall back to 2-level if this call fails. > + */ > +/* 64 bit guests need 8 pages for evtchn_pending and evtchn_mask for > + * 256k event channels while 32 bit ones only need 1 page for 32k > + * event channels. */ > +#define EVTCHN_MAX_L3_PAGES 8 > +struct evtchn_register_3level { > + /* IN parameters. */ > + uint32_t nr_pages; /* for evtchn_{pending,mask} */ > + uint32_t nr_vcpus; /* for l2sel_{mfns,offsets} */ > + XEN_GUEST_HANDLE(xen_pfn_t) evtchn_pending; > + XEN_GUEST_HANDLE(xen_pfn_t) evtchn_mask; > + XEN_GUEST_HANDLE(xen_pfn_t) l2sel_mfns; > + XEN_GUEST_HANDLE(xen_pfn_t) l2sel_offsets; > +}; Registering per-VCPU resources at start-of-day doesn't seem like the right thing to do here. Why waste resources for VCPUs that are never going to be used? And I don't think we want to constraint how VCPU hotplug works by requiring resource for all possible VCPUs to be allocated up-front. If there isn't enough resource for all VCPUs to all use the 3-level ABI then I think the preferred trade off is to offline the VCPUs that cannot get resources and not fallback to the 2-level ABI. The event channel limit is a hard scalability limit, number of VCPUs is a soft limit, so a guest is more likely to gracefully degrade in usefulness if it loses VCPUs instead of available event channels. Obviously, if 3-level is requested and the boot VCPUs fails to enable it, then it should fallback to 2-level because this is better than just panicking. > +typedef struct evtchn_register_3level evtchn_register_3level_t; > +DEFINE_XEN_GUEST_HANDLE(evtchn_register_3level_t); > + > +/* commands: > + * EVTCHN_EXTENDED_QUERY(0): query supported extended event channel types, > + * _NONE supported types are or'ed in return value of > + * the hypercall. > + * EVTCHN_EXTENDED_*: specific extended event channel subcommand. Combining query and register makes no sense. > + */ > +#define EVTCHN_EXTENDED_QUERY 0 > +/* supported extended event channel */ > +#define EVTCHN_EXTENDED_NONE 0 > +#define _EVTCHN_EXTENDED_L3 0 > +#define EVTCHN_EXTENDED_L3 (1U << _EVTCHN_EXTENDED_L3) > +struct evtchn_register_extended { > + /* IN parameters. */ > + uint32_t cmd; > + union { > + evtchn_register_3level_t l3; > + } u; > +}; Adding new members to this union as new event channels ABI are implemented are going to change its size and potentially the alignment of the union member. This seems a potential for future ABI compatibility problems. See also by comment to patch 18. There doesn't seem to be any value in having a single register sub-op for all possible future ABIs. Just add a new sub-op for each new ABI. David