On Tue, Aug 05, 2014 at 12:57:28PM +0100, Pawel Moll wrote: > > On Tue, 2014-08-05 at 00:00 +0100, Michael S. Tsirkin wrote: > > > * Re-added alignment requirement section - it existed in the > > > original version of the spec, so no extra limitations here. > > > > What is an "original version"? I could not find this in draft 01. > > Damn, you're right! > > I was referring to the 0.9.x spec, but reading it again I can only see > the R/W restriction and a somehow enigmatic note that the registers are > 32-bit wide. I was sure I put it there, as this was one of the first > things I did in my device implementation back there... > > > > Signed-off-by: Pawel Moll <
pawel.moll@arm.com> > > > --- > > > content.tex 21 ++++++++++++++++----- > > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > > > diff --git a/content.tex b/content.tex > > > index 17ba3b8..769bef2 100644 > > > --- a/content.tex > > > +++ b/content.tex > > > @@ -1997,8 +1997,8 @@ All register values are organized as Little Endian. > > > } > > > hline > > > 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 can > > > + 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}. > > > } > > > @@ -2058,7 +2058,11 @@ 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 a value describing a version of the device-specific configuration (see field{Config}). > > > + The driver can then access the configuration space and then read field{ConfigGeneration} again. > > > + If no part of the configuration space has changed, field{ConfigGeneration} returns the same value. > > > + If the value is different, the configuration space access was not atomic and the driver must perform the operation again. > > > + See also
ef {sec:Basic Facilities of a Virtio Device / Device Configuration Space}. > > > } > > > hline > > > mmioreg{Config}{Configuration space}{0x100+}{RW}{ > > I take that the description above is clear now? > > > > @@ -2083,10 +2087,12 @@ 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. > > > > > > -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. > > > > "MAY only" is really confusing. You are using MAY here to > > describe a mandatory requirement. Can be re-written with "MUST NOT"? > > Actually, I will remove this "MAY" part of the sentence completely. It > was original Rusty's wording, but it's not really necessary here, I > believe (it's also gone from PCI section, I see). For PCI there is instead a non-normative note suggesting how to avoid wrap-arounds. > > Also, surely reset changes this field to 0? > > Not sure about that. From the driver point of view it doesn't matter > what value it is, as long as it is different or not. Absolutely, I'm not saying we must require this, I am merely saying if reset clears this field, this violates the "may only change on read" requirement. > > > +The device MUST NOT access virtual queue contents when field{QueueReady} is zero (0x0). > > > + > > > 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
ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout} > > > @@ -2094,6 +2100,11 @@ table
ef{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register > > > 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}. > > > > OK this was kind of implied all the time ... you think adding this text > > is error correction? > > And this is an interesting question. > > As I said above, I was convinced it just got lost on the way, but I was > proven wrong. As you said above, this is exactly what everyone thought > and implemented. > > Is it a material change then? I would like to say no, but I will > understand if anyone shouts that it is. > > > > +The driver MAY use other accesses for the device-specific configuration space, > > > +depending on the device type. > > > > Why are you adding this option? > > If there's a 4 byte register it is a pain for devices to support > > 1 byte accesses, and we gain nothing from it. > > But 2.3 does not limit the configuration space field size nor alignment, > does it? No but there is no need: you look at each specific field, you see what the size and alignment is. > If particular, the existing drivers do memcpy-like > byte-after-byte operations. They are pre-1.0, we will fix them. > I'd be more than happy to remove this > exception, but I don't see how can I do that. > > Pawel Are you supporting transitional devices for MMIO? If no you don't care what happens with existing drivers. -- MST