Re: [RFC PATCH 1/3] Add a field in struct domain to indicate evtchn level.

[Wei Liu <Wei.Liu2@citrix.com>]

On Wed, 2013-01-02 at 11:11 +0000, David Vrabel wrote:
> On 31/12/12 18:22, Wei Liu wrote:
> > From: Wei Liu <liuw@liuw.name>
> 
> The first patch needs to add the ABI documentation.
> 
> It's not clear what this is all for from just the changeset descriptions.
> 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  xen/common/event_channel.c |    1 +
> >  xen/include/xen/sched.h    |   15 ++++++++++++++-
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> > index 89f0ca7..87e422e 100644
> > --- a/xen/common/event_channel.c
> > +++ b/xen/common/event_channel.c
> > @@ -1173,6 +1173,7 @@ void notify_via_xen_event_channel(struct domain *ld, int lport)
> >  int evtchn_init(struct domain *d)
> >  {
> >      spin_lock_init(&d->event_lock);
> > +    d->evtchn_level = 2;
> 
> I'd be inclined to have something like:
> 
> /* Original ABI: 2 level event channels */
> #define EVTCHN_LEVEL_DEFAULT 2
> ...
>     d->evtchn_level = EVTCHN_LEVEL_DEFAULT;
> 
> 
> >      if ( get_free_port(d) != 0 )
> >          return -EINVAL;
> >      evtchn_from_port(d, 0)->state = ECS_RESERVED;
> > diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> > index 6c55039..1c43e0a 100644
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -50,7 +50,19 @@ extern struct domain *dom0;
> >  #else
> >  #define BITS_PER_EVTCHN_WORD(d) (has_32bit_shinfo(d) ? 32 : BITS_PER_LONG)
> >  #endif
> > -#define MAX_EVTCHNS(d) (BITS_PER_EVTCHN_WORD(d) * BITS_PER_EVTCHN_WORD(d))
> > +#define MAX_EVTCHNS_L2(d) (BITS_PER_EVTCHN_WORD(d) * BITS_PER_EVTCHN_WORD(d))
> > +#define MAX_EVTCHNS_L3(d) (MAX_EVTCHNS_L2(d) * BITS_PER_EVTCHN_WORD(d))
> > +#define MAX_EVTCHNS(d) ({ int __v = 0;          \
> > +            switch ( d->evtchn_level ) {        \
> > +            case 2:                             \
> > +                __v = MAX_EVTCHNS_L2(d); break; \
> > +            case 3:                             \
> > +                __v = MAX_EVTCHNS_L3(d); break; \
> > +            default:                            \
> > +                BUG();                          \
> > +            };                                  \
> > +            __v;})
> 
> Here you BUG if d->evtchn_level is bad, but in other places in later
> patches with similar you have a printk.  Need to be more consistent here.
> 

Given that d->evtchn_level is only manipulated by the hypervisor, I
think a BUG() here is correct.  I will add a BUG() after printk's in
other places if necessary.  If I switch from 'switch' to ops pointers,
this problem will automatically go away.

> On the Linux side you use a set of event channel ops instead of
> repeatedly testing d->evtchn_level.  Would this also be a better
> approach for the Xen side?

I will have a look at this.


Wei.