Hi Cornelia, Thank you so much for your review :)
> From: Cornelia Huck [mailto:cornelia.h...@de.ibm.com] > 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)" <arei.gong...@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. > 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
