[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v4] virtio-crypto specification
|
From: |
Zeng, Xin |
|
Subject: |
Re: [Qemu-devel] [RFC v4] virtio-crypto specification |
|
Date: |
Mon, 25 Jul 2016 06:19:49 +0000 |
On Monday, July 25, 2016 9:38 AM Gonglei (Arei) wrote:
>
> Hi Xin,
>
>
> > -----Original Message-----
> > From: Zeng, Xin [mailto:address@hidden
> > Sent: Friday, July 22, 2016 1:31 PM
> > To: Gonglei (Arei); address@hidden;
> > address@hidden
> > Cc: Hanweidong (Randy); Stefan Hajnoczi; Cornelia Huck;
> > address@hidden; Lingli Deng; Jani Kokkonen; Luonengjun; Huangpeng
> > (Peter); Zhoujian (jay, Euler); chenshanxi 00222737; 'Ola
> > address@hidden'; 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:address@hidden
> > > > Sent: Friday, July 22, 2016 8:48 AM
> > > > To: Gonglei (Arei); address@hidden; qemu-
> > > address@hidden
> > > > Cc: Hanweidong (Randy); Stefan Hajnoczi; Cornelia Huck;
> > > > address@hidden; Lingli Deng; Jani Kokkonen; Luonengjun; Huangpeng
> > > > (Peter); Zhoujian (jay, Euler); chenshanxi 00222737; 'Ola
> > > > address@hidden'; 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. :)
>
> 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 <address@hidden>
> > > > >
> > > > > 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.
> Regards,
> -Gonglei