Re: [PATCH v8 1/3] xen/events: modify struct evtchn layout

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 27/11/2020 11:42, Jan Beulich wrote:
> On 25.11.2020 11:51, Juergen Gross wrote:
> > In order to avoid latent races when updating an event channel put
> > xen_consumer and pending fields in different bytes. This is no problem
> > right now, but especially the pending indicator isn't used only when
> > initializing an event channel (unlike xen_consumer), so any future
> > addition to this byte would need to be done with a potential race kept
> > in mind.
> >
> > At the same time move some other fields around to have less implicit
> > paddings and to keep related fields more closely together.
> >
> > Finally switch struct evtchn to no longer use fixed sized types where
> > not needed.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> with one more adjustment (can be done while committing, I guess):
>
> > --- a/xen/include/xen/sched.h
> > +++ b/xen/include/xen/sched.h
> > @@ -93,31 +93,33 @@ struct evtchn
> >   #define ECS_PIRQ         4 /* Channel is bound to a physical IRQ line.       
> > */
> >   #define ECS_VIRQ         5 /* Channel is bound to a virtual IRQ line.        
> > */
> >   #define ECS_IPI          6 /* Channel is bound to a virtual IPI line.        
> > */
> > -    u8  state;             /* ECS_* */
> > -    u8  xen_consumer:XEN_CONSUMER_BITS; /* Consumer in Xen if nonzero */
> > -    u8  pending:1;
> > -    u16 notify_vcpu_id;    /* VCPU for local delivery notification */
> > -    u32 port;
> > +    unsigned char state;   /* ECS_* */
> > +#ifndef NDEBUG
> > +    unsigned char old_state; /* State when taking lock in write mode. */
> > +#endif
> > +    unsigned char xen_consumer:XEN_CONSUMER_BITS; /* Consumer in Xen if != 0 */
> > +    unsigned int port;
> evtchn_port_t, to be in line with ...
>
> >       union {
> >           struct {
> >               domid_t remote_domid;
> > -        } unbound;     /* state == ECS_UNBOUND */
> > +        } unbound;          /* state == ECS_UNBOUND */
> >           struct {
> >               evtchn_port_t  remote_port;
> >               struct domain *remote_dom;
> > -        } interdomain; /* state == ECS_INTERDOMAIN */
> > +        } interdomain;      /* state == ECS_INTERDOMAIN */
> >           struct {
> > -            u32            irq;
> > +            unsigned int   irq;
> >               evtchn_port_t  next_port;
> >               evtchn_port_t  prev_port;
> ... three of the fields above from here.
>
> > -        } pirq;        /* state == ECS_PIRQ */
> > -        u16 virq;      /* state == ECS_VIRQ */
> > +        } pirq;             /* state == ECS_PIRQ */
> > +        unsigned int virq;  /* state == ECS_VIRQ */
> >       } u;
> > -    u8 priority;
> > -#ifndef NDEBUG
> > -    u8 old_state;      /* State when taking lock in write mode. */
> > -#endif
> > -    u32 fifo_lastq;    /* Data for fifo events identifying last queue. */
> > +
> > +    bool pending;                  /* FIFO event channels only. */
> > +    unsigned char priority;        /* FIFO event channels only. */
> > +    unsigned short notify_vcpu_id; /* VCPU for local delivery notification */
> I have to admit though that I'm not fully happy with the uses of
> "unsigned char" and "unsigned short". Yes, I did ask for this
> change (based on ./CODING_STYLE), but I did also hint towards the
> use of bitfields. If bitfields aren't an option here to achieve
> the desired dense packing, perhaps this desire should be permitted
> as another reason to use fixed width types. (Question goes more
> towards everyone who cares than to you specifically.)
I think uint*_t would make sense here because they are storing 
information received from an hypercall (all the fields should be fixed 
size there).
But I am also fine the current patch as it is still readable.

Cheers,

--
Julien Grall