[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH v8 1/2] virtio-crypto: Add virtio crypto device specification
On Tue, Aug 30, 2016 at 08:12:15PM +0800, Gonglei wrote:
Hi,
I have read through part of the spec and suggest mostly grammar fixes
below.
> The virtio crypto device is a virtual crypto device (ie. hardware
> crypto accelerator card). The virtio crypto device can provide
> five crypto services: CIPHER, MAC, HASH, AEAD, KDF, ASYM, PRIMITIVE.
>
> In this patch, CIPHER, MAC, HASH, AEAD services are introduced.
>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Cornelia Huck <cornelia.huck@de.ibm.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Lingli Deng <denglingli@chinamobile.com>
> CC: Jani Kokkonen <Jani.Kokkonen@huawei.com>
> CC: Ola Liljedahl <Ola.Liljedahl@arm.com>
> CC: Varun Sethi <Varun.Sethi@freescale.com>
> CC: Zeng Xin <xin.zeng@intel.com>
> CC: Keating Brian <brian.a.keating@intel.com>
> CC: Ma Liang J <liang.j.ma@intel.com>
> CC: Griffin John <john.griffin@intel.com>
> CC: Hanweidong <hanweidong@huawei.com>
> CC: Mihai Claudiu Caraman <mike.caraman@nxp.com>
> ---
> content.tex | 2 +
> virtio-crypto.tex | 835 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 837 insertions(+)
> create mode 100644 virtio-crypto.tex
>
> diff --git a/content.tex b/content.tex
> index 4b45678..ab75f78 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -5750,6 +5750,8 @@ descriptor for the \field{sense_len}, \field{residual},
> \field{status_qualifier}, \field{status}, \field{response} and
> \field{sense} fields.
>
> +\input{virtio-crypto.tex}
> +
> \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>
> Currently there are three device-independent feature bits defined:
> diff --git a/virtio-crypto.tex b/virtio-crypto.tex
> new file mode 100644
> index 0000000..491fc25
> --- /dev/null
> +++ b/virtio-crypto.tex
> @@ -0,0 +1,835 @@
> +\section{Crypto Device}\label{sec:Device Types / Crypto Device}
> +
> +The virtio crypto device is a virtual crypto device, and is a kind of
> +virtual hardware accelerators for virtual machine. The encryption and
s/accelerators/accelerator/
s/virtual machine/virtual machines/
> +decryption requests of are placed in the data queue, and handled by the
s/of//
> +real crypto accelerators finally. The second queue is the control queue,
> +which is used to create or destroy session for symmetric algorithms, and
s/session/sessions/
> +control some advanced features in the future. The virtio crypto
> +device can provide seven crypto services: CIPHER, MAC, HASH, AEAD,
> +KDF, ASYM, PRIMITIVE.
> +
> +\subsection{Device ID}\label{sec:Device Types / Crypto Device / Device ID}
> +
> +20
> +
> +\subsection{Virtqueues}\label{sec:Device Types / Crypto Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] dataq1
> +\item[\ldots]
> +\item[N-1] dataqN
> +\item[N] controlq
> +\end{description}
> +
> +N is set by \field{max_dataqueues} (\field{max_dataqueues} >= 1).
I suggest dropping (\field{max_dataqueues} >= 1) since this constraint
already is expressed in the device normative section below. Things
can go out of sync if they are duplicated.
> +
> +\subsection{Feature bits}\label{sec:Device Types / Crypto Device / Feature bits}
> + None currently defined
> +
> +\subsection{Device configuration layout}\label{sec:Device Types / Crypto Device / Device configuration layout}
> +
> +Thirteen driver-read-only configuration fields are currently defined.
I count only 12 fields. I suggest saying "The following
driver-read-only configuration fields are currently defined:" instead.
> +\begin{lstlisting}
> +struct virtio_crypto_config {
> + le16 status;
> + le16 max_dataqueues;
> + le32 crypto_services;
> + /* 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;
> +};
> +\end{lstlisting}
> +
> +The first field, \field{status} is currently defined: VIRTIO_CRYPTO_S_HW_READY.
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_S_HW_READY (1 << 0)
> +\end{lstlisting}
> +
> +The following driver-read-only field, \field{max_dataqueuess} specifies the
> +maximum number of data virtqueues (dataq1\ldots dataqN). The crypto_services
> +shows the crypto services the virtio crypto supports. The service currently
s/service/services/
> +defined are:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_NO_SERVICE (0) /* cipher services */
> +#define VIRTIO_CRYPTO_SERVICE_CIPHER (1) /* cipher services */
How are these constants used: 1 << VIRTIO_CRYPTO_NO_SERVICE?
I suggest eliminating the 0 bit constants since they can be deduced from
the fact that all other bits are zeroed. There is no need for a
dedicated "NO_SERVICE" constant.
> +#define VIRTIO_CRYPTO_SERVICE_HASH (2) /* hash service */
> +#define VIRTIO_CRYPTO_SERVICE_MAC (3) /* MAC (Message Authentication Codes) service */
> +#define VIRTIO_CRYPTO_SERVICE_AEAD (4) /* AEAD(Authenticated Encryption with Associated Data) service */
> +\end{lstlisting}
> +
> +The last driver-read-only fields specify detailed algorithms mask which
s/mask/masks/
> +the device offered for corresponding service. The below CIPHER algorithms
s/the device offered for corresponding service/the device offers for
corresponding services/
> +are defined currently:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_NO_CIPHER 0
> +#define VIRTIO_CRYPTO_CIPHER_ARC4 1
> +#define VIRTIO_CRYPTO_CIPHER_AES_ECB 2
> +#define VIRTIO_CRYPTO_CIPHER_AES_CBC 3
> +#define VIRTIO_CRYPTO_CIPHER_AES_CTR 4
> +#define VIRTIO_CRYPTO_CIPHER_DES_ECB 5
> +#define VIRTIO_CRYPTO_CIPHER_DES_CBC 6
> +#define VIRTIO_CRYPTO_CIPHER_3DES_ECB 7
> +#define VIRTIO_CRYPTO_CIPHER_3DES_CBC 8
> +#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
> +\end{lstlisting}
> +
> +The below HASH algorithms are defined currently:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_NO_HASH 0
> +#define VIRTIO_CRYPTO_HASH_MD5 1
> +#define VIRTIO_CRYPTO_HASH_SHA1 2
> +#define VIRTIO_CRYPTO_HASH_SHA_224 3
> +#define VIRTIO_CRYPTO_HASH_SHA_256 4
> +#define VIRTIO_CRYPTO_HASH_SHA_384 5
> +#define VIRTIO_CRYPTO_HASH_SHA_512 6
> +#define VIRTIO_CRYPTO_HASH_SHA3_224 7
> +#define VIRTIO_CRYPTO_HASH_SHA3_256 8
> +#define VIRTIO_CRYPTO_HASH_SHA3_384 9
> +#define VIRTIO_CRYPTO_HASH_SHA3_512 10
> +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE128 11
> +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE256 12
> +\end{lstlisting}
Perhaps all masks should be 64-bit because the 32-bit hash_algo field
already has 13 bits defined so room for future expansion is limited.
> +
> +The below MAC algorithms are defined currently:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_NO_MAC 0
> +#define VIRTIO_CRYPTO_MAC_HMAC_MD5 1
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA1 2
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_224 3
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_256 4
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_384 5
> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_512 6
> +#define VIRTIO_CRYPTO_MAC_CMAC_3DES 25
> +#define VIRTIO_CRYPTO_MAC_CMAC_AES 26
> +#define VIRTIO_CRYPTO_MAC_CMAC_KASUMI_F9 27
> +#define VIRTIO_CRYPTO_MAC_CMAC_SNOW3G_UIA2 28
> +#define VIRTIO_CRYPTO_MAC_GMAC_AES 41
> +#define VIRTIO_CRYPTO_MAC_GMAC_TWOFISH 42
> +#define VIRTIO_CRYPTO_MAC_CBCMAC_AES 49
> +#define VIRTIO_CRYPTO_MAC_CBCMAC_KASUMI_F9 50
> +#define VIRTIO_CRYPTO_MAC_XCBC_AES 53
> +#define VIRTIO_CRYPTO_MAC_ZUC_EIA3 54
> +\end{lstlisting}
> +
> +The below AEAD algorithms are defined currently:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_NO_AEAD 0
> +#define VIRTIO_CRYPTO_AEAD_GCM 1
> +#define VIRTIO_CRYPTO_AEAD_CCM 2
> +#define VIRTIO_CRYPTO_AEAD_CHACHA20_POLY1305 3
> +\end{lstlisting}
> +
> +\subsubsection{Device Requirements: Device configuration layout}\label{sec:Device Types / Crypto Device / Device configuration layout / Device Requirements: Device configuration layout}
> +
> +\begin{itemize*}
This section should use \devicenormative{\subsection}{...}. I'm not
sure why you used a regular \subsubsection{} followed by
\begin{itemize*}.
> +\item The device MUST set \field{max_dataqueues} to between 1 and 65535 inclusive.
> +\item The device SHOULD set \field{status} according to the status of the hardware-backed implementation.
> +\item The device MUST set \field{crypto_services} according to the crypto services which the device offered.
> +\item The device MUST set detailed algorithms mask according to \field{crypto_services} field.
> +\end{itemize*}
> +
> +\subsubsection{Driver Requirements: Device configuration layout}\label{sec:Device Types / Crypto Device / Device configuration layout / Driver Requirements: Device configuration layout}
> +
> +\begin{itemize*}
\drivernormative{\subsection}{...}
> +\item The driver MUST read the ready \field{status} from the bottom bit of status to
> + check whether the hardware-backed implementation is ready or not.
> +\item The driver MAY read \field{max_dataqueues} field to discover how many data queues the device supports.
> +\item The driver MUST read \field{crypto_services} field to discover which services the device is able to offer.
> +\item The driver MUST read detail \field{algorithms} field according to \field{crypto_services} field.
s/detail/the detailed/
> +\end{itemize*}
> +
> +\subsection{Device Initialization}\label{sec:Device Types / Crypto Device / Device Initialization}
> +
> +\subsubsection{Driver Requirements: Device Initialization}\label{sec:Device Types / Crypto Device / Device Initialization / Driver Requirements: Device Initialization}
> +
> +\begin{itemize*}
> +\item The driver MUST identify and initialize data virtqueue, up to \field{max_dataqueues}.
"The driver MUST identify and initialize up to \field{max_dataqueues}
data virtqueues."
Besides the grammar fixes I think this version makes it clearer that the
driver does not have to initialize all data virtqueues if it wants to
use fewer.
> +\item The driver MUST identify the control virtqueue.
> +\item The driver MUST identify the ready status of hardware-backend from \field{status} field.
> +\item The driver MUST read the supported crypto services from bits of \field{crypto_servies}.
> +\item The driver MUST read the supported algorithms according to \field{crypto_services} field.
> +\end{itemize*}
> +
> +\subsubsection{Device Requirements: Device Initialization}\label{sec:Device Types / Crypto Device / Device Initialization / Device Requirements: Device Initialization}
> +
> +\begin{itemize*}
> +\item The device MUST be configured at least one real accelerator as the backend accelerator which executes real crypto operations.
The spec does not have to require a "real" accelerator. A pure software
implementation should be able to conform to the spec. I would drop this
line.
> +\item The device MUST write the \field{crypto_services} field according to the capacities of the backend accelerator.
> +\item The device MUST write the corresponding algorithms field according to the \field{crypto_services} field.
"Device Types / Crypto Device / Device configuration layout / Device Requirements: Device configuration layout" already expresses the same thing. This text seems unnecessary.
> +\end{itemize*}
> +
> +\subsection{Device Operation}\label{sec:Device Types / Crypto Device / Device Operation}
> +
> +Packets can be transmitted by placing them in both the controlq and dataq.
> +The packet are consisted of general header and services specific request,
Grammar: "Packets consist of a generic header and a service-specific request."
> +Where 'general header' is for all crypto request, 'service specific request'
s/request/requests/
> +is composed of operation parameter + output data + input data in generally.
s/is/are/
s/in generally/in general/
> +Operation parameters are algorithm-specific parameters, output data is the
> +data should be operated, input data is the "operation result + result buffer".
> +The general header of controlq:
> +
> +\begin{lstlisting}
> +#define VIRTIO_CRYPTO_OPCODE(service, op) ((service << 8) | (op))
> +
> +struct virtio_crypto_ctrl_header{
> +#define VIRTIO_CRYPTO_CIPHER_CREATE_SESSION VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x02)
> +#define VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x03)
> +#define VIRTIO_CRYPTO_HASH_CREATE_SESSION VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x02)
> +#define VIRTIO_CRYPTO_HASH_DESTROY_SESSION VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x03)
> +#define VIRTIO_CRYPTO_MAC_CREATE_SESSION VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x02)
> +#define VIRTIO_CRYPTO_MAC_DESTROY_SESSION VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x03)
> +#define VIRTIO_CRYPTO_AEAD_CREATE_SESSION VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x02)
> +#define VIRTIO_CRYPTO_AEAD_DESTROY_SESSION VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x03)
> + le32 opcode;
> + /* algo should be service-specific algorithms */
> + le32 algo;
> + /* control flag to control the request */
> + le16 flag;
> + /* data virtqeueu id */
s/virtqeueu/virtqueue/
> + le16 queue_id;
> +};
> +\end{lstlisting}
> +
> +The general header of dataq:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_op_header {
> +#define VIRTIO_CRYPTO_CIPHER_ENCRYPT VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x00)
> +#define VIRTIO_CRYPTO_CIPHER_DECRYPT VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_CIPHER, 0x01)
> +#define VIRTIO_CRYPTO_HASH VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_HASH, 0x00)
> +#define VIRTIO_CRYPTO_MAC VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_MAC, 0x00)
> +#define VIRTIO_CRYPTO_AEAD_ENCRYPT VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x00)
> +#define VIRTIO_CRYPTO_AEAD_DECRYPT VIRTIO_CRYPTO_OPCODE(VIRTIO_CRYPTO_SERVICE_AEAD, 0x01)
> + le32 opcode;
> + /* algo should be service-specific algorithms */
> + le32 algo;
> + /* control flag to control the request */
> + le16 flag;
> + /* data virtqeueu id */
s/virtqeueu/virtqueue/
> + le16 queue_id;
> + /* session_id should be service-specific algorithms */
> + le64 session_id;
This field is not 64-bit aligned. The compiler will insert 32-bit
padding. Please move the field to a 64-bit aligned boundary so that the
binary layout does not change according to the compiler/architecture.
> +};
> +\end{lstlisting}
> +
> +\subsubsection{Control virtqueue}\label{sec:Device Types / Crypto Device / Device Operation / Control virtqueue}
> +
> +The driver uses the control virtqueue to send control commands to the
> +device which finish the non-data plane operations, such as session
s/finish/handles/
> +operations (see \ref{sec:Device Types / Crypto Device / Device Operation / Session operation}).
> +The packet of controlq:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_op_ctrl_req {
> + struct virtio_crypto_ctrl_header header;
> +
> + union {
> + struct virtio_crypto_sym_create_session_req sym_create_session;
> + struct virtio_crypto_hash_create_session_req hash_create_session;
> + struct virtio_crypto_mac_create_session_req mac_create_session;
> + struct virtio_crypto_aead_create_session_req aead_create_session;
> + struct virtio_crypto_destroy_session_req sym_destroy_session;
> + } u;
> +};
> +\end{lstlisting}
> +
> +The header is the general header, the union is algorithm-specific type,
/is algorithm-specific type/is an algorithm-specific type/
> +which is set by the driver. All the properties in the union will be shown as follow.
s/follow/follows/
> +
> +\subsubsection{Session operation}\label{sec:Device Types / Crypto Device / Device Operation / 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:
> +
> +\begin{enumerate}
> +\item The operation (cipher, hash/mac or both, and if both, the order in
> + which the algorithms should be applied).
> +\item The cipher setup data, including the cipher algorithm and mode,
> + the key and its length, and the direction (encrypt or decrypt).
> +\item The hash/mac setup data, including the hash algorithm or mac algorithm,
> + and digest result length (to allow for truncation).
> +\begin{itemize*}
> +\item Authenticated mode can refer to MAC, which requires that the key and
> + its length are also specified.
> +\item For nested mode, the inner and outer prefix data and length are specified,
> + as well as the outer hash algorithm.
> +\end{itemize*}
> +\end{enumerate}
> +
> +\paragraph{Session operation: HASH session}\label{sec:Device Types / Crypto Device / Device Operation / Session operation / Session operation: HASH session}
> +
> +The packet of HASH session is:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_hash_session_para {
> + /* See VIRTIO_CRYPTO_HASH_* above */
> + le32 algo;
> + /* hash result length */
> + le32 hash_result_len;
> +};
> +struct virtio_crypto_hash_create_session_req {
> + // Device-readable part
> + struct virtio_crypto_hash_session_para parameter;
> + // Device-writable part
> + le64 session_id;
> + u8 status;
> +};
> +\end{lstlisting}
> +
> +\paragraph{Session operation: MAC session}\label{sec:Device Types / Crypto Device / Device
> +Operation / Session operation / Session operation: MAC session}
> +
> +The packet of MAC session is:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_mac_session_para {
> + /* See VIRTIO_CRYPTO_MAC_* above */
> + le32 algo;
> + /* hash result length */
> + le32 hash_result_len;
> + /* length of authenticated key */
> + le32 auth_key_len;
> +};
> +struct virtio_crypto_mac_create_session_req {
> + // Device-readable part
> + struct virtio_crypto_mac_session_para parameter;
> + // Device-writable part
> + le64 session_id;
This field is not 64-bit aligned. I suggest manually adding a le32
padding field before it.
Please check all other structs defined in this spec. You can also use
the pahole(1) utility to inspect struct layout chosen by your compiler.
There must be no compiler-generated padding.
> + u8 status;
> +};
> +\end{lstlisting}
> +
> +\paragraph{Session operation: Symmetric algorithms session}\label{sec:Device Types / Crypto Device / Device
> +Operation / Session operation / Session operation: Symmetric algorithms session}
> +
> +The request of symmetric session includes two parts, CIPHER algorithms and chain
> +algorithms (chaining cipher and hash/mac). The packet of symmetric session is:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_cipher_session_para {
> + /* See VIRTIO_CRYPTO_CIPHER* above */
> + le32 algo;
> + /* length of key */
In bytes or in bits?
Attachment:
signature.asc
Description: PGP signature
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]