[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio] Re: [PATCH v9 14/16] VIRTIO_F_NOTIFICATION_DATA: extra data to devices
On Wed, 7 Mar 2018 17:05:24 +0100 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 03/07/2018 03:49 PM, Cornelia Huck wrote: > >>>> +When VIRTIO_F_NOTIFICATION_DATA has been negotiated, > >>>> +the driver notifies the device by writing the following > >>>> +32-bit value to the Queue Notify address: > >>>> +\begin{lstlisting} > >>>> +le32 vqn : 16, > >>>> + next_off : 15, > >>>> + next_wrap : 1; > >>> Don't we want to write this as > >>> > >>> le32 vqn : 16; > >>> le32 next_off :15; > >>> le32 next_wrap : 1; > >>> > >>> ? > >> Same thing in C, but would be more confusing IMHO since it will be up to > >> the reader to figure out which fields comprise the 32 bit integer. > > It looked weird to me. Other opinions? > > > > Regarding the c11 standard the two are equivalent. Thus it does not > matter to me which notation is used. AFAIK bit-fields are only defined > in the context of structs (and/or unions), so I assumed that. Putting a > struct around it would be much better IMHO. > > I don't agree with Michael's argument about 'which fields comprise the > 32 bit integer', as IMHO it does not make sense in terms of c11. > > Consider > > struct A { > uint32_t a:30, b:1, c:2:, d:8; > }; > > I think, in this particular case the notation ain't very helpful in > figuring out what comprises what. For that reason, if I really need > to choose, I would side with Connie on this one. > > But there is another, more significant problem IMHO. The guarantees > provided by the C language (c11) regarding the resulting memory layout > are not sufficient to reason about it like Michael's comment and > the bit's of the draft imply. To know the memory layout we need the > ABI specification for the given platform on top of the C standard. > > So if the bit fields are about in memory layout, I find the stuff > problematic. If however we use bit-fields only to define how arithmetic > works, then we are fine. I'm not sure we should go down the C standard rabbit hole. People have gotten lost in there. If the clarification of what we mean by this notation (patch 1 + the update sent later) is not enough, I'd rather prefer us to add a clarifying sentence/diagram/... there. I was mainly bothered by the change to the definition in this patch...
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]