Re: [virtio] Re: [virtio-comment] [PATCH RFC] virtio-pci: new configuration layout

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Mon, 2 Sep 2013 18:16:45 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Sep 02, 2013 at 04:50:24PM +0200, Cornelia Huck wrote:
> > On Mon, 02 Sep 2013 10:56:20 +0930
> > Rusty Russell <rusty@au1.ibm.com> wrote:
> > 
> > > Cornelia Huck <cornelia.huck@de.ibm.com> writes:
> > > > Some remarks from my side...
> > > >
> > > > On Tue, 27 Aug 2013 10:38:59 +0300
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >
> > > >> On Tue, Aug 27, 2013 at 04:24:47PM +0930, Rusty Russell wrote:
> > > >> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > > >> > > This is the new configuration layout.
> > > >> > >
> > > >> > > Notes:
> > > >> > > - Everything is LE
> > > >> > > - There's a feature bit that means spec 1.0 compliant.
> > > >> > > - Both devices and drivers can either require the 1.0 interface
> > > >> > >   or try to include compatibility support. The spec isn't forcing
> > > >> > >   this decision.
> > > >> > 
> > > >> > Hmm, this kind includes other changes already proposed, like the LE
> > > >> > change and the framing change.  I think this conceptually splits nicely:
> > > >> > 
> > > >> > 1) Feature bit 32 proposal.
> > > >> > 2) Endian change.
> > > >> > 3) Framing change.
> > > >> > 4) PCI layout change.
> > > >> 
> > > >> Right - they are mostly in different parts of the document.
> > > >> I put it all together so it's easy to see how we intend to
> > > >> handle the transition.
> > > >> So is everyone OK with keeping this in a single patch?
> > > >
> > > > The new feature bit is supposed to cover all of this, right? Then this
> > > > should be one patch.
> > > 
> > > Well, the new feature would be "1.0 compliant" which will cover much
> > > more than this.  We'll have a whole document on the differences between
> > > legacy and 1.0, in fact.
> > > 
> > > >> > > - I kept documentation of the legacy interface around, and added notes
> > > >> > >   on transition inline. They are in separate sections each clearly marked
> > > >> > >   "Legacy Interface" so we'll be able to separate them out
> > > >> > >   from the final document as necessary - for now I think it's easier
> > > >> > >   to keep it all together.
> > > >> > 
> > > >> > Good thinking: most of us know the current spec so it's definitely
> > > >> > clearer.  And makes sure we're thinking about the transition.
> > > >> > 
> > > >> > > Only virtio PCI has been converted.
> > > >> > > Let's discuss this on the meeting tonight, once we figure out PCI
> > > >> > > we can do something similar for MMIO and CCW.
> > > >> > 
> > > >> > > @@ -137,6 +139,11 @@ Feature bits are allocated as follows:
> > > >> > >    24 to 31: Feature bits reserved for extensions to the queue and 
> > > >> > >    feature negotiation mechanisms
> > > >> > >  
> > > >> > > +  32: Feature bit must be set for any device compliant with this
> > > >> > > +  revision of the specification, and acknowledged by all device drivers.
> > > >
> > > > Would it make sense to have a bit 33 "rings big endian" whose validity
> > > > depends on bit 32 set? This would make it possible for ccw to keep its
> > > > current endianness.
> > > 
> > > Very awkward.  Our experience with bi-endian devices on PowerPC suggests
> > > it's far better to not negotiate endian.  It also avoids a branch
> > > everywhere in the driver and host.
> > 
> > Ok, so let's skip this feature bit and make virtio-ccw big endian for
> > the ring?
> 
> No, that just makes it impossible to have common ring parsing code
> used by both ccw and other transports.
> 
> Plan is that ccw registers stay big endian, what was previously native
> endian becomes little endian.

I assume by "registers" you mean the ccw payload? These obviously need
to stay big endian for architectural reasons anyway.

But the ring code? I'm really worried about performance hits here. Have
not come around to test this, but I'll try to get some numbers with a
prototype.

> 
> But it's hard to handle this since Appendix Y isn't included
> ATM.  Is someone going to merge it?
>