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

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

On Sat, 12 Oct 2013 21:38:59 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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.
> > 
> > > 
> > > 
> > > > > 
> > > > > If not and we need an extra hand-shake - shouldn't it be
> > > > > generic and not transport specific?
> > > > > 
> > > > 
> > > > I'm trying to introduce a revision mechanism here, which pci/mmio seem
> > > > to have natively but ccw hasn't.
> > > > If we'll never want to use the
> > > > exisiting revision mechanism for anything virtio specific, we can
> > > > probably ditch this. But if we do, all transports should have something
> > > > like that.
> > > 
> > > 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.

The driver should be able to try any revision at any order. In the end,
the device might support any of the revisions it tries, and the device
might want to support drivers with different versions. There also might
be a 'bad apple' version in there that drivers/devices might want to
avoid. Starting with the newest version is best practice.

I agree that we get combinatorial explosion that way, but I'm not sure
that it would make sense to enforce decreasing the version
consecutively (after all, the first successfully set version will
stick.)

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