Re: [PATCH v7 2/3] xen/events: modify struct evtchn layout

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.11.2020 08:01, Juergen Gross wrote:
> In order to avoid latent races when updating an event channel put
> xen_consumer and pending fields in different bytes.

I think there's a little more to be said here as to what the
actual risk is, as the two fields are - afaict - at present
fine the way they're declared.

> @@ -94,9 +93,10 @@ struct evtchn
>  #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 */

I see no reason to use a full byte for this one; in fact I
was considering whether it, state, and old_state couldn't
share storage (the latest when we run into space issues with
this struct). (In this context I'm also observing that
old_state could get away with just 2 bits, i.e. all three
fields would fit in a single byte.)

> -    u8  pending:1;
> -    u16 notify_vcpu_id;    /* VCPU for local delivery notification */
> +#ifndef NDEBUG
> +    u8  old_state;     /* State when taking lock in write mode. */
> +#endif
> +    u8  xen_consumer;  /* Consumer in Xen if nonzero */
>      u32 port;
>      union {
>          struct {
> @@ -113,11 +113,13 @@ struct evtchn
>          } pirq;        /* state == ECS_PIRQ */
>          u16 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. */
> +
> +    /* FIFO event channels only. */
> +    u8  pending;
> +    u8  priority;
> +    u16 notify_vcpu_id;    /* VCPU for local delivery notification */

This field definitely isn't FIFO-only.

Also for all fields you touch anyway, may I ask that you switch to
uint<N>_t or, in the case of "pending", bool?

Jan