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