OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]


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


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

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

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

Are you supporting transitional devices for MMIO?
If no you don't care what happens with existing drivers.


-- 
MST


[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]