qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC v4] virtio-crypto specification


From: Gonglei (Arei)
Subject: Re: [Qemu-devel] [RFC v4] virtio-crypto specification
Date: Mon, 25 Jul 2016 01:38:19 +0000

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

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. 

Regards,
-Gonglei

reply via email to

[Prev in Thread] Current Thread [Next in Thread]