Re: [RFC v2] virtio-crypto specification

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

On Tue, 5 Apr 2016 09:14:09 +0000
"Gonglei (Arei)" <arei.gonglei@huawei.com> 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.

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

> VIRTIO_CRYPTO_F_MQ (1)
> 	Device supports multiqueue to encrypt and decrypt.

As commented by Stefan, just drop this.

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

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

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

s/On/One/

> #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?

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

Do you want to provide indications in this field beyond sym vs. asym,
e.g. which strength is available?

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

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

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

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

> •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/ ?

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

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