Re: [virtio] [PATCH 3/3] VIRTIO-110: ARM's feedback for MMIO chapter, clarifications

[Pawel Moll <pawel.moll@arm.com>]

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 \ref {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 \ref {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).

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

> > +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 \ref{tab:Virtio Trasport Options / Virtio Over MMIO / MMIO Device Register Layout}
> > @@ -2094,6 +2100,11 @@ table \ref{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 \ref{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? If particular, the existing drivers do memcpy-like
byte-after-byte operations. I'd be more than happy to remove this
exception, but I don't see how can I do that.

Paweł