Re: [Xen-devel] [RFC PATCH V3 06/22] Define extended event channel registration interface

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, 2013-02-28 at 12:32 +0000, David Vrabel wrote:
> 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 <wei.liu2@xxxxxxxxxx>
> > ---
> >  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.
> 

My first thought was that it is important for Dom0 or driver domains to
get what they want otherwise the whole system suffers from performance
degradation.

I'm considering per-cpu registration call now, for the reason of saving
resources.

> 
> > + */
> > +#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.
> 

One way to solve this is to have that union contain pointers to specific
structures, but as you suggested, just add sub-op for each new ABI is
Ok.


Wei.



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