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

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Jan 31, 2014 at 03:25:31PM +1030, Rusty Russell wrote:
> "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.

Sounds good, let's do that. And let's add a comment
where we define leXX.

> 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
> 
> 
>         
>