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

[Zeng, Xin]

On  Tuesday, August 9, 2016 6:44 PM  Cornelia Huck wrote:
> -----Original Message-----
> From: Cornelia Huck [mailto:address@hidden
> Sent: Tuesday, August 9, 2016 6:44 PM
> To: Zeng, Xin <address@hidden>
> Cc: Michael S. Tsirkin <address@hidden>; Gonglei (Arei)
> <address@hidden>; address@hidden; virtio-
> address@hidden; Huangpeng (Peter)
> <address@hidden>; Luonengjun <address@hidden>;
> address@hidden; address@hidden; Jani Kokkonen
> <address@hidden>; address@hidden;
> address@hidden; Keating, Brian A <address@hidden>;
> Ma, Liang J <address@hidden>; Griffin, John <address@hidden>;
> Hanweidong (Randy) <address@hidden>; Huangweidong (C)
> <address@hidden>
> Subject: Re: [PATCH v6 1/2] virtio-crypto: Add virtio crypto device
> specification
> 
> On Mon, 8 Aug 2016 06:27:15 +0000
> "Zeng, Xin" <address@hidden> wrote:
> 
> > On Thu, Friday, August 05, 2016 3:56 AM, Michael S. Tsirkin wrote:
> > > -----Original Message-----
> > > From: Michael S. Tsirkin [mailto:address@hidden
> > > Sent: Friday, August 05, 2016 3:56 AM
> > > To: Gonglei (Arei)
> > > Cc: Zeng, Xin; address@hidden; address@hidden;
> > > Huangpeng (Peter); Luonengjun; address@hidden;
> > > address@hidden; address@hidden; Jani Kokkonen;
> > > address@hidden; address@hidden; 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.

The frontend driver and the backend device's compatibility is what I 
considered. We can't avoid the issue in the device's implementation, 
so it's better to let the newer frontend driver know whether the device 
it's currently using has some defects so that it could use workaround 
instead.
Thanks.

> 
> 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.