OASIS Virtual I/O Device (VIRTIO) TC

Some of these are more and some are less important. Please be careful, there is always a chance that I misunderstood something and that I'm trying to change for the worse. I've identified some other weak point's too, but tried to proceed in order the more grave and the better I think I understand it (and thus propose a fix) the higher the priority. Some stuff I've left for my notifiers patch set. This one and that one will have to get merged if both turn out to be good. Halil Pasic (5): packed-ring: fix used descriptors but meant buffers consistency: clean up stale wording for v1.1 terms packed-packed: fix supplying buffers description packed-ring: reposition drivernormative on driver notifications packed-ring: tweak driver code example content.tex 14 ++++---- packed-ring.tex 96 +++++++++++++++++++++++++++++------------------------- 2 files changed, 59 insertions(+), 51 deletions(-)

Occasionally we still use used and available ring instead of Device and Driver Area respectively. Same goes for descriptor table with the difference that depending on the context we need either Descriptor Area or descriptor ring. Let's clean these up, at least where the cleanup is trivial. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- content.tex 14 +++++++------- packed-ring.tex 10 ++++++---- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/content.tex b/content.tex index 7a92cb1..e3ac662 100644 --- a/content.tex +++ b/content.tex @@ -1247,7 +1247,7 @@ The driver typically does this as follows, for each virtqueue a device has: item Optionally, select a smaller virtqueue size and write it to field{queue_size}. -item Allocate and zero Descriptor Table, Available and Used rings for the +item Allocate and zero Descriptor Area, Device Area and Driver Area for the virtqueue in contiguous physical memory. item Optionally, if MSI-X capability is present and enabled on the @@ -1769,9 +1769,9 @@ nor behaviour: writing to field{QueueSel}. } hline - mmioreg{QueueAlign}{Used Ring alignment in the virtual queue}{0x03c}{W}{% + mmioreg{QueueAlign}{Device Area alignment in the virtual queue}{0x03c}{W}{% Writing to this register notifies the device about alignment - boundary of the Used Ring in bytes. This value should be a power + boundary of the Device Area in bytes. This value should be a power of 2 and applies to the queue selected by writing to field{QueueSel}. } hline @@ -1779,7 +1779,7 @@ nor behaviour: Writing to this register notifies the device about location of the virtual queue in the Guest's physical address space. This value is the index number of a page starting with the queue - Descriptor Table. Value zero (0x0) means physical address zero + Descriptor Area. Value zero (0x0) means physical address zero (0x00000000) and is illegal. When the driver stops using the queue it writes zero (0x0) to this register. Reading from this register returns the currently used page @@ -1830,7 +1830,7 @@ The virtual queue is configured as follows: queue is not available. item Allocate and zero the queue pages in contiguous virtual - memory, aligning the Used Ring to an optimal boundary (usually + memory, aligning the Device Area to an optimal boundary (usually page size). The driver should choose a queue size smaller than or equal to field{QueueNumMax}. @@ -2066,8 +2066,8 @@ struct vq_info_block { end{lstlisting} field{desc}, field{driver} and field{device} contain the guest -addresses for the descriptor area, -available area and used area for queue field{index}, respectively. The actual +addresses for the Descriptor Area, +Driver Area and Device Area for queue field{index}, respectively. The actual virtqueue size (number of allocated buffers) is transmitted in field{num}. devicenormative{paragraph}{Configuring a Virtqueue}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Configuring a Virtqueue} diff --git a/packed-ring.tex b/packed-ring.tex index eb006b0..e656fed 100644 --- a/packed-ring.tex +++ b/packed-ring.tex @@ -445,7 +445,9 @@ struct pvirtq_event_suppress { }; end{lstlisting} -devicenormative{subsection}{The Virtqueue Descriptor Table}{Basic Facilities of a Virtio Device / Packed Virtqueues / The Virtqueue Descriptor Table} +devicenormative{subsection}{The Virtqueue Descriptor Ring}{Basic +Facilities of a Virtio Device / Packed Virtqueues / The Virtqueue +Descriptor Ring} A device MUST NOT write to a device-readable buffer, and a device SHOULD NOT read a device-writable buffer. A device MUST NOT use a descriptor unless it observes @@ -454,7 +456,7 @@ the VIRTQ_DESC_F_AVAIL bit in its field{flags} being changed A device MUST NOT change a descriptor after changing it's the VIRTQ_DESC_F_USED bit in its field{flags}. -drivernormative{subsection}{The Virtqueue Descriptor Table}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Table} +drivernormative{subsection}{The Virtqueue Descriptor Ring}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Ring} A driver MUST NOT change a descriptor unless it observes the VIRTQ_DESC_F_USED bit in its field{flags} being changed. A driver MUST NOT change a descriptor after changing @@ -498,7 +500,7 @@ were made available by the driver. The device MAY limit the number of buffers it will allow in a list. -drivernormative{subsection}{Indirect Descriptors}{Basic Facilities of a Virtio Device / Packed Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors} +drivernormative{subsection}{Indirect Descriptors}{Basic Facilities of a Virtio Device / Packed Virtqueues / The Virtqueue Descriptor Ring / Indirect Descriptors} The driver MUST NOT set the DESC_F_INDIRECT flag unless the VIRTIO_F_INDIRECT_DESC feature was negotiated. The driver MUST NOT set any flags except DESC_F_WRITE within an indirect descriptor. @@ -541,7 +543,7 @@ What follows are the requirements of each stage in more detail. For each buffer element, b: egin{enumerate} -item Get the next descriptor table entry, d +item Get the next descriptor ring entry, d item Get the next free buffer id value item Set field{d.addr} to the physical address of the start of b item Set field{d.len} to the length of b. -- 1.7.1

On Mon, 9 Apr 2018 20:58:39 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> Occasionally we still use used and available ring instead of Device and
> Driver Area respectively. Same goes for descriptor table with the
> difference that depending on the context we need either Descriptor Area
> or descriptor ring.
>
> Let's clean these up, at least where the cleanup is trivial.
>
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
> content.tex | 14 +++++++-------
> packed-ring.tex | 10 ++++++----
> 2 files changed, 13 insertions(+), 11 deletions(-)
>

> -\drivernormative{\subsection}{The Virtqueue Descriptor Table}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Table}
> +\drivernormative{\subsection}{The Virtqueue Descriptor Ring}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Ring}

While you're touching it anyway: s/PAcked/Packed/ ? (Probably needs a
change in conformance.tex as well.)

> A driver MUST NOT change a descriptor unless it observes
> the VIRTQ_DESC_F_USED bit in its \field{flags} being changed.
> A driver MUST NOT change a descriptor after changing

On Mon, 9 Apr 2018 20:58:39 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > Occasionally we still use used and available ring instead of Device and > Driver Area respectively. Same goes for descriptor table with the > difference that depending on the context we need either Descriptor Area > or descriptor ring. > > Let's clean these up, at least where the cleanup is trivial. > > Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > --- > content.tex 14 +++++++------- > packed-ring.tex 10 ++++++---- > 2 files changed, 13 insertions(+), 11 deletions(-) > > -drivernormative{subsection}{The Virtqueue Descriptor Table}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Table} > +drivernormative{subsection}{The Virtqueue Descriptor Ring}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Ring} While you're touching it anyway: s/PAcked/Packed/ ? (Probably needs a change in conformance.tex as well.) > A driver MUST NOT change a descriptor unless it observes > the VIRTQ_DESC_F_USED bit in its field{flags} being changed. > A driver MUST NOT change a descriptor after changing

On 04/10/2018 11:30 AM, Cornelia Huck wrote:
> On Mon, 9 Apr 2018 20:58:39 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>
>> Occasionally we still use used and available ring instead of Device and
>> Driver Area respectively. Same goes for descriptor table with the
>> difference that depending on the context we need either Descriptor Area
>> or descriptor ring.
>>
>> Let's clean these up, at least where the cleanup is trivial.
>>
>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> ---
>> content.tex | 14 +++++++-------
>> packed-ring.tex | 10 ++++++----
>> 2 files changed, 13 insertions(+), 11 deletions(-)
>>
>
>> -\drivernormative{\subsection}{The Virtqueue Descriptor Table}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Table}
>> +\drivernormative{\subsection}{The Virtqueue Descriptor Ring}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Ring}
>
> While you're touching it anyway: s/PAcked/Packed/ ? (Probably needs a
> change in conformance.tex as well.)
>

Yeah, the thing with unrelated changes. Was considering it myself,
and decided not to since I'm doing basically the same change in
may places here. But I can do it next time.

Regards,
Halil

On 04/10/2018 11:30 AM, Cornelia Huck wrote: > On Mon, 9 Apr 2018 20:58:39 +0200 > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > >> Occasionally we still use used and available ring instead of Device and >> Driver Area respectively. Same goes for descriptor table with the >> difference that depending on the context we need either Descriptor Area >> or descriptor ring. >> >> Let's clean these up, at least where the cleanup is trivial. >> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> >> --- >> content.tex 14 +++++++------- >> packed-ring.tex 10 ++++++---- >> 2 files changed, 13 insertions(+), 11 deletions(-) >> > >> -drivernormative{subsection}{The Virtqueue Descriptor Table}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Table} >> +drivernormative{subsection}{The Virtqueue Descriptor Ring}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Ring} > > While you're touching it anyway: s/PAcked/Packed/ ? (Probably needs a > change in conformance.tex as well.) > Yeah, the thing with unrelated changes. Was considering it myself, and decided not to since I'm doing basically the same change in may places here. But I can do it next time. Regards, Halil

On Tue, 10 Apr 2018 12:04:51 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 04/10/2018 11:30 AM, Cornelia Huck wrote:
> > On Mon, 9 Apr 2018 20:58:39 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >
> >> Occasionally we still use used and available ring instead of Device and
> >> Driver Area respectively. Same goes for descriptor table with the
> >> difference that depending on the context we need either Descriptor Area
> >> or descriptor ring.
> >>
> >> Let's clean these up, at least where the cleanup is trivial.
> >>
> >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> >> ---
> >> content.tex | 14 +++++++-------
> >> packed-ring.tex | 10 ++++++----
> >> 2 files changed, 13 insertions(+), 11 deletions(-)
> >>
> >
> >> -\drivernormative{\subsection}{The Virtqueue Descriptor Table}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Table}
> >> +\drivernormative{\subsection}{The Virtqueue Descriptor Ring}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Ring}
> >
> > While you're touching it anyway: s/PAcked/Packed/ ? (Probably needs a
> > change in conformance.tex as well.)
> >
>
> Yeah, the thing with unrelated changes. Was considering it myself,
> and decided not to since I'm doing basically the same change in
> may places here. But I can do it next time.

OK, if it's in more than one place, let's do it in a separate patch and
clean up all of them.

On Tue, 10 Apr 2018 12:04:51 +0200 Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > On 04/10/2018 11:30 AM, Cornelia Huck wrote: > > On Mon, 9 Apr 2018 20:58:39 +0200 > > Halil Pasic <pasic@linux.vnet.ibm.com> wrote: > > > >> Occasionally we still use used and available ring instead of Device and > >> Driver Area respectively. Same goes for descriptor table with the > >> difference that depending on the context we need either Descriptor Area > >> or descriptor ring. > >> > >> Let's clean these up, at least where the cleanup is trivial. > >> > >> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> > >> --- > >> content.tex 14 +++++++------- > >> packed-ring.tex 10 ++++++---- > >> 2 files changed, 13 insertions(+), 11 deletions(-) > >> > > > >> -drivernormative{subsection}{The Virtqueue Descriptor Table}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Table} > >> +drivernormative{subsection}{The Virtqueue Descriptor Ring}{Basic Facilities of a Virtio Device / PAcked Virtqueues / The Virtqueue Descriptor Ring} > > > > While you're touching it anyway: s/PAcked/Packed/ ? (Probably needs a > > change in conformance.tex as well.) > > > > Yeah, the thing with unrelated changes. Was considering it myself, > and decided not to since I'm doing basically the same change in > may places here. But I can do it next time. OK, if it's in more than one place, let's do it in a separate patch and clean up all of them.

The drivernormative was simply in the wrong place. It does not belong to the code example. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- packed-ring.tex 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-) diff --git a/packed-ring.tex b/packed-ring.tex index d93df3b..388850b 100644 --- a/packed-ring.tex +++ b/packed-ring.tex @@ -600,6 +600,10 @@ Suppression Structure Format}. The driver has to be careful to expose the new field{flags} value before checking if notifications are suppressed. +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. + subsubsection{Implementation Example}label{sec:Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Implementation Example} Below is a driver code example. It does not attempt to reduce @@ -658,11 +662,6 @@ if (vq->device_event.flags != RING_EVENT_FLAGS_DISABLE) { 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 -- 1.7.1

On 09/04/2018 20:58, Halil Pasic wrote:
> The driver has to be careful to expose the new \field{flags}
> value before checking if notifications are suppressed.

This paragraph should also be reworked to be part of the normative text
below, I think? The memory barrier must be "between two things", and
only with this paragraph you know that it's between the flags write and
the suppression structure read.

Thanks,

Paolo

> +\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.
> +

On 09/04/2018 20:58, Halil Pasic wrote: > The driver has to be careful to expose the new field{flags} > value before checking if notifications are suppressed. This paragraph should also be reworked to be part of the normative text below, I think? The memory barrier must be "between two things", and only with this paragraph you know that it's between the flags write and the suppression structure read. Thanks, Paolo > +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. > +

On 04/09/2018 09:51 PM, Paolo Bonzini wrote:
> On 09/04/2018 20:58, Halil Pasic wrote:
>> The driver has to be careful to expose the new \field{flags}
>> value before checking if notifications are suppressed.
>
> This paragraph should also be reworked to be part of the normative text
> below, I think? The memory barrier must be "between two things", and
> only with this paragraph you know that it's between the flags write and
> the suppression structure read.
>
All I did was to move the device normative to a better place. IMHO
it's current place is obviously wrong. So, I read your comment as
there should be a patch on top that further integrates the paragraphs
made adjacent by this patch. Is that right?

About the normative statements. I hinted before that I don't really
understand the role of normative statements in this specification. To
be more precise, my naive understanding of their role is in conflict
with the reality of the specification.

Can someone tell me what is supposed to go in a normative statements
and what is supposed to go outside? Along with an estimate how good
are we at adhering to those rules.

Regards,
Halil

>
>> +\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.
>> +
>
>
> ---------------------------------------------------------------------
> To unsubscribe from this mail list, you must leave the OASIS TC that
> generates this mail. Follow this link to all your TCs in OASIS at:
> https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php
>

On 04/09/2018 09:51 PM, Paolo Bonzini wrote: > On 09/04/2018 20:58, Halil Pasic wrote: >> The driver has to be careful to expose the new field{flags} >> value before checking if notifications are suppressed. > > This paragraph should also be reworked to be part of the normative text > below, I think? The memory barrier must be "between two things", and > only with this paragraph you know that it's between the flags write and > the suppression structure read. > All I did was to move the device normative to a better place. IMHO it's current place is obviously wrong. So, I read your comment as there should be a patch on top that further integrates the paragraphs made adjacent by this patch. Is that right? About the normative statements. I hinted before that I don't really understand the role of normative statements in this specification. To be more precise, my naive understanding of their role is in conflict with the reality of the specification. Can someone tell me what is supposed to go in a normative statements and what is supposed to go outside? Along with an estimate how good are we at adhering to those rules. Regards, Halil > >> +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. >> + > > > --------------------------------------------------------------------- > To unsubscribe from this mail list, you must leave the OASIS TC that > generates this mail. Follow this link to all your TCs in OASIS at: > https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php >

On 10/04/2018 12:25, Halil Pasic wrote:
> All I did was to move the device normative to a better place. IMHO
> it's current place is obviously wrong.

I agree with that - but IMO you're now separating two related paragraphs.

> So, I read your comment as
> there should be a patch on top that further integrates the paragraphs
> made adjacent by this patch. Is that right?

It can even be the same patch, reworked to put something like this in
the normative text:

The driver MUST ensure the write to the \field{flags} field is performed
before the read of the Driver Event Suppression, in order to avoid
missing a notification from the device.

> About the normative statements. I hinted before that I don't really
> understand the role of normative statements in this specification. To
> be more precise, my naive understanding of their role is in conflict
> with the reality of the specification.
>
> Can someone tell me what is supposed to go in a normative statements
> and what is supposed to go outside? Along with an estimate how good
> are we at adhering to those rules.

Anything that is declared "MUST", "SHOULD" or "MAY" constitutes a
normative statement.

Paolo

On 10/04/2018 12:25, Halil Pasic wrote: > All I did was to move the device normative to a better place. IMHO > it's current place is obviously wrong. I agree with that - but IMO you're now separating two related paragraphs. > So, I read your comment as > there should be a patch on top that further integrates the paragraphs > made adjacent by this patch. Is that right? It can even be the same patch, reworked to put something like this in the normative text: The driver MUST ensure the write to the field{flags} field is performed before the read of the Driver Event Suppression, in order to avoid missing a notification from the device. > About the normative statements. I hinted before that I don't really > understand the role of normative statements in this specification. To > be more precise, my naive understanding of their role is in conflict > with the reality of the specification. > > Can someone tell me what is supposed to go in a normative statements > and what is supposed to go outside? Along with an estimate how good > are we at adhering to those rules. Anything that is declared "MUST", "SHOULD" or "MAY" constitutes a normative statement. Paolo

On 04/10/2018 01:05 PM, Paolo Bonzini wrote:
> On 10/04/2018 12:25, Halil Pasic wrote:
>> All I did was to move the device normative to a better place. IMHO
>> it's current place is obviously wrong.
>
> I agree with that - but IMO you're now separating two related paragraphs.
>

I'm not separating them they were separated, but I get your point.

>> So, I read your comment as
>> there should be a patch on top that further integrates the paragraphs
>> made adjacent by this patch. Is that right?
>
> It can even be the same patch, reworked to put something like this in
> the normative text:
>
> The driver MUST ensure the write to the \field{flags} field is performed
> before the read of the Driver Event Suppression, in order to avoid
> missing a notification from the device.
>

Will do a it, but I prefer to make this an extra patch.

>> About the normative statements. I hinted before that I don't really
>> understand the role of normative statements in this specification. To
>> be more precise, my naive understanding of their role is in conflict
>> with the reality of the specification.
>>
>> Can someone tell me what is supposed to go in a normative statements
>> and what is supposed to go outside? Along with an estimate how good
>> are we at adhering to those rules.
>
> Anything that is declared "MUST", "SHOULD" or "MAY" constitutes a
> normative statement.
>

The problem here is, that this sentence you reformulated as a
'MUST' sentence was formulated as a 'has to be careful' sentence.

My concern (regarding the whole spec) is the completeness and self
the containment of it's normative portion -- I'm not sure if either is
pursued rigorously. For instance take 'Supplying Buffers to The
Device' either for split or for packed. The algorithms described
there aren't constituting a normative section. Do you think these
can be inferred from the normative sections?

Are the normative statements (e.g in v1.0 cs4) sufficient to
guarantee conforming implementations work (interoperably)?

In math we have primitive notions, definitions, axioms, theorems, proofs
and hypotheses. Textbooks also have less formal text (beyond the
stuff listed) to either help with the understanding, or provide background.

My naive understanding of the purpose of non-normative sections of this
specification is: definitions, background (e.g. legacy interface),
and explanations facilitating *easier* understanding.

To stay at the math analogy, the device normative statements are
most akin to axioms I guess. As axioms along with primitive notions
and definitions, what constitutes a formal system.

So that's where my question is coming from. And I'm not keen to
put too much work into this. I'm just curious how should I reason
about stuff (when reviewing and voting for example). Sorry for getting
philosophical.

Regards,

Halil

On 04/10/2018 01:05 PM, Paolo Bonzini wrote: > On 10/04/2018 12:25, Halil Pasic wrote: >> All I did was to move the device normative to a better place. IMHO >> it's current place is obviously wrong. > > I agree with that - but IMO you're now separating two related paragraphs. > I'm not separating them they were separated, but I get your point. >> So, I read your comment as >> there should be a patch on top that further integrates the paragraphs >> made adjacent by this patch. Is that right? > > It can even be the same patch, reworked to put something like this in > the normative text: > > The driver MUST ensure the write to the field{flags} field is performed > before the read of the Driver Event Suppression, in order to avoid > missing a notification from the device. > Will do a it, but I prefer to make this an extra patch. >> About the normative statements. I hinted before that I don't really >> understand the role of normative statements in this specification. To >> be more precise, my naive understanding of their role is in conflict >> with the reality of the specification. >> >> Can someone tell me what is supposed to go in a normative statements >> and what is supposed to go outside? Along with an estimate how good >> are we at adhering to those rules. > > Anything that is declared "MUST", "SHOULD" or "MAY" constitutes a > normative statement. > The problem here is, that this sentence you reformulated as a 'MUST' sentence was formulated as a 'has to be careful' sentence. My concern (regarding the whole spec) is the completeness and self the containment of it's normative portion -- I'm not sure if either is pursued rigorously. For instance take 'Supplying Buffers to The Device' either for split or for packed. The algorithms described there aren't constituting a normative section. Do you think these can be inferred from the normative sections? Are the normative statements (e.g in v1.0 cs4) sufficient to guarantee conforming implementations work (interoperably)? In math we have primitive notions, definitions, axioms, theorems, proofs and hypotheses. Textbooks also have less formal text (beyond the stuff listed) to either help with the understanding, or provide background. My naive understanding of the purpose of non-normative sections of this specification is: definitions, background (e.g. legacy interface), and explanations facilitating *easier* understanding. To stay at the math analogy, the device normative statements are most akin to axioms I guess. As axioms along with primitive notions and definitions, what constitutes a formal system. So that's where my question is coming from. And I'm not keen to put too much work into this. I'm just curious how should I reason about stuff (when reviewing and voting for example). Sorry for getting philosophical. Regards, Halil

On 10/04/2018 16:59, Halil Pasic wrote:
>> I agree with that - but IMO you're now separating two related paragraphs.
>
> I'm not separating them they were separated, but I get your point.

Yeah, I meant separating them with a heading.

>> The driver MUST ensure the write to the \field{flags} field is performed
>> before the read of the Driver Event Suppression, in order to avoid
>> missing a notification from the device.
>
> Will do a it, but I prefer to make this an extra patch.

Sure, yhat's your choice.

>>> About the normative statements. I hinted before that I don't really
>>> understand the role of normative statements in this specification. To
>>> be more precise, my naive understanding of their role is in conflict
>>> with the reality of the specification.
>>>
>>> Can someone tell me what is supposed to go in a normative statements
>>> and what is supposed to go outside? Along with an estimate how good
>>> are we at adhering to those rules.
>> Anything that is declared "MUST", "SHOULD" or "MAY" constitutes a
>> normative statement.
>
> The problem here is, that this sentence you reformulated as a
> 'MUST' sentence was formulated as a 'has to be careful' sentence.

That was a mistake, in my opinion.

> My concern (regarding the whole spec) is the completeness and self
> the containment of it's normative portion -- I'm not sure if either is
> pursued rigorously. For instance take 'Supplying Buffers to The
> Device' either for split or for packed. The algorithms described
> there aren't constituting a normative section. Do you think these
> can be inferred from the normative sections?

Probably more from chapters such as "Basic Facilities of a Virtio
Device"; that is, it's the only way to use them so it need not be
normative. Those chapters in turn could use some update with respect to
memory barriers, and sometimes lack some normative text.

For example "The Virtqueue Available Ring" for split virtqueues has
almost no normative text, and for "The Virtqueue Used Ring" (split
virtqueues too) "The device MUST set len prior to updating the used idx"
should mention a memory barrier.

> Are the normative statements (e.g in v1.0 cs4) sufficient to
> guarantee conforming implementations work (interoperably)?
>
> In math we have primitive notions, definitions, axioms, theorems, proofs
> and hypotheses. Textbooks also have less formal text (beyond the
> stuff listed) to either help with the understanding, or provide background.
>
> My naive understanding of the purpose of non-normative sections of this
> specification is: definitions, background (e.g. legacy interface),
> and explanations facilitating *easier* understanding.

Yes.

> To stay at the math analogy, the device normative statements are
> most akin to axioms I guess. As axioms along with primitive notions
> and definitions, what constitutes a formal system.

Certainly not "fundamental" axioms (those would be things like a formal
memory model), but I understand what you mean and I think it makes sense.

Paolo

> So that's where my question is coming from. And I'm not keen to
> put too much work into this. I'm just curious how should I reason
> about stuff (when reviewing and voting for example). Sorry for getting
> philosophical.

On 10/04/2018 16:59, Halil Pasic wrote: >> I agree with that - but IMO you're now separating two related paragraphs. > > I'm not separating them they were separated, but I get your point. Yeah, I meant separating them with a heading. >> The driver MUST ensure the write to the field{flags} field is performed >> before the read of the Driver Event Suppression, in order to avoid >> missing a notification from the device. > > Will do a it, but I prefer to make this an extra patch. Sure, yhat's your choice. >>> About the normative statements. I hinted before that I don't really >>> understand the role of normative statements in this specification. To >>> be more precise, my naive understanding of their role is in conflict >>> with the reality of the specification. >>> >>> Can someone tell me what is supposed to go in a normative statements >>> and what is supposed to go outside? Along with an estimate how good >>> are we at adhering to those rules. >> Anything that is declared "MUST", "SHOULD" or "MAY" constitutes a >> normative statement. > > The problem here is, that this sentence you reformulated as a > 'MUST' sentence was formulated as a 'has to be careful' sentence. That was a mistake, in my opinion. > My concern (regarding the whole spec) is the completeness and self > the containment of it's normative portion -- I'm not sure if either is > pursued rigorously. For instance take 'Supplying Buffers to The > Device' either for split or for packed. The algorithms described > there aren't constituting a normative section. Do you think these > can be inferred from the normative sections? Probably more from chapters such as "Basic Facilities of a Virtio Device"; that is, it's the only way to use them so it need not be normative. Those chapters in turn could use some update with respect to memory barriers, and sometimes lack some normative text. For example "The Virtqueue Available Ring" for split virtqueues has almost no normative text, and for "The Virtqueue Used Ring" (split virtqueues too) "The device MUST set len prior to updating the used idx" should mention a memory barrier. > Are the normative statements (e.g in v1.0 cs4) sufficient to > guarantee conforming implementations work (interoperably)? > > In math we have primitive notions, definitions, axioms, theorems, proofs > and hypotheses. Textbooks also have less formal text (beyond the > stuff listed) to either help with the understanding, or provide background. > > My naive understanding of the purpose of non-normative sections of this > specification is: definitions, background (e.g. legacy interface), > and explanations facilitating *easier* understanding. Yes. > To stay at the math analogy, the device normative statements are > most akin to axioms I guess. As axioms along with primitive notions > and definitions, what constitutes a formal system. Certainly not "fundamental" axioms (those would be things like a formal memory model), but I understand what you mean and I think it makes sense. Paolo > So that's where my question is coming from. And I'm not keen to > put too much work into this. I'm just curious how should I reason > about stuff (when reviewing and voting for example). Sorry for getting > philosophical.

On Tue, Apr 10, 2018 at 04:59:12PM +0200, Halil Pasic wrote:
> My concern (regarding the whole spec) is the completeness and self
> the containment of it's normative portion -- I'm not sure if either is
> pursued rigorously.

No it isn't, and the reason is we do not want to bring the development
to a complete halt :). Our charter is

Enhancing the performance of virtual devices by standardizing key
features of the VIRTIO (Virtual I/O) Device Specification

Our job is thus to facilitate development by standardizing where it's
helpful.

If we start asking people to write out a formal spec for any tiny
change before they can release code, development will stop and
performance will suffer.

And it's a balance: it should be clear what is going on,
and if people can write it rigorously without confusing
matters, it is helpful.

But we don't want something like e.g. the C or C++ language spec,
where you can read all of it and still have no idea how to use it.

> For instance take 'Supplying Buffers to The
> Device' either for split or for packed. The algorithms described
> there aren't constituting a normative section. Do you think these
> can be inferred from the normative sections?

No but it's ok for normative sections to refer to the non-normative
ones.


--
MST

On Tue, Apr 10, 2018 at 04:59:12PM +0200, Halil Pasic wrote: > My concern (regarding the whole spec) is the completeness and self > the containment of it's normative portion -- I'm not sure if either is > pursued rigorously. No it isn't, and the reason is we do not want to bring the development to a complete halt :). Our charter is Enhancing the performance of virtual devices by standardizing key features of the VIRTIO (Virtual I/O) Device Specification Our job is thus to facilitate development by standardizing where it's helpful. If we start asking people to write out a formal spec for any tiny change before they can release code, development will stop and performance will suffer. And it's a balance: it should be clear what is going on, and if people can write it rigorously without confusing matters, it is helpful. But we don't want something like e.g. the C or C++ language spec, where you can read all of it and still have no idea how to use it. > For instance take 'Supplying Buffers to The > Device' either for split or for packed. The algorithms described > there aren't constituting a normative section. Do you think these > can be inferred from the normative sections? No but it's ok for normative sections to refer to the non-normative ones. -- MST

On 04/20/2018 05:59 PM, Michael S. Tsirkin wrote:
> On Tue, Apr 10, 2018 at 04:59:12PM +0200, Halil Pasic wrote:
>> My concern (regarding the whole spec) is the completeness and self
>> the containment of it's normative portion -- I'm not sure if either is
>> pursued rigorously.
>
> No it isn't, and the reason is we do not want to bring the development
> to a complete halt :). Our charter is
>
> Enhancing the performance of virtual devices by standardizing key
> features of the VIRTIO (Virtual I/O) Device Specification
>
> Our job is thus to facilitate development by standardizing where it's
> helpful.
>
> If we start asking people to write out a formal spec for any tiny
> change before they can release code, development will stop and
> performance will suffer.
>
> And it's a balance: it should be clear what is going on,
> and if people can write it rigorously without confusing
> matters, it is helpful.
>
> But we don't want something like e.g. the C or C++ language spec,
> where you can read all of it and still have no idea how to use it.
>

I understand.

>> For instance take 'Supplying Buffers to The
>> Device' either for split or for packed. The algorithms described
>> there aren't constituting a normative section. Do you think these
>> can be inferred from the normative sections?
>
> No but it's ok for normative sections to refer to the non-normative
> ones.
>

Was, kind of, the point I tried to make. I've (for myself) given up on
these ideals. That's why I did not bother to think about isn't that sentence
normative section material. And I don't intend to clean up normative
vs non-normative.

Anyway, it is good to have it spelled out what is the objective of this document,
and what is not. I will continue to try to improve the specification,
in accordance to that -- and avoid hanging myself up on what is not
considered a primary objective.

Many thanks!

Regards,
Halil

On 04/20/2018 05:59 PM, Michael S. Tsirkin wrote: On Tue, Apr 10, 2018 at 04:59:12PM +0200, Halil Pasic wrote: My concern (regarding the whole spec) is the completeness and self the containment of it's normative portion -- I'm not sure if either is pursued rigorously. No it isn't, and the reason is we do not want to bring the development to a complete halt :). Our charter is Enhancing the performance of virtual devices by standardizing key features of the VIRTIO (Virtual I/O) Device Specification Our job is thus to facilitate development by standardizing where it's helpful. If we start asking people to write out a formal spec for any tiny change before they can release code, development will stop and performance will suffer. And it's a balance: it should be clear what is going on, and if people can write it rigorously without confusing matters, it is helpful. But we don't want something like e.g. the C or C++ language spec, where you can read all of it and still have no idea how to use it. I understand. For instance take 'Supplying Buffers to The Device' either for split or for packed. The algorithms described there aren't constituting a normative section. Do you think these can be inferred from the normative sections? No but it's ok for normative sections to refer to the non-normative ones. Was, kind of, the point I tried to make. I've (for myself) given up on these ideals. That's why I did not bother to think about isn't that sentence normative section material. And I don't intend to clean up normative vs non-normative. Anyway, it is good to have it spelled out what is the objective of this document, and what is not. I will continue to try to improve the specification, in accordance to that -- and avoid hanging myself up on what is not considered a primary objective. Many thanks! Regards, Halil

On Fri, Apr 20, 2018 at 06:22:40PM +0200, Halil Pasic wrote:
> Was, kind of, the point I tried to make. I've (for myself) given up on
> these ideals. That's why I did not bother to think about isn't that sentence
> normative section material. And I don't intend to clean up normative
> vs non-normative.

Makes sense.

We do need to keep RFC2119 keywords out of the rest of the text simply to
avoid confusing people - from experience if we don't then during public
review people jump on it immediately and say hey this must go into the
normative section. It's just something that's easy to spot :)

--
MST

On Fri, Apr 20, 2018 at 06:22:40PM +0200, Halil Pasic wrote: > Was, kind of, the point I tried to make. I've (for myself) given up on > these ideals. That's why I did not bother to think about isn't that sentence > normative section material. And I don't intend to clean up normative > vs non-normative. Makes sense. We do need to keep RFC2119 keywords out of the rest of the text simply to avoid confusing people - from experience if we don't then during public review people jump on it immediately and say hey this must go into the normative section. It's just something that's easy to spot :) -- MST

This patch does three things. Emphasises that the example is pseudo-code as we tend to stick to C otherwise and tweaks the wording, fixes a typo in the pseudo code, and removes a line of code that I neither can see the necessity for nor the sense of. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- packed-ring.tex 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-) diff --git a/packed-ring.tex b/packed-ring.tex index 388850b..f619069 100644 --- a/packed-ring.tex +++ b/packed-ring.tex @@ -604,9 +604,11 @@ value before checking if notifications are suppressed. The driver MUST perform a suitable memory barrier before reading the Driver Event Suppression structure, to avoid missing a notification. -subsubsection{Implementation Example}label{sec:Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Implementation Example} +subsubsection{Pseudo-Code Example}label{sec:Basic Facilities of a +Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / +Pseudo-Code Example} -Below is a driver code example. It does not attempt to reduce +Below is driver pseudo-code example supplying buffers to the device. It does not attempt to reduce the number of device interrupts, neither does it support the VIRTIO_F_RING_EVENT_IDX feature. @@ -621,7 +623,6 @@ sgs = 0; for (each buffer element b) { sgs++; - vq->ids[vq->next_avail] = -1; vq->desc[vq->next_avail].address = get_addr(b); vq->desc[vq->next_avail].len = get_len(b); @@ -645,7 +646,7 @@ for (each buffer element b) { if (vq->next_avail >= vq->size) { vq->next_avail = 0; - vq->avail_wrap_count ^= 1; + vq->avail_wrap_count ^= 1; } } vq->sgs[id] = sgs; -- 1.7.1

There was a discrepancy between the pseudo code and the textual description of the algorithm describing how shall a driver supply buffers to the device. The textual description did not make sense to me, so I've changed that one to be more in sync with the pseudo code. Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com> --- Still a bit shaky. Please review carefully! This patch may as well be rooted in a terrible misunderstanding. --- packed-ring.tex 64 ++++++++++++++++++++++++++++++------------------------- 1 files changed, 35 insertions(+), 29 deletions(-) diff --git a/packed-ring.tex b/packed-ring.tex index e656fed..d93df3b 100644 --- a/packed-ring.tex +++ b/packed-ring.tex @@ -527,27 +527,12 @@ when using the packed virtqueue format in more detail. The driver offers buffers to one of the device's virtqueues as follows: egin{enumerate} -item The driver places the buffer into free descriptor(s) in the Descriptor Ring. - -item The driver performs a suitable memory barrier to ensure that it updates - the descriptor(s) before checking for notification suppression. - -item If notifications are not suppressed, the driver notifies the device - of the new available buffers. -end{enumerate} - -What follows are the requirements of each stage in more detail. - -subsubsection{Placing Available Buffers Into The Descriptor Ring}label{sec:Basic Facilities of a Virtio Device / Virtqueues / Supplying Buffers to The Device / Placing Available Buffers Into The Descriptor Ring} - -For each buffer element, b: - -egin{enumerate} -item Get the next descriptor ring entry, d item Get the next free buffer id value +item For each buffer element, b: +egin{enumerate} +item Get the next free descriptor ring entry, d item Set field{d.addr} to the physical address of the start of b item Set field{d.len} to the length of b. -item Set field{d.id} to the buffer id item Calculate the flags as follows: egin{enumerate} item If b is device-writable, set the VIRTQ_DESC_F_WRITE bit to 1, otherwise 0 @@ -556,20 +541,41 @@ For each buffer element, b: end{enumerate} item Perform a memory barrier to ensure that the descriptor has been initialized -item Set field{d.flags} to the calculated flags value +item If b is not the first buffer element set field{d.flags} to the + calculated flags value, otherwise save it item If d is the last descriptor in the ring, toggle the - Driver Ring Wrap Counter + Driver Ring Wrap Counter and reset d to the head of the ring item Otherwise, increment d to point at the next descriptor end{enumerate} +item Point d to the descriptor corresponding to the last buffer element + and set field{d.id} to the buffer id +item Save some value associated with the buffer id that makes the + skipping forward described in section
ef{sec:Packed Virtqueues / + Indirect Flag: Scatter-Gather Support} and in section
ef{sec:Packed + Virtqueues / In-Order Use of Buffers} +item Point d to the descriptor corresponding to the first buffer element + and set field{d.flags} to the value calculated and saved +item The driver performs a suitable memory barrier to ensure that all + writes done up to this point precede all writhes done after this + point as observed by the device. +item Point d to the descriptor corresponding to the first buffer element + and set field{d.flags} to the value calculated and saved +item The driver performs a suitable memory barrier to ensure that it updates + the descriptor(s) before checking for notification suppression. +item If notifications are not suppressed, the driver notifies the device + of the new available buffers. +end{enumerate} -This makes a single descriptor buffer available. However, in -general the driver MAY make use of a batch of descriptors as part +This makes a single virtio buffer available. However, in +general the driver MAY make use of a batch of buffers as part of a single request. In that case, it defers updating -the descriptor flags for the first descriptor -(and the previous memory barrier) until after the rest of -the descriptors have been initialized. +the descriptor flags for the first descriptor of the first +virtio buffer (and the preceding memory barrier) until after the rest of +the descriptors have been written. The notification steps are performed +only after the after the flags of the first descriptor of the first +virtio buffer are set. -Once the descriptor field{flags} field is updated by the driver, this exposes +Once the field{flags} field of the first descriptor is updated by the driver, this exposes the descriptor and its contents. The device MAY access the descriptor and any following descriptors the driver created and the memory they refer to immediately. @@ -577,9 +583,9 @@ memory they refer to immediately. drivernormative{paragraph}{Updating flags}{Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Updating flags} -The driver MUST perform a suitable memory barrier before the -field{flags} update, to ensure the -device sees the most up-to-date copy. +The driver MUST perform a suitable memory barrier before updating the +field{flags} of the first descriptor (to ensure the device sees +all the previous changes) . subsubsection{Notifying The Device}label{sec:Basic Facilities of a Virtio Device / Packed Virtqueues / Supplying Buffers to The Device / Notifying The Device} -- 1.7.1

ping

Some of the patches received feedback suggesting changes are to
be pursued. Some patches did not receive any.

I would prefer having at least a 'the right direction'/'seems wrong' type
of feedback for each patch before sending a non RFC with the requests
made here addressed.

On 04/09/2018 08:58 PM, Halil Pasic wrote:
> Some of these are more and some are less important. Please
> be careful, there is always a chance that I misunderstood something
> and that I'm trying to change for the worse.
>
> I've identified some other weak point's too, but tried to
> proceed in order the more grave and the better I think I
> understand it (and thus propose a fix) the higher the priority.
>
> Some stuff I've left for my notifiers patch set. This one
> and that one will have to get merged if both turn out to
> be good.
>
> Halil Pasic (5):
> packed-ring: fix used descriptors but meant buffers
> consistency: clean up stale wording for v1.1 terms
> packed-packed: fix supplying buffers description
> packed-ring: reposition drivernormative on driver notifications
> packed-ring: tweak driver code example
>
> content.tex | 14 ++++----
> packed-ring.tex | 96 +++++++++++++++++++++++++++++-------------------------
> 2 files changed, 59 insertions(+), 51 deletions(-)
>
>
> ---------------------------------------------------------------------
> To unsubscribe from this mail list, you must leave the OASIS TC that
> generates this mail. Follow this link to all your TCs in OASIS at:
> https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php
>

ping Some of the patches received feedback suggesting changes are to be pursued. Some patches did not receive any. I would prefer having at least a 'the right direction'/'seems wrong' type of feedback for each patch before sending a non RFC with the requests made here addressed. On 04/09/2018 08:58 PM, Halil Pasic wrote: Some of these are more and some are less important. Please be careful, there is always a chance that I misunderstood something and that I'm trying to change for the worse. I've identified some other weak point's too, but tried to proceed in order the more grave and the better I think I understand it (and thus propose a fix) the higher the priority. Some stuff I've left for my notifiers patch set. This one and that one will have to get merged if both turn out to be good. Halil Pasic (5): packed-ring: fix used descriptors but meant buffers consistency: clean up stale wording for v1.1 terms packed-packed: fix supplying buffers description packed-ring: reposition drivernormative on driver notifications packed-ring: tweak driver code example content.tex 14 ++++---- packed-ring.tex 96 +++++++++++++++++++++++++++++------------------------- 2 files changed, 59 insertions(+), 51 deletions(-) --------------------------------------------------------------------- To unsubscribe from this mail list, you must leave the OASIS TC that generates this mail. Follow this link to all your TCs in OASIS at: https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php

I've decided not send a v1 until the fate of the notifications
series gets deiced (so I can hopefully base these on those). So,
some more time for comments.

Regards,
Halil


On 04/20/2018 05:00 PM, Halil Pasic wrote:
> ping
>
> Some of the patches received feedback suggesting changes are to
> be pursued. Some patches did not receive any.
>
> I would prefer having at least a 'the right direction'/'seems wrong' type
> of feedback for each patch before sending a non RFC with the requests
> made here addressed.
>
> On 04/09/2018 08:58 PM, Halil Pasic wrote:
>> Some of these are more and some are less important. Please
>> be careful, there is always a chance that I misunderstood something
>> and that I'm trying to change for the worse.
>>
>> I've identified some other weak point's too, but tried to
>> proceed in order the more grave and the better I think I
>> understand it (and thus propose a fix) the higher the priority.
>>
>> Some stuff I've left for my notifiers patch set. This one
>> and that one will have to get merged if both turn out to
>> be good.
>>
>> Halil Pasic (5):
>>    packed-ring: fix used descriptors but meant buffers
>>    consistency: clean up stale  wording for v1.1 terms
>>    packed-packed: fix supplying buffers description
>>    packed-ring: reposition drivernormative on driver notifications
>>    packed-ring: tweak driver code example
>>
>>   content.tex     |   14 ++++----
>>   packed-ring.tex |   96 +++++++++++++++++++++++++++++-------------------------
>>   2 files changed, 59 insertions(+), 51 deletions(-)

I've decided not send a v1 until the fate of the notifications series gets deiced (so I can hopefully base these on those). So, some more time for comments. Regards, Halil On 04/20/2018 05:00 PM, Halil Pasic wrote: ping Some of the patches received feedback suggesting changes are to be pursued. Some patches did not receive any. I would prefer having at least a 'the right direction'/'seems wrong' type of feedback for each patch before sending a non RFC with the requests made here addressed. On 04/09/2018 08:58 PM, Halil Pasic wrote: Some of these are more and some are less important. Please be careful, there is always a chance that I misunderstood something and that I'm trying to change for the worse. I've identified some other weak point's too, but tried to proceed in order the more grave and the better I think I understand it (and thus propose a fix) the higher the priority. Some stuff I've left for my notifiers patch set. This one and that one will have to get merged if both turn out to be good. Halil Pasic (5):    packed-ring: fix used descriptors but meant buffers    consistency: clean up stale  wording for v1.1 terms    packed-packed: fix supplying buffers description    packed-ring: reposition drivernormative on driver notifications    packed-ring: tweak driver code example   content.tex         14 ++++----   packed-ring.tex     96 +++++++++++++++++++++++++++++-------------------------   2 files changed, 59 insertions(+), 51 deletions(-)