[PATCH] VIRTIO-101: ARM's feedback for MMIO chapter

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.