Re: [virtio-dev] [PATCH] virtio-ccw: introduce revisions

[Rusty Russell <rusty@au1.ibm.com>]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Thu, Oct 10, 2013 at 03:53:19PM +0200, Cornelia Huck wrote:
>> On Thu, 10 Oct 2013 16:23:50 +0300
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> > On Thu, Oct 10, 2013 at 02:40:28PM +0200, Cornelia Huck wrote:
>> > > On Thu, 10 Oct 2013 14:15:33 +0300
>> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > > 
>> > > > On Thu, Oct 10, 2013 at 01:02:57PM +0200, Cornelia Huck wrote:
>> > > > > On Thu, 10 Oct 2013 11:34:30 +0300
>> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > > > > 
>> > > > > > On Tue, Oct 08, 2013 at 05:19:05PM +0200, Cornelia Huck wrote:
>> > > > > > > Provide a new ccw that allows devices and drivers to operate on selected
>> > > > > > > revision levels.
>> > > > > > > 
>> > > > > > > VIRTIO-42
>> > > > > > > 
>> > > > > > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>> > > > > > 
>> > > > > > So basically, CCW_CMD_SET_VIRTIO_REV re-implements the VIRTIO_1 feature
>> > > > > > bit in a device specific manner.
>> > > > > > Now that we have FEATURES_OK, this does not seem to be needed?
>> > > > > 
>> > > > > The idea was to implement something like a 'revision id', which we
>> > > > > didn't have for ccw. It also allows for negotiating (the driver can try
>> > > > > revisions until it finds one that the device accepts) and extra
>> > > > > configuration specifics (the currently unused data field). As it needs
>> > > > > to be done before any other channel commands, it also allows for early
>> > > > > fencing.
>> > > > 
>> > > > So does not the new initialization sequence and the new
>> > > > FEATURES_OK flag handle all that?
>> > > > I just did a commit - could you take a look please?
>> > > 
>> > > I don't think the features stuff would allow for extra configuration
>> > > data?
>> > 
>> > Yes but what is this extra data?
>> 
>> This might be special queue-configuration limits or so. Just a
>> placeholder so that we don't need to come up with a new mechanism later.

I *do* like the separation of transport version from virtio version.
This way, versioning is done at the device discovery stage which means
we don't ever touch features or status bits on an incompatible version.

Sure, for v1 we could combine the two.  But I'm not so sure about future
transitions.  eg. v2 may not set VERSION_1 and then how would legacy
devices be detected?

I think this explicit step is cleanest, and very much fitting with the
ccw transport.

>> > So far revisions didn't work well for PCI.
>> > Basically the issue is that it's a vague kind of thing.
>> > Imagine that we had the proposed mechanism in place.
>> > Which revision does a transitional driver advertise?
>> > I guess it could start with some value and decrement until success,
>> > but you don't actually seem to say in spec that it should.
>> 
>> That's actually what I had in mind: Try a revision; if it doesn't work,
>> try another one. If your driver does not want to support legacy
>> devices, don't try revision 0.
>> 
>> Was my description too vague?
>> 
>> > 
>
> Somewhat:
> 	+A driver SHOULD start with trying to set the highest revision it
> 	+supports and continue with lower revisions if it gets a command reject.
>
> question is this: is driver supporting N required to query N-1 then?
> Does this require to try all of N-2, N-3 or does this allow any
> random subset N,N-2,N-4?
>
> If the later, I would be a bit concerned that it will be messy to test.

I thought the wording was pretty clear: it even allows skipping
unsupported versions.

> BTW Rusty said all MUST/SHOULD need to be lower case.

Personally I like caps, but the preamble says to be lower case.  We're
going to turn this into some xml format eventually, so it's not
critical.

Cheers,
Rusty.