[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio] [PATCH] VIRTIO-110: ARM's feedback for MMIO chapter
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~\ref{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 \ref {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 \ref {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 \ref{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 \ref{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 \ref{sec:General Initialization And Device Operation / Device Initialization}~\nameref{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
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]