Hi Xin,
> -----Original Message----- > From: Zeng, Xin [mailto:xin.z...@intel.com] > Sent: Friday, July 22, 2016 1:31 PM > To: Gonglei (Arei); virtio-...@lists.oasis-open.org; qemu-devel@nongnu.org > Cc: Hanweidong (Randy); Stefan Hajnoczi; Cornelia Huck; m...@redhat.com; > Lingli Deng; Jani Kokkonen; Luonengjun; Huangpeng (Peter); Zhoujian (jay, > Euler); chenshanxi 00222737; 'Ola liljed...@arm.com'; Varun Sethi > Subject: RE: [RFC v4] virtio-crypto specification > > On Friday, July 22, 2016 10:53 AM Gonglei (Arei) wrote: > > > > Hi Xin, > > > > Thank you so much for your great comments. > > I agree with you almostly except some trivial detals. > > Please see my below replies. > > > > And I'll submit V5 next week, and you can finish the asym algos parts if you > > like. > > Let's co-work to finish the virtio-crypto spec, shall we? > > > > That's great. > > > Regards, > > -Gonglei > > > > > > > -----Original Message----- > > > From: Zeng, Xin [mailto:xin.z...@intel.com] > > > Sent: Friday, July 22, 2016 8:48 AM > > > To: Gonglei (Arei); virtio-...@lists.oasis-open.org; qemu- > > de...@nongnu.org > > > Cc: Hanweidong (Randy); Stefan Hajnoczi; Cornelia Huck; m...@redhat.com; > > > Lingli Deng; Jani Kokkonen; Luonengjun; Huangpeng (Peter); Zhoujian (jay, > > > Euler); chenshanxi 00222737; 'Ola liljed...@arm.com'; Varun Sethi > > > Subject: RE: [RFC v4] virtio-crypto specification > > > > > > On Sunday, June 26, 2016 5:35 PM, Gonglei (Arei) Wrote: > > > > Hi all, > > > > > > > > This is the specification (version 4) about a new virtio crypto device. > > > > > > > > > > In general, our comments around this proposal are listed below: > > > 1. Suggest to introduce crypto services into virtio crypto device. The > > services > > > currently defined are CIPHER, MAC, HASH, AEAD, KDF, ASYM, PRIMITIVE. > > > > Yes, I agree, whether DRBG/NDRBG are included in PRIMITIVE service or > > not? > > If not, we'd better add another separate service. > > Yes, I think we can add these two into PRIMITIVE services. > > > > > > 2. Suggest to define a unified crypto request format that is consisted of > > > general header + service specific request, Where 'general header' is for > > > all > > > crypto request, 'service specific request' is composed of > > > operation parameter + input data + output data in generally. > > > operation parameter is algorithm-specific parameters, > > > input data is the data should be operated , > > > output data is the "operation result + result buffer". > > > > > It makes sense. Good. > > > > > #define VIRTIO_CRYPTO_OPCODE(service, op) (((service)<<8) | (op)) > > > struct virtio_crypto_op_header { > > > #define VIRTIO_CRYPTO_CIPHER_ENCRYPT > > > VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_CIPHER, 0x00) > > > #define VIRTIO_CRYPTO_CIPHER_DECRYPT > > > VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_CIPHER, 0x01) > > > #define VIRTIO_CRYPTO_HASH > > > VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_HASH, 0x00) > > > #define VIRTIO_CRYPTO_MAC > > > VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_MAC, 0x00) > > > #define VIRTIO_CRYPTO_KDF > > > VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_KDF, 0x00) > > > #define VIRTIO_CRYPTO_ASYM_KEY_GEN > > > VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_ASYM, 0x00) > > > #define VIRTIO_CRYPTO_ASYM_KEY_EXCHG > > > VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_ASYM, 0x01) > > > #define VIRTIO_CRYPTO_ASYM_SIGN > > > VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_ASYM, 0x02) > > > #define VIRTIO_CRYPTO_ASYM_VERIFY > > > VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_ASYM, 0x03) > > > #define VIRTIO_CRYPTO_ASYM_ENCRYPT > > > VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_ASYM, 0x04) > > > #define VIRTIO_CRYPTO_ASYM_DECRYPT > > > VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_ASYM, 0x05) > > > #define VIRTIO_CRYPTO_AEAD_ENCRYPT > > > VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_AEAD, 0x00) > > > #define VIRTIO_CRYPTO_AEAD_DECRYPT > > > VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_AEAD, 0x01) > > > #define VIRTIO_CRYPTO_PRIMITIVE > > > VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_PRIMITIVE, 0x00) > > > u32 opcode; > > > u8 algo; /*service-specific algorithms*/ > > > u8 flag; /*control flag*/ > > > > We'd better add a U64 session_id property here for service-specific > > algorithms. > > > > Can we put session_id into parameter filed inside service-specific request? > For ASYM service, it doesn't need session_id. > And for HASH service, it might not need a session_id as well. > I don't think so, the function of session_id property is the same with algo property here, It's also service-specific algorithms. We don't know which session_id we could use if we put the field inside request for chain-algorithms (chain cipher and mac). For HASH service, I think we'd better keep corresponding with the mac hand cipher, using session for operation, which have the same functions and APIs too. > > > }; > > > > > > Take rsa_sign_request as example, > > > A rsa sign service specific request is defined as: > > > struct virtio_crypto_asym_rsa_sign_req{ > > > struct virtio_crypto_rsa_sign_para parameter; > > > struct virtio_crypto_rsa_sign_input idata; > > > struct virtio_crypto_rsa_sign_output odata; > > > }; > > > > > > A complete crypto service request is defined as: > > > struct virtio_crypto_op_data_req { > > > struct virtio_crypto_op_header header; > > > union { > > > struct virtio_crypto_asym_rsa_sign_req > > > rsa_sign_req; > > > /*other service request*/ > > > }u; > > > }; > > > > > I wanted to do this in fact. ;) > > > > > More detailed comments are embedded below: > > > > > > > Changes from v3: > > > > - Don't use enum is the spec but macros in specific structures. > > > > [Michael > & > > > > Stefan] > > > > - Add two complete structures for session creation and closing, so that > > > > the spec is clear on how to lay out the request. [Stefan] > > > > - Definite the crypto operation request with assigned structure, in > > > > this > > way, > > > > each data request only occupies *one entry* of the Vring descriptor > > table, > > > > which *improves* the *throughput* of data transferring. > > > > > > > > Changes from v2: > > > > - Reserve virtio device ID 20 for crypto device. [Cornelia] > > > > - Drop all feature bits, those capabilities are offered by the device > > > > all the > > > > time. [Stefan & Cornelia] > > > > - Add a new section 1.4.2 for driver requirements. [Stefan] > > > > - Use definite type definition instead of enum type in some structure. > > > > [Stefan] > > > > - Add virtio_crypto_cipher_alg definition. [Stefan] > > > > - Add a "Device requirements" section as using MUST. [Stefan] > > > > - Some grammar nits fixes and typo fixes. [Stefan & Cornelia] > > > > - Add one VIRTIO_CRYPTO_S_STARTED status for the driver as the flag > of > > > > virtio-crypto device started and can work now. > > > > > > > > Great thanks for Stefan and Cornelia! > > > > > > > > Changes from v1: > > > > - Drop the feature bit definition for each algorithm, and using config > > space > > > > instead [Cornelia] > > > > - Add multiqueue support and add corresponding feature bit > > > > - Update Encryption process and header definition > > > > - Add session operation process and add corresponding header > > description > > > > - Other better description in order to fit for virtio spec [Michael] > > > > - Some other trivial fixes. > > > > > > > > If you have any comments, please let me know, thanks :) > > > > > > > > > > > > Virtio-crypto device Spec > > > > Signed-off-by: > > > > Gonglei <arei.gong...@huawei.com> > > > > > > > > 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. > > > > The second queue is the control queue, which is used to create or > > destroy > > > > session for symmetric algorithms, and to control some advanced features > > in > > > > the future. > > > > 1.1 Device ID > > > > 20 > > > > 1.2 Virtqueues > > > > 0 dataq > > > > … > > > > N-1 dataq > > > > N controlq > > > > N is set by max_virtqueues (max_virtqueues >= 1). > > > > 1.3 Feature bits > > > > There are no feature bits (yet). > > > > 1.4 Device configuration layout > > > > Three driver-read-only configuration fields are currently defined. One > > read- > > > > only bit (for the device) is currently defined for the status field: > > > > VIRTIO_CRYPTO_S_HW_READY. One read-only bit (for the driver) is > > currently > > > > defined for the status field: VIRTIO_CRYPTO_S_STARTED. > > > > #define VIRTIO_CRYPTO_S_HW_READY (1 << 0) #define > > > > VIRTIO_CRYPTO_S_STARTED (1 << 1) > > > > > > > > The following driver-read-only field, max_virtqueues specifies the > > maximum > > > > number of data virtqueues (dataq1. . .dataqN) . > > > > struct virtio_crypto_config { > > > > le16 status; > > > > le16 max_virtqueues; > > > > le32 algorithms; > > > > }; > > > > > > > > The last driver-read-only field, algorithms specifies the algorithms > > > > which > > the > > > > device offered. 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_ASYM (1 << 1) > > > > > > Suggest to change the virtio_crypto_config structure to following > structure > > to > > > define detail algorithms that the device supports in device configuration > > field. > > > struct virtio_crypto_config { > > > le32 version; > > > le16 status; > > > le16 max_dataqueues; > > > #define VIRTIO_CRYPTO_S_CIPHER 0 /*cipher services*/ > > > #define VIRTIO_CRYPTO_S_HASH 1 /*hash service*/ > > > #define VIRTIO_CRYPTO_S_MAC 2 /*MAC(Message Authentication Codes) > > > service*/ > > > #define VIRTIO_CRYPTO_S_AEAD 3 /* AEAD(Authenticated Encryption > > with > > > Associated Data) service*/ > > > #define VIRTIO_CRYPTO_S_KDF 4 /*KDF(Key Derivation Functions) > > service*/ > > > #define VIRTIO_CRYPTO_S_ASYM 5 /*asymmetric service*/ > > > #define VIRTIO_CRYPTO_S_PRIMITIVE 6 /*Essential mathematics > > computing > > > service*/ > > > > I'd like to use s/ VIRTIO_CRYPTO_S_/ VIRTIO_CRYPTO_SERIVCE_/g, avoiding > > to > > conflict with VIRTIO_CRYPTO_SATAUS_ which all virtio spec always used. > > That's OK. > > > > > > le32 crypto_servcies; /*overall services mask*/ > > > /*detailed algorithms mask*/ > > > le32 cipher_algo_l; > > > le32 cipher_algo_h; > > > le32 hash_algo; > > > le32 mac_algo_l; > > > le32 mac_algo_h; > > > le32 asym_algo; > > > le32 kdf_algo; > > > le32 aead_algo; > > > le32 primitive_algo; > > > }; > > > > > > 15 bits are defined for cipher algorithms currently. > > > #define VIRTIO_CRYPTO_CIPHER_ARC4 0 > > > #define VIRTIO_CRYPTO_CIPHER_AES_ECB 1 > > > #define VIRTIO_CRYPTO_CIPHER_AES_CBC 2 > > > #define VIRTIO_CRYPTO_CIPHER_AES_CTR 3 > > > #define VIRTIO_CRYPTO_CIPHER_DES_ECB 6 > > > #define VIRTIO_CRYPTO_CIPHER_DES_CBC 7 > > > #define VIRTIO_CRYPTO_CIPHER_3DES_ECB 8 > > > #define VIRTIO_CRYPTO_CIPHER_3DES_CBC 9 > > > #define VIRTIO_CRYPTO_CIPHER_3DES_CTR 9 > > > #define VIRTIO_CRYPTO_CIPHER_KASUMI_F8 10 > > > #define VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2 11 > > > #define VIRTIO_CRYPTO_CIPHER_AES_F8 12 > > > #define VIRTIO_CRYPTO_CIPHER_AES_XTS 13 > > > #define VIRTIO_CRYPTO_CIPHER_ZUC_EEA3 14 > > > > > > 12 bits are defined for Hash algorithms currently. > > > #define VIRTIO_CRYPTO_HASH_MD5 0 > > > #define VIRTIO_CRYPTO_HASH_SHA1 1 > > > #define VIRTIO_CRYPTO_HASH_SHA_224 2 > > > #define VIRTIO_CRYPTO_HASH_SHA_256 3 > > > #define VIRTIO_CRYPTO_HASH_SHA_384 4 > > > #define VIRTIO_CRYPTO_HASH_SHA_512 5 > > > #define VIRTIO_CRYPTO_HASH_SHA3_224 6 > > > #define VIRTIO_CRYPTO_HASH_SHA3_256 7 > > > #define VIRTIO_CRYPTO_HASH_SHA3_384 8 > > > #define VIRTIO_CRYPTO_HASH_SHA3_512 9 > > > #define VIRTIO_CRYPTO_HASH_SHA3_SHAKE128 10 > > > #define VIRTIO_CRYPTO_HASH_SHA3_SHAKE256 11 > > > > > > 15 bits are defined for MAC algorithms currently > > > #define VIRTIO_CRYPTO_MAC_HMAC_MD5 0 > > > #define VIRTIO_CRYPTO_MAC_HMAC_SHA1 1 > > > #define VIRTIO_CRYPTO_MAC_HMAC_SHA_224 2 > > > #define VIRTIO_CRYPTO_MAC_HMAC_SHA_256 3 > > > #define VIRTIO_CRYPTO_MAC_HMAC_SHA_384 4 > > > #define VIRTIO_CRYPTO_MAC_HMAC_SHA_512 5 > > > #define VIRTIO_CRYPTO_MAC_CMAC_3DES 24 > > > > Why not 6 here? I'd like to keep increasing steadily, we can extend > > the value when some other algorithms are supported. > > The intension is to keep the same kind of algorithms into continuous bits > as possible as. > It's okay for your intension, but we can't predict how many HMACs or CMACs we will support, maybe 30, 40 in the future. So I think the reserve maybe not fit your initial purpose. It's better keeping increasing steadily IMHO. Regards, -Gonglei
