Re: [Qemu-devel] [PATCH v6 1/2] virtio-crypto: Add virtio crypto device specification

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

On Mon, 8 Aug 2016 06:27:15 +0000
"Zeng, Xin" <xin.zeng@intel.com> wrote:

> On Thu, Friday, August 05, 2016 3:56 AM, Michael S. Tsirkin wrote:
> > -----Original Message-----
> > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > Sent: Friday, August 05, 2016 3:56 AM
> > To: Gonglei (Arei)
> > Cc: Zeng, Xin; qemu-devel@nongnu.org; virtio-dev@lists.oasis-open.org;
> > Huangpeng (Peter); Luonengjun; cornelia.huck@de.ibm.com;
> > stefanha@redhat.com; denglingli@chinamobile.com; Jani Kokkonen;
> > Ola.Liljedahl@arm.com; Varun.Sethi@freescale.com; Keating, Brian A; Ma,
> > Liang J; Griffin, John; Hanweidong (Randy); Huangweidong (C)
> > Subject: Re: [PATCH v6 1/2] virtio-crypto: Add virtio crypto device
> > specification
> > 
> > On Thu, Aug 04, 2016 at 11:24:26AM +0000, Gonglei (Arei) wrote:
> > > > > +The first driver-read-only field, \field{version} specifies the
> > > > > +virtio crypto's version, which is reserved for back-compatibility
> > > > > +in future.It's currently defined for the version field:
> > > > > +
> > > > > +\begin{lstlisting}
> > > > > +#define VIRTIO_CRYPTO_VERSION_1  (1)
> > > >
> > > > Suggest to remove this macro,
> > > > Do you think a version which is composed of major version and minor
> > > > version is better?
> > > >
> > >
> > > I think we should tell the developer how to set the value of version
> > > field, but I have no idea about which value or form is better, so I
> > > used 1 as the first version. What's your opinion?
> > 
> > My opinion is that you should drop this completely. We do feature bits, not
> > version numbers in virtio. We do not want each device doing its own thing for
> > compatibility.
> > 
> 
> But as I mentioned before,  considering the bug fix case, if each backend device 
> release need a feature bit meaning "some bugs are fixed", are the feature bits
> enough? 
> Physical devices usually have a revision ID to mark its version, could we have a
> revision Id field for each virtio device to distinguish the the virtio devices which 
> have the same feature sets but have different version? 

I think we really need to decouple device features from bugs in a
certain implementation.

Let's say we have a working virtio-crypto device in qemu 3.1, and a
working virtio-crypto device in another hypervisor 1.3. Both advertise
version 1.

Now we realize that we completely messed up the implementation of a
certain algorithm in qemu. We release qemu 3.2 with this fixed. The
other hypervisor 1.3 was not broken, and therefore they don't need to
do an update.

What should happen to 'version' now? If we keep it at 1, it does not
convey any further information. If we bump it to 2 in qemu 3.2, the
guest still does not know whether a version 1 device is broken or not:
It might be running in the other hypervisor 1.3, which is fine. We
would have basically forced the other hypervisor to release 1.4 for the
version bump to 2 (and deprecated a perfectly fine implementation).

For the above case, the only sane solutions I see are (a) have the host
admin deal with it by either updating qemu or disabling the broken
feature, or (b) have the guest discover the hypervisor and its version
and avoid using the feature if the version is known bad.

If the 'bugs' are rather design problems in the specification of an
algorithm, we should replace it with a v2 specification and guard that
via a feature bit. That is hopefully rare.

tl;dr - I think we should just drop version as it is not helpful.