Re: [virtio-comment] [PATCH 1/1] ccw: ccw payload description

[Rusty Russell <rusty@au1.ibm.com>]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Thu, Jan 30, 2014 at 01:10:29PM +0100, Cornelia Huck wrote:
>> I think what makes this complicated is that we specify datastructures
>> that are supposed to work for both legacy and non-legacy, when they
>> are in fact different (at least on be).
>
> So how about something like attribute((virtio_endian)) (a bit too long)
> or ve32 (slightly ugly).
>
>> How about adding "A note on legacy interfaces" for each of these cases,
>> where we specify the data structures with native-endian where
>> appropriate, and keep the explicit le notation for the standard? We'd
>> end up with some duplication, but we'd be very clear on what is
>> expected.
>
> In practice, this will make it harder to write transitional
> drivers. Driver authors will have to reverse-engineer field
> layouts and note that offsets happen to match.
> That's not being nice to readers.
>
> Take a look at virtio drivers that Rusty wrote:
> ring layouts *don't* use le tags because sometimes
> they are used with be.

Sure, but it will always be hard to write such drivers.  There shouldn't
be many people doing that though.  And they'll be doing it for backwards
compatibility, where they'll presumably test...

I'm more concerned about people writing non-legacy drivers and getting
endianness wrong.  But if you feel strongly, we could put a comment
above each one?

Cheers,
Rusty.

diff --git a/content.tex b/content.tex
index 48698fc..fa33435 100644
--- a/content.tex
+++ b/content.tex
@@ -349,6 +349,7 @@ Drivers MUST NOT add a descriptor chain over than $2^{32}$ bytes long in total;
 this implies that loops in the descriptor chain are forbidden!
 
 \begin{lstlisting}
+        /* Note: LEGACY version was not little endian! */
 	struct vring_desc {
 		/* Address (guest-physical). */
 		le64 addr;
@@ -409,6 +410,7 @@ the device MUST ignore the write-only flag (flags\&VRING_DESC_F_WRITE) in the de
 \subsection{The Virtqueue Available Ring}\label{sec:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Available Ring}
 
 \begin{lstlisting}
+        /* Note: LEGACY version was not little endian! */
 	struct vring_avail {
 	#define VRING_AVAIL_F_NO_INTERRUPT      1
 		le16 flags;
@@ -445,6 +447,7 @@ entirely.
 \subsection{The Virtqueue Used Ring}\label{sec:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Used Ring}
 
 \begin{lstlisting}
+        /* Note: LEGACY version was not little endian! */
 	struct vring_used {
 	#define VRING_USED_F_NO_NOTIFY  1
 		le16 flags;
@@ -453,6 +456,7 @@ entirely.
 		le16 avail_event; /* Only if VIRTIO_RING_F_EVENT_IDX */
 	};
 
+        /* Note: LEGACY version was not little endian! */
 	/* le32 is used here for ids for padding reasons. */
 	struct vring_used_elem {
 		/* Index of start of used descriptor chain. */
@@ -2435,6 +2439,7 @@ and transmitq0..transmitqN respectively;
 is negotiated.  Legal values for this field are 1 to 0x8000.
 
 \begin{lstlisting}
+        /* Note: LEGACY version was not little endian! */
 	struct virtio_net_config {
 		u8 mac[6];
 		le16 status;
@@ -2521,6 +2526,7 @@ placed in the receiveq0..receiveqN. In each case, the packet
 itself is preceeded by a header:
 
 \begin{lstlisting}
+        /* Note: LEGACY version was not little endian! */
 	struct virtio_net_hdr {
 	#define VIRTIO_NET_HDR_F_NEEDS_CSUM    1
 		u8 flags;
@@ -2848,6 +2854,7 @@ queue incoming packets into one of the multiple receiveq0..receiveqN
 depending on the packet flow.
 
 \begin{lstlisting}
+        /* Note: LEGACY version was not little endian! */
 	struct virtio_net_ctrl_mq {
 		le16 virtqueue_pairs;
 	};
@@ -2984,6 +2991,7 @@ present. The availability of the others all depend on various feature
 bits as indicated above.
 
 \begin{lstlisting}
+        /* Note: LEGACY version was not little endian! */
 	struct virtio_blk_config {
 		le64 capacity;
 		le32 size_max;
@@ -3241,6 +3249,7 @@ data and outgoing characters are placed in the transmit queue.
   acknowledging the feature.
 
 \begin{lstlisting}
+        /* Note: LEGACY version was not little endian! */
 	struct virtio_console_config {
 		le16 cols;
 		le16 rows;
@@ -3318,6 +3327,7 @@ when a port is closed or hot-unplugged.
   buffer and the events associated are:
 
 \begin{lstlisting}
+        /* Note: LEGACY version was not little endian! */
 	struct virtio_console_control {
 		le32 id;    /* Port number */
 		le16 event; /* The kind of control event */
@@ -3507,6 +3517,7 @@ as follows:
   compatibility, unsupported statistics should be omitted.
 
 \begin{lstlisting}
+        /* Note: LEGACY version was not little endian! */
 	struct virtio_balloon_stat {
 	#define VIRTIO_BALLOON_S_SWAP_IN  0
 	#define VIRTIO_BALLOON_S_SWAP_OUT 1
@@ -3598,6 +3609,7 @@ targets that receive and process the requests.
   and cdb_size are writable by the driver.
 
 \begin{lstlisting}
+        /* Note: LEGACY version was not little endian! */
 	struct virtio_scsi_config {
 		le32 num_queues;
 		le32 seg_max;
@@ -3682,6 +3694,7 @@ consumed with no order constraints.
 Requests have the following format:
 
 \begin{lstlisting}
+        /* Note: LEGACY version was not little endian! */
 	struct virtio_scsi_req_cmd {
 		// Read-only
 		u8 lun[8];
@@ -3842,6 +3855,7 @@ The following commands are defined:
 	#define VIRTIO_SCSI_T_TMF_QUERY_TASK           6
 	#define VIRTIO_SCSI_T_TMF_QUERY_TASK_SET       7
 
+        /* Note: LEGACY version was not little endian! */
 	struct virtio_scsi_ctrl_tmf
 	{
 		// Read-only part