Hi Stefan,
> -----Original Message----- > From: Stefan Hajnoczi [mailto:stefa...@redhat.com] > Sent: Friday, September 02, 2016 8:07 PM > 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. > Thank you very much! Forgive me for my poor English please :) I'll fix them in the following version. > > 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.gong...@huawei.com> > > CC: Michael S. Tsirkin <m...@redhat.com> > > CC: Cornelia Huck <cornelia.h...@de.ibm.com> > > CC: Stefan Hajnoczi <stefa...@redhat.com> > > CC: Lingli Deng <denglin...@chinamobile.com> > > CC: Jani Kokkonen <jani.kokko...@huawei.com> > > CC: Ola Liljedahl <ola.liljed...@arm.com> > > CC: Varun Sethi <varun.se...@freescale.com> > > CC: Zeng Xin <xin.z...@intel.com> > > CC: Keating Brian <brian.a.keat...@intel.com> > > CC: Ma Liang J <liang.j...@intel.com> > > CC: Griffin John <john.grif...@intel.com> > > CC: Hanweidong <hanweid...@huawei.com> > > CC: Mihai Claudiu Caraman <mike.cara...@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. > Good points. > > +#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. > Actually the cipher's mask and mac's mask had used 64-bit. Xin, what's your opinion? > > + > > +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*}. > Ok. > > +\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. > Maybe we can change this line with " The device MUST be configured at least one accelerator which executes crypto operations " I think this is better than drop it. > > +\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. > Yes, as Alex's comments, I'll redefine all structures to keep 64bit aligned. > > +}; > > +\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. > Ok, thanks! > > + 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? Bytes. All lengths are bytes in the spec. Regards, -Gonglei
