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

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Sep 02, 2013 at 06:24:17PM +0200, Cornelia Huck wrote:
> On Mon, 2 Sep 2013 18:52:07 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Sep 02, 2013 at 05:32:12PM +0200, Cornelia Huck wrote:
> > > 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.
> > 
> > 
> > Why would you expect the performance hit be bigger than for
> > the PCI bus?
> 
> We can't really compare with virtio-pci on s390 (won't work with s390
> pci), but as both host and guest are always big endian, we'd need to do
> a byteswap on both sides where we previously needed none.

Well that's exactly the same with pci if both host and guest
are big endian.

> > 
> > > Have
> > > not come around to test this, but I'll try to get some numbers with a
> > > prototype.
> > 
> > That would be very nice. All my boxes with KVM support are
> > little endian.
> > 
> > > > 
> > > > But it's hard to handle this since Appendix Y isn't included
> > > > ATM.  Is someone going to merge it?
> > > > 
> >