> -----Original Message-----
> From: Zeng, Xin [mailto:xin.z...@intel.com]
> Sent: Monday, July 25, 2016 2:20 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 Monday, July 25, 2016 9:38 AM Gonglei (Arei) wrote:
> >
> > 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).
>
> I do think alg is necessary for all crypto services, but some services use a
> session
> to maintain the algorithm information, and some don't because they needn't
> session, e.g, asymmetric serves.
> Anyway, I think it's ok to maintain this field in this header, but we should
> clarify
> how to fill this header clearly. :)
>
Sure.
> >
> > 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.
> >
>
> Why not try to make it as possible as? I don't see any harm here.
>
Ok, let's make it.
Regards,
Gonglei
