OASIS Virtual I/O Device (VIRTIO) TC

* Typos and language mistakes in 4.2.1, 4.2.2 and 4.2.2.2. * Extra clarifications for QueueNum, QueueReady, ConfigGeneration and InterruptACK. * Decapitalised MUSTs in 4.2.4 Legacy Interface. Signed-off-by: Pawel Moll <pawel.moll@arm.com> --- content.tex 62 +++++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 36 insertions(+), 26 deletions(-) diff --git a/content.tex b/content.tex index f2a9e79..2f09419 100644 --- a/content.tex +++ b/content.tex @@ -1877,7 +1877,7 @@ following sections. subsection{MMIO Device Discovery}label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO Device Discovery} -Unlike PCI, MMIO provides no generic device discovery. For each +Unlike PCI, MMIO provides no generic device discovery mechanism. For each device, the guest OS will need to know the location of the registers and interrupt(s) used. The suggested binding for systems using flattened device trees is shown in this example: @@ -1893,7 +1893,7 @@ virtio_block@1e000 { subsection{MMIO Device Register Layout}label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout} -MMIO virtio devices provides a set of memory mapped control +MMIO virtio devices provide a set of memory mapped control registers followed by a device-specific configuration space, described in the table~
ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout}. @@ -1946,7 +1946,7 @@ All register values are organized as Little Endian. hline mmioreg{DeviceFeatures}{Flags representing features the device supports}{0x010}{R}{% Reading from this register returns 32 consecutive flag bits, - first bit depending on the last value written to + the least significant bit depending on the last value written to field{DeviceFeaturesSel}. Access to this register returns bits $field{DeviceFeaturesSel}*32$ to $(field{DeviceFeaturesSel}*32)+31$, eg. feature bits 0 to 31 if field{DeviceFeaturesSel} is set to 0 and @@ -1960,7 +1960,7 @@ All register values are organized as Little Endian. } hline mmioreg{DriverFeatures}{Flags representing device features understood and activated by the driver}{0x020}{W}{% - Writing to this register sets 32 consecutive flag bits, first + Writing to this register sets 32 consecutive flag bits, the least significant bit depending on the last value written to field{DriverFeaturesSel}. Access to this register sets bits $field{DriverFeaturesSel}*32$ to $(field{DriverFeaturesSel}*32)+31$, eg. feature bits 0 to 31 if @@ -1989,16 +1989,16 @@ All register values are organized as Little Endian. } hline mmioreg{QueueNum}{Virtual queue size}{0x038}{W}{% - Queue size is the number of elements in the queue, therefore size - of the Descriptor Table and both Available and Used rings. + Queue size is the number of elements in the queue, therefore in each + of the Descriptor Table, the Available Ring and the Used Ring. Writing to this register notifies the device what size of the queue the driver will use. This applies to the queue selected by writing to field{QueueSel}. } hline mmioreg{QueueReady}{Virtual queue ready bit}{0x044}{RW}{% - Writing one (0x1) to this register notifies the device that the - virtual queue is ready to be used. Reading from this register + Writing one (0x1) to this register notifies the device that it may + execute requests from this virtual queue. Reading from this register returns the last value written to it. Both read and write accesses apply to the queue selected by writing to field{QueueSel}. } @@ -2022,8 +2022,9 @@ All register values are organized as Little Endian. } hline mmioreg{InterruptACK}{Interrupt acknowledge}{0x064}{W}{% - Writing to this register notifies the device that the interrupt - has been handled, as per values for {InterruptStatus}. + Writing a value with bits set as defined in field{InterruptStatus} + to this register notifies the device that events causing + the interrupt have been handled. } hline mmioreg{Status}{Device status}{0x070}{RW}{% @@ -2057,7 +2058,10 @@ All register values are organized as Little Endian. } hline mmioreg{ConfigGeneration}{Configuration atomicity value}{0x0fc}{R}{ - Changes every time the configuration noticeably changes (see
ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. + Reading from this register returns an arbitrary value. Subsequent read returns the same + value if no part of the device-specific configuration has changed since the previous read + or a different value otherwise. + See also
ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. } hline mmioreg{Config}{Configuration space}{0x100+}{RW}{ @@ -2076,22 +2080,28 @@ The device MUST return value 0x2 in field{Version}. The device MUST present each event by setting the corresponding bit in field{InterruptStatus} from the moment it takes place, until the driver acknowledges the interrupt -by writing a corresponding bit mask to the InterruptACK register. Bits which +by writing a corresponding bit mask to the field{InterruptACK} register. Bits which do not represent events which took place MUST be zero. Upon reset, the device MUST clear all bits in field{InterruptStatus} and ready bits in the field{QueueReady} register for all queues in the device. -The device MUST change field{ConfigGeneration} if there is any risk of a -device seeing an inconsistent configuration state, but it MAY only change the value +The device MUST change value returned in field{ConfigGeneration} if there is any risk of a +driver seeing an inconsistent configuration state, but it MAY only change the value after a configuration read operation. drivernormative{subsubsection}{MMIO Device Register Layout}{Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout} The driver MUST NOT access memory locations not described in the -table (or, in case of the configuration space, described in the device specification), +table
ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout} +(or, in case of the configuration space, described in the device specification), MUST NOT write to the read-only registers (direction R) and MUST NOT read from the write-only registers (direction W). +The driver MUST only use 32 bit wide and aligned reads and writes to access the control registers +described in table
ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout}. +The driver MAY use other accesses for the device-specific configuration space, +depending on the device type. + The driver MUST ignore a device with field{MagicValue} which is not 0x74726976, although it MAY report an error. @@ -2110,7 +2120,7 @@ or equal to the value presented by the device in field{QueueNumMax}. When field{QueueReady} is not zero, the driver MUST NOT access field{QueueNum}, field{QueueDescLow}, field{QueueDescHigh}, -field{QueueAvailLow}, field{QueueAvailHigh}, field{QueueUsedLow}, field{QueueUsedHigh} +field{QueueAvailLow}, field{QueueAvailHigh}, field{QueueUsedLow}, field{QueueUsedHigh}. To stop using the queue the driver MUST write zero (0x0) to this field{QueueReady} and MUST read the value back to ensure @@ -2118,8 +2128,8 @@ synchronization. The driver MUST ignore undefined bits in field{InterruptStatus}. -The MUST write the events it handled into field{InterruptACK} when -it finishes handling an interrupt. +The driver MUST write a value with a bit mask describing events it handled into field{InterruptACK} when +it finishes handling an interrupt and MUST NOT set any of the undefined bits in the value. subsection{MMIO-specific Initialization And Device Operation}label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation} @@ -2216,7 +2226,7 @@ nor behaviour: endlastfoot mmioreg{MagicValue}{Magic value}{0x000}{R}{} hline - mmioreg{Version}{Device version number}{0x004}{R}{Legacy device MUST return value 0x1.} + mmioreg{Version}{Device version number}{0x004}{R}{Legacy device must return value 0x1.} hline mmioreg{DeviceID}{Virtio Subsystem Device ID}{0x008}{R}{} hline @@ -2231,9 +2241,9 @@ nor behaviour: mmioreg{GuestFeaturesSel}{Activated (guest) features word selection}{0x024}{W}{} hline mmioreg{GuestPageSize}{Guest page size}{0x028}{W}{% - The driver MUST write the guest page size in bytes to the + The driver must write the guest page size in bytes to the register during initialization, before any queues are used. - This value MUST be a power of 2 and is used by the device to + This value must be a power of 2 and is used by the device to calculate the Guest address of the first queue page (see QueuePFN). } @@ -2264,7 +2274,7 @@ nor behaviour: hline mmioreg{QueueAlign}{Used Ring 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 MUST be a power + boundary of the Used Ring in bytes. This value must be a power of 2 and applies to the queue selected by writing to field{QueueSel}. } hline @@ -2274,7 +2284,7 @@ nor behaviour: is the index number of a page starting with the queue Descriptor Table. Value zero (0x0) means physical address zero (0x00000000) and is illegal. When the driver stops using the - queue it MUST write zero (0x0) to this register. + queue it must write zero (0x0) to this register. Reading from this register returns the currently used page number of the queue, therefore a value other than zero (0x0) means that the queue is in use. @@ -2293,7 +2303,7 @@ nor behaviour: flags. Writing non-zero values to this register sets the status flags, indicating the OS/driver progress. Writing zero (0x0) to this - register triggers a device reset. The device MUST + register triggers a device reset. The device must set field{QueuePFN} to zero (0x0) for all queues in the device. Also see
ef{sec:General Initialization And Device Operation / Device Initialization}~
ameref{sec:General Initialization And Device Operation / Device Initialization}. } @@ -2303,7 +2313,7 @@ nor behaviour: end{longtable} The virtual queue page size is defined by writing to field{GuestPageSize}, -as written by the guest. The driver MUST do this before the +as written by the guest. The driver must do this before the virtual queues are configured. The virtual queue layout follows @@ -2324,7 +2334,7 @@ The virtual queue is configured as follows: item Allocate and zero the queue pages in contiguous virtual memory, aligning the Used Ring to an optimal boundary (usually - page size). The driver MUST choose a queue size smaller than or + page size). The driver must choose a queue size smaller than or equal to field{QueueNumMax}. item Notify the device about the queue size by writing the size to -- 1.9.1

On Mon, Jul 21, 2014 at 06:02:41PM +0100, Pawel Moll wrote: > * Typos and language mistakes in 4.2.1, 4.2.2 and 4.2.2.2. > * Extra clarifications for QueueNum, QueueReady, ConfigGeneration > and InterruptACK. > * Decapitalised MUSTs in 4.2.4 Legacy Interface. It's not very clear what does this mean even if decapitalized. What exactly does this section do? Is it like the PCI, included here to allow for transitional devices and/or drivers? In that case, I think you do want MUST etc, and you want to also add these to list of conformance statements. Take a look at how PCI and core sections did this for one option. There are cases where text clearly says "legacy". If what you mean is in fact transitional devices, then let's say just this, and keep MUST intact. If instead you want to describe what legacy devices did then I think de-capitalizing does not go quite far enough, better reword it to avoid "must" altogether. > > Signed-off-by: Pawel Moll <pawel.moll@arm.com> How about we split this out to several patches? E.g. typo fixes could get a separate issue # and get applied. > --- > content.tex 62 +++++++++++++++++++++++++++++++++++-------------------------- > 1 file changed, 36 insertions(+), 26 deletions(-) > > diff --git a/content.tex b/content.tex > index f2a9e79..2f09419 100644 > --- a/content.tex > +++ b/content.tex > @@ -1877,7 +1877,7 @@ following sections. > > subsection{MMIO Device Discovery}label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO Device Discovery} > > -Unlike PCI, MMIO provides no generic device discovery. For each > +Unlike PCI, MMIO provides no generic device discovery mechanism. For each > device, the guest OS will need to know the location of the registers > and interrupt(s) used. The suggested binding for systems using > flattened device trees is shown in this example: trivial > @@ -1893,7 +1893,7 @@ virtio_block@1e000 { > > subsection{MMIO Device Register Layout}label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout} > > -MMIO virtio devices provides a set of memory mapped control > +MMIO virtio devices provide a set of memory mapped control > registers followed by a device-specific configuration space, > described in the table~
ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout}. > typo fix > @@ -1946,7 +1946,7 @@ All register values are organized as Little Endian. > hline > mmioreg{DeviceFeatures}{Flags representing features the device supports}{0x010}{R}{% > Reading from this register returns 32 consecutive flag bits, > - first bit depending on the last value written to > + the least significant bit depending on the last value written to > field{DeviceFeaturesSel}. Access to this register returns > bits $field{DeviceFeaturesSel}*32$ to $(field{DeviceFeaturesSel}*32)+31$, eg. > feature bits 0 to 31 if field{DeviceFeaturesSel} is set to 0 and trivial > @@ -1960,7 +1960,7 @@ All register values are organized as Little Endian. > } > hline > mmioreg{DriverFeatures}{Flags representing device features understood and activated by the driver}{0x020}{W}{% > - Writing to this register sets 32 consecutive flag bits, first > + Writing to this register sets 32 consecutive flag bits, the least significant > bit depending on the last value written to field{DriverFeaturesSel}. > Access to this register sets bits $field{DriverFeaturesSel}*32$ > to $(field{DriverFeaturesSel}*32)+31$, eg. feature bits 0 to 31 if trivial > @@ -1989,16 +1989,16 @@ All register values are organized as Little Endian. > } > hline > mmioreg{QueueNum}{Virtual queue size}{0x038}{W}{% > - Queue size is the number of elements in the queue, therefore size > - of the Descriptor Table and both Available and Used rings. > + Queue size is the number of elements in the queue, therefore in each > + of the Descriptor Table, the Available Ring and the Used Ring. > Writing to this register notifies the device what size of the > queue the driver will use. This applies to the queue selected by > writing to field{QueueSel}. > } > hline trivial > mmioreg{QueueReady}{Virtual queue ready bit}{0x044}{RW}{% > - Writing one (0x1) to this register notifies the device that the > - virtual queue is ready to be used. Reading from this register > + Writing one (0x1) to this register notifies the device that it may > + execute requests from this virtual queue. Reading from this register > returns the last value written to it. Both read and write > accesses apply to the queue selected by writing to field{QueueSel}. > } would be better to avoid "may" here, and just describe how driver uses it. Actually, I noticed MMIO seems to allow stopping one queue selectively for the driver: To stop using the queue the driver MUST write zero (0x0) to this field{QueueReady} and MUST read the value back to ensure synchronization. But does not require anything from the device. Is it really useful? How about we only allow driver to write 0 for now, and require reset to stop the queue? I also wonder: When field{QueueReady} is not zero, the driver MUST NOT access field{QueueNum}, field{QueueDescLow}, field{QueueDescHigh}, field{QueueAvailLow}, field{QueueAvailHigh}, field{QueueUsedLow}, field{QueueUsedHigh} access really isn't allowed? Or only write? > @@ -2022,8 +2022,9 @@ All register values are organized as Little Endian. > } > hline > mmioreg{InterruptACK}{Interrupt acknowledge}{0x064}{W}{% > - Writing to this register notifies the device that the interrupt > - has been handled, as per values for {InterruptStatus}. > + Writing a value with bits set as defined in field{InterruptStatus} > + to this register notifies the device that events causing > + the interrupt have been handled. > } > hline > mmioreg{Status}{Device status}{0x070}{RW}{% > @@ -2057,7 +2058,10 @@ All register values are organized as Little Endian. > } > hline > mmioreg{ConfigGeneration}{Configuration atomicity value}{0x0fc}{R}{ > - Changes every time the configuration noticeably changes (see
ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. > + Reading from this register returns an arbitrary value. Subsequent read returns the same > + value if no part of the device-specific configuration has changed since the previous read > + or a different value otherwise. > + See also
ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. Hmm I think this is a problem. This apparently means that you must check the value after each byte read. We explicitly wanted to allow this: x = generation y = x do x = y read byte read byte read byte read byte read byte read byte y = generation while y != x Why not say explicitly that it starts at 0 and increments on each read? Also, do you want to stress that incrementing on each read is a bad idea? > } > hline > mmioreg{Config}{Configuration space}{0x100+}{RW}{ > @@ -2076,22 +2080,28 @@ The device MUST return value 0x2 in field{Version}. > > The device MUST present each event by setting the corresponding bit in field{InterruptStatus} from the > moment it takes place, until the driver acknowledges the interrupt > -by writing a corresponding bit mask to the InterruptACK register. Bits which > +by writing a corresponding bit mask to the field{InterruptACK} register. Bits which > do not represent events which took place MUST be zero. > > Upon reset, the device MUST clear all bits in field{InterruptStatus} and ready bits in the > field{QueueReady} register for all queues in the device. > trivial > -The device MUST change field{ConfigGeneration} if there is any risk of a > -device seeing an inconsistent configuration state, but it MAY only change the value > +The device MUST change value returned in field{ConfigGeneration} if there is any risk of a > +driver seeing an inconsistent configuration state, but it MAY only change the value > after a configuration read operation. > Not sure why this is better: what does "value returned" mean? Returned to whom? I personally think it was fine before, or maybe add a ref to a more detailed explanation. > drivernormative{subsubsection}{MMIO Device Register Layout}{Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout} > The driver MUST NOT access memory locations not described in the > -table (or, in case of the configuration space, described in the device specification), > +table
ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout} > +(or, in case of the configuration space, described in the device specification), > MUST NOT write to the read-only registers (direction R) and > MUST NOT read from the write-only registers (direction W). > > +The driver MUST only use 32 bit wide and aligned reads and writes to access the control registers > +described in table
ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout}. > +The driver MAY use other accesses for the device-specific configuration space, > +depending on the device type. > + change not described in commit log. I think this is not strong enough: it allows e.g. 1 byte accesses within 4 byte fields as long as they are in device specific sections, and we would really rather not support such weird things. pci has this for device : The driver MUST access each field using the ``natural'' access method, i.e. 32-bit accesses for 32-bit fields, 16-bit accesses for 16-bit fields and 8-bit accesses for 8-bit fields. there is also text explaining that transitional devices must support accesses other than natural-width. > The driver MUST ignore a device with field{MagicValue} which is not 0x74726976, > although it MAY report an error. > > @@ -2110,7 +2120,7 @@ or equal to the value presented by the device in field{QueueNumMax}. > > When field{QueueReady} is not zero, the driver MUST NOT access > field{QueueNum}, field{QueueDescLow}, field{QueueDescHigh}, > -field{QueueAvailLow}, field{QueueAvailHigh}, field{QueueUsedLow}, field{QueueUsedHigh} > +field{QueueAvailLow}, field{QueueAvailHigh}, field{QueueUsedLow}, field{QueueUsedHigh}. > > To stop using the queue the driver MUST write zero (0x0) to this > field{QueueReady} and MUST read the value back to ensure > @@ -2118,8 +2128,8 @@ synchronization. > > The driver MUST ignore undefined bits in field{InterruptStatus}. > > -The MUST write the events it handled into field{InterruptACK} when > -it finishes handling an interrupt. > +The driver MUST write a value with a bit mask describing events it handled into field{InterruptACK} when > +it finishes handling an interrupt and MUST NOT set any of the undefined bits in the value. > > subsection{MMIO-specific Initialization And Device Operation}label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation} > > @@ -2216,7 +2226,7 @@ nor behaviour: > endlastfoot > mmioreg{MagicValue}{Magic value}{0x000}{R}{} > hline > - mmioreg{Version}{Device version number}{0x004}{R}{Legacy device MUST return value 0x1.} > + mmioreg{Version}{Device version number}{0x004}{R}{Legacy device must return value 0x1.} > hline > mmioreg{DeviceID}{Virtio Subsystem Device ID}{0x008}{R}{} > hline > @@ -2231,9 +2241,9 @@ nor behaviour: > mmioreg{GuestFeaturesSel}{Activated (guest) features word selection}{0x024}{W}{} > hline > mmioreg{GuestPageSize}{Guest page size}{0x028}{W}{% > - The driver MUST write the guest page size in bytes to the > + The driver must write the guest page size in bytes to the > register during initialization, before any queues are used. > - This value MUST be a power of 2 and is used by the device to > + This value must be a power of 2 and is used by the device to > calculate the Guest address of the first queue page > (see QueuePFN). > } > @@ -2264,7 +2274,7 @@ nor behaviour: > hline > mmioreg{QueueAlign}{Used Ring 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 MUST be a power > + boundary of the Used Ring in bytes. This value must be a power > of 2 and applies to the queue selected by writing to field{QueueSel}. > } > hline > @@ -2274,7 +2284,7 @@ nor behaviour: > is the index number of a page starting with the queue > Descriptor Table. Value zero (0x0) means physical address zero > (0x00000000) and is illegal. When the driver stops using the > - queue it MUST write zero (0x0) to this register. > + queue it must write zero (0x0) to this register. > Reading from this register returns the currently used page > number of the queue, therefore a value other than zero (0x0) > means that the queue is in use. > @@ -2293,7 +2303,7 @@ nor behaviour: > flags. > Writing non-zero values to this register sets the status flags, > indicating the OS/driver progress. Writing zero (0x0) to this > - register triggers a device reset. The device MUST > + register triggers a device reset. The device must > set field{QueuePFN} to zero (0x0) for all queues in the device. > Also see
ef{sec:General Initialization And Device Operation / Device Initialization}~
ameref{sec:General Initialization And Device Operation / Device Initialization}. > } > @@ -2303,7 +2313,7 @@ nor behaviour: > end{longtable} > > The virtual queue page size is defined by writing to field{GuestPageSize}, > -as written by the guest. The driver MUST do this before the > +as written by the guest. The driver must do this before the > virtual queues are configured. > > The virtual queue layout follows > @@ -2324,7 +2334,7 @@ The virtual queue is configured as follows: > > item Allocate and zero the queue pages in contiguous virtual > memory, aligning the Used Ring to an optimal boundary (usually > - page size). The driver MUST choose a queue size smaller than or > + page size). The driver must choose a queue size smaller than or > equal to field{QueueNumMax}. > > item Notify the device about the queue size by writing the size to > -- > 1.9.1 Above MUST->must for legacy discussed at the top already. > --------------------------------------------------------------------- > 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 Wed, Jul 30, 2014 at 04:15:51PM +0200, Michael S. Tsirkin wrote: > How about we split this out to several patches? E.g. typo fixes > could get a separate issue # and get applied. Pawel, would you like to address these comments? In theory we could vote on standard tomorrow (7 days after review concluded), looks like we won't be able to, but I'd like us to get everything in shape a week from now.

On Mon, 2014-08-04 at 13:49 +0100, Michael S. Tsirkin wrote: > On Wed, Jul 30, 2014 at 04:15:51PM +0200, Michael S. Tsirkin wrote: > > How about we split this out to several patches? E.g. typo fixes > > could get a separate issue # and get applied. > > Pawel, would you like to address these comments? > In theory we could vote on standard tomorrow (7 days after review > concluded), looks like we won't be able to, but > I'd like us to get everything in shape a week > from now. Ok, I'll do this tonight. Pawel

On Mon, Aug 04, 2014 at 01:57:41PM +0100, Pawel Moll wrote: > On Mon, 2014-08-04 at 13:49 +0100, Michael S. Tsirkin wrote: > > On Wed, Jul 30, 2014 at 04:15:51PM +0200, Michael S. Tsirkin wrote: > > > How about we split this out to several patches? E.g. typo fixes > > > could get a separate issue # and get applied. > > > > Pawel, would you like to address these comments? > > In theory we could vote on standard tomorrow (7 days after review > > concluded), looks like we won't be able to, but > > I'd like us to get everything in shape a week > > from now. > > Ok, I'll do this tonight. > > Pawel Thanks! Also please note this: "Non-Material Change" is any change to the content of a Work Product that does not add or remove any feature of the Work Product and that: (a) constitutes only error corrections, editorial changes, or formatting changes; or (b) is a pro forma change to content required by TC Administration. when you send patches, it would help submission if you include a short explanation why you deem the changes non-material unless it's obvious e.g. for typo fixes. -- MST

On Wed, 2014-07-30 at 15:15 +0100, Michael S. Tsirkin wrote: > On Mon, Jul 21, 2014 at 06:02:41PM +0100, Pawel Moll wrote: > > * Typos and language mistakes in 4.2.1, 4.2.2 and 4.2.2.2. > > * Extra clarifications for QueueNum, QueueReady, ConfigGeneration > > and InterruptACK. > > * Decapitalised MUSTs in 4.2.4 Legacy Interface. > > It's not very clear what does this mean even if decapitalized. > > What exactly does this section do? > > Is it like the PCI, included here to allow for transitional > devices and/or drivers? > > In that case, I think you do want MUST etc, and you want to > also add these to list of conformance statements. > Take a look at how PCI and core sections did this for one option. > > There are cases where text clearly says "legacy". > If what you mean is in fact transitional devices, > then let's say just this, and keep MUST intact. > > If instead you want to describe what legacy devices did > then I think de-capitalizing does not go quite far enough, > better reword it to avoid "must" altogether. Ok, will remove musts. > > mmioreg{QueueReady}{Virtual queue ready bit}{0x044}{RW}{% > > - Writing one (0x1) to this register notifies the device that the > > - virtual queue is ready to be used. Reading from this register > > + Writing one (0x1) to this register notifies the device that it may > > + execute requests from this virtual queue. Reading from this register > > returns the last value written to it. Both read and write > > accesses apply to the queue selected by writing to field{QueueSel}. > > } > > would be better to avoid "may" here, and just describe how > driver uses it. I actually like the wording Brian suggested - it's very clear. I will just replace "may" with "can" to avoid any references to normative sections. > Actually, I noticed MMIO seems to allow stopping one queue > selectively for the driver: > > To stop using the queue the driver MUST write zero (0x0) to this > field{QueueReady} and MUST read the value back to ensure > synchronization. > > But does not require anything from the device. The "synchronization" bit is crucial, and would be understood by any silicon designer. But I agree the devices's responsibilities are not clear. I will add a sentence in the device section. > Is it really useful? How about we only allow driver to write 0 > for now, and require reset to stop the queue? But this is exactly how I read the PCI equivalent: item[field{queue_enable}] The driver uses this to selectively prevent the device from executing requests from this virtqueue. 1 - enabled; 0 - disabled. I can change it, but it's going to be a material change, I afraid. > I also wonder: > When field{QueueReady} is not zero, the driver MUST NOT access > field{QueueNum}, field{QueueDescLow}, field{QueueDescHigh}, > field{QueueAvailLow}, field{QueueAvailHigh}, field{QueueUsedLow}, > field{QueueUsedHigh} > > access really isn't allowed? Or only write? Reads are banned by definition - those are all write-only registers. > > @@ -2057,7 +2058,10 @@ All register values are organized as Little Endian. > > } > > hline > > mmioreg{ConfigGeneration}{Configuration atomicity value}{0x0fc}{R}{ > > - Changes every time the configuration noticeably changes (see
ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. > > + Reading from this register returns an arbitrary value. Subsequent read returns the same > > + value if no part of the device-specific configuration has changed since the previous read > > + or a different value otherwise. > > + See also
ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. > > Hmm I think this is a problem. This apparently means that you must check > the value after each byte read. We explicitly wanted to allow this: > > x = generation > y = x > > do > x = y > read byte > read byte > read byte > read byte > read byte > read byte > > y = generation > while y != x > > Why not say explicitly that it starts at 0 > and increments on each read? Hold on. Either I completely misunderstood the way the generation counter works, or the description exactly fits what you described. You read a the generation counter before config space reads. Then you do your reads, as many as you wish. Then you read the generation counter again. If there was no change in between, the "subsequent read returns the same value if no part of the device-specific configuration has changed since the previous read". I will just add clarification that the "subsequent read" is about the counter, not the config space. If the configuration space does not change in your example, you'll get x == y in the last line. > > -The device MUST change field{ConfigGeneration} if there is any risk of a > > -device seeing an inconsistent configuration state, but it MAY only change the value > > +The device MUST change value returned in field{ConfigGeneration} if there is any risk of a > > +driver seeing an inconsistent configuration state, but it MAY only change the value > > after a configuration read operation. > > > > Not sure why this is better: what does "value returned" mean? Returned > to whom? I personally think it was fine before, or maybe add a ref > to a more detailed explanation. "Value returned" is the language used in most of the register operations used above. I've checked with a friendly technical writer and he prefers the changed wording. > > drivernormative{subsubsection}{MMIO Device Register Layout}{Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout} > > The driver MUST NOT access memory locations not described in the > > -table (or, in case of the configuration space, described in the device specification), > > +table
ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout} > > +(or, in case of the configuration space, described in the device specification), > > MUST NOT write to the read-only registers (direction R) and > > MUST NOT read from the write-only registers (direction W). > > > > +The driver MUST only use 32 bit wide and aligned reads and writes to access the control registers > > +described in table
ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout}. > > +The driver MAY use other accesses for the device-specific configuration space, > > +depending on the device type. > > + > > change not described in commit log. Yeah, missed it. > I think this is not strong enough: it allows e.g. 1 byte > accesses within 4 byte fields as long as they are > in device specific sections, and we would really rather > not support such weird things. No, it doesn't. It says "32 bit _wide_ and _aligned_ reads" - this terminology is commonly used in low-level hardware documentation. Pawel

On Mon, Aug 04, 2014 at 06:38:29PM +0100, Pawel Moll wrote: > On Wed, 2014-07-30 at 15:15 +0100, Michael S. Tsirkin wrote: > > On Mon, Jul 21, 2014 at 06:02:41PM +0100, Pawel Moll wrote: > > > * Typos and language mistakes in 4.2.1, 4.2.2 and 4.2.2.2. > > > * Extra clarifications for QueueNum, QueueReady, ConfigGeneration > > > and InterruptACK. > > > * Decapitalised MUSTs in 4.2.4 Legacy Interface. > > > > It's not very clear what does this mean even if decapitalized. > > > > What exactly does this section do? > > > > Is it like the PCI, included here to allow for transitional > > devices and/or drivers? > > > > In that case, I think you do want MUST etc, and you want to > > also add these to list of conformance statements. > > Take a look at how PCI and core sections did this for one option. > > > > There are cases where text clearly says "legacy". > > If what you mean is in fact transitional devices, > > then let's say just this, and keep MUST intact. > > > > If instead you want to describe what legacy devices did > > then I think de-capitalizing does not go quite far enough, > > better reword it to avoid "must" altogether. > > Ok, will remove musts. > > > > mmioreg{QueueReady}{Virtual queue ready bit}{0x044}{RW}{% > > > - Writing one (0x1) to this register notifies the device that the > > > - virtual queue is ready to be used. Reading from this register > > > + Writing one (0x1) to this register notifies the device that it may > > > + execute requests from this virtual queue. Reading from this register > > > returns the last value written to it. Both read and write > > > accesses apply to the queue selected by writing to field{QueueSel}. > > > } > > > > would be better to avoid "may" here, and just describe how > > driver uses it. > > I actually like the wording Brian suggested - it's very clear. I will > just replace "may" with "can" to avoid any references to normative > sections. > > > Actually, I noticed MMIO seems to allow stopping one queue > > selectively for the driver: > > > > To stop using the queue the driver MUST write zero (0x0) to this > > field{QueueReady} and MUST read the value back to ensure > > synchronization. > > > > But does not require anything from the device. > > The "synchronization" bit is crucial, and would be understood by any > silicon designer. But I agree the devices's responsibilities are not > clear. I will add a sentence in the device section. > > > Is it really useful? How about we only allow driver to write 0 > > for now, and require reset to stop the queue? > > But this is exactly how I read the PCI equivalent: > > item[field{queue_enable}] > The driver uses this to selectively prevent the device from > executing requests from this virtqueue. > 1 - enabled; 0 - disabled. But later we have The driver MUST NOT write a 0 to field{queue_enable}. > I can change it, but it's going to be a material change, I afraid. > > > I also wonder: > > When field{QueueReady} is not zero, the driver MUST NOT access > > field{QueueNum}, field{QueueDescLow}, field{QueueDescHigh}, > > field{QueueAvailLow}, field{QueueAvailHigh}, field{QueueUsedLow}, > > field{QueueUsedHigh} > > > > access really isn't allowed? Or only write? > > Reads are banned by definition - those are all write-only registers. > > > > @@ -2057,7 +2058,10 @@ All register values are organized as Little Endian. > > > } > > > hline > > > mmioreg{ConfigGeneration}{Configuration atomicity value}{0x0fc}{R}{ > > > - Changes every time the configuration noticeably changes (see
ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. > > > + Reading from this register returns an arbitrary value. Subsequent read returns the same > > > + value if no part of the device-specific configuration has changed since the previous read > > > + or a different value otherwise. > > > + See also
ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. > > > > Hmm I think this is a problem. This apparently means that you must check > > the value after each byte read. We explicitly wanted to allow this: > > > > x = generation > > y = x > > > > do > > x = y > > read byte > > read byte > > read byte > > read byte > > read byte > > read byte > > > > y = generation > > while y != x > > > > Why not say explicitly that it starts at 0 > > and increments on each read? > > Hold on. Either I completely misunderstood the way the generation > counter works, or the description exactly fits what you described. Imagine you have field A, and generation counter is G. Now imagine a multi-byte field A changes multiple times. Your text allows this for the device: A = 1 G = 1 x = G = 1 read byte 1 read byte 2 A = 0xf000000f G = 2 read byte 3 read byte 4 A = 3 G = 1 read byte 5 read byte 6 y = G = 1 now we see y == x and so we do not repear the loop. Bytes 3 and 4 were read from A = 0xf000000f, bytes 1 2 5 and 6 from A = 1. The pci text on the other hand, requires that counter is incremented so unless it overflows, all is well. Thus multi-byte fields < 256 bytes are ok. > You read a the generation counter before config space reads. Then you do > your reads, as many as you wish. Then you read the generation counter > again. If there was no change in between, the "subsequent read returns > the same > value if no part of the device-specific configuration has changed since > the previous read". I will just add clarification that the "subsequent > read" is about the counter, not the config space. > > If the configuration space does not change in your example, you'll get x > == y in the last line. Problem is, you allow arbitrary changes so it can go back to the previous value. > > > -The device MUST change field{ConfigGeneration} if there is any risk of a > > > -device seeing an inconsistent configuration state, but it MAY only change the value > > > +The device MUST change value returned in field{ConfigGeneration} if there is any risk of a > > > +driver seeing an inconsistent configuration state, but it MAY only change the value > > > after a configuration read operation. > > > > > > > Not sure why this is better: what does "value returned" mean? Returned > > to whom? I personally think it was fine before, or maybe add a ref > > to a more detailed explanation. > > "Value returned" is the language used in most of the register operations > used above. I've checked with a friendly technical writer and he prefers > the changed wording. > > > > drivernormative{subsubsection}{MMIO Device Register Layout}{Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout} > > > The driver MUST NOT access memory locations not described in the > > > -table (or, in case of the configuration space, described in the device specification), > > > +table
ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout} > > > +(or, in case of the configuration space, described in the device specification), > > > MUST NOT write to the read-only registers (direction R) and > > > MUST NOT read from the write-only registers (direction W). > > > > > > +The driver MUST only use 32 bit wide and aligned reads and writes to access the control registers > > > +described in table
ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout}. > > > +The driver MAY use other accesses for the device-specific configuration space, > > > +depending on the device type. > > > + > > > > change not described in commit log. > > Yeah, missed it. > > > I think this is not strong enough: it allows e.g. 1 byte > > accesses within 4 byte fields as long as they are > > in device specific sections, and we would really rather > > not support such weird things. > > No, it doesn't. It says "32 bit _wide_ and _aligned_ reads" - this > terminology is commonly used in low-level hardware documentation. > > Pawel I don't get it. You explicitly say "MAY use other accesses for the device-specific configuration space". If there is a 4 byte field in a device specific space then this allows 1 byte accesses. This is only ok for transitional devices. Drivers must never do this.

On Mon, Aug 04, 2014 at 06:38:29PM +0100, Pawel Moll wrote: > On Wed, 2014-07-30 at 15:15 +0100, Michael S. Tsirkin wrote: > > On Mon, Jul 21, 2014 at 06:02:41PM +0100, Pawel Moll wrote: > > > * Typos and language mistakes in 4.2.1, 4.2.2 and 4.2.2.2. > > > * Extra clarifications for QueueNum, QueueReady, ConfigGeneration > > > and InterruptACK. > > > * Decapitalised MUSTs in 4.2.4 Legacy Interface. > > > > It's not very clear what does this mean even if decapitalized. > > > > What exactly does this section do? > > > > Is it like the PCI, included here to allow for transitional > > devices and/or drivers? > > > > In that case, I think you do want MUST etc, and you want to > > also add these to list of conformance statements. > > Take a look at how PCI and core sections did this for one option. > > > > There are cases where text clearly says "legacy". > > If what you mean is in fact transitional devices, > > then let's say just this, and keep MUST intact. > > > > If instead you want to describe what legacy devices did > > then I think de-capitalizing does not go quite far enough, > > better reword it to avoid "must" altogether. > > Ok, will remove musts. > > > > mmioreg{QueueReady}{Virtual queue ready bit}{0x044}{RW}{% > > > - Writing one (0x1) to this register notifies the device that the > > > - virtual queue is ready to be used. Reading from this register > > > + Writing one (0x1) to this register notifies the device that it may > > > + execute requests from this virtual queue. Reading from this register > > > returns the last value written to it. Both read and write > > > accesses apply to the queue selected by writing to field{QueueSel}. > > > } > > > > would be better to avoid "may" here, and just describe how > > driver uses it. > > I actually like the wording Brian suggested - it's very clear. I will > just replace "may" with "can" to avoid any references to normative > sections. > > > Actually, I noticed MMIO seems to allow stopping one queue > > selectively for the driver: > > > > To stop using the queue the driver MUST write zero (0x0) to this > > field{QueueReady} and MUST read the value back to ensure > > synchronization. > > > > But does not require anything from the device. > > The "synchronization" bit is crucial, and would be understood by any > silicon designer. But I agree the devices's responsibilities are not > clear. I will add a sentence in the device section. > > > Is it really useful? How about we only allow driver to write 0 > > for now, and require reset to stop the queue? > > But this is exactly how I read the PCI equivalent: > > item[field{queue_enable}] > The driver uses this to selectively prevent the device from > executing requests from this virtqueue. > 1 - enabled; 0 - disabled. > > I can change it, but it's going to be a material change, I afraid. > > > I also wonder: > > When field{QueueReady} is not zero, the driver MUST NOT access > > field{QueueNum}, field{QueueDescLow}, field{QueueDescHigh}, > > field{QueueAvailLow}, field{QueueAvailHigh}, field{QueueUsedLow}, > > field{QueueUsedHigh} > > > > access really isn't allowed? Or only write? > > Reads are banned by definition - those are all write-only registers. > > > > @@ -2057,7 +2058,10 @@ All register values are organized as Little Endian. > > > } > > > hline > > > mmioreg{ConfigGeneration}{Configuration atomicity value}{0x0fc}{R}{ > > > - Changes every time the configuration noticeably changes (see
ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. > > > + Reading from this register returns an arbitrary value. Subsequent read returns the same > > > + value if no part of the device-specific configuration has changed since the previous read > > > + or a different value otherwise. > > > + See also
ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. > > > > Hmm I think this is a problem. This apparently means that you must check > > the value after each byte read. We explicitly wanted to allow this: > > > > x = generation > > y = x > > > > do > > x = y > > read byte > > read byte > > read byte > > read byte > > read byte > > read byte > > > > y = generation > > while y != x > > > > Why not say explicitly that it starts at 0 > > and increments on each read? > > Hold on. Either I completely misunderstood the way the generation > counter works, or the description exactly fits what you described. > > You read a the generation counter before config space reads. Then you do > your reads, as many as you wish. Then you read the generation counter > again. If there was no change in between, the "subsequent read returns > the same > value if no part of the device-specific configuration has changed since > the previous read". I will just add clarification that the "subsequent > read" is about the counter, not the config space. Yes, that is what tripped me up. This clarification would be enough I think. > If the configuration space does not change in your example, you'll get x > == y in the last line. > > > > -The device MUST change field{ConfigGeneration} if there is any risk of a > > > -device seeing an inconsistent configuration state, but it MAY only change the value > > > +The device MUST change value returned in field{ConfigGeneration} if there is any risk of a > > > +driver seeing an inconsistent configuration state, but it MAY only change the value > > > after a configuration read operation. > > > > > > > Not sure why this is better: what does "value returned" mean? Returned > > to whom? I personally think it was fine before, or maybe add a ref > > to a more detailed explanation. > > "Value returned" is the language used in most of the register operations > used above. I've checked with a friendly technical writer and he prefers > the changed wording. > > > > drivernormative{subsubsection}{MMIO Device Register Layout}{Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout} > > > The driver MUST NOT access memory locations not described in the > > > -table (or, in case of the configuration space, described in the device specification), > > > +table
ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout} > > > +(or, in case of the configuration space, described in the device specification), > > > MUST NOT write to the read-only registers (direction R) and > > > MUST NOT read from the write-only registers (direction W). > > > > > > +The driver MUST only use 32 bit wide and aligned reads and writes to access the control registers > > > +described in table
ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout}. > > > +The driver MAY use other accesses for the device-specific configuration space, > > > +depending on the device type. > > > + > > > > change not described in commit log. > > Yeah, missed it. > > > I think this is not strong enough: it allows e.g. 1 byte > > accesses within 4 byte fields as long as they are > > in device specific sections, and we would really rather > > not support such weird things. > > No, it doesn't. It says "32 bit _wide_ and _aligned_ reads" - this > terminology is commonly used in low-level hardware documentation. > > Pawel > > > --------------------------------------------------------------------- > 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