[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, 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 \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. 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 \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ł 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.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]