Re: [PATCH v3] evtchn/fifo: don't enforce higher than necessary alignment

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 22/04/2021 10:19, Jan Beulich wrote:
> On 21.04.2021 21:52, Julien Grall wrote:
> > Hi,
> >
> > On 21/04/2021 15:36, Jan Beulich wrote:
> > > Neither the code nor the original commit provide any justification for
> > > the need to 8-byte align the struct in all cases. Enforce just as much
> > > alignment as the structure actually needs - 4 bytes - by using alignof()
> > > instead of a literal number.
> > I had another fresh look today at this patch. The 32-bit padding is
> > right after the field 'ready'.
> >
> > I can't for sure tell how the second half is going to ever be used and how.
> >
> > However, one possibility would be to extend the field 'ready' to 64-bit.
> > With the current code, we could easily make a single 64-bit access
> > without having to know whether the guest is able to interpret the top half.
> I don't think extending field sizes is generally to be considered ABI-
> compatible. I also don't think we can re-use the field at all, as I
> couldn't find any checking of it being zero (input) or it getting set
> to zero (output). 
That's would be fine so long we have a flag to control it. We can still 
write unconditionally because a guest can't rely on the pad...
> struct evtchn_init_control, which in principle could
> be a way to convey respective controlling flags, similarly has no room
> for extension, as its _pad[] also doesn't look to get checked anywhere.
Right, we would need to have a different way to convey. Yet, I am still 
unconvinced of the benefits change offer in this patch.
I am not going to Nack. So another maintainer in "THE REST" can express 
support for your patch and ack it.
Cheers,

--
Julien Grall