[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. > > 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. 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 \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ł > > > --------------------------------------------------------------------- > 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]