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


But later we have
	The driver MUST NOT write a 0 to \field{queue_enable}.


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

Imagine you have field A, and generation counter is G.
Now imagine a multi-byte field A changes multiple times. Your text
allows this for the device:

A = 1
G = 1
x = G = 1
     read byte 1
     read byte 2

A = 0xf000000f
G = 2

     read byte 3
     read byte 4

A = 3
G = 1

     read byte 5
     read byte 6
 
     y = G = 1


now we see y == x and so we do not
repear the loop. Bytes 3 and 4 were read from
A = 0xf000000f, bytes 1 2 5 and 6 from A = 1.


The pci text on the other hand, requires that counter is
incremented so unless it overflows, all is well.
Thus multi-byte fields < 256 bytes are ok.

> 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.
> 
> If the configuration space does not change in your example, you'll get x
> == y in the last line. 

Problem is, you allow arbitrary changes so it can go back
to the previous value.

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

I don't get it. You explicitly say "MAY use other accesses for the
device-specific configuration space".
If there is a 4 byte field
in a device specific space then this allows 1 byte accesses.

This is only ok for transitional devices.
Drivers must never do this.




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