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

[Gonglei (Arei)]

Hi,

>
> Subject: Re: [Qemu-devel] [PATCH v14 1/2] virtio-crypto: Add virtio crypto
> device specification
> 
> On Sun, 20 Nov 2016 08:45:57 +0000
> gong lei <address@hidden> wrote:
> 
> > On 2016/11/17 2:11, Halil Pasic wrote:
> > > On 11/11/2016 10:23 AM, Gonglei wrote:
> 
> > >> +The value of the \field{status} field is VIRTIO_CRYPTO_S_HW_READY or
> VIRTIO_CRYPTO_S_STARTED.
> > >> +
> > >> +\begin{lstlisting}
> > >> +#define VIRTIO_CRYPTO_S_HW_READY  (1 << 0)
> > >> +#define VIRTIO_CRYPTO_S_STARTED  (1 << 1)
> > >> +\end{lstlisting}
> > >> +
> > > Could not really figure out what this status actually does and how does
> > > it relate to the device status field if at all.
> > >
> > > Furthermore I see no mention of VIRTIO_CRYPTO_S_STARTED except for
> this
> > > one, so the only thing I can think of is that it's the initial value and
> > > means hardware not ready (you state these are the only two values).
> > Good catch. I set VIRTIO_CRYPTO_S_STARTED in the driver if the
> > virtio-crypto driver is ready to
> > work in the guest (registing to the Linux Crypto Framework and the
> > device is ready), vice versa.
> 
> Can you use DRIVER_OK in the generic status field for that?
> 
> >
> > > This however does not seem consistent with what your QEMU reference
> > > implementation does. Another thing is your implementations seem to
> > > use VIRTIO_CRYPTO_S_HW_READY as flag but your specification would
> > > (prohibit combining flags because you get another value).
> > The QEMU side doesn't use VIRTIO_CRYPTO_S_STARTED, so maybe I can
> remove
> > it from
> > the spec and define it in the driver. Does it make sense?
> 
> I think it makes most sense if you consider this status field to be
> device-set only. The HW_READY flag makes sense as it gives the driver
> additional information (much like the LINK_UP flag for virtio-net), but
> I'm not sure what driver status you need beyond what you can already
> provide via the generic status field.
> 
> In any case, the status field should use flags and not mutually
> exclusive values.

Yes, HW_READY flag is enough. The driver starts the device (aka. registers 
cipher algs)
once detects the HW_READY is set and stops the device
(aka. Unregisters crypto algs template) once the HW_READY flag
is cleared.

I'll remove VIRTIO_CRYPTO_S_STARTED flag from the spec.

Thanks,
-Gonglei