All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [RFC v4] virtio-crypto specification
@ 2016-06-26  9:35 Gonglei (Arei)
  2016-07-08  7:27 ` Gonglei (Arei)
  2016-07-22  0:48 ` [Qemu-devel] " Zeng, Xin
  0 siblings, 2 replies; 10+ messages in thread
From: Gonglei (Arei) @ 2016-06-26  9:35 UTC (permalink / raw)
  To: virtio-dev, qemu-devel, Gonglei (Arei)
  Cc: Hanweidong (Randy),
	Stefan Hajnoczi, Cornelia Huck, mst, Lingli Deng, Jani Kokkonen,
	Luonengjun, Huangpeng (Peter), Zhoujian (jay, Euler),
	chenshanxi 00222737, 'Ola Liljedahl@arm.com',
	Varun Sethi

Hi all,

This is the specification (version 4) about a new virtio crypto device. 

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.gonglei@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)
1.4.1	Device Requirements: Device configuration layout
The device MUST set max_virtqueues to between 1 and 65535 inclusive.

The device SHOULD set status according to the status of the hardware-backed implementation.

The device MUST set algorithms according to the algorithms which the device offered.
1.4.2	Driver Requirements: Device configuration layout
The driver MUST read the ready status from the bottom bit of status to check whether the hardware-backed implementation is ready or not.
The driver MUST read the algorithms to discover the algorithms which the device supports.
1.5	Device Initialization
A driver would perform a typical initialization routine like so:
1. Identify and initialize data virtqueue, up to max_virtqueues.
2. Identify the control virtqueue.
3. Identify the ready status of hardware-backend comes from the bottom bit of status.
4. Read the supported algorithms from bits of algorithms. 
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).
•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.

The controlq virtqueue is used to control session operations, including creation or close. The request is preceded by a header:
struct virtio_crypto_sym_ctlhdr {
    /* control type  */
    u8 type;
};
Two bits are currently defined for the control header type: 
#define VIRTIO_CRYPTO_CTRL_CREATE_SESSION  1
#define VIRTIO_CRYPTO_CTRL_CLOSE_SESSION  2

1.6.1.1 Session creation operation
A request of creating a session including the following information:
struct virtio_crypto_sym_session_creation {
	struct virtio_crypto_sym_ctlhdr   ctlhdr;  /* OUT */
	struct virtio_crypto_sym_session_op  session_op;  /* OUT */
	struct virtio_crypto_sym_session_op_inhdr  inhdr;  /* IN */
};
The details of specific structure, including struct virtio_crypto_sym_session_op and struct virtio_crypto_sym_session_op_inhdr  are defined by the following sections.
1.6.1.1.1 Driver Requirements: Session creation operation
The driver MUST set the control type with VIRTIO_CRYPTO_CTRL_CREATE_SESSION before the request is preceded by an operation header when executing session creation:
typedef struct virtio_crypto_sym_session_op {
/**< No operation */
#define VIRTIO_CRYPTO_SYM_OP_NONE    0 
/**< Cipher only operation on the data */
#define VIRTIO_CRYPTO_SYM_OP_CIPHER    1
/**< Hash only operation on the data */
#define VIRTIO_CRYPTO_SYM_OP_HASH     2
/**< Chain any cipher with any hash operation */
#define VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING    3
	u8 op_type; /* Operation type */
    virtio_crypto_sym_cipher_t cipher_setup_data;
	virtio_crypto_sym_hash_t hash_setup_data;
/* Perform the hash operation followed by the cipher operation */
#define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_HASH_THEN_CIPHER	1
/* Perform the cipher operation followed by the hash operation */
#define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_CIPHER_THEN_HASH	2
    u8 alg_chain_order;
} virtio_crypto_sym_session_op_t;
And the structures definition details are:
typedef struct virtio_crypto_sym_hash_auth_mode {
	/* length of authenticated key */
 	le32 auth_key_len;
 	/* The length of the additional authenticated data (AAD) in bytes */
 	le32 aad_len;
} virtio_crypto_sym_hash_auth_mode_t;

typedef struct virtio_crypto_sym_cipher {
/* Option to not do any cryptography */
#define VIRTIO_CRYPTO_NO_CIPHER	0
#define VIRTIO_CRYPTO_CIPHER_DES	1
#define VIRTIO_CRYPTO_CIPHER_DES_CBC	2
#define VIRTIO_CRYPTO_CIPHER_DES3	3
#define VIRTIO_CRYPTO_CIPHER_DES3_CBC	4
#define VIRTIO_CRYPTO_CIPHER_AES	5
#define VIRTIO_CRYPTO_CIPHER_AES_CBC	6
#define VIRTIO_CRYPTO_CIPHER_KASUMI_F8	7
    le32 alg;  /* cipher algorithm type (ie. aes-cbc ) */
    /* length of key */
    le32 keylen;
#define VIRTIO_CRYPTO_DECRYPT	0
#define VIRTIO_CRYPTO_ENCRYPT	1
	u8 op; /* encrypt or decrypt */
} virtio_crypto_sym_cipher_t;

typedef struct virtio_crypto_sym_hash {
/* Option to not do any hash */
#define VIRTIO_CRYPTO_NO_HASH	0
#define VIRTIO_CRYPTO_HASH_MD5	1
#define VIRTIO_CRYPTO_HASH_SHA1	2
#define VIRTIO_CRYPTO_HASH_SHA1_96	3
#define VIRTIO_CRYPTO_HASH_SHA224	4
#define VIRTIO_CRYPTO_HASH_SHA256	5
#define VIRTIO_CRYPTO_HASH_SHA384	6
#define VIRTIO_CRYPTO_HASH_SHA512	7
#define VIRTIO_CRYPTO_HASH_AES_XCBC	   8
#define VIRTIO_CRYPTO_HASH_AES_XCBC_96  9
#define VIRTIO_CRYPTO_HASH_KASUMI_F9	10
    le32 hash_alg;  /* hash algorithm type */
#define VIRTIO_CRYPTO_SYM_HASH_MODE_PLAIN	1
#define VIRTIO_CRYPTO_SYM_HASH_MODE_AUTH	2	
#define VIRTIO_CRYPTO_SYM_HASH_MODE_NESTED	3
u8 hash_mode; /* mode of hash operation, including authenticated/plain/nested hash */
	/* hash result length */
    le32 hash_result_len;
    virtio_crypto_sym_hash_auth_mode_t auth_mode_setup_data;
} virtio_crypto_sym_hash_t;
The driver MUST set the control type with VIRTIO_CRYPTO_CTRL_CLOSE_SESSION and pass the session_id to the device when executing session close.
1.6.1.1.2 Device Requirements: Session creation operation
The device MUST return a session identifier to the driver when the device finishes the processing of session close. The session creation request MUST end by a tailer:
typedef struct virtio_crypto_sym_session_op_inhdr {
	u8     status;
	le64    session_id;
} virtio_crypto_sym_session_op_inhdr_t;
Both status and session_id are written by the device: either VIRTIO_CRYPTO_CTRL_OK for success, VIRTIO_CRYPTO_CTRL_ERR for creation failed or device error.
#define VIRTIO_CRYPTO_CTRL_OK	0
#define VIRTIO_CRYPTO_CTRL_ERR	1
1.6.1.2 Session closing operation
A request of closing a session including the following information:
struct virtio_crypto_sym_session_creation {
	struct virtio_crypto_sym_ctlhdr   ctlhdr;  /* OUT */
	le64  session_id;  /* OUT */
	u8 status;  /* IN */
};
1.6.1.2.1 Driver Requirements: Session closing operation
The driver MUST set the control type with VIRTIO_CRYPTO_CTRL_CLOSE_SESSION, and the session_id MUST be a valid value which assigned by the device when a session was created. 
1.6.1.2.2 Device Requirements: Session closing operation
Status is written by the device: either VIRTIO_CRYPTO_CTRL_OK for success, VIRTIO_CRYPTO_CTRL_ERR for creation failed or device error.
#define VIRTIO_CRYPTO_CTRL_OK	0
#define VIRTIO_CRYPTO_CTRL_ERR	1
1.6.2	Encryption operation
1.6.2.1 Driver Requirements: Encryption operation
The encryption and decryption requests and the corresponding results are transmitted by placing them in dataq. The symmetric algorithms requests are preceded by a header:
struct virtio_crypto_sym_op_hdr {
	/* the backend returned session identifier */
	le64 session_id;
	/* length of initial vector */
	le32 iv_len;
	/* iv offset in the whole crypto data memory */
	le32 iv_offset;
	/* length of additional auth data */
	le32 auth_len;
	/* additional auth data offset in the whole crypto data memory */
	le32 additional_auth_offset;
	/* cipher start source offest */
	le32 cipher_start_src_offset;
	le32 len_to_cipher;
	/* hash start source offest */
	le32 hash_start_src_offset;
	le32 len_to_hash;
	/* length of source data */
	le32 source_len;
} ;
The encryption request MUST end by a tailer:
typedef struct virtio_crypto_sym_op_inhdr {
	u8     status;
} virtio_crypto_sym _op_inhdr_t;
The specific content of symmetric algorithms requests SHOULD be same as below:
struct virtio_crypto_sym_op_data {
	struct virtio_crypto_sym_op_hdr  hdr_info;
	le64 iv_addr; /* iv guest address */ 
	le64 auth_data_addr; /* associated data guest address */ 
	le64 src_data_addr; /* source data guest address */ 
	le64 dst_data_addr; /* destination data guest address */
	le64 digest_result_addr; /* digest result guest address */
	le64 inhdr_addr; /* in-header guest address */ 
};
In this way, each data request only occupies one entry of the Vring descriptor table, which improves the throughput of data transferring.
1.6.2.2 Device Requirements: Encryption operation
The struct virtio_crypto_sym_op_inhdr’s status byte is written by the device: either VIRTIO_CRYPTO_S_OK for success, VIRTIO_CRYPTO_S_ERR for device or driver error, VIRTIO_CRYPTO_S_BADMSG for verification failed when decrypt AEAD algorithms:
#define VIRTIO_CRYPTO_S_OK    0
#define VIRTIO_CRYPTO_S_ERR    1
#define VIRTIO_CRYPTO_S_BADMSG    2

1.6.2.3 Steps of encryption Operation
Step1: Create a session:
1.	The driver fills out the context message, including algorithm name, key, keylen etc;
2.	The driver sends a context message to the backend device by controlq;
3.	The device creates 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 fills out the encrypt requests;
2.	Put the requests into dataq and kick the virtqueue;
3.	The device executes the encryption operation according the requests' arguments;
4.	The device returns the encryption result to the driver by dataq;
5.	The driver callback handle the result and over.

Note: the driver MAY support both synchronous and asynchronous encryption. Then the performance is poor in synchronous operation because frequent context switching and virtualization overhead. The driver SHOULD by preference use asynchronous encryption.
1.6.3	Decryption Operation
The decryption process is the same with encryption.
1.6.3.1 Device Requirements: Decryption operation
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) MUST be returned the driver.

Regards,
-Gonglei





^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [RFC v4] virtio-crypto specification
  2016-06-26  9:35 [Qemu-devel] [RFC v4] virtio-crypto specification Gonglei (Arei)
@ 2016-07-08  7:27 ` Gonglei (Arei)
  2016-07-14 12:16   ` Stefan Hajnoczi
  2016-07-14 12:21   ` [Qemu-devel] [virtio-dev] " Stefan Hajnoczi
  2016-07-22  0:48 ` [Qemu-devel] " Zeng, Xin
  1 sibling, 2 replies; 10+ messages in thread
From: Gonglei (Arei) @ 2016-07-08  7:27 UTC (permalink / raw)
  To: Gonglei (Arei), virtio-dev, qemu-devel
  Cc: Hanweidong (Randy),
	Stefan Hajnoczi, Cornelia Huck, mst, Lingli Deng, Jani Kokkonen,
	Huangpeng (Peter), Zhoujian (jay, Euler),
	'Ola Liljedahl@arm.com',
	Varun Sethi

Ping...

I'd like to know what's the process of a new virtio device can be merged? 
Anybody can tell me? Thanks a lot.

Taking Virtio-gpu as an example, the virtio-gpu had been merged in Linux kernel
And Qemu communities, but the corresponding virtio-gpu spec hadn't been merged
yet. Why was that? Am I missing some rules? Thanks. 


Regards,
-Gonglei


> -----Original Message-----
> From: Gonglei (Arei)
> Sent: Sunday, June 26, 2016 5:35 PM
> To: virtio-dev@lists.oasis-open.org; qemu-devel@nongnu.org; Gonglei (Arei)
> Cc: Hanweidong (Randy); Stefan Hajnoczi; Cornelia Huck; mst@redhat.com;
> Lingli Deng; Jani Kokkonen; Luonengjun; Huangpeng (Peter); Zhoujian (jay,
> Euler); chenshanxi 00222737; 'Ola Liljedahl@arm.com'; Varun Sethi
> Subject: [RFC v4] virtio-crypto specification
> 
> Hi all,
> 
> This is the specification (version 4) about a new virtio crypto device.
> 
> 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.gonglei@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)
> 1.4.1	Device Requirements: Device configuration layout
> The device MUST set max_virtqueues to between 1 and 65535 inclusive.
> 
> The device SHOULD set status according to the status of the hardware-backed
> implementation.
> 
> The device MUST set algorithms according to the algorithms which the device
> offered.
> 1.4.2	Driver Requirements: Device configuration layout
> The driver MUST read the ready status from the bottom bit of status to check
> whether the hardware-backed implementation is ready or not.
> The driver MUST read the algorithms to discover the algorithms which the
> device supports.
> 1.5	Device Initialization
> A driver would perform a typical initialization routine like so:
> 1. Identify and initialize data virtqueue, up to max_virtqueues.
> 2. Identify the control virtqueue.
> 3. Identify the ready status of hardware-backend comes from the bottom bit of
> status.
> 4. Read the supported algorithms from bits of algorithms.
> 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).
> •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.
> 
> The controlq virtqueue is used to control session operations, including creation
> or close. The request is preceded by a header:
> struct virtio_crypto_sym_ctlhdr {
>     /* control type  */
>     u8 type;
> };
> Two bits are currently defined for the control header type:
> #define VIRTIO_CRYPTO_CTRL_CREATE_SESSION  1
> #define VIRTIO_CRYPTO_CTRL_CLOSE_SESSION  2
> 
> 1.6.1.1 Session creation operation
> A request of creating a session including the following information:
> struct virtio_crypto_sym_session_creation {
> 	struct virtio_crypto_sym_ctlhdr   ctlhdr;  /* OUT */
> 	struct virtio_crypto_sym_session_op  session_op;  /* OUT */
> 	struct virtio_crypto_sym_session_op_inhdr  inhdr;  /* IN */
> };
> The details of specific structure, including struct virtio_crypto_sym_session_op
> and struct virtio_crypto_sym_session_op_inhdr  are defined by the following
> sections.
> 1.6.1.1.1 Driver Requirements: Session creation operation
> The driver MUST set the control type with
> VIRTIO_CRYPTO_CTRL_CREATE_SESSION before the request is preceded by an
> operation header when executing session creation:
> typedef struct virtio_crypto_sym_session_op {
> /**< No operation */
> #define VIRTIO_CRYPTO_SYM_OP_NONE    0
> /**< Cipher only operation on the data */
> #define VIRTIO_CRYPTO_SYM_OP_CIPHER    1
> /**< Hash only operation on the data */
> #define VIRTIO_CRYPTO_SYM_OP_HASH     2
> /**< Chain any cipher with any hash operation */
> #define VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING    3
> 	u8 op_type; /* Operation type */
>     virtio_crypto_sym_cipher_t cipher_setup_data;
> 	virtio_crypto_sym_hash_t hash_setup_data;
> /* Perform the hash operation followed by the cipher operation */
> #define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_HASH_THEN_CIPHER
> 	1
> /* Perform the cipher operation followed by the hash operation */
> #define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_CIPHER_THEN_HASH
> 	2
>     u8 alg_chain_order;
> } virtio_crypto_sym_session_op_t;
> And the structures definition details are:
> typedef struct virtio_crypto_sym_hash_auth_mode {
> 	/* length of authenticated key */
>  	le32 auth_key_len;
>  	/* The length of the additional authenticated data (AAD) in bytes */
>  	le32 aad_len;
> } virtio_crypto_sym_hash_auth_mode_t;
> 
> typedef struct virtio_crypto_sym_cipher {
> /* Option to not do any cryptography */
> #define VIRTIO_CRYPTO_NO_CIPHER	0
> #define VIRTIO_CRYPTO_CIPHER_DES	1
> #define VIRTIO_CRYPTO_CIPHER_DES_CBC	2
> #define VIRTIO_CRYPTO_CIPHER_DES3	3
> #define VIRTIO_CRYPTO_CIPHER_DES3_CBC	4
> #define VIRTIO_CRYPTO_CIPHER_AES	5
> #define VIRTIO_CRYPTO_CIPHER_AES_CBC	6
> #define VIRTIO_CRYPTO_CIPHER_KASUMI_F8	7
>     le32 alg;  /* cipher algorithm type (ie. aes-cbc ) */
>     /* length of key */
>     le32 keylen;
> #define VIRTIO_CRYPTO_DECRYPT	0
> #define VIRTIO_CRYPTO_ENCRYPT	1
> 	u8 op; /* encrypt or decrypt */
> } virtio_crypto_sym_cipher_t;
> 
> typedef struct virtio_crypto_sym_hash {
> /* Option to not do any hash */
> #define VIRTIO_CRYPTO_NO_HASH	0
> #define VIRTIO_CRYPTO_HASH_MD5	1
> #define VIRTIO_CRYPTO_HASH_SHA1	2
> #define VIRTIO_CRYPTO_HASH_SHA1_96	3
> #define VIRTIO_CRYPTO_HASH_SHA224	4
> #define VIRTIO_CRYPTO_HASH_SHA256	5
> #define VIRTIO_CRYPTO_HASH_SHA384	6
> #define VIRTIO_CRYPTO_HASH_SHA512	7
> #define VIRTIO_CRYPTO_HASH_AES_XCBC	   8
> #define VIRTIO_CRYPTO_HASH_AES_XCBC_96  9
> #define VIRTIO_CRYPTO_HASH_KASUMI_F9	10
>     le32 hash_alg;  /* hash algorithm type */
> #define VIRTIO_CRYPTO_SYM_HASH_MODE_PLAIN	1
> #define VIRTIO_CRYPTO_SYM_HASH_MODE_AUTH	2
> #define VIRTIO_CRYPTO_SYM_HASH_MODE_NESTED	3
> u8 hash_mode; /* mode of hash operation, including
> authenticated/plain/nested hash */
> 	/* hash result length */
>     le32 hash_result_len;
>     virtio_crypto_sym_hash_auth_mode_t auth_mode_setup_data;
> } virtio_crypto_sym_hash_t;
> The driver MUST set the control type with
> VIRTIO_CRYPTO_CTRL_CLOSE_SESSION and pass the session_id to the device
> when executing session close.
> 1.6.1.1.2 Device Requirements: Session creation operation
> The device MUST return a session identifier to the driver when the device
> finishes the processing of session close. The session creation request MUST
> end by a tailer:
> typedef struct virtio_crypto_sym_session_op_inhdr {
> 	u8     status;
> 	le64    session_id;
> } virtio_crypto_sym_session_op_inhdr_t;
> Both status and session_id are written by the device: either
> VIRTIO_CRYPTO_CTRL_OK for success, VIRTIO_CRYPTO_CTRL_ERR for
> creation failed or device error.
> #define VIRTIO_CRYPTO_CTRL_OK	0
> #define VIRTIO_CRYPTO_CTRL_ERR	1
> 1.6.1.2 Session closing operation
> A request of closing a session including the following information:
> struct virtio_crypto_sym_session_creation {
> 	struct virtio_crypto_sym_ctlhdr   ctlhdr;  /* OUT */
> 	le64  session_id;  /* OUT */
> 	u8 status;  /* IN */
> };
> 1.6.1.2.1 Driver Requirements: Session closing operation
> The driver MUST set the control type with
> VIRTIO_CRYPTO_CTRL_CLOSE_SESSION, and the session_id MUST be a valid
> value which assigned by the device when a session was created.
> 1.6.1.2.2 Device Requirements: Session closing operation
> Status is written by the device: either VIRTIO_CRYPTO_CTRL_OK for success,
> VIRTIO_CRYPTO_CTRL_ERR for creation failed or device error.
> #define VIRTIO_CRYPTO_CTRL_OK	0
> #define VIRTIO_CRYPTO_CTRL_ERR	1
> 1.6.2	Encryption operation
> 1.6.2.1 Driver Requirements: Encryption operation
> The encryption and decryption requests and the corresponding results are
> transmitted by placing them in dataq. The symmetric algorithms requests are
> preceded by a header:
> struct virtio_crypto_sym_op_hdr {
> 	/* the backend returned session identifier */
> 	le64 session_id;
> 	/* length of initial vector */
> 	le32 iv_len;
> 	/* iv offset in the whole crypto data memory */
> 	le32 iv_offset;
> 	/* length of additional auth data */
> 	le32 auth_len;
> 	/* additional auth data offset in the whole crypto data memory */
> 	le32 additional_auth_offset;
> 	/* cipher start source offest */
> 	le32 cipher_start_src_offset;
> 	le32 len_to_cipher;
> 	/* hash start source offest */
> 	le32 hash_start_src_offset;
> 	le32 len_to_hash;
> 	/* length of source data */
> 	le32 source_len;
> } ;
> The encryption request MUST end by a tailer:
> typedef struct virtio_crypto_sym_op_inhdr {
> 	u8     status;
> } virtio_crypto_sym _op_inhdr_t;
> The specific content of symmetric algorithms requests SHOULD be same as
> below:
> struct virtio_crypto_sym_op_data {
> 	struct virtio_crypto_sym_op_hdr  hdr_info;
> 	le64 iv_addr; /* iv guest address */
> 	le64 auth_data_addr; /* associated data guest address */
> 	le64 src_data_addr; /* source data guest address */
> 	le64 dst_data_addr; /* destination data guest address */
> 	le64 digest_result_addr; /* digest result guest address */
> 	le64 inhdr_addr; /* in-header guest address */
> };
> In this way, each data request only occupies one entry of the Vring descriptor
> table, which improves the throughput of data transferring.
> 1.6.2.2 Device Requirements: Encryption operation
> The struct virtio_crypto_sym_op_inhdr’s status byte is written by the device:
> either VIRTIO_CRYPTO_S_OK for success, VIRTIO_CRYPTO_S_ERR for device or
> driver error, VIRTIO_CRYPTO_S_BADMSG for verification failed when decrypt
> AEAD algorithms:
> #define VIRTIO_CRYPTO_S_OK    0
> #define VIRTIO_CRYPTO_S_ERR    1
> #define VIRTIO_CRYPTO_S_BADMSG    2
> 
> 1.6.2.3 Steps of encryption Operation
> Step1: Create a session:
> 1.	The driver fills out the context message, including algorithm name, key,
> keylen etc;
> 2.	The driver sends a context message to the backend device by controlq;
> 3.	The device creates 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 fills out the encrypt requests;
> 2.	Put the requests into dataq and kick the virtqueue;
> 3.	The device executes the encryption operation according the requests'
> arguments;
> 4.	The device returns the encryption result to the driver by dataq;
> 5.	The driver callback handle the result and over.
> 
> Note: the driver MAY support both synchronous and asynchronous encryption.
> Then the performance is poor in synchronous operation because frequent
> context switching and virtualization overhead. The driver SHOULD by
> preference use asynchronous encryption.
> 1.6.3	Decryption Operation
> The decryption process is the same with encryption.
> 1.6.3.1 Device Requirements: Decryption operation
> 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) MUST be
> returned the driver.
> 
> Regards,
> -Gonglei
> 
> 
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [RFC v4] virtio-crypto specification
  2016-07-08  7:27 ` Gonglei (Arei)
@ 2016-07-14 12:16   ` Stefan Hajnoczi
  2016-07-14 12:21   ` [Qemu-devel] [virtio-dev] " Stefan Hajnoczi
  1 sibling, 0 replies; 10+ messages in thread
From: Stefan Hajnoczi @ 2016-07-14 12:16 UTC (permalink / raw)
  To: Gonglei (Arei)
  Cc: virtio-dev, qemu-devel, 'Ola Liljedahl@arm.com',
	mst, Hanweidong (Randy), Huangpeng (Peter),
	Stefan Hajnoczi, Zhoujian (jay, Euler),
	Cornelia Huck, Varun Sethi, Jani Kokkonen, Lingli Deng

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]

On Fri, Jul 08, 2016 at 07:27:59AM +0000, Gonglei (Arei) wrote:
> I'd like to know what's the process of a new virtio device can be merged? 
> Anybody can tell me? Thanks a lot.
> 
> Taking Virtio-gpu as an example, the virtio-gpu had been merged in Linux kernel
> And Qemu communities, but the corresponding virtio-gpu spec hadn't been merged
> yet. Why was that? Am I missing some rules? Thanks. 

That's the wrong way around and should be avoided.  It would be bad if
guests started running with drivers that don't implement the same
specification as QEMU.

Therefore the virtio spec should be merged first.  Maybe the Linux
driver can be merged under staging or marked experimental, but it should
not be considered stable before the spec has been accepted by the VIRTIO
Technical Committee.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [virtio-dev] RE: [RFC v4] virtio-crypto specification
  2016-07-08  7:27 ` Gonglei (Arei)
  2016-07-14 12:16   ` Stefan Hajnoczi
@ 2016-07-14 12:21   ` Stefan Hajnoczi
  1 sibling, 0 replies; 10+ messages in thread
From: Stefan Hajnoczi @ 2016-07-14 12:21 UTC (permalink / raw)
  To: Gonglei (Arei)
  Cc: virtio-dev, qemu-devel, Hanweidong (Randy),
	Cornelia Huck, mst, Lingli Deng, Jani Kokkonen, Huangpeng (Peter),
	Zhoujian (jay, Euler), 'Ola Liljedahl@arm.com',
	Varun Sethi

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

On Fri, Jul 08, 2016 at 07:27:59AM +0000, Gonglei (Arei) wrote:
> Ping...
> 
> I'd like to know what's the process of a new virtio device can be merged? 
> Anybody can tell me? Thanks a lot.
> 
> Taking Virtio-gpu as an example, the virtio-gpu had been merged in Linux kernel
> And Qemu communities, but the corresponding virtio-gpu spec hadn't been merged
> yet. Why was that? Am I missing some rules? Thanks. 

That's the wrong way around and should be avoided.  It would be bad if
guests started running with drivers that don't implement the same
specification as QEMU.

Therefore the virtio spec should be merged first.  Maybe the Linux
driver can be merged under staging or marked experimental, but it should
not be considered stable before the spec has been accepted by the VIRTIO
Technical Committee.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [RFC v4] virtio-crypto specification
  2016-06-26  9:35 [Qemu-devel] [RFC v4] virtio-crypto specification Gonglei (Arei)
  2016-07-08  7:27 ` Gonglei (Arei)
@ 2016-07-22  0:48 ` Zeng, Xin
  2016-07-22  2:52   ` Gonglei (Arei)
  1 sibling, 1 reply; 10+ messages in thread
From: Zeng, Xin @ 2016-07-22  0:48 UTC (permalink / raw)
  To: 'Gonglei (Arei)', virtio-dev, qemu-devel
  Cc: Hanweidong (Randy),
	Stefan Hajnoczi, Cornelia Huck, mst, Lingli Deng, Jani Kokkonen,
	Luonengjun, Huangpeng (Peter), Zhoujian (jay, Euler),
	chenshanxi 00222737, 'Ola Liljedahl@arm.com',
	Varun Sethi

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

#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*/
};

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;
};

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.gonglei@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*/
    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
#define VIRTIO_CRYPTO_MAC_CMAC_AES                 25
#define VIRTIO_CRYPTO_MAC_CMAC_KASUMI_F9             26
#define VIRTIO_CRYPTO_MAC_CMAC_ SNOW3G_UIA2        27
#define VIRTIO_CRYPTO_MAC_GMAC_AES                 40
#define VIRTIO_CRYPTO_MAC_GMAC_TWOFISH             41
#define VIRTIO_CRYPTO_MAC_CBCMAC_AES               48
#define VIRTIO_CRYPTO_MAC_CBCMAC_KASUMI_F9         49
#define VIRTIO_CRYPTO_MAC_XCBC_AES                 52

5 bits are defined for asymmetric algorithms currently.
#define VIRTIO_CRYPTO_ASYM_RSA                  0
#define VIRTIO_CRYPTO_ASYM_DSA                  1
#define VIRTIO_CRYPTO_ASYM_DH                   2
#define VIRTIO_CRYPTO_ASYM_ECDSA                3
#define VIRTIO_CRYPTO_ASYM_ECDH                 4

7 bits are defined for KDF algorithms currently.
#define VIRTIO_CRYPTO_KDF_NONE 0
#define VIRTIO_CRYPTO_KDF_ANSI-X9.42 1
#define VIRTIO_CRYPTO_KDF_ANSI-X9.62 2
#define VIRTIO_CRYPTO_KDF_IKEv2 3
#define VIRTIO_CRYPTO_KDF_TLS_1.0 4
#define VIRTIO_CRYPTO_KDF_TLS_1.2 5
#define VIRTIO_CRYPTO_KDF_NIST-800-56-Concatenation 6

2 bits are defined for AEAD algorithms currently.
#define VIRTIO_CRYPTO_AEAD_GCM   0
#define VIRTIO_CRYPTO_AEAD_CCM    1

4 bits are defined for PRIMITIVE algorithms currently
#define VIRTIO_CRYPTO_PRIMITIVE_LNMOD_EXP               0
#define VIRTIO_CRYPTO_PRIMITIVE_LNMOD_INV               1
#define VIRTIO_CRYPTO_PRIMITIVE_ ECPOINT_MULTIPLY  2
#define VIRTIO_CRYPTO_PRIMITIVE_ ECPOINT_VERIFY  3

> 1.4.1	Device Requirements: Device configuration layout

Add "The device MUST set the version in version filed."

> The device MUST set max_virtqueues to between 1 and 65535 inclusive.
> 
> The device SHOULD set status according to the status of the hardware-
> backed implementation.
> 
> The device MUST set algorithms according to the algorithms which the device
> offered.
> 1.4.2	Driver Requirements: Device configuration layout
> The driver MUST read the ready status from the bottom bit of status to check
> whether the hardware-backed implementation is ready or not.

Add "The driver MAY read max_dataqueues field to discover how many data queues 
the device supports."

> The driver MUST read the algorithms to discover the algorithms which the
> device supports.

Because of the presence of overall service field and detail algorithms field,  suggest 
To change this to "
The driver MUST read crypto_services field to discover which services the device is 
able to offer.
The driver MUST read detail algorithms field according to crypto_services field.
"

> 1.5	Device Initialization
> A driver would perform a typical initialization routine like so:
> 1. Identify and initialize data virtqueue, up to max_virtqueues.
> 2. Identify the control virtqueue.
> 3. Identify the ready status of hardware-backend comes from the bottom bit
> of status.
> 4. Read the supported algorithms from bits of algorithms.
> 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).
> •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.
> 
> The controlq virtqueue is used to control session operations, including
> creation or close. The request is preceded by a header:
> struct virtio_crypto_sym_ctlhdr {
>     /* control type  */
>     u8 type;
> };
> Two bits are currently defined for the control header type:
> #define VIRTIO_CRYPTO_CTRL_CREATE_SESSION  1 #define
> VIRTIO_CRYPTO_CTRL_CLOSE_SESSION  2
> 

Suggest to change virtio_crypto_sym_ctlhdr structure to the a general 
header below, and define a unified control request  structure to keep 
consistent with crypto service request format.
struct virtio_crypto_ctrl_header{
#define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION		VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_CIPHER, 0x02)
#define VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION		VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_CIPHER, 0x03)
#define VIRTIO_CRYPTO_MAC_CREATE_SESSION		VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_MAC, 0x02)
#define VIRTIO_CRYPTO_MAC_DESTROY_SESSION		VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_MAC, 0x03)
	u32 opcode;
	u8 algo;
	u8 flag;
};

> 1.6.1.1 Session creation operation
> A request of creating a session including the following information:
> struct virtio_crypto_sym_session_creation {
> 	struct virtio_crypto_sym_ctlhdr   ctlhdr;  /* OUT */
> 	struct virtio_crypto_sym_session_op  session_op;  /* OUT */
> 	struct virtio_crypto_sym_session_op_inhdr  inhdr;  /* IN */ }; The
> details of specific structure, including struct virtio_crypto_sym_session_op
> and struct virtio_crypto_sym_session_op_inhdr  are defined by the following
> sections.
> 1.6.1.1.1 Driver Requirements: Session creation operation The driver MUST
> set the control type with VIRTIO_CRYPTO_CTRL_CREATE_SESSION before
> the request is preceded by an operation header when executing session
> creation:
> typedef struct virtio_crypto_sym_session_op { /**< No operation */
> #define VIRTIO_CRYPTO_SYM_OP_NONE    0
> /**< Cipher only operation on the data */
> #define VIRTIO_CRYPTO_SYM_OP_CIPHER    1
> /**< Hash only operation on the data */
> #define VIRTIO_CRYPTO_SYM_OP_HASH     2
> /**< Chain any cipher with any hash operation */
> #define VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING    3
> 	u8 op_type; /* Operation type */
>     virtio_crypto_sym_cipher_t cipher_setup_data;
> 	virtio_crypto_sym_hash_t hash_setup_data;
> /* Perform the hash operation followed by the cipher operation */
> #define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_HASH_THEN_CIPHER
> 	1
> /* Perform the cipher operation followed by the hash operation */
> #define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_CIPHER_THEN_HASH
> 	2
>     u8 alg_chain_order;
> } virtio_crypto_sym_session_op_t;
> And the structures definition details are:
> typedef struct virtio_crypto_sym_hash_auth_mode {
> 	/* length of authenticated key */
>  	le32 auth_key_len;
>  	/* The length of the additional authenticated data (AAD) in bytes */
>  	le32 aad_len;
> } virtio_crypto_sym_hash_auth_mode_t;
> 
> typedef struct virtio_crypto_sym_cipher {
> /* Option to not do any cryptography */
> #define VIRTIO_CRYPTO_NO_CIPHER	0
> #define VIRTIO_CRYPTO_CIPHER_DES	1
> #define VIRTIO_CRYPTO_CIPHER_DES_CBC	2
> #define VIRTIO_CRYPTO_CIPHER_DES3	3
> #define VIRTIO_CRYPTO_CIPHER_DES3_CBC	4
> #define VIRTIO_CRYPTO_CIPHER_AES	5
> #define VIRTIO_CRYPTO_CIPHER_AES_CBC	6
> #define VIRTIO_CRYPTO_CIPHER_KASUMI_F8	7
>     le32 alg;  /* cipher algorithm type (ie. aes-cbc ) */
>     /* length of key */
>     le32 keylen;
> #define VIRTIO_CRYPTO_DECRYPT	0
> #define VIRTIO_CRYPTO_ENCRYPT	1
> 	u8 op; /* encrypt or decrypt */
> } virtio_crypto_sym_cipher_t;
> 
> typedef struct virtio_crypto_sym_hash {
> /* Option to not do any hash */
> #define VIRTIO_CRYPTO_NO_HASH	0
> #define VIRTIO_CRYPTO_HASH_MD5	1
> #define VIRTIO_CRYPTO_HASH_SHA1	2
> #define VIRTIO_CRYPTO_HASH_SHA1_96	3
> #define VIRTIO_CRYPTO_HASH_SHA224	4
> #define VIRTIO_CRYPTO_HASH_SHA256	5
> #define VIRTIO_CRYPTO_HASH_SHA384	6
> #define VIRTIO_CRYPTO_HASH_SHA512	7
> #define VIRTIO_CRYPTO_HASH_AES_XCBC	   8
> #define VIRTIO_CRYPTO_HASH_AES_XCBC_96  9
> #define VIRTIO_CRYPTO_HASH_KASUMI_F9	10
>     le32 hash_alg;  /* hash algorithm type */
> #define VIRTIO_CRYPTO_SYM_HASH_MODE_PLAIN	1
> #define VIRTIO_CRYPTO_SYM_HASH_MODE_AUTH	2
> #define VIRTIO_CRYPTO_SYM_HASH_MODE_NESTED	3
> u8 hash_mode; /* mode of hash operation, including
> authenticated/plain/nested hash */
> 	/* hash result length */
>     le32 hash_result_len;
>     virtio_crypto_sym_hash_auth_mode_t auth_mode_setup_data; }
> virtio_crypto_sym_hash_t; The driver MUST set the control type with
> VIRTIO_CRYPTO_CTRL_CLOSE_SESSION and pass the session_id to the
> device when executing session close.

This structure mixed HASH and MAC, suggest  to define separated 
services structure, cipher service structure can reference that.

> 1.6.1.1.2 Device Requirements: Session creation operation The device MUST
> return a session identifier to the driver when the device finishes the
> processing of session close. The session creation request MUST end by a
> tailer:
> typedef struct virtio_crypto_sym_session_op_inhdr {
> 	u8     status;
> 	le64    session_id;
> } virtio_crypto_sym_session_op_inhdr_t;
> Both status and session_id are written by the device: either
> VIRTIO_CRYPTO_CTRL_OK for success, VIRTIO_CRYPTO_CTRL_ERR for
> creation failed or device error.
> #define VIRTIO_CRYPTO_CTRL_OK	0
> #define VIRTIO_CRYPTO_CTRL_ERR	1
> 1.6.1.2 Session closing operation
> A request of closing a session including the following information:
> struct virtio_crypto_sym_session_creation {
> 	struct virtio_crypto_sym_ctlhdr   ctlhdr;  /* OUT */
> 	le64  session_id;  /* OUT */
> 	u8 status;  /* IN */
> };
> 1.6.1.2.1 Driver Requirements: Session closing operation The driver MUST set
> the control type with VIRTIO_CRYPTO_CTRL_CLOSE_SESSION, and the
> session_id MUST be a valid value which assigned by the device when a
> session was created.
> 1.6.1.2.2 Device Requirements: Session closing operation Status is written by
> the device: either VIRTIO_CRYPTO_CTRL_OK for success,
> VIRTIO_CRYPTO_CTRL_ERR for creation failed or device error.
> #define VIRTIO_CRYPTO_CTRL_OK	0
> #define VIRTIO_CRYPTO_CTRL_ERR	1

Need two sets of error code be defined(see 1.6.2.2 )? The unified error code for all
virtio crypto device should be better. We suggest the error codes below for virtio
crypto devices:
#define VIRTIO_CRYPTO_OP_OK    0
#define VIRTIO_CRYPTO_OP_ERR    1
#define VIRTIO_CRYPTO_OP_BADMSG    2
#define VIRTIO_CRYPTO_OP_NOTSUPP   3

> 1.6.2	Encryption operation
> 1.6.2.1 Driver Requirements: Encryption operation The encryption and
> decryption requests and the corresponding results are transmitted by placing
> them in dataq. The symmetric algorithms requests are preceded by a header:
> struct virtio_crypto_sym_op_hdr {
> 	/* the backend returned session identifier */
> 	le64 session_id;
> 	/* length of initial vector */
> 	le32 iv_len;
> 	/* iv offset in the whole crypto data memory */
> 	le32 iv_offset;
> 	/* length of additional auth data */
> 	le32 auth_len;
> 	/* additional auth data offset in the whole crypto data memory */
> 	le32 additional_auth_offset;
> 	/* cipher start source offest */
> 	le32 cipher_start_src_offset;
> 	le32 len_to_cipher;
> 	/* hash start source offest */
> 	le32 hash_start_src_offset;
> 	le32 len_to_hash;
> 	/* length of source data */
> 	le32 source_len;
> } ;
> The encryption request MUST end by a tailer:
> typedef struct virtio_crypto_sym_op_inhdr {
> 	u8     status;
> } virtio_crypto_sym _op_inhdr_t;
> The specific content of symmetric algorithms requests SHOULD be same as
> below:
> struct virtio_crypto_sym_op_data {
> 	struct virtio_crypto_sym_op_hdr  hdr_info;
> 	le64 iv_addr; /* iv guest address */
> 	le64 auth_data_addr; /* associated data guest address */
> 	le64 src_data_addr; /* source data guest address */
> 	le64 dst_data_addr; /* destination data guest address */
> 	le64 digest_result_addr; /* digest result guest address */
> 	le64 inhdr_addr; /* in-header guest address */ }; In this way, each
> data request only occupies one entry of the Vring descriptor table, which
> improves the throughput of data transferring.

Suggest to change virtio_crypto_sym_op_data to unified format listed above.
All these can also be fitted into one vring descriptor.

> 1.6.2.2 Device Requirements: Encryption operation The struct
> virtio_crypto_sym_op_inhdr’s status byte is written by the device: either
> VIRTIO_CRYPTO_S_OK for success, VIRTIO_CRYPTO_S_ERR for device or
> driver error, VIRTIO_CRYPTO_S_BADMSG for verification failed when
> decrypt AEAD algorithms:
> #define VIRTIO_CRYPTO_S_OK    0
> #define VIRTIO_CRYPTO_S_ERR    1
> #define VIRTIO_CRYPTO_S_BADMSG    2

Same comments for error code above.

> 
> 1.6.2.3 Steps of encryption Operation
> Step1: Create a session:
> 1.	The driver fills out the context message, including algorithm name,
> key, keylen etc;
> 2.	The driver sends a context message to the backend device by
> controlq;
> 3.	The device creates 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 fills out the encrypt requests;
> 2.	Put the requests into dataq and kick the virtqueue;
> 3.	The device executes the encryption operation according the
> requests' arguments;
> 4.	The device returns the encryption result to the driver by dataq;
> 5.	The driver callback handle the result and over.
> 
> Note: the driver MAY support both synchronous and asynchronous
> encryption. Then the performance is poor in synchronous operation because
> frequent context switching and virtualization overhead. The driver SHOULD
> by preference use asynchronous encryption.
> 1.6.3	Decryption Operation
> The decryption process is the same with encryption.
> 1.6.3.1 Device Requirements: Decryption operation 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) MUST be returned the driver.
> 
> Regards,
> -Gonglei
> 
> 
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [RFC v4] virtio-crypto specification
  2016-07-22  0:48 ` [Qemu-devel] " Zeng, Xin
@ 2016-07-22  2:52   ` Gonglei (Arei)
  2016-07-22  5:31     ` Zeng, Xin
  0 siblings, 1 reply; 10+ messages in thread
From: Gonglei (Arei) @ 2016-07-22  2:52 UTC (permalink / raw)
  To: Zeng, Xin, virtio-dev, qemu-devel
  Cc: Hanweidong (Randy),
	Stefan Hajnoczi, Cornelia Huck, mst, Lingli Deng, Jani Kokkonen,
	Luonengjun, Huangpeng (Peter), Zhoujian (jay, Euler),
	chenshanxi 00222737, 'Ola Liljedahl@arm.com',
	Varun Sethi

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?

Regards,
-Gonglei


> -----Original Message-----
> From: Zeng, Xin [mailto:xin.zeng@intel.com]
> Sent: Friday, July 22, 2016 8:48 AM
> To: Gonglei (Arei); virtio-dev@lists.oasis-open.org; qemu-devel@nongnu.org
> Cc: Hanweidong (Randy); Stefan Hajnoczi; Cornelia Huck; mst@redhat.com;
> Lingli Deng; Jani Kokkonen; Luonengjun; Huangpeng (Peter); Zhoujian (jay,
> Euler); chenshanxi 00222737; 'Ola Liljedahl@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.

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

> };
> 
> 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.gonglei@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.

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

> #define VIRTIO_CRYPTO_MAC_CMAC_AES                 25
> #define VIRTIO_CRYPTO_MAC_CMAC_KASUMI_F9             26
> #define VIRTIO_CRYPTO_MAC_CMAC_ SNOW3G_UIA2        27
> #define VIRTIO_CRYPTO_MAC_GMAC_AES                 40
> #define VIRTIO_CRYPTO_MAC_GMAC_TWOFISH             41
> #define VIRTIO_CRYPTO_MAC_CBCMAC_AES               48

Dose this duplicate with VIRTIO_CRYPTO_MAC_CMAC_AES ?

> #define VIRTIO_CRYPTO_MAC_CBCMAC_KASUMI_F9         49

Duplicates with VIRTIO_CRYPTO_MAC_CMAC_KASUMI_F9 ?

> #define VIRTIO_CRYPTO_MAC_XCBC_AES                 52
> 
> 5 bits are defined for asymmetric algorithms currently.
> #define VIRTIO_CRYPTO_ASYM_RSA                  0
> #define VIRTIO_CRYPTO_ASYM_DSA                  1
> #define VIRTIO_CRYPTO_ASYM_DH                   2
> #define VIRTIO_CRYPTO_ASYM_ECDSA                3
> #define VIRTIO_CRYPTO_ASYM_ECDH                 4
> 
> 7 bits are defined for KDF algorithms currently.
> #define VIRTIO_CRYPTO_KDF_NONE 0
> #define VIRTIO_CRYPTO_KDF_ANSI-X9.42 1
> #define VIRTIO_CRYPTO_KDF_ANSI-X9.62 2
> #define VIRTIO_CRYPTO_KDF_IKEv2 3
> #define VIRTIO_CRYPTO_KDF_TLS_1.0 4
> #define VIRTIO_CRYPTO_KDF_TLS_1.2 5
> #define VIRTIO_CRYPTO_KDF_NIST-800-56-Concatenation 6
> 
> 2 bits are defined for AEAD algorithms currently.
> #define VIRTIO_CRYPTO_AEAD_GCM   0
> #define VIRTIO_CRYPTO_AEAD_CCM    1
> 
> 4 bits are defined for PRIMITIVE algorithms currently
> #define VIRTIO_CRYPTO_PRIMITIVE_LNMOD_EXP               0
> #define VIRTIO_CRYPTO_PRIMITIVE_LNMOD_INV               1
> #define VIRTIO_CRYPTO_PRIMITIVE_ ECPOINT_MULTIPLY  2
> #define VIRTIO_CRYPTO_PRIMITIVE_ ECPOINT_VERIFY  3
> 
> > 1.4.1	Device Requirements: Device configuration layout
> 
> Add "The device MUST set the version in version filed."
> 
For compatibility? Okay.

> > The device MUST set max_virtqueues to between 1 and 65535 inclusive.
> >
> > The device SHOULD set status according to the status of the hardware-
> > backed implementation.
> >
> > The device MUST set algorithms according to the algorithms which the device
> > offered.
> > 1.4.2	Driver Requirements: Device configuration layout
> > The driver MUST read the ready status from the bottom bit of status to check
> > whether the hardware-backed implementation is ready or not.
> 
> Add "The driver MAY read max_dataqueues field to discover how many data
> queues
> the device supports."
> 
OK.

> > The driver MUST read the algorithms to discover the algorithms which the
> > device supports.
> 
> Because of the presence of overall service field and detail algorithms field,
> suggest
> To change this to "
> The driver MUST read crypto_services field to discover which services the
> device is
> able to offer.
> The driver MUST read detail algorithms field according to crypto_services field.
> "
> 
Yes, makes sense.

> > 1.5	Device Initialization
> > A driver would perform a typical initialization routine like so:
> > 1. Identify and initialize data virtqueue, up to max_virtqueues.
> > 2. Identify the control virtqueue.
> > 3. Identify the ready status of hardware-backend comes from the bottom bit
> > of status.
> > 4. Read the supported algorithms from bits of algorithms.
> > 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).
> > •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.
> >
> > The controlq virtqueue is used to control session operations, including
> > creation or close. The request is preceded by a header:
> > struct virtio_crypto_sym_ctlhdr {
> >     /* control type  */
> >     u8 type;
> > };
> > Two bits are currently defined for the control header type:
> > #define VIRTIO_CRYPTO_CTRL_CREATE_SESSION  1 #define
> > VIRTIO_CRYPTO_CTRL_CLOSE_SESSION  2
> >
> 
> Suggest to change virtio_crypto_sym_ctlhdr structure to the a general
> header below, and define a unified control request  structure to keep
> consistent with crypto service request format.
> struct virtio_crypto_ctrl_header{
> #define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION
> 	VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_CIPHER, 0x02)
> #define VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION
> 	VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_CIPHER, 0x03)
> #define VIRTIO_CRYPTO_MAC_CREATE_SESSION
> 	VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_MAC, 0x02)
> #define VIRTIO_CRYPTO_MAC_DESTROY_SESSION
> 	VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_MAC, 0x03)
> 	u32 opcode;
> 	u8 algo;
> 	u8 flag;
> };
> 
We MUST add a queue_id property in header, which shows which data virtqueue
we will used for multiqueue support.
 
> > 1.6.1.1 Session creation operation
> > A request of creating a session including the following information:
> > struct virtio_crypto_sym_session_creation {
> > 	struct virtio_crypto_sym_ctlhdr   ctlhdr;  /* OUT */
> > 	struct virtio_crypto_sym_session_op  session_op;  /* OUT */
> > 	struct virtio_crypto_sym_session_op_inhdr  inhdr;  /* IN */ }; The
> > details of specific structure, including struct virtio_crypto_sym_session_op
> > and struct virtio_crypto_sym_session_op_inhdr  are defined by the following
> > sections.
> > 1.6.1.1.1 Driver Requirements: Session creation operation The driver MUST
> > set the control type with VIRTIO_CRYPTO_CTRL_CREATE_SESSION before
> > the request is preceded by an operation header when executing session
> > creation:
> > typedef struct virtio_crypto_sym_session_op { /**< No operation */
> > #define VIRTIO_CRYPTO_SYM_OP_NONE    0
> > /**< Cipher only operation on the data */
> > #define VIRTIO_CRYPTO_SYM_OP_CIPHER    1
> > /**< Hash only operation on the data */
> > #define VIRTIO_CRYPTO_SYM_OP_HASH     2
> > /**< Chain any cipher with any hash operation */
> > #define VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING    3
> > 	u8 op_type; /* Operation type */
> >     virtio_crypto_sym_cipher_t cipher_setup_data;
> > 	virtio_crypto_sym_hash_t hash_setup_data;
> > /* Perform the hash operation followed by the cipher operation */
> > #define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_HASH_THEN_CIPHER
> > 	1
> > /* Perform the cipher operation followed by the hash operation */
> > #define VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_CIPHER_THEN_HASH
> > 	2
> >     u8 alg_chain_order;
> > } virtio_crypto_sym_session_op_t;
> > And the structures definition details are:
> > typedef struct virtio_crypto_sym_hash_auth_mode {
> > 	/* length of authenticated key */
> >  	le32 auth_key_len;
> >  	/* The length of the additional authenticated data (AAD) in bytes */
> >  	le32 aad_len;
> > } virtio_crypto_sym_hash_auth_mode_t;
> >
> > typedef struct virtio_crypto_sym_cipher {
> > /* Option to not do any cryptography */
> > #define VIRTIO_CRYPTO_NO_CIPHER	0
> > #define VIRTIO_CRYPTO_CIPHER_DES	1
> > #define VIRTIO_CRYPTO_CIPHER_DES_CBC	2
> > #define VIRTIO_CRYPTO_CIPHER_DES3	3
> > #define VIRTIO_CRYPTO_CIPHER_DES3_CBC	4
> > #define VIRTIO_CRYPTO_CIPHER_AES	5
> > #define VIRTIO_CRYPTO_CIPHER_AES_CBC	6
> > #define VIRTIO_CRYPTO_CIPHER_KASUMI_F8	7
> >     le32 alg;  /* cipher algorithm type (ie. aes-cbc ) */
> >     /* length of key */
> >     le32 keylen;
> > #define VIRTIO_CRYPTO_DECRYPT	0
> > #define VIRTIO_CRYPTO_ENCRYPT	1
> > 	u8 op; /* encrypt or decrypt */
> > } virtio_crypto_sym_cipher_t;
> >
> > typedef struct virtio_crypto_sym_hash {
> > /* Option to not do any hash */
> > #define VIRTIO_CRYPTO_NO_HASH	0
> > #define VIRTIO_CRYPTO_HASH_MD5	1
> > #define VIRTIO_CRYPTO_HASH_SHA1	2
> > #define VIRTIO_CRYPTO_HASH_SHA1_96	3
> > #define VIRTIO_CRYPTO_HASH_SHA224	4
> > #define VIRTIO_CRYPTO_HASH_SHA256	5
> > #define VIRTIO_CRYPTO_HASH_SHA384	6
> > #define VIRTIO_CRYPTO_HASH_SHA512	7
> > #define VIRTIO_CRYPTO_HASH_AES_XCBC	   8
> > #define VIRTIO_CRYPTO_HASH_AES_XCBC_96  9
> > #define VIRTIO_CRYPTO_HASH_KASUMI_F9	10
> >     le32 hash_alg;  /* hash algorithm type */
> > #define VIRTIO_CRYPTO_SYM_HASH_MODE_PLAIN	1
> > #define VIRTIO_CRYPTO_SYM_HASH_MODE_AUTH	2
> > #define VIRTIO_CRYPTO_SYM_HASH_MODE_NESTED	3
> > u8 hash_mode; /* mode of hash operation, including
> > authenticated/plain/nested hash */
> > 	/* hash result length */
> >     le32 hash_result_len;
> >     virtio_crypto_sym_hash_auth_mode_t auth_mode_setup_data; }
> > virtio_crypto_sym_hash_t; The driver MUST set the control type with
> > VIRTIO_CRYPTO_CTRL_CLOSE_SESSION and pass the session_id to the
> > device when executing session close.
> 
> This structure mixed HASH and MAC, suggest  to define separated
> services structure, cipher service structure can reference that.
> 
OK, and we can still name it 'sym', include plain cipher and chain-algorithms.

> > 1.6.1.1.2 Device Requirements: Session creation operation The device MUST
> > return a session identifier to the driver when the device finishes the
> > processing of session close. The session creation request MUST end by a
> > tailer:
> > typedef struct virtio_crypto_sym_session_op_inhdr {
> > 	u8     status;
> > 	le64    session_id;
> > } virtio_crypto_sym_session_op_inhdr_t;
> > Both status and session_id are written by the device: either
> > VIRTIO_CRYPTO_CTRL_OK for success, VIRTIO_CRYPTO_CTRL_ERR for
> > creation failed or device error.
> > #define VIRTIO_CRYPTO_CTRL_OK	0
> > #define VIRTIO_CRYPTO_CTRL_ERR	1
> > 1.6.1.2 Session closing operation
> > A request of closing a session including the following information:
> > struct virtio_crypto_sym_session_creation {
> > 	struct virtio_crypto_sym_ctlhdr   ctlhdr;  /* OUT */
> > 	le64  session_id;  /* OUT */
> > 	u8 status;  /* IN */
> > };
> > 1.6.1.2.1 Driver Requirements: Session closing operation The driver MUST set
> > the control type with VIRTIO_CRYPTO_CTRL_CLOSE_SESSION, and the
> > session_id MUST be a valid value which assigned by the device when a
> > session was created.
> > 1.6.1.2.2 Device Requirements: Session closing operation Status is written by
> > the device: either VIRTIO_CRYPTO_CTRL_OK for success,
> > VIRTIO_CRYPTO_CTRL_ERR for creation failed or device error.
> > #define VIRTIO_CRYPTO_CTRL_OK	0
> > #define VIRTIO_CRYPTO_CTRL_ERR	1
> 
> Need two sets of error code be defined(see 1.6.2.2 )? The unified error code for
> all
> virtio crypto device should be better. We suggest the error codes below for
> virtio
> crypto devices:
> #define VIRTIO_CRYPTO_OP_OK    0
> #define VIRTIO_CRYPTO_OP_ERR    1
> #define VIRTIO_CRYPTO_OP_BADMSG    2
> #define VIRTIO_CRYPTO_OP_NOTSUPP   3
> 
Makes sense.

> > 1.6.2	Encryption operation
> > 1.6.2.1 Driver Requirements: Encryption operation The encryption and
> > decryption requests and the corresponding results are transmitted by placing
> > them in dataq. The symmetric algorithms requests are preceded by a header:
> > struct virtio_crypto_sym_op_hdr {
> > 	/* the backend returned session identifier */
> > 	le64 session_id;
> > 	/* length of initial vector */
> > 	le32 iv_len;
> > 	/* iv offset in the whole crypto data memory */
> > 	le32 iv_offset;
> > 	/* length of additional auth data */
> > 	le32 auth_len;
> > 	/* additional auth data offset in the whole crypto data memory */
> > 	le32 additional_auth_offset;
> > 	/* cipher start source offest */
> > 	le32 cipher_start_src_offset;
> > 	le32 len_to_cipher;
> > 	/* hash start source offest */
> > 	le32 hash_start_src_offset;
> > 	le32 len_to_hash;
> > 	/* length of source data */
> > 	le32 source_len;
> > } ;
> > The encryption request MUST end by a tailer:
> > typedef struct virtio_crypto_sym_op_inhdr {
> > 	u8     status;
> > } virtio_crypto_sym _op_inhdr_t;
> > The specific content of symmetric algorithms requests SHOULD be same as
> > below:
> > struct virtio_crypto_sym_op_data {
> > 	struct virtio_crypto_sym_op_hdr  hdr_info;
> > 	le64 iv_addr; /* iv guest address */
> > 	le64 auth_data_addr; /* associated data guest address */
> > 	le64 src_data_addr; /* source data guest address */
> > 	le64 dst_data_addr; /* destination data guest address */
> > 	le64 digest_result_addr; /* digest result guest address */
> > 	le64 inhdr_addr; /* in-header guest address */ }; In this way, each
> > data request only occupies one entry of the Vring descriptor table, which
> > improves the throughput of data transferring.
> 
> Suggest to change virtio_crypto_sym_op_data to unified format listed above.
> All these can also be fitted into one vring descriptor.
> 
Sure.

> > 1.6.2.2 Device Requirements: Encryption operation The struct
> > virtio_crypto_sym_op_inhdr’s status byte is written by the device: either
> > VIRTIO_CRYPTO_S_OK for success, VIRTIO_CRYPTO_S_ERR for device or
> > driver error, VIRTIO_CRYPTO_S_BADMSG for verification failed when
> > decrypt AEAD algorithms:
> > #define VIRTIO_CRYPTO_S_OK    0
> > #define VIRTIO_CRYPTO_S_ERR    1
> > #define VIRTIO_CRYPTO_S_BADMSG    2
> 
> Same comments for error code above.
> 
> >
> > 1.6.2.3 Steps of encryption Operation
> > Step1: Create a session:
> > 1.	The driver fills out the context message, including algorithm name,
> > key, keylen etc;
> > 2.	The driver sends a context message to the backend device by
> > controlq;
> > 3.	The device creates 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 fills out the encrypt requests;
> > 2.	Put the requests into dataq and kick the virtqueue;
> > 3.	The device executes the encryption operation according the
> > requests' arguments;
> > 4.	The device returns the encryption result to the driver by dataq;
> > 5.	The driver callback handle the result and over.
> >
> > Note: the driver MAY support both synchronous and asynchronous
> > encryption. Then the performance is poor in synchronous operation because
> > frequent context switching and virtualization overhead. The driver SHOULD
> > by preference use asynchronous encryption.
> > 1.6.3	Decryption Operation
> > The decryption process is the same with encryption.
> > 1.6.3.1 Device Requirements: Decryption operation 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) MUST be returned the driver.
> >
> > Regards,
> > -Gonglei
> >
> >
> >


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [RFC v4] virtio-crypto specification
  2016-07-22  2:52   ` Gonglei (Arei)
@ 2016-07-22  5:31     ` Zeng, Xin
  2016-07-25  1:38       ` Gonglei (Arei)
  0 siblings, 1 reply; 10+ messages in thread
From: Zeng, Xin @ 2016-07-22  5:31 UTC (permalink / raw)
  To: Gonglei (Arei), virtio-dev, qemu-devel
  Cc: Hanweidong (Randy),
	Stefan Hajnoczi, Cornelia Huck, mst, Lingli Deng, Jani Kokkonen,
	Luonengjun, Huangpeng (Peter), Zhoujian (jay, Euler),
	chenshanxi 00222737, 'Ola Liljedahl@arm.com',
	Varun Sethi

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.zeng@intel.com]
> > Sent: Friday, July 22, 2016 8:48 AM
> > To: Gonglei (Arei); virtio-dev@lists.oasis-open.org; qemu-
> devel@nongnu.org
> > Cc: Hanweidong (Randy); Stefan Hajnoczi; Cornelia Huck; mst@redhat.com;
> > Lingli Deng; Jani Kokkonen; Luonengjun; Huangpeng (Peter); Zhoujian (jay,
> > Euler); chenshanxi 00222737; 'Ola Liljedahl@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.

> > };
> >
> > 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.gonglei@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.

> 
> > #define VIRTIO_CRYPTO_MAC_CMAC_AES                 25
> > #define VIRTIO_CRYPTO_MAC_CMAC_KASUMI_F9             26
> > #define VIRTIO_CRYPTO_MAC_CMAC_ SNOW3G_UIA2        27
> > #define VIRTIO_CRYPTO_MAC_GMAC_AES                 40
> > #define VIRTIO_CRYPTO_MAC_GMAC_TWOFISH             41
> > #define VIRTIO_CRYPTO_MAC_CBCMAC_AES               48
> 
> Dose this duplicate with VIRTIO_CRYPTO_MAC_CMAC_AES ?

CMAC_AES is different with CBCMAC_AES, there is a history
about these.

> 
> > #define VIRTIO_CRYPTO_MAC_CBCMAC_KASUMI_F9         49
> 
> Duplicates with VIRTIO_CRYPTO_MAC_CMAC_KASUMI_F9 ?

CMAC_KASUMI_F9 is also different with CBCMAC_KASUMI_F9.

> 
> > #define VIRTIO_CRYPTO_MAC_XCBC_AES                 52
> >
> > 5 bits are defined for asymmetric algorithms currently.
> > #define VIRTIO_CRYPTO_ASYM_RSA                  0
> > #define VIRTIO_CRYPTO_ASYM_DSA                  1
> > #define VIRTIO_CRYPTO_ASYM_DH                   2
> > #define VIRTIO_CRYPTO_ASYM_ECDSA                3
> > #define VIRTIO_CRYPTO_ASYM_ECDH                 4
> >
> > 7 bits are defined for KDF algorithms currently.
> > #define VIRTIO_CRYPTO_KDF_NONE 0
> > #define VIRTIO_CRYPTO_KDF_ANSI-X9.42 1
> > #define VIRTIO_CRYPTO_KDF_ANSI-X9.62 2
> > #define VIRTIO_CRYPTO_KDF_IKEv2 3
> > #define VIRTIO_CRYPTO_KDF_TLS_1.0 4
> > #define VIRTIO_CRYPTO_KDF_TLS_1.2 5
> > #define VIRTIO_CRYPTO_KDF_NIST-800-56-Concatenation 6
> >
> > 2 bits are defined for AEAD algorithms currently.
> > #define VIRTIO_CRYPTO_AEAD_GCM   0
> > #define VIRTIO_CRYPTO_AEAD_CCM    1
> >
> > 4 bits are defined for PRIMITIVE algorithms currently
> > #define VIRTIO_CRYPTO_PRIMITIVE_LNMOD_EXP               0
> > #define VIRTIO_CRYPTO_PRIMITIVE_LNMOD_INV               1
> > #define VIRTIO_CRYPTO_PRIMITIVE_ ECPOINT_MULTIPLY  2
> > #define VIRTIO_CRYPTO_PRIMITIVE_ ECPOINT_VERIFY  3
> >
> > > 1.4.1	Device Requirements: Device configuration layout
> >
> > Add "The device MUST set the version in version filed."
> >
> For compatibility? Okay.
> 
> > > The device MUST set max_virtqueues to between 1 and 65535 inclusive.
> > >
> > > The device SHOULD set status according to the status of the hardware-
> > > backed implementation.
> > >
> > > The device MUST set algorithms according to the algorithms which the
> device
> > > offered.
> > > 1.4.2	Driver Requirements: Device configuration layout
> > > The driver MUST read the ready status from the bottom bit of status to
> check
> > > whether the hardware-backed implementation is ready or not.
> >
> > Add "The driver MAY read max_dataqueues field to discover how many
> data
> > queues
> > the device supports."
> >
> OK.
> 
> > > The driver MUST read the algorithms to discover the algorithms which the
> > > device supports.
> >
> > Because of the presence of overall service field and detail algorithms field,
> > suggest
> > To change this to "
> > The driver MUST read crypto_services field to discover which services the
> > device is
> > able to offer.
> > The driver MUST read detail algorithms field according to crypto_services
> field.
> > "
> >
> Yes, makes sense.
> 
> > > 1.5	Device Initialization
> > > A driver would perform a typical initialization routine like so:
> > > 1. Identify and initialize data virtqueue, up to max_virtqueues.
> > > 2. Identify the control virtqueue.
> > > 3. Identify the ready status of hardware-backend comes from the bottom
> bit
> > > of status.
> > > 4. Read the supported algorithms from bits of algorithms.
> > > 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).
> > > •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.
> > >
> > > The controlq virtqueue is used to control session operations, including
> > > creation or close. The request is preceded by a header:
> > > struct virtio_crypto_sym_ctlhdr {
> > >     /* control type  */
> > >     u8 type;
> > > };
> > > Two bits are currently defined for the control header type:
> > > #define VIRTIO_CRYPTO_CTRL_CREATE_SESSION  1 #define
> > > VIRTIO_CRYPTO_CTRL_CLOSE_SESSION  2
> > >
> >
> > Suggest to change virtio_crypto_sym_ctlhdr structure to the a general
> > header below, and define a unified control request  structure to keep
> > consistent with crypto service request format.
> > struct virtio_crypto_ctrl_header{
> > #define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION
> > 	VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_CIPHER, 0x02)
> > #define VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION
> > 	VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_CIPHER, 0x03)
> > #define VIRTIO_CRYPTO_MAC_CREATE_SESSION
> > 	VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_MAC, 0x02)
> > #define VIRTIO_CRYPTO_MAC_DESTROY_SESSION
> > 	VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_S_MAC, 0x03)
> > 	u32 opcode;
> > 	u8 algo;
> > 	u8 flag;
> > };
> >
> We MUST add a queue_id property in header, which shows which data
> virtqueue
> we will used for multiqueue support.
> 

Ok.

> > > 1.6.1.1 Session creation operation
> > > A request of creating a session including the following information:
> > > struct virtio_crypto_sym_session_creation {
> > > 	struct virtio_crypto_sym_ctlhdr   ctlhdr;  /* OUT */
> > > 	struct virtio_crypto_sym_session_op  session_op;  /* OUT */
> > > 	struct virtio_crypto_sym_session_op_inhdr  inhdr;  /* IN */ }; The
> > > details of specific structure, including struct
> virtio_crypto_sym_session_op
> > > and struct virtio_crypto_sym_session_op_inhdr  are defined by the
> following
> > > sections.
> > > 1.6.1.1.1 Driver Requirements: Session creation operation The driver
> MUST
> > > set the control type with VIRTIO_CRYPTO_CTRL_CREATE_SESSION before
> > > the request is preceded by an operation header when executing session
> > > creation:
> > > typedef struct virtio_crypto_sym_session_op { /**< No operation */
> > > #define VIRTIO_CRYPTO_SYM_OP_NONE    0
> > > /**< Cipher only operation on the data */
> > > #define VIRTIO_CRYPTO_SYM_OP_CIPHER    1
> > > /**< Hash only operation on the data */
> > > #define VIRTIO_CRYPTO_SYM_OP_HASH     2
> > > /**< Chain any cipher with any hash operation */
> > > #define VIRTIO_CRYPTO_SYM_OP_ALGORITHM_CHAINING    3
> > > 	u8 op_type; /* Operation type */
> > >     virtio_crypto_sym_cipher_t cipher_setup_data;
> > > 	virtio_crypto_sym_hash_t hash_setup_data;
> > > /* Perform the hash operation followed by the cipher operation */
> > > #define
> VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_HASH_THEN_CIPHER
> > > 	1
> > > /* Perform the cipher operation followed by the hash operation */
> > > #define
> VIRTIO_CRYPTO_SYM_ALG_CHAIN_ORDER_CIPHER_THEN_HASH
> > > 	2
> > >     u8 alg_chain_order;
> > > } virtio_crypto_sym_session_op_t;
> > > And the structures definition details are:
> > > typedef struct virtio_crypto_sym_hash_auth_mode {
> > > 	/* length of authenticated key */
> > >  	le32 auth_key_len;
> > >  	/* The length of the additional authenticated data (AAD) in bytes */
> > >  	le32 aad_len;
> > > } virtio_crypto_sym_hash_auth_mode_t;
> > >
> > > typedef struct virtio_crypto_sym_cipher {
> > > /* Option to not do any cryptography */
> > > #define VIRTIO_CRYPTO_NO_CIPHER	0
> > > #define VIRTIO_CRYPTO_CIPHER_DES	1
> > > #define VIRTIO_CRYPTO_CIPHER_DES_CBC	2
> > > #define VIRTIO_CRYPTO_CIPHER_DES3	3
> > > #define VIRTIO_CRYPTO_CIPHER_DES3_CBC	4
> > > #define VIRTIO_CRYPTO_CIPHER_AES	5
> > > #define VIRTIO_CRYPTO_CIPHER_AES_CBC	6
> > > #define VIRTIO_CRYPTO_CIPHER_KASUMI_F8	7
> > >     le32 alg;  /* cipher algorithm type (ie. aes-cbc ) */
> > >     /* length of key */
> > >     le32 keylen;
> > > #define VIRTIO_CRYPTO_DECRYPT	0
> > > #define VIRTIO_CRYPTO_ENCRYPT	1
> > > 	u8 op; /* encrypt or decrypt */
> > > } virtio_crypto_sym_cipher_t;
> > >
> > > typedef struct virtio_crypto_sym_hash {
> > > /* Option to not do any hash */
> > > #define VIRTIO_CRYPTO_NO_HASH	0
> > > #define VIRTIO_CRYPTO_HASH_MD5	1
> > > #define VIRTIO_CRYPTO_HASH_SHA1	2
> > > #define VIRTIO_CRYPTO_HASH_SHA1_96	3
> > > #define VIRTIO_CRYPTO_HASH_SHA224	4
> > > #define VIRTIO_CRYPTO_HASH_SHA256	5
> > > #define VIRTIO_CRYPTO_HASH_SHA384	6
> > > #define VIRTIO_CRYPTO_HASH_SHA512	7
> > > #define VIRTIO_CRYPTO_HASH_AES_XCBC	   8
> > > #define VIRTIO_CRYPTO_HASH_AES_XCBC_96  9
> > > #define VIRTIO_CRYPTO_HASH_KASUMI_F9	10
> > >     le32 hash_alg;  /* hash algorithm type */
> > > #define VIRTIO_CRYPTO_SYM_HASH_MODE_PLAIN	1
> > > #define VIRTIO_CRYPTO_SYM_HASH_MODE_AUTH	2
> > > #define VIRTIO_CRYPTO_SYM_HASH_MODE_NESTED	3
> > > u8 hash_mode; /* mode of hash operation, including
> > > authenticated/plain/nested hash */
> > > 	/* hash result length */
> > >     le32 hash_result_len;
> > >     virtio_crypto_sym_hash_auth_mode_t auth_mode_setup_data; }
> > > virtio_crypto_sym_hash_t; The driver MUST set the control type with
> > > VIRTIO_CRYPTO_CTRL_CLOSE_SESSION and pass the session_id to the
> > > device when executing session close.
> >
> > This structure mixed HASH and MAC, suggest  to define separated
> > services structure, cipher service structure can reference that.
> >
> OK, and we can still name it 'sym', include plain cipher and chain-algorithms.
> 
> > > 1.6.1.1.2 Device Requirements: Session creation operation The device
> MUST
> > > return a session identifier to the driver when the device finishes the
> > > processing of session close. The session creation request MUST end by a
> > > tailer:
> > > typedef struct virtio_crypto_sym_session_op_inhdr {
> > > 	u8     status;
> > > 	le64    session_id;
> > > } virtio_crypto_sym_session_op_inhdr_t;
> > > Both status and session_id are written by the device: either
> > > VIRTIO_CRYPTO_CTRL_OK for success, VIRTIO_CRYPTO_CTRL_ERR for
> > > creation failed or device error.
> > > #define VIRTIO_CRYPTO_CTRL_OK	0
> > > #define VIRTIO_CRYPTO_CTRL_ERR	1
> > > 1.6.1.2 Session closing operation
> > > A request of closing a session including the following information:
> > > struct virtio_crypto_sym_session_creation {
> > > 	struct virtio_crypto_sym_ctlhdr   ctlhdr;  /* OUT */
> > > 	le64  session_id;  /* OUT */
> > > 	u8 status;  /* IN */
> > > };
> > > 1.6.1.2.1 Driver Requirements: Session closing operation The driver MUST
> set
> > > the control type with VIRTIO_CRYPTO_CTRL_CLOSE_SESSION, and the
> > > session_id MUST be a valid value which assigned by the device when a
> > > session was created.
> > > 1.6.1.2.2 Device Requirements: Session closing operation Status is written
> by
> > > the device: either VIRTIO_CRYPTO_CTRL_OK for success,
> > > VIRTIO_CRYPTO_CTRL_ERR for creation failed or device error.
> > > #define VIRTIO_CRYPTO_CTRL_OK	0
> > > #define VIRTIO_CRYPTO_CTRL_ERR	1
> >
> > Need two sets of error code be defined(see 1.6.2.2 )? The unified error
> code for
> > all
> > virtio crypto device should be better. We suggest the error codes below for
> > virtio
> > crypto devices:
> > #define VIRTIO_CRYPTO_OP_OK    0
> > #define VIRTIO_CRYPTO_OP_ERR    1
> > #define VIRTIO_CRYPTO_OP_BADMSG    2
> > #define VIRTIO_CRYPTO_OP_NOTSUPP   3
> >
> Makes sense.
> 
> > > 1.6.2	Encryption operation
> > > 1.6.2.1 Driver Requirements: Encryption operation The encryption and
> > > decryption requests and the corresponding results are transmitted by
> placing
> > > them in dataq. The symmetric algorithms requests are preceded by a
> header:
> > > struct virtio_crypto_sym_op_hdr {
> > > 	/* the backend returned session identifier */
> > > 	le64 session_id;
> > > 	/* length of initial vector */
> > > 	le32 iv_len;
> > > 	/* iv offset in the whole crypto data memory */
> > > 	le32 iv_offset;
> > > 	/* length of additional auth data */
> > > 	le32 auth_len;
> > > 	/* additional auth data offset in the whole crypto data memory */
> > > 	le32 additional_auth_offset;
> > > 	/* cipher start source offest */
> > > 	le32 cipher_start_src_offset;
> > > 	le32 len_to_cipher;
> > > 	/* hash start source offest */
> > > 	le32 hash_start_src_offset;
> > > 	le32 len_to_hash;
> > > 	/* length of source data */
> > > 	le32 source_len;
> > > } ;
> > > The encryption request MUST end by a tailer:
> > > typedef struct virtio_crypto_sym_op_inhdr {
> > > 	u8     status;
> > > } virtio_crypto_sym _op_inhdr_t;
> > > The specific content of symmetric algorithms requests SHOULD be same
> as
> > > below:
> > > struct virtio_crypto_sym_op_data {
> > > 	struct virtio_crypto_sym_op_hdr  hdr_info;
> > > 	le64 iv_addr; /* iv guest address */
> > > 	le64 auth_data_addr; /* associated data guest address */
> > > 	le64 src_data_addr; /* source data guest address */
> > > 	le64 dst_data_addr; /* destination data guest address */
> > > 	le64 digest_result_addr; /* digest result guest address */
> > > 	le64 inhdr_addr; /* in-header guest address */ }; In this way, each
> > > data request only occupies one entry of the Vring descriptor table, which
> > > improves the throughput of data transferring.
> >
> > Suggest to change virtio_crypto_sym_op_data to unified format listed
> above.
> > All these can also be fitted into one vring descriptor.
> >
> Sure.
> 
> > > 1.6.2.2 Device Requirements: Encryption operation The struct
> > > virtio_crypto_sym_op_inhdr’s status byte is written by the device: either
> > > VIRTIO_CRYPTO_S_OK for success, VIRTIO_CRYPTO_S_ERR for device or
> > > driver error, VIRTIO_CRYPTO_S_BADMSG for verification failed when
> > > decrypt AEAD algorithms:
> > > #define VIRTIO_CRYPTO_S_OK    0
> > > #define VIRTIO_CRYPTO_S_ERR    1
> > > #define VIRTIO_CRYPTO_S_BADMSG    2
> >
> > Same comments for error code above.
> >
> > >
> > > 1.6.2.3 Steps of encryption Operation
> > > Step1: Create a session:
> > > 1.	The driver fills out the context message, including algorithm name,
> > > key, keylen etc;
> > > 2.	The driver sends a context message to the backend device by
> > > controlq;
> > > 3.	The device creates 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 fills out the encrypt requests;
> > > 2.	Put the requests into dataq and kick the virtqueue;
> > > 3.	The device executes the encryption operation according the
> > > requests' arguments;
> > > 4.	The device returns the encryption result to the driver by dataq;
> > > 5.	The driver callback handle the result and over.
> > >
> > > Note: the driver MAY support both synchronous and asynchronous
> > > encryption. Then the performance is poor in synchronous operation
> because
> > > frequent context switching and virtualization overhead. The driver
> SHOULD
> > > by preference use asynchronous encryption.
> > > 1.6.3	Decryption Operation
> > > The decryption process is the same with encryption.
> > > 1.6.3.1 Device Requirements: Decryption operation 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) MUST be returned the
> driver.
> > >
> > > Regards,
> > > -Gonglei
> > >
> > >
> > >


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [RFC v4] virtio-crypto specification
  2016-07-22  5:31     ` Zeng, Xin
@ 2016-07-25  1:38       ` Gonglei (Arei)
  2016-07-25  6:19         ` Zeng, Xin
  0 siblings, 1 reply; 10+ messages in thread
From: Gonglei (Arei) @ 2016-07-25  1:38 UTC (permalink / raw)
  To: Zeng, Xin, virtio-dev, qemu-devel
  Cc: Hanweidong (Randy),
	Stefan Hajnoczi, Cornelia Huck, mst, Lingli Deng, Jani Kokkonen,
	Luonengjun, Huangpeng (Peter), Zhoujian (jay, Euler),
	chenshanxi 00222737, 'Ola Liljedahl@arm.com',
	Varun Sethi

Hi Xin,


> -----Original Message-----
> From: Zeng, Xin [mailto:xin.zeng@intel.com]
> Sent: Friday, July 22, 2016 1:31 PM
> To: Gonglei (Arei); virtio-dev@lists.oasis-open.org; qemu-devel@nongnu.org
> Cc: Hanweidong (Randy); Stefan Hajnoczi; Cornelia Huck; mst@redhat.com;
> Lingli Deng; Jani Kokkonen; Luonengjun; Huangpeng (Peter); Zhoujian (jay,
> Euler); chenshanxi 00222737; 'Ola Liljedahl@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.zeng@intel.com]
> > > Sent: Friday, July 22, 2016 8:48 AM
> > > To: Gonglei (Arei); virtio-dev@lists.oasis-open.org; qemu-
> > devel@nongnu.org
> > > Cc: Hanweidong (Randy); Stefan Hajnoczi; Cornelia Huck; mst@redhat.com;
> > > Lingli Deng; Jani Kokkonen; Luonengjun; Huangpeng (Peter); Zhoujian (jay,
> > > Euler); chenshanxi 00222737; 'Ola Liljedahl@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.gonglei@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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [RFC v4] virtio-crypto specification
  2016-07-25  1:38       ` Gonglei (Arei)
@ 2016-07-25  6:19         ` Zeng, Xin
  2016-07-25  7:16           ` Gonglei (Arei)
  0 siblings, 1 reply; 10+ messages in thread
From: Zeng, Xin @ 2016-07-25  6:19 UTC (permalink / raw)
  To: Gonglei (Arei), virtio-dev, qemu-devel
  Cc: Hanweidong (Randy),
	Stefan Hajnoczi, Cornelia Huck, mst, Lingli Deng, Jani Kokkonen,
	Luonengjun, Huangpeng (Peter), Zhoujian (jay, Euler),
	chenshanxi 00222737, 'Ola Liljedahl@arm.com',
	Varun Sethi



On Monday, July 25, 2016 9:38 AM Gonglei (Arei) wrote:
> 
> Hi Xin,
> 
> 
> > -----Original Message-----
> > From: Zeng, Xin [mailto:xin.zeng@intel.com]
> > Sent: Friday, July 22, 2016 1:31 PM
> > To: Gonglei (Arei); virtio-dev@lists.oasis-open.org;
> > qemu-devel@nongnu.org
> > Cc: Hanweidong (Randy); Stefan Hajnoczi; Cornelia Huck;
> > mst@redhat.com; Lingli Deng; Jani Kokkonen; Luonengjun; Huangpeng
> > (Peter); Zhoujian (jay, Euler); chenshanxi 00222737; 'Ola
> > Liljedahl@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.zeng@intel.com]
> > > > Sent: Friday, July 22, 2016 8:48 AM
> > > > To: Gonglei (Arei); virtio-dev@lists.oasis-open.org; qemu-
> > > devel@nongnu.org
> > > > Cc: Hanweidong (Randy); Stefan Hajnoczi; Cornelia Huck;
> > > > mst@redhat.com; Lingli Deng; Jani Kokkonen; Luonengjun; Huangpeng
> > > > (Peter); Zhoujian (jay, Euler); chenshanxi 00222737; 'Ola
> > > > Liljedahl@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. :)

> 
> 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.gonglei@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.

> Regards,
> -Gonglei

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Qemu-devel] [RFC v4] virtio-crypto specification
  2016-07-25  6:19         ` Zeng, Xin
@ 2016-07-25  7:16           ` Gonglei (Arei)
  0 siblings, 0 replies; 10+ messages in thread
From: Gonglei (Arei) @ 2016-07-25  7:16 UTC (permalink / raw)
  To: Zeng, Xin, virtio-dev, qemu-devel
  Cc: Hanweidong (Randy),
	Stefan Hajnoczi, Cornelia Huck, mst, Lingli Deng, Jani Kokkonen,
	Luonengjun, Huangpeng (Peter), Zhoujian (jay, Euler),
	chenshanxi 00222737, 'Ola Liljedahl@arm.com',
	Varun Sethi


> -----Original Message-----
> From: Zeng, Xin [mailto:xin.zeng@intel.com]
> Sent: Monday, July 25, 2016 2:20 PM
> To: Gonglei (Arei); virtio-dev@lists.oasis-open.org; qemu-devel@nongnu.org
> Cc: Hanweidong (Randy); Stefan Hajnoczi; Cornelia Huck; mst@redhat.com;
> Lingli Deng; Jani Kokkonen; Luonengjun; Huangpeng (Peter); Zhoujian (jay,
> Euler); chenshanxi 00222737; 'Ola Liljedahl@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.zeng@intel.com]
> > > Sent: Friday, July 22, 2016 1:31 PM
> > > To: Gonglei (Arei); virtio-dev@lists.oasis-open.org;
> > > qemu-devel@nongnu.org
> > > Cc: Hanweidong (Randy); Stefan Hajnoczi; Cornelia Huck;
> > > mst@redhat.com; Lingli Deng; Jani Kokkonen; Luonengjun; Huangpeng
> > > (Peter); Zhoujian (jay, Euler); chenshanxi 00222737; 'Ola
> > > Liljedahl@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.zeng@intel.com]
> > > > > Sent: Friday, July 22, 2016 8:48 AM
> > > > > To: Gonglei (Arei); virtio-dev@lists.oasis-open.org; qemu-
> > > > devel@nongnu.org
> > > > > Cc: Hanweidong (Randy); Stefan Hajnoczi; Cornelia Huck;
> > > > > mst@redhat.com; Lingli Deng; Jani Kokkonen; Luonengjun; Huangpeng
> > > > > (Peter); Zhoujian (jay, Euler); chenshanxi 00222737; 'Ola
> > > > > Liljedahl@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.gonglei@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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-07-25  7:22 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-26  9:35 [Qemu-devel] [RFC v4] virtio-crypto specification Gonglei (Arei)
2016-07-08  7:27 ` Gonglei (Arei)
2016-07-14 12:16   ` Stefan Hajnoczi
2016-07-14 12:21   ` [Qemu-devel] [virtio-dev] " Stefan Hajnoczi
2016-07-22  0:48 ` [Qemu-devel] " Zeng, Xin
2016-07-22  2:52   ` Gonglei (Arei)
2016-07-22  5:31     ` Zeng, Xin
2016-07-25  1:38       ` Gonglei (Arei)
2016-07-25  6:19         ` Zeng, Xin
2016-07-25  7:16           ` Gonglei (Arei)

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.