Re: [virtio] [PATCH] VIRTIO-110: ARM's feedback for MMIO chapter

[Pawel Moll <pawel.moll@arm.com>]

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

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

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.

Paweł