Re: [virtio-dev] Re: [virtio] Re: [PATCH v9 14/16] VIRTIO_F_NOTIFICATION_DATA: extra data to devices

[Halil Pasic <pasic@linux.vnet.ibm.com>]

On 03/08/2018 05:19 PM, Michael S. Tsirkin wrote:
> On Thu, Mar 08, 2018 at 02:03:31PM +0100, Halil Pasic wrote:
>> One stray idea was something like
>>
>> be32 (A:16;B:15;C:1);
>>
>> With 
>> * A occupying bits 0-15
>> * B occupying bits 16-30
>> * C occupying bit 30
>>
>> And bit n of B (n \in [0..15] being the n-16-th bit of the be32
>> subdivided into fields.
>>
>> The idea behind () is that it ain't unusual for tuples, and
>> also the most common grouping semantic is fitting in a sense
>> that all the fields are together the be32. The separation by
>> semicolon is to make it obvious that this has nothing to do
>> with C and that it's not intended to be implemented with C
>> bit-fields.
> 
> OK let's look at a real life example:
> 
> struct desc_event {
> 	le16 (
> 	     desc_event_off : 15; /* Descriptor Event Offset */
> 	     desc_event_wrap : 1; /* Descriptor Event Wrap Counter */
> 	);
> 	le16 (
> 	      desc_event_flags : 2; /* Descriptor Event Flags */
> 	      reserved : 14; /* Reserved, set to 0 */
> 	);
> };
> 
> As an option (2), I suggest curly brackets which look a bit more
> consistent:
> 
> struct desc_event {
> 	le16 {
> 	     desc_event_off : 15; /* Descriptor Event Offset */
> 	     desc_event_wrap : 1; /* Descriptor Event Wrap Counter */
> 	};> 	le16 {
> 	      desc_event_flags : 2; /* Descriptor Event Flags */
> 	      reserved : 14; /* Reserved, set to 0 */
> 	};
> };
> 
> Cornelia, Halil - any preferences? Ack on one of the above two?
> 

I'm fine with the curly braces as well. Important for me
is, that we are different enough from C. What you propose here
is already good enough for me, but you will find a couple of
ideas below, which could (IMHO) make it even better.

> introduction text accordingly (using curly braces, will adopt
> accordingly):
> 
> When documenting sub-byte data fields, C-like bitfield notation is used.

How about something like:

Some of the fields to be defined in this specification don't
start or don't end on byte boundary. Such fields are  called bit-fields.
A set of bit-fileds is always defined sub-division of an integer typed field.

What I don't like the original sentence is:
* sub-byte: for me it sounds like smaller than a byte, but A is obvoiusly larger
than a byte
* C-like bitfield notation: We just intentionally moved away from
the C bit-field syntax. The term bitfield ain't defined in the context of this
specification -- I can't tell if it's necessary to.



> Fields within an integer are always listed in order, with their lengths,
> from the least significant to the most significant bit. The fields
> are considered unsigned integers of the specified width with the next in
> significance relationship on the bits preserved.
> 
> For example:
> \begin{lstlisting}
> struct S {
> 	be16 {
> 	     A : 15;
> 	     B : 1;
> 	};

I think it may be beneficial to have a name for the complete be16.
Real life example:

le32 {
	vqn : 16;
	next_off : 15;
	next_wrap : 1;
} notification_data;



> 	be16 C;
> }

We could make the example look like this

struct S {
	be16 {
	     A : 15;
	     B : 1;
	} x;
	be16 y;
}

> \end{lstlisting}
> documents the value A stored in the low 15 bit of a 16 bit
> integer and the value B stored in the high bit of the 16 bit
s/of a 16 bit integer/\field{x}/

> integer, the integer in turn using the big-endian byte order
> and being stored at the beginning of the structure S,
> and being followed immediately by an unsigned integer C

s/C/\field{y}/

> stored at offset of 2 bytes (16 bits) from the beginning of
> the structure.
> 
> Note that this notation somewhat resembles the C bitfield syntax but
> should not be naively converted to a bitfield notation for portable
> code: it matches the way bitfields are packed by C compilers on
> little-endian architectures but not the way bitfields are packed by C
> compilers on big-endian architectures.
> 
> Assuming that CPU_TO_BE16 converts a 16-bit integer from a native
> CPU to the big-endian byte order, the following is the equivalent
> portable C code to generate a value in this format:

s/in this format/to be stored into \filed{x}.

> \begin{lstlisting}
> CPU_TO_BE16(B << 15 | A)
> \end{lstlisting}
> 
> 

Thanks for your patience!

Halil