Re: [Qemu-devel] [RFC v2] virtio-crypto specification

[Gonglei (Arei)]

Hi Cornelia,

Thank you so much for your review :)


> From: Cornelia Huck [mailto:address@hidden
> Sent: Tuesday, April 12, 2016 4:00 PM
> Subject: Re: [RFC v2] virtio-crypto specification
> 
> On Tue, 5 Apr 2016 09:14:09 +0000
> "Gonglei (Arei)" <address@hidden> wrote:
> 
> 
> >                  Virtio-crypto device Spec
> >
> > 1   Crypto Device
> > The virtio crypto device is a virtual crypto device (ie. hardware crypto
> accelerator card). The encryption and decryption requests of are placed in the
> data queue, and handled by the real hardware crypto accelerators finally. A
> second queue is the control queue, which is used to create or destroy session
> for symmetric algorithms, and to control some advanced features.
> > 1.1 Device ID
> > 65535 (experimental)
> 
> I think you should just go ahead and reserve a device ID for this.
> 
OK, I want to reserve 20 as Crypto device's ID as 19 is reserved for Socket 
device.

> >
> > 1.2 Virtqueues
> > 0  dataq
> > …
> > N-1  dataq
> > N  controlq
> >
> > N=1 if VIRTIO_CRYPTO_F_MQ is not negotiated, otherwise N is set by
> max_virtqueues.
> >
> > 1.3 Feature bits
> > VIRTIO_CRYPTO_F_STATUS (0)
> >     Configuration status field is available.
> 
> I'm wondering if this really needs to be made optional?
> 
I'm not sure. :(

> > VIRTIO_CRYPTO_F_MQ (1)
> >     Device supports multiqueue to encrypt and decrypt.
> 
> As commented by Stefan, just drop this.
> 
Yep.

> > VIRTIO_CRYPTO_F_ALGS (2)
> >     Configuration algorithms field is available.
> 
> I'd also think that we always want this field, so this feature bit
> would be superfluous as well.
> 
Yes, I agree. This feature is the same with VIRTIO_CRYPTO_F_MQ.

> >
> > 1.4 Device configuration layout
> > Three driver-read-only configuration fields are currently defined. The 
> > status
> only exists if VIRTIO_CRYPTO_F_STATUS is set.
> 
> I think the status field should just always be provided. It's nice if
> config layout doesn't change.
> 
OK, I agree.

> > On read-only bit (for the driver) is currently defined for the status field:
> VIRTIO_CRYPTO_S_HW_READY.
> 
> s/On/One/
> 
Fixed.

> > #define VIRTIO_CRYPTO_S_HW_READY 1
> 
> Do you have any plans for further status bits? E.g., do you want to be
> able to distinguish between hw not provided and hw in error?
> 
TBH I haven't use other status bits, but I think we should identify the status 
HW not provided.
As this situation of HW in error relies on the HW API, which is not necessary.

> >
> > The following driver-read-only field, max_virtqueues only exists if
> VIRTIO_CRYPTO_F_MQ is set. This field specifies the maximum number of data
> virtqueues (dataq1. . .dataqN) that can be configured once
> VIRTIO_CRYPTO_F_MQ is negotiated.
> > struct virtio_crypto_config {
> >     le16 status;
> >     le16 max_virtqueues;
> >     le32 algorithms;
> > }
> >
> > The last driver-read-only field, algorithms only exists if
> VIRTIO_CRYPTO_F_ALGS is set. This field specifies the algorithms which the
> device offered once VIRTIO_CRYPTO_F_ALGS is negotiated. Two read-only bits
> (for the driver) are currently defined for the algorithms field:
> VIRTIO_CRYPTO_ALG_SYM and VIRTIO_CRYPTO_ALG_ASYM.
> > #define VIRTIO_CRYPTO_ALG_SYM  (1 << 0)
> > #define VIRTIO_CRYPTO_ALG_SYM  (1 << 1)
> 
> I think the second one should be "ASYM".
> 
Yes, it's a typo.

> Do you want to provide indications in this field beyond sym vs. asym,
> e.g. which strength is available?
> 
Yes.
I focused on symmetric algorithms which have a relative
unified interface at present, but for asymmetric algorithms have different 
interfaces,
such as DRBG/RSA/DH etc. I think this field should be extended since we add a 
new
algorithm support.

> As said before, I don't think this should be negotiable. Just provide
> this information at all time.
> 
Yes.

> > 1.4.1       Device Requirements: Device configuration layout
> > The device MUST set max_virtqueues to between 1 and 0x8000 inclusive, if it
> offers VIRTIO_CRYPTO_F_MQ.
> 
> Where does "0x8000" come from?
> 
It come from 5.1.4.1 (Network device). It seems that a wrong value, it should 
be 65535=[(1 << 16) -1].

> > If the driver does not negotiate the VIRTIO_CRYPTO_F_STATUS feature, it
> SHOULD assume the hardware-backed implementation is ready, otherwise it
> SHOULD read the ready status from the bottom bit of status.
> > If the driver does not negotiate the VIRTIO_CRYPTO_F_ALGS feature, it
> SHOULD assume the
> > device support all algorithms.
> 
> That would beg the question what "all algorithms" are :) If you want to
> be able to extend the list of available algorithms later on, this field
> needs to be mandatory.
> 
OK.

> > 1.5 Device Initialization
> > A driver would perform a typical initialization routine like so:
> > 1. Identify and initialize data virtqueue, up to N if VIRTIO_CRYPTO_F_MQ
> feature bit is negotiated, N=max_virtqueues, otherwise identify N=1.
> > 2. Identify the control virtqueue.
> > 3. If the VIRTIO_CRYPTO_F_STATUS feature bit is negotiated, the ready
> status of hardware-backend comes from the bottom bit of status. Otherwise,
> the driver assumes it’s active.
> > 4. If the VIRTIO_CRYPTO_F_ALGS feature bit is negotiated, the driver can
> read the supported algorithms from bits of algorithms. Otherwise, the driver
> assumes all algorithms are supported.
> > 1.6 Device Operation
> > 1.6.1       Session operation
> > The symmetric algorithms have the concept of sessions. A session is a handle
> which describes the
> > cryptographic parameters to be applied to a number of buffers. The data
> within a session handle includes the following:
> > •1. The operation (cipher, hash or both, and if both, the order in which the
> algorithms should be applied).
> 
> I think you want the driver to be able to discover what the device
> supports before it starts issuing requests.
> 
Yes, you're right.

> > •2. The cipher setup data, including the cipher algorithm and mode, the key
> and its length, and the direction (encrypt or decrypt).
> > •3. The hash setup data, including the hash algorithm, mode (plain, nested 
> > or
> authenticated), and digest result length (to allow for truncation).
> >     Authenticated mode can refer to HMAC, which requires that the key and
> its length are also specified. It is also used for GCM and CCM authenticated
> encryption, in which case the AAD length is also specified.
> >     For nested mode, the inner and outer prefix data and length are
> specified, as well as the outer hash algorithm.
> 
> I'll defer looking at the actual interface...
> 
> (...)
> 
> > 1.6.2.1 Steps of encryption Operation
> > Both ctrlq and dataq virtqueue are bidirectional.
> > Step1: Create a session:
> > 1.  The driver fill out the context message, include algorithm name, key,
> keylen etc;
> > 2.  The driver send a context message to the backend device by controlq;
> > 3.  The device create a session using the message transmitted by controlq;
> > 4.  Return the session id to the driver.
> > Step 2: Execute the detail encryption operation:
> > 1.  The driver fill out the encrypt requests;
> > 2.  Put the requests into dataq and kick the virtqueue;
> > 3.  The device execute the encryption operation according the requests’
> arguments;
> > 4.  The device return the encryption result to the driver by dataq;
> > 5.  The driver callback handle the result and over.
> >
> > Note: the driver CAN support both synchronous and asynchronous
> encryption.
> 
> s/CAN/MAY/ ?
> 
Ha-ha, maybe I should search the difference between CAN and MAY form the 
dictionary ;)

> > Then the performance is poor in synchronous operation because frequent
> context switching and virtualization overhead.
> 
> The difference would depend on the device implementation, I guess?
> 
> > The driver SHOULD by preference use asynchronous encryption.
> 
> So is the sync mode intended as a fallback?
> 
Yes. 

> > 1.6.3       Decryption Operation
> > The decryption process is the same with encryption, except that the device
> MUST verify and return the verify result to the driver. If the verify result 
> is not
> correct, VIRTIO_CRYPTO_S_BADMSG (bad message) SHOULD be returned the
> driver.
> 
> s/SHOULD/MUST/ ?
> 
> I'd think it'd would be essential that the driver knows about a failed
> verification?

Yes, it's true.

Regards,
-Gonglei