OASIS Virtual I/O Device (VIRTIO) TC

On Fri, Feb 16, 2018 at 09:24:12AM +0200, Michael S. Tsirkin wrote:
[...]
> +\subsection{Scatter-Gather Support}
> +\label{sec:Packed Virtqueues / Scatter-Gather Support}
> +
> +Some drivers need an ability to supply a list of multiple buffer
> +elements (also known as a scatter/gather list) with a request.
> +Two optional features support this: descriptor
> +chaining and indirect descriptors.
> +
> +If neither feature has been negotiated, each buffer is
> +physically-contigious, either read-only or write-only and is
> +described completely by a single descriptor.

Based on the updates in "Descriptor Chaining" section
and the below note in cover letter:

- dropped _F_DESC_LIST, 1.0 includes this unconditionally, we
can do same

Descriptor chaining is always available.

> +
[...]
> +\subsection{Event Suppression Structure Format}\label{sec:Basic
> +Facilities of a Virtio Device / Packed Virtqueues / Event Suppression Structure
> +Format}
> +
> +The following structure is used to reduce the number of
> +notifications sent between driver and device.
> +
> +\begin{lstlisting}
> +__le16 desc_event_off : 15; /* Descriptor Event Offset */
> +int desc_event_wrap : 1; /* Descriptor Event Wrap Counter */

Is this `int` a typo?

> +__le16 desc_event_flags : 2; /* Descriptor Event Flags */
> +\end{lstlisting}
[...]
> +\subsubsection{Implementation Example}\label{sec:Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Implementation Example}
> +
> +Below is an example driver code. It does not attempt to reduce
> +the number of device interrupts, neither does it support
> +the VIRTIO_F_RING_EVENT_IDX feature.
> +
> +\begin{lstlisting}
> +
> +first = vq->next_avail;
> +id = alloc_id(vq);
> +
> +for (each buffer element b) {
> + vq->desc[vq->next_avail].address = get_addr(b);
> + vq->desc[vq->next_avail].len = get_len(b);
> + init_desc(vq->next_avail, b);
> + avail = vq->avail_wrap_count;
> + used = !vq->avail_wrap_count;
> + f = get_flags(b) | (avail << VIRTQ_DESC_F_AVAIL) | (used << VIRTQ_DESC_F_USED);

In this version, above two flags are defined as:

#define VIRTQ_DESC_F_AVAIL (1 << 7)
#define VIRTQ_DESC_F_USED (1 << 15)

So we couldn't use them to shift the bit like this.

> + /* Don't mark the 1st descriptor available until all of them are ready. */
> + if (vq->next_avail == first) {
> + flags = f;
> + } else {
> + vq->desc[vq->next_avail].flags = f;
> + }
> +
> + vq->next_avail++;
> +
> + if (vq->next_avail >= vq->size) {
> + vq->next_avail = 0;
> + vq->avail_wrap_count \^= 1;
> + }

Maybe it's better to not mix using tab and space in
this example code.

> +
> +
> +}
> +/* ID included in the last descriptor in the list */
> +vq->desc[vq->next_avail].id = id;

Maybe it should be something like this:

vq->desc[prev].id = id;

Otherwise, the other fields (e.g. flags) of this desc (the
vq->desc[vq->next_avail]) also need to be updated.

> +write_memory_barrier();
> +vq->desc[first].flags = flags;
> +
> +memory_barrier();
> +
> +if (vq->device_event.flags != 0x2) {

Maybe it should be:

if (vq->device_event.flags != 0x1) {

As the flags definitions in this version are:

00b enable events
01b disable events
10b enable events for a specific descriptor
(as specified by Descriptor Ring Change Event Offset/Wrap Counter).
Only valid if VIRTIO_F_RING_EVENT_IDX has been negotiated.
11b reserved

> + notify_device(vq);
> +}
> +
> +\end{lstlisting}
> +
> +
> +\drivernormative{\paragraph}{Notifying The Device}{Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Notifying The Device}
> +The driver MUST perform a suitable memory barrier before reading
> +the Driver Event Suppression structure, to avoid missing a notification.
> +
> +\subsection{Receiving Used Buffers From The Device}\label{sec:Basic Facilities of a Virtio Device / Packed Virtqueues / Receiving Used Buffers From The Device}
> +
> +Once the device has used buffers referred to by a descriptor (read from or written to them, or
> +parts of both, depending on the nature of the virtqueue and the
> +device), it interrupts the driver
> +as detailed in section \ref{sec:Basic
> +Facilities of a Virtio Device / Packed Virtqueues / Event
> +Suppression Structure Format}.
> +
> +\begin{note}
> +For optimal performance, a driver MAY disable interrupts while processing
> +the used buffers, but beware the problem of missing interrupts between
> +emptying the ring and reenabling interrupts. This is usually handled by
> +re-checking for more used buffers after interrups are re-enabled:
> +\end{note}
> +
> +\begin{lstlisting}
> +vq->driver_event.flags = 0x2;

If my understanding is correct, this is to disable interrupt.
So maybe it should be:

vq->driver_event.flags = 0x1;

> +
> +for (;;) {
> + struct virtq_desc *d = vq->desc[vq->next_used];
> +
> + flags = d->flags;
> + bool avail = flags & (1 << VIRTQ_DESC_F_AVAIL);
> + bool used = flags & (1 << VIRTQ_DESC_F_USED);

We couldn't use them to shift the bit like this in this version.

> +
> + if (avail != used) {
> + vq->driver_event.flags = 0x1;

If my understanding is correct, this is to enable interrupt.
So maybe it should be:

vq->driver_event.flags = 0x0;

> + memory_barrier();
> +
> + flags = d->flags;
> + bool avail = flags & (1 << VIRTQ_DESC_F_AVAIL);
> + bool used = flags & (1 << VIRTQ_DESC_F_USED);

We couldn't use them to shift the bit like this in this version.

> + if (avail != used) {
> + break;
> + }
> +
> + vq->driver_event.flags = 0x2;

If my understanding is correct, this is to disable interrupt.
So maybe it should be:

vq->driver_event.flags = 0x1;

> + }
> +
> + read_memory_barrier();
> + process_buffer(d);
> + vq->next_used++;
> + if (vq->next_used >= vq->size) {
> + vq->next_used = 0;
> + }
> +}
> +\end{lstlisting}
> --
> MST
>

On Sat, Feb 24, 2018 at 01:17:08PM +0800, Tiwei Bie wrote:
> On Fri, Feb 16, 2018 at 09:24:12AM +0200, Michael S. Tsirkin wrote:
> [...]
> > +\subsection{Scatter-Gather Support}
> > +\label{sec:Packed Virtqueues / Scatter-Gather Support}
> > +
> > +Some drivers need an ability to supply a list of multiple buffer
> > +elements (also known as a scatter/gather list) with a request.
> > +Two optional features support this: descriptor
> > +chaining and indirect descriptors.
> > +
> > +If neither feature has been negotiated, each buffer is
> > +physically-contigious, either read-only or write-only and is
> > +described completely by a single descriptor.
>
> Based on the updates in "Descriptor Chaining" section
> and the below note in cover letter:
>
> - dropped _F_DESC_LIST, 1.0 includes this unconditionally, we
> can do same
>
> Descriptor chaining is always available.

That's right - I should fix this.

> > +
> [...]
> > +\subsection{Event Suppression Structure Format}\label{sec:Basic
> > +Facilities of a Virtio Device / Packed Virtqueues / Event Suppression Structure
> > +Format}
> > +
> > +The following structure is used to reduce the number of
> > +notifications sent between driver and device.
> > +
> > +\begin{lstlisting}
> > +__le16 desc_event_off : 15; /* Descriptor Event Offset */
> > +int desc_event_wrap : 1; /* Descriptor Event Wrap Counter */
>
> Is this `int` a typo?

It's a single bit so I think it does not matter.
What type would you like me to use instead?

> > +__le16 desc_event_flags : 2; /* Descriptor Event Flags */
> > +\end{lstlisting}
> [...]
> > +\subsubsection{Implementation Example}\label{sec:Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Implementation Example}
> > +
> > +Below is an example driver code. It does not attempt to reduce
> > +the number of device interrupts, neither does it support
> > +the VIRTIO_F_RING_EVENT_IDX feature.
> > +
> > +\begin{lstlisting}
> > +
> > +first = vq->next_avail;
> > +id = alloc_id(vq);
> > +
> > +for (each buffer element b) {
> > + vq->desc[vq->next_avail].address = get_addr(b);
> > + vq->desc[vq->next_avail].len = get_len(b);
> > + init_desc(vq->next_avail, b);
> > + avail = vq->avail_wrap_count;
> > + used = !vq->avail_wrap_count;
> > + f = get_flags(b) | (avail << VIRTQ_DESC_F_AVAIL) | (used << VIRTQ_DESC_F_USED);
>
> In this version, above two flags are defined as:
>
> #define VIRTQ_DESC_F_AVAIL (1 << 7)
> #define VIRTQ_DESC_F_USED (1 << 15)
>
> So we couldn't use them to shift the bit like this.

You are right. I guess we should change all flags to be bit numbers.

> > + /* Don't mark the 1st descriptor available until all of them are ready. */
> > + if (vq->next_avail == first) {
> > + flags = f;
> > + } else {
> > + vq->desc[vq->next_avail].flags = f;
> > + }
> > +
> > + vq->next_avail++;
> > +
> > + if (vq->next_avail >= vq->size) {
> > + vq->next_avail = 0;
> > + vq->avail_wrap_count \^= 1;
> > + }
>
> Maybe it's better to not mix using tab and space in
> this example code.

I'm not sure it matters for latex but I agree we should be
consistent.

> > +
> > +
> > +}
> > +/* ID included in the last descriptor in the list */
> > +vq->desc[vq->next_avail].id = id;
>
> Maybe it should be something like this:
>
> vq->desc[prev].id = id;
>
> Otherwise, the other fields (e.g. flags) of this desc (the
> vq->desc[vq->next_avail]) also need to be updated.
>
> > +write_memory_barrier();
> > +vq->desc[first].flags = flags;
> > +
> > +memory_barrier();
> > +
> > +if (vq->device_event.flags != 0x2) {
>
> Maybe it should be:
>
> if (vq->device_event.flags != 0x1) {
>
> As the flags definitions in this version are:
>
> 00b enable events
> 01b disable events
> 10b enable events for a specific descriptor
> (as specified by Descriptor Ring Change Event Offset/Wrap Counter).
> Only valid if VIRTIO_F_RING_EVENT_IDX has been negotiated.
> 11b reserved

I'll recheck it and reply to above two separately.


> > + notify_device(vq);
> > +}
> > +
> > +\end{lstlisting}
> > +
> > +
> > +\drivernormative{\paragraph}{Notifying The Device}{Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Notifying The Device}
> > +The driver MUST perform a suitable memory barrier before reading
> > +the Driver Event Suppression structure, to avoid missing a notification.
> > +
> > +\subsection{Receiving Used Buffers From The Device}\label{sec:Basic Facilities of a Virtio Device / Packed Virtqueues / Receiving Used Buffers From The Device}
> > +
> > +Once the device has used buffers referred to by a descriptor (read from or written to them, or
> > +parts of both, depending on the nature of the virtqueue and the
> > +device), it interrupts the driver
> > +as detailed in section \ref{sec:Basic
> > +Facilities of a Virtio Device / Packed Virtqueues / Event
> > +Suppression Structure Format}.
> > +
> > +\begin{note}
> > +For optimal performance, a driver MAY disable interrupts while processing
> > +the used buffers, but beware the problem of missing interrupts between
> > +emptying the ring and reenabling interrupts. This is usually handled by
> > +re-checking for more used buffers after interrups are re-enabled:
> > +\end{note}
> > +
> > +\begin{lstlisting}
> > +vq->driver_event.flags = 0x2;
>
> If my understanding is correct, this is to disable interrupt.
> So maybe it should be:
>
> vq->driver_event.flags = 0x1;

I'll check.

> > +
> > +for (;;) {
> > + struct virtq_desc *d = vq->desc[vq->next_used];
> > +
> > + flags = d->flags;
> > + bool avail = flags & (1 << VIRTQ_DESC_F_AVAIL);
> > + bool used = flags & (1 << VIRTQ_DESC_F_USED);
>
> We couldn't use them to shift the bit like this in this version.
>
> > +
> > + if (avail != used) {
> > + vq->driver_event.flags = 0x1;
>
> If my understanding is correct, this is to enable interrupt.
> So maybe it should be:
>
> vq->driver_event.flags = 0x0;
>
> > + memory_barrier();
> > +
> > + flags = d->flags;
> > + bool avail = flags & (1 << VIRTQ_DESC_F_AVAIL);
> > + bool used = flags & (1 << VIRTQ_DESC_F_USED);
>
> We couldn't use them to shift the bit like this in this version.
>
> > + if (avail != used) {
> > + break;
> > + }
> > +
> > + vq->driver_event.flags = 0x2;
>
> If my understanding is correct, this is to disable interrupt.
> So maybe it should be:
>
> vq->driver_event.flags = 0x1;


Thanks for all the comments, will address.

> > + }
> > +
> > + read_memory_barrier();
> > + process_buffer(d);
> > + vq->next_used++;
> > + if (vq->next_used >= vq->size) {
> > + vq->next_used = 0;
> > + }
> > +}
> > +\end{lstlisting}
> > --
> > MST
> >

On Sat, Feb 24, 2018 at 01:17:08PM +0800, Tiwei Bie wrote: > On Fri, Feb 16, 2018 at 09:24:12AM +0200, Michael S. Tsirkin wrote: > [...] > > +subsection{Scatter-Gather Support} > > +label{sec:Packed Virtqueues / Scatter-Gather Support} > > + > > +Some drivers need an ability to supply a list of multiple buffer > > +elements (also known as a scatter/gather list) with a request. > > +Two optional features support this: descriptor > > +chaining and indirect descriptors. > > + > > +If neither feature has been negotiated, each buffer is > > +physically-contigious, either read-only or write-only and is > > +described completely by a single descriptor. > > Based on the updates in "Descriptor Chaining" section > and the below note in cover letter: > > - dropped _F_DESC_LIST, 1.0 includes this unconditionally, we > can do same > > Descriptor chaining is always available. That's right - I should fix this. > > + > [...] > > +subsection{Event Suppression Structure Format}label{sec:Basic > > +Facilities of a Virtio Device / Packed Virtqueues / Event Suppression Structure > > +Format} > > + > > +The following structure is used to reduce the number of > > +notifications sent between driver and device. > > + > > +egin{lstlisting} > > +__le16 desc_event_off : 15; /* Descriptor Event Offset */ > > +int desc_event_wrap : 1; /* Descriptor Event Wrap Counter */ > > Is this `int` a typo? It's a single bit so I think it does not matter. What type would you like me to use instead? > > +__le16 desc_event_flags : 2; /* Descriptor Event Flags */ > > +end{lstlisting} > [...] > > +subsubsection{Implementation Example}label{sec:Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Implementation Example} > > + > > +Below is an example driver code. It does not attempt to reduce > > +the number of device interrupts, neither does it support > > +the VIRTIO_F_RING_EVENT_IDX feature. > > + > > +egin{lstlisting} > > + > > +first = vq->next_avail; > > +id = alloc_id(vq); > > + > > +for (each buffer element b) { > > + vq->desc[vq->next_avail].address = get_addr(b); > > + vq->desc[vq->next_avail].len = get_len(b); > > + init_desc(vq->next_avail, b); > > + avail = vq->avail_wrap_count; > > + used = !vq->avail_wrap_count; > > + f = get_flags(b) (avail << VIRTQ_DESC_F_AVAIL) (used << VIRTQ_DESC_F_USED); > > In this version, above two flags are defined as: > > #define VIRTQ_DESC_F_AVAIL (1 << 7) > #define VIRTQ_DESC_F_USED (1 << 15) > > So we couldn't use them to shift the bit like this. You are right. I guess we should change all flags to be bit numbers. > > + /* Don't mark the 1st descriptor available until all of them are ready. */ > > + if (vq->next_avail == first) { > > + flags = f; > > + } else { > > + vq->desc[vq->next_avail].flags = f; > > + } > > + > > + vq->next_avail++; > > + > > + if (vq->next_avail >= vq->size) { > > + vq->next_avail = 0; > > + vq->avail_wrap_count ^= 1; > > + } > > Maybe it's better to not mix using tab and space in > this example code. I'm not sure it matters for latex but I agree we should be consistent. > > + > > + > > +} > > +/* ID included in the last descriptor in the list */ > > +vq->desc[vq->next_avail].id = id; > > Maybe it should be something like this: > > vq->desc[prev].id = id; > > Otherwise, the other fields (e.g. flags) of this desc (the > vq->desc[vq->next_avail]) also need to be updated. > > > +write_memory_barrier(); > > +vq->desc[first].flags = flags; > > + > > +memory_barrier(); > > + > > +if (vq->device_event.flags != 0x2) { > > Maybe it should be: > > if (vq->device_event.flags != 0x1) { > > As the flags definitions in this version are: > > 00b enable events > 01b disable events > 10b enable events for a specific descriptor > (as specified by Descriptor Ring Change Event Offset/Wrap Counter). > Only valid if VIRTIO_F_RING_EVENT_IDX has been negotiated. > 11b reserved I'll recheck it and reply to above two separately. > > + notify_device(vq); > > +} > > + > > +end{lstlisting} > > + > > + > > +drivernormative{paragraph}{Notifying The Device}{Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Notifying The Device} > > +The driver MUST perform a suitable memory barrier before reading > > +the Driver Event Suppression structure, to avoid missing a notification. > > + > > +subsection{Receiving Used Buffers From The Device}label{sec:Basic Facilities of a Virtio Device / Packed Virtqueues / Receiving Used Buffers From The Device} > > + > > +Once the device has used buffers referred to by a descriptor (read from or written to them, or > > +parts of both, depending on the nature of the virtqueue and the > > +device), it interrupts the driver > > +as detailed in section
ef{sec:Basic > > +Facilities of a Virtio Device / Packed Virtqueues / Event > > +Suppression Structure Format}. > > + > > +egin{note} > > +For optimal performance, a driver MAY disable interrupts while processing > > +the used buffers, but beware the problem of missing interrupts between > > +emptying the ring and reenabling interrupts. This is usually handled by > > +re-checking for more used buffers after interrups are re-enabled: > > +end{note} > > + > > +egin{lstlisting} > > +vq->driver_event.flags = 0x2; > > If my understanding is correct, this is to disable interrupt. > So maybe it should be: > > vq->driver_event.flags = 0x1; I'll check. > > + > > +for (;;) { > > + struct virtq_desc *d = vq->desc[vq->next_used]; > > + > > + flags = d->flags; > > + bool avail = flags & (1 << VIRTQ_DESC_F_AVAIL); > > + bool used = flags & (1 << VIRTQ_DESC_F_USED); > > We couldn't use them to shift the bit like this in this version. > > > + > > + if (avail != used) { > > + vq->driver_event.flags = 0x1; > > If my understanding is correct, this is to enable interrupt. > So maybe it should be: > > vq->driver_event.flags = 0x0; > > > + memory_barrier(); > > + > > + flags = d->flags; > > + bool avail = flags & (1 << VIRTQ_DESC_F_AVAIL); > > + bool used = flags & (1 << VIRTQ_DESC_F_USED); > > We couldn't use them to shift the bit like this in this version. > > > + if (avail != used) { > > + break; > > + } > > + > > + vq->driver_event.flags = 0x2; > > If my understanding is correct, this is to disable interrupt. > So maybe it should be: > > vq->driver_event.flags = 0x1; Thanks for all the comments, will address. > > + } > > + > > + read_memory_barrier(); > > + process_buffer(d); > > + vq->next_used++; > > + if (vq->next_used >= vq->size) { > > + vq->next_used = 0; > > + } > > +} > > +end{lstlisting} > > -- > > MST > >

On Sun, Feb 25, 2018 at 08:49:10PM +0200, Michael S. Tsirkin wrote:
> On Sat, Feb 24, 2018 at 01:17:08PM +0800, Tiwei Bie wrote:
> > On Fri, Feb 16, 2018 at 09:24:12AM +0200, Michael S. Tsirkin wrote:
[...]
> > > +\subsection{Event Suppression Structure Format}\label{sec:Basic
> > > +Facilities of a Virtio Device / Packed Virtqueues / Event Suppression Structure
> > > +Format}
> > > +
> > > +The following structure is used to reduce the number of
> > > +notifications sent between driver and device.
> > > +
> > > +\begin{lstlisting}
> > > +__le16 desc_event_off : 15; /* Descriptor Event Offset */
> > > +int desc_event_wrap : 1; /* Descriptor Event Wrap Counter */
> >
> > Is this `int` a typo?
>
> It's a single bit so I think it does not matter.
> What type would you like me to use instead?

It looks a bit strange to use different types here, and
that's why I asked. If there is no particular reason to
use `int` here, maybe it's better to keep using __le16.

Besides, just for fun. For C language, I checked gcc and
clang. It seems that `int desc_event_wrap:1;` is a signed
type. So, e.g. `p->desc_event_wrap == 1` is always false.

Best regards,
Tiwei Bie

>
> > > +__le16 desc_event_flags : 2; /* Descriptor Event Flags */
> > > +\end{lstlisting}
[...]

On Mon, Feb 26, 2018 at 06:51:11PM +0800, Tiwei Bie wrote:
> On Sun, Feb 25, 2018 at 08:49:10PM +0200, Michael S. Tsirkin wrote:
> > On Sat, Feb 24, 2018 at 01:17:08PM +0800, Tiwei Bie wrote:
> > > On Fri, Feb 16, 2018 at 09:24:12AM +0200, Michael S. Tsirkin wrote:
> [...]
> > > > +\subsection{Event Suppression Structure Format}\label{sec:Basic
> > > > +Facilities of a Virtio Device / Packed Virtqueues / Event Suppression Structure
> > > > +Format}
> > > > +
> > > > +The following structure is used to reduce the number of
> > > > +notifications sent between driver and device.
> > > > +
> > > > +\begin{lstlisting}
> > > > +__le16 desc_event_off : 15; /* Descriptor Event Offset */
> > > > +int desc_event_wrap : 1; /* Descriptor Event Wrap Counter */
> > >
> > > Is this `int` a typo?
> >
> > It's a single bit so I think it does not matter.
> > What type would you like me to use instead?
>
> It looks a bit strange to use different types here, and
> that's why I asked. If there is no particular reason to
> use `int` here, maybe it's better to keep using __le16.
>
> Besides, just for fun. For C language, I checked gcc and
> clang. It seems that `int desc_event_wrap:1;` is a signed
> type. So, e.g. `p->desc_event_wrap == 1` is always false.
>
> Best regards,
> Tiwei Bie

I'll switch to u8 here, IMHO le16 for a single bit
is really confusing. There's no byte order for a single byte.

> >
> > > > +__le16 desc_event_flags : 2; /* Descriptor Event Flags */
> > > > +\end{lstlisting}
> [...]

On Mon, Feb 26, 2018 at 06:51:11PM +0800, Tiwei Bie wrote: > On Sun, Feb 25, 2018 at 08:49:10PM +0200, Michael S. Tsirkin wrote: > > On Sat, Feb 24, 2018 at 01:17:08PM +0800, Tiwei Bie wrote: > > > On Fri, Feb 16, 2018 at 09:24:12AM +0200, Michael S. Tsirkin wrote: > [...] > > > > +subsection{Event Suppression Structure Format}label{sec:Basic > > > > +Facilities of a Virtio Device / Packed Virtqueues / Event Suppression Structure > > > > +Format} > > > > + > > > > +The following structure is used to reduce the number of > > > > +notifications sent between driver and device. > > > > + > > > > +egin{lstlisting} > > > > +__le16 desc_event_off : 15; /* Descriptor Event Offset */ > > > > +int desc_event_wrap : 1; /* Descriptor Event Wrap Counter */ > > > > > > Is this `int` a typo? > > > > It's a single bit so I think it does not matter. > > What type would you like me to use instead? > > It looks a bit strange to use different types here, and > that's why I asked. If there is no particular reason to > use `int` here, maybe it's better to keep using __le16. > > Besides, just for fun. For C language, I checked gcc and > clang. It seems that `int desc_event_wrap:1;` is a signed > type. So, e.g. `p->desc_event_wrap == 1` is always false. > > Best regards, > Tiwei Bie I'll switch to u8 here, IMHO le16 for a single bit is really confusing. There's no byte order for a single byte. > > > > > > +__le16 desc_event_flags : 2; /* Descriptor Event Flags */ > > > > +end{lstlisting} > [...]

On Mon, Feb 26, 2018 at 10:38:13PM +0200, Michael S. Tsirkin wrote:
> On Mon, Feb 26, 2018 at 06:51:11PM +0800, Tiwei Bie wrote:
> > On Sun, Feb 25, 2018 at 08:49:10PM +0200, Michael S. Tsirkin wrote:
> > > On Sat, Feb 24, 2018 at 01:17:08PM +0800, Tiwei Bie wrote:
> > > > On Fri, Feb 16, 2018 at 09:24:12AM +0200, Michael S. Tsirkin wrote:
> > [...]
> > > > > +\subsection{Event Suppression Structure Format}\label{sec:Basic
> > > > > +Facilities of a Virtio Device / Packed Virtqueues / Event Suppression Structure
> > > > > +Format}
> > > > > +
> > > > > +The following structure is used to reduce the number of
> > > > > +notifications sent between driver and device.
> > > > > +
> > > > > +\begin{lstlisting}
> > > > > +__le16 desc_event_off : 15; /* Descriptor Event Offset */
> > > > > +int desc_event_wrap : 1; /* Descriptor Event Wrap Counter */
> > > >
> > > > Is this `int` a typo?
> > >
> > > It's a single bit so I think it does not matter.
> > > What type would you like me to use instead?
> >
> > It looks a bit strange to use different types here, and
> > that's why I asked. If there is no particular reason to
> > use `int` here, maybe it's better to keep using __le16.
> >
> > Besides, just for fun. For C language, I checked gcc and
> > clang. It seems that `int desc_event_wrap:1;` is a signed
> > type. So, e.g. `p->desc_event_wrap == 1` is always false.
> >
> > Best regards,
> > Tiwei Bie
>
> I'll switch to u8 here, IMHO le16 for a single bit
> is really confusing. There's no byte order for a single byte.

Sorry, I just realized that I misunderstood your point
previously.. Just to double check, `desc_event_off` and
`desc_event_wrap` are not in the same 2 bytes?

Previously I thought both of them are in the first 2
bytes. As it said it's a structure, and the fields are
defined in a way very similar to the bit field in C.

In C,

struct {
__le16 desc_event_off : 15;
int desc_event_wrap : 1;
__le16 desc_event_flags : 2;
};

struct {
__le16 desc_event_off : 15;
__le16 desc_event_wrap : 1;
__le16 desc_event_flags : 2;
};

struct {
__le16 desc_event_off : 15,
desc_event_wrap : 1;
__le16 desc_event_flags : 2;
};

All above means `desc_event_off` and `desc_event_wrap`
are in the first 2 bytes. So I thought the `int` is a
typo. And I thought they are in the first 2 bytes (which
is little-endian).

Best regards,
Tiwei Bie

>
> > >
> > > > > +__le16 desc_event_flags : 2; /* Descriptor Event Flags */
> > > > > +\end{lstlisting}
> > [...]

On Tue, Feb 27, 2018 at 09:49:28AM +0800, Tiwei Bie wrote:
> On Mon, Feb 26, 2018 at 10:38:13PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Feb 26, 2018 at 06:51:11PM +0800, Tiwei Bie wrote:
> > > On Sun, Feb 25, 2018 at 08:49:10PM +0200, Michael S. Tsirkin wrote:
> > > > On Sat, Feb 24, 2018 at 01:17:08PM +0800, Tiwei Bie wrote:
> > > > > On Fri, Feb 16, 2018 at 09:24:12AM +0200, Michael S. Tsirkin wrote:
> > > [...]
> > > > > > +\subsection{Event Suppression Structure Format}\label{sec:Basic
> > > > > > +Facilities of a Virtio Device / Packed Virtqueues / Event Suppression Structure
> > > > > > +Format}
> > > > > > +
> > > > > > +The following structure is used to reduce the number of
> > > > > > +notifications sent between driver and device.
> > > > > > +
> > > > > > +\begin{lstlisting}
> > > > > > +__le16 desc_event_off : 15; /* Descriptor Event Offset */
> > > > > > +int desc_event_wrap : 1; /* Descriptor Event Wrap Counter */
> > > > >
> > > > > Is this `int` a typo?
> > > >
> > > > It's a single bit so I think it does not matter.
> > > > What type would you like me to use instead?
> > >
> > > It looks a bit strange to use different types here, and
> > > that's why I asked. If there is no particular reason to
> > > use `int` here, maybe it's better to keep using __le16.
> > >
> > > Besides, just for fun. For C language, I checked gcc and
> > > clang. It seems that `int desc_event_wrap:1;` is a signed
> > > type. So, e.g. `p->desc_event_wrap == 1` is always false.
> > >
> > > Best regards,
> > > Tiwei Bie
> >
> > I'll switch to u8 here, IMHO le16 for a single bit
> > is really confusing. There's no byte order for a single byte.
>
> Sorry, I just realized that I misunderstood your point
> previously.. Just to double check, `desc_event_off` and
> `desc_event_wrap` are not in the same 2 bytes?
>
> Previously I thought both of them are in the first 2
> bytes. As it said it's a structure, and the fields are
> defined in a way very similar to the bit field in C.
>
> In C,
>
> struct {
> __le16 desc_event_off : 15;
> int desc_event_wrap : 1;
> __le16 desc_event_flags : 2;
> };
>
> struct {
> __le16 desc_event_off : 15;
> __le16 desc_event_wrap : 1;
> __le16 desc_event_flags : 2;
> };
>
> struct {
> __le16 desc_event_off : 15,
> desc_event_wrap : 1;
> __le16 desc_event_flags : 2;
> };
>
> All above means `desc_event_off` and `desc_event_wrap`
> are in the first 2 bytes. So I thought the `int` is a
> typo. And I thought they are in the first 2 bytes (which
> is little-endian).
>
> Best regards,
> Tiwei Bie

Yes but desc_event_wrap itself is completely within the
second byte. So the question of byte-order does not arise.

Right, and above is also identical to:

struct {
__le16 desc_event_off : 15,
u8 desc_event_wrap : 1;
__le16 desc_event_flags : 2;
};

introduction explains:

\item[u8, u16, u32, u64] An unsigned integer of the specified length in bits.

\item[le16, le32, le64] An unsigned integer of the specified length in bits,
in little-endian byte order.



> >
> > > >
> > > > > > +__le16 desc_event_flags : 2; /* Descriptor Event Flags */
> > > > > > +\end{lstlisting}
> > > [...]

On Tue, Feb 27, 2018 at 09:49:28AM +0800, Tiwei Bie wrote: > On Mon, Feb 26, 2018 at 10:38:13PM +0200, Michael S. Tsirkin wrote: > > On Mon, Feb 26, 2018 at 06:51:11PM +0800, Tiwei Bie wrote: > > > On Sun, Feb 25, 2018 at 08:49:10PM +0200, Michael S. Tsirkin wrote: > > > > On Sat, Feb 24, 2018 at 01:17:08PM +0800, Tiwei Bie wrote: > > > > > On Fri, Feb 16, 2018 at 09:24:12AM +0200, Michael S. Tsirkin wrote: > > > [...] > > > > > > +subsection{Event Suppression Structure Format}label{sec:Basic > > > > > > +Facilities of a Virtio Device / Packed Virtqueues / Event Suppression Structure > > > > > > +Format} > > > > > > + > > > > > > +The following structure is used to reduce the number of > > > > > > +notifications sent between driver and device. > > > > > > + > > > > > > +egin{lstlisting} > > > > > > +__le16 desc_event_off : 15; /* Descriptor Event Offset */ > > > > > > +int desc_event_wrap : 1; /* Descriptor Event Wrap Counter */ > > > > > > > > > > Is this `int` a typo? > > > > > > > > It's a single bit so I think it does not matter. > > > > What type would you like me to use instead? > > > > > > It looks a bit strange to use different types here, and > > > that's why I asked. If there is no particular reason to > > > use `int` here, maybe it's better to keep using __le16. > > > > > > Besides, just for fun. For C language, I checked gcc and > > > clang. It seems that `int desc_event_wrap:1;` is a signed > > > type. So, e.g. `p->desc_event_wrap == 1` is always false. > > > > > > Best regards, > > > Tiwei Bie > > > > I'll switch to u8 here, IMHO le16 for a single bit > > is really confusing. There's no byte order for a single byte. > > Sorry, I just realized that I misunderstood your point > previously.. Just to double check, `desc_event_off` and > `desc_event_wrap` are not in the same 2 bytes? > > Previously I thought both of them are in the first 2 > bytes. As it said it's a structure, and the fields are > defined in a way very similar to the bit field in C. > > In C, > > struct { > __le16 desc_event_off : 15; > int desc_event_wrap : 1; > __le16 desc_event_flags : 2; > }; > > struct { > __le16 desc_event_off : 15; > __le16 desc_event_wrap : 1; > __le16 desc_event_flags : 2; > }; > > struct { > __le16 desc_event_off : 15, > desc_event_wrap : 1; > __le16 desc_event_flags : 2; > }; > > All above means `desc_event_off` and `desc_event_wrap` > are in the first 2 bytes. So I thought the `int` is a > typo. And I thought they are in the first 2 bytes (which > is little-endian). > > Best regards, > Tiwei Bie Yes but desc_event_wrap itself is completely within the second byte. So the question of byte-order does not arise. Right, and above is also identical to: struct { __le16 desc_event_off : 15, u8 desc_event_wrap : 1; __le16 desc_event_flags : 2; }; introduction explains: item[u8, u16, u32, u64] An unsigned integer of the specified length in bits. item[le16, le32, le64] An unsigned integer of the specified length in bits, in little-endian byte order. > > > > > > > > > > > > +__le16 desc_event_flags : 2; /* Descriptor Event Flags */ > > > > > > +end{lstlisting} > > > [...]

On Tue, Feb 27, 2018 at 10:17:17PM +0200, Michael S. Tsirkin wrote:
> On Tue, Feb 27, 2018 at 09:49:28AM +0800, Tiwei Bie wrote:
> > On Mon, Feb 26, 2018 at 10:38:13PM +0200, Michael S. Tsirkin wrote:
> > > On Mon, Feb 26, 2018 at 06:51:11PM +0800, Tiwei Bie wrote:
> > > > On Sun, Feb 25, 2018 at 08:49:10PM +0200, Michael S. Tsirkin wrote:
> > > > > On Sat, Feb 24, 2018 at 01:17:08PM +0800, Tiwei Bie wrote:
> > > > > > On Fri, Feb 16, 2018 at 09:24:12AM +0200, Michael S. Tsirkin wrote:
> > > > [...]
> > > > > > > +\subsection{Event Suppression Structure Format}\label{sec:Basic
> > > > > > > +Facilities of a Virtio Device / Packed Virtqueues / Event Suppression Structure
> > > > > > > +Format}
> > > > > > > +
> > > > > > > +The following structure is used to reduce the number of
> > > > > > > +notifications sent between driver and device.
> > > > > > > +
> > > > > > > +\begin{lstlisting}
> > > > > > > +__le16 desc_event_off : 15; /* Descriptor Event Offset */
> > > > > > > +int desc_event_wrap : 1; /* Descriptor Event Wrap Counter */
> > > > > >
> > > > > > Is this `int` a typo?
> > > > >
> > > > > It's a single bit so I think it does not matter.
> > > > > What type would you like me to use instead?
> > > >
> > > > It looks a bit strange to use different types here, and
> > > > that's why I asked. If there is no particular reason to
> > > > use `int` here, maybe it's better to keep using __le16.
> > > >
> > > > Besides, just for fun. For C language, I checked gcc and
> > > > clang. It seems that `int desc_event_wrap:1;` is a signed
> > > > type. So, e.g. `p->desc_event_wrap == 1` is always false.
> > > >
> > > > Best regards,
> > > > Tiwei Bie
> > >
> > > I'll switch to u8 here, IMHO le16 for a single bit
> > > is really confusing. There's no byte order for a single byte.
> >
> > Sorry, I just realized that I misunderstood your point
> > previously.. Just to double check, `desc_event_off` and
> > `desc_event_wrap` are not in the same 2 bytes?
> >
> > Previously I thought both of them are in the first 2
> > bytes. As it said it's a structure, and the fields are
> > defined in a way very similar to the bit field in C.
> >
> > In C,
> >
> > struct {
> > __le16 desc_event_off : 15;
> > int desc_event_wrap : 1;
> > __le16 desc_event_flags : 2;
> > };
> >
> > struct {
> > __le16 desc_event_off : 15;
> > __le16 desc_event_wrap : 1;
> > __le16 desc_event_flags : 2;
> > };
> >
> > struct {
> > __le16 desc_event_off : 15,
> > desc_event_wrap : 1;
> > __le16 desc_event_flags : 2;
> > };
> >
> > All above means `desc_event_off` and `desc_event_wrap`
> > are in the first 2 bytes. So I thought the `int` is a
> > typo. And I thought they are in the first 2 bytes (which
> > is little-endian).
> >
> > Best regards,
> > Tiwei Bie
>
> Yes but desc_event_wrap itself is completely within the
> second byte. So the question of byte-order does not arise.
>
> Right, and above is also identical to:
>
> struct {
> __le16 desc_event_off : 15,
> u8 desc_event_wrap : 1;
> __le16 desc_event_flags : 2;
> };
>
> introduction explains:
>
> \item[u8, u16, u32, u64] An unsigned integer of the specified length in bits.
>
> \item[le16, le32, le64] An unsigned integer of the specified length in bits,
> in little-endian byte order.
>

I've got your point now. Thanks! ;-)
BTW, maybe it's better to remove the `__` prefix for
`__le16` to keep consistency.

Best regards,
Tiwei Bie

>
>
> > >
> > > > >
> > > > > > > +__le16 desc_event_flags : 2; /* Descriptor Event Flags */
> > > > > > > +\end{lstlisting}
> > > > [...]

On Wed, Feb 28, 2018 at 05:19:15PM +0800, Tiwei Bie wrote:
> On Tue, Feb 27, 2018 at 10:17:17PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Feb 27, 2018 at 09:49:28AM +0800, Tiwei Bie wrote:
> > > On Mon, Feb 26, 2018 at 10:38:13PM +0200, Michael S. Tsirkin wrote:
> > > > On Mon, Feb 26, 2018 at 06:51:11PM +0800, Tiwei Bie wrote:
> > > > > On Sun, Feb 25, 2018 at 08:49:10PM +0200, Michael S. Tsirkin wrote:
> > > > > > On Sat, Feb 24, 2018 at 01:17:08PM +0800, Tiwei Bie wrote:
> > > > > > > On Fri, Feb 16, 2018 at 09:24:12AM +0200, Michael S. Tsirkin wrote:
> > > > > [...]
> > > > > > > > +\subsection{Event Suppression Structure Format}\label{sec:Basic
> > > > > > > > +Facilities of a Virtio Device / Packed Virtqueues / Event Suppression Structure
> > > > > > > > +Format}
> > > > > > > > +
> > > > > > > > +The following structure is used to reduce the number of
> > > > > > > > +notifications sent between driver and device.
> > > > > > > > +
> > > > > > > > +\begin{lstlisting}
> > > > > > > > +__le16 desc_event_off : 15; /* Descriptor Event Offset */
> > > > > > > > +int desc_event_wrap : 1; /* Descriptor Event Wrap Counter */
> > > > > > >
> > > > > > > Is this `int` a typo?
> > > > > >
> > > > > > It's a single bit so I think it does not matter.
> > > > > > What type would you like me to use instead?
> > > > >
> > > > > It looks a bit strange to use different types here, and
> > > > > that's why I asked. If there is no particular reason to
> > > > > use `int` here, maybe it's better to keep using __le16.
> > > > >
> > > > > Besides, just for fun. For C language, I checked gcc and
> > > > > clang. It seems that `int desc_event_wrap:1;` is a signed
> > > > > type. So, e.g. `p->desc_event_wrap == 1` is always false.
> > > > >
> > > > > Best regards,
> > > > > Tiwei Bie
> > > >
> > > > I'll switch to u8 here, IMHO le16 for a single bit
> > > > is really confusing. There's no byte order for a single byte.
> > >
> > > Sorry, I just realized that I misunderstood your point
> > > previously.. Just to double check, `desc_event_off` and
> > > `desc_event_wrap` are not in the same 2 bytes?
> > >
> > > Previously I thought both of them are in the first 2
> > > bytes. As it said it's a structure, and the fields are
> > > defined in a way very similar to the bit field in C.
> > >
> > > In C,
> > >
> > > struct {
> > > __le16 desc_event_off : 15;
> > > int desc_event_wrap : 1;
> > > __le16 desc_event_flags : 2;
> > > };
> > >
> > > struct {
> > > __le16 desc_event_off : 15;
> > > __le16 desc_event_wrap : 1;
> > > __le16 desc_event_flags : 2;
> > > };
> > >
> > > struct {
> > > __le16 desc_event_off : 15,
> > > desc_event_wrap : 1;
> > > __le16 desc_event_flags : 2;
> > > };
> > >
> > > All above means `desc_event_off` and `desc_event_wrap`
> > > are in the first 2 bytes. So I thought the `int` is a
> > > typo. And I thought they are in the first 2 bytes (which
> > > is little-endian).
> > >
> > > Best regards,
> > > Tiwei Bie
> >
> > Yes but desc_event_wrap itself is completely within the
> > second byte. So the question of byte-order does not arise.
> >
> > Right, and above is also identical to:
> >
> > struct {
> > __le16 desc_event_off : 15,
> > u8 desc_event_wrap : 1;
> > __le16 desc_event_flags : 2;
> > };
> >
> > introduction explains:
> >
> > \item[u8, u16, u32, u64] An unsigned integer of the specified length in bits.
> >
> > \item[le16, le32, le64] An unsigned integer of the specified length in bits,
> > in little-endian byte order.
> >
>
> I've got your point now. Thanks! ;-)
> BTW, maybe it's better to remove the `__` prefix for
> `__le16` to keep consistency.
>
> Best regards,
> Tiwei Bie

I agree. I think I will stop using the bitfields - they seem to
cause too much confusion. Just

struct event {
__le16 event_desc; /* bits 0-14: desc_off. 15 - desc_wrap. */
__le16 event_flags; /* bits 0-1: event_flags, 2-15 - reserved */
};

> >
> >
> > > >
> > > > > >
> > > > > > > > +__le16 desc_event_flags : 2; /* Descriptor Event Flags */
> > > > > > > > +\end{lstlisting}
> > > > > [...]

On Wed, Feb 28, 2018 at 05:19:15PM +0800, Tiwei Bie wrote: > On Tue, Feb 27, 2018 at 10:17:17PM +0200, Michael S. Tsirkin wrote: > > On Tue, Feb 27, 2018 at 09:49:28AM +0800, Tiwei Bie wrote: > > > On Mon, Feb 26, 2018 at 10:38:13PM +0200, Michael S. Tsirkin wrote: > > > > On Mon, Feb 26, 2018 at 06:51:11PM +0800, Tiwei Bie wrote: > > > > > On Sun, Feb 25, 2018 at 08:49:10PM +0200, Michael S. Tsirkin wrote: > > > > > > On Sat, Feb 24, 2018 at 01:17:08PM +0800, Tiwei Bie wrote: > > > > > > > On Fri, Feb 16, 2018 at 09:24:12AM +0200, Michael S. Tsirkin wrote: > > > > > [...] > > > > > > > > +subsection{Event Suppression Structure Format}label{sec:Basic > > > > > > > > +Facilities of a Virtio Device / Packed Virtqueues / Event Suppression Structure > > > > > > > > +Format} > > > > > > > > + > > > > > > > > +The following structure is used to reduce the number of > > > > > > > > +notifications sent between driver and device. > > > > > > > > + > > > > > > > > +egin{lstlisting} > > > > > > > > +__le16 desc_event_off : 15; /* Descriptor Event Offset */ > > > > > > > > +int desc_event_wrap : 1; /* Descriptor Event Wrap Counter */ > > > > > > > > > > > > > > Is this `int` a typo? > > > > > > > > > > > > It's a single bit so I think it does not matter. > > > > > > What type would you like me to use instead? > > > > > > > > > > It looks a bit strange to use different types here, and > > > > > that's why I asked. If there is no particular reason to > > > > > use `int` here, maybe it's better to keep using __le16. > > > > > > > > > > Besides, just for fun. For C language, I checked gcc and > > > > > clang. It seems that `int desc_event_wrap:1;` is a signed > > > > > type. So, e.g. `p->desc_event_wrap == 1` is always false. > > > > > > > > > > Best regards, > > > > > Tiwei Bie > > > > > > > > I'll switch to u8 here, IMHO le16 for a single bit > > > > is really confusing. There's no byte order for a single byte. > > > > > > Sorry, I just realized that I misunderstood your point > > > previously.. Just to double check, `desc_event_off` and > > > `desc_event_wrap` are not in the same 2 bytes? > > > > > > Previously I thought both of them are in the first 2 > > > bytes. As it said it's a structure, and the fields are > > > defined in a way very similar to the bit field in C. > > > > > > In C, > > > > > > struct { > > > __le16 desc_event_off : 15; > > > int desc_event_wrap : 1; > > > __le16 desc_event_flags : 2; > > > }; > > > > > > struct { > > > __le16 desc_event_off : 15; > > > __le16 desc_event_wrap : 1; > > > __le16 desc_event_flags : 2; > > > }; > > > > > > struct { > > > __le16 desc_event_off : 15, > > > desc_event_wrap : 1; > > > __le16 desc_event_flags : 2; > > > }; > > > > > > All above means `desc_event_off` and `desc_event_wrap` > > > are in the first 2 bytes. So I thought the `int` is a > > > typo. And I thought they are in the first 2 bytes (which > > > is little-endian). > > > > > > Best regards, > > > Tiwei Bie > > > > Yes but desc_event_wrap itself is completely within the > > second byte. So the question of byte-order does not arise. > > > > Right, and above is also identical to: > > > > struct { > > __le16 desc_event_off : 15, > > u8 desc_event_wrap : 1; > > __le16 desc_event_flags : 2; > > }; > > > > introduction explains: > > > > item[u8, u16, u32, u64] An unsigned integer of the specified length in bits. > > > > item[le16, le32, le64] An unsigned integer of the specified length in bits, > > in little-endian byte order. > > > > I've got your point now. Thanks! ;-) > BTW, maybe it's better to remove the `__` prefix for > `__le16` to keep consistency. > > Best regards, > Tiwei Bie I agree. I think I will stop using the bitfields - they seem to cause too much confusion. Just struct event { __le16 event_desc; /* bits 0-14: desc_off. 15 - desc_wrap. */ __le16 event_flags; /* bits 0-1: event_flags, 2-15 - reserved */ }; > > > > > > > > > > > > > > > > > > > > > > +__le16 desc_event_flags : 2; /* Descriptor Event Flags */ > > > > > > > > +end{lstlisting} > > > > > [...]

On Wed, 28 Feb 2018 17:20:29 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> I agree. I think I will stop using the bitfields - they seem to
> cause too much confusion. Just
>
> struct event {
> __le16 event_desc; /* bits 0-14: desc_off. 15 - desc_wrap. */
> __le16 event_flags; /* bits 0-1: event_flags, 2-15 - reserved */
> };

Looks sensible; but while you're add it, drop the leading underscores
as well? The spec only talks about 'le16' and friends.

[Also, reserved-and-to-be-ignored or reserved-and-must-be-0?]

On Wed, 28 Feb 2018 17:20:29 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > I agree. I think I will stop using the bitfields - they seem to > cause too much confusion. Just > > struct event { > __le16 event_desc; /* bits 0-14: desc_off. 15 - desc_wrap. */ > __le16 event_flags; /* bits 0-1: event_flags, 2-15 - reserved */ > }; Looks sensible; but while you're add it, drop the leading underscores as well? The spec only talks about 'le16' and friends. [Also, reserved-and-to-be-ignored or reserved-and-must-be-0?]

On Tue, Feb 27, 2018 at 09:49:28AM +0800, Tiwei Bie wrote:
> struct {
> __le16 desc_event_off : 15,
> desc_event_wrap : 1;
> __le16 desc_event_flags : 2;
> };

I decided on this format in the end. Thanks for the suggestion!

--
MST

On Tue, Feb 27, 2018 at 09:49:28AM +0800, Tiwei Bie wrote: > struct { > __le16 desc_event_off : 15, > desc_event_wrap : 1; > __le16 desc_event_flags : 2; > }; I decided on this format in the end. Thanks for the suggestion! -- MST