Hi Halil, Thanks for your comments firstly.
> > On 01/18/2017 09:22 AM, Gonglei wrote: > > The virtio crypto device is a virtual crypto device (ie. hardware > > crypto accelerator card). Currently, the virtio crypto device provides > > the following crypto services: CIPHER, MAC, HASH, and AEAD. > > > > In this patch, CIPHER, MAC, HASH, AEAD services are introduced. > > > > VIRTIO-153 > > > > 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: Mihai Claudiu Caraman <mike.cara...@nxp.com> > > --- > > content.tex | 2 + > > virtio-crypto.tex | 1245 > +++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 1247 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..732cd30 > > --- /dev/null > > +++ b/virtio-crypto.tex > > @@ -0,0 +1,1245 @@ > > +\section{Crypto Device}\label{sec:Device Types / Crypto Device} > > + > > +The virtio crypto device is a virtual cryptography device as well as a > > kind of > > +virtual hardware accelerator for virtual machines. The encryption and > > +decryption requests are placed in any of the data active queues and are > ultimately handled by the > > Am I the only one having a problem with 'data active queues'? Maybe 'data queues' here is enough? > I have objected on this before. > > > +backend crypto accelerators. The second kind of queue is the control queue > used to create > > +or destroy sessions for symmetric algorithms and will control some > advanced > > +features in the future. The virtio crypto device provides the following > > crypto > > +services: CIPHER, MAC, HASH, and AEAD. > > + > > + > > +\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}. > > + > > +\subsection{Feature bits}\label{sec:Device Types / Crypto Device / Feature > bits} > > + > > +VIRTIO_CRYPTO_F_NON_SESSION_MODE (0) non-session mode is available. > > +VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE (1) non-session mode is > available for CIPHER service. > > +VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE (2) non-session mode is > available for HASH service. > > +VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE (3) non-session mode is > available for MAC service. > > +VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE (4) non-session mode is > available for AEAD service. > > + > > I'm not sure that "non-session" entirely correct grammatically. I would > prefer sessionless as alternatively proposed by Stefan, or even stateless. > I think stateless is the phrase most frequently used to describe what > we want to introduce -- that is basically response = f(request) and > not response = f(request, server_state) where the server_state is > a is determined by a series of previous interactions between the server > and the client). > Makes sense. I discussed with Xin about this on call meeting two weeks ago. I decide to use stateless mode instead of non-session mode here. > > +\subsubsection{Feature bit requirements}\label{sec:Device Types / Crypto > Device / Feature bits} > > + > > +Some crypto feature bits require other crypto feature bits > > +(see \ref{drivernormative:Basic Facilities of a Virtio Device / Feature > > Bits}): > > + > > +\begin{description} > > +\item[VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE] Requires > VIRTIO_CRYPTO_F_NON_SESSION_MODE. > > +\item[VIRTIO_CRYPTO_F_HASH_NON_SESSION_MODE] Requires > VIRTIO_CRYPTO_F_NON_SESSION_MODE. > > +\item[VIRTIO_CRYPTO_F_MAC_NON_SESSION_MODE] Requires > VIRTIO_CRYPTO_F_NON_SESSION_MODE. > > +\item[VIRTIO_CRYPTO_F_AEAD_NON_SESSION_MODE] Requires > VIRTIO_CRYPTO_F_NON_SESSION_MODE. > > +\end{description} > > + > > +\subsection{Device configuration layout}\label{sec:Device Types / Crypto > Device / Device configuration layout} > > + > > +The following driver-read-only configuration fields are defined: > > + > > +\begin{lstlisting} > > +struct virtio_crypto_config { > > + le32 status; > > + le32 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 aead_algo; > > + /* Maximum length of cipher key */ > > + le32 max_cipher_key_len; > > + /* Maximum length of authenticated key */ > > + le32 max_auth_key_len; > > + le32 reserved; > > + /* Maximum size of each crypto request's content */ > > + le64 max_size; > > +}; > > +\end{lstlisting} > > + > > +The value of the \field{status} field is VIRTIO_CRYPTO_S_HW_READY or > ~VIRTIO_CRYPTO_S_HW_READY. > > Not entirely happy with this. What you want to say is reserved > for future use, or? Would it make sense to have a general note > -- in a similar fashion like for 'sizes are in bytes' -- for > reserved for future use? > > One possible formulation would be: > > "In this specification, unless explicitly stated otherwise, > fields and bits reserved for future use shall be zeroed out. > Both the a device or a driver device and the driver should > detect violations of this rule, and deny the requested > operation in an appropriate way if possible." > > Cornelia also provided a good suggestion about this. :) > > + > > +\begin{lstlisting} > > +#define VIRTIO_CRYPTO_S_HW_READY (1 << 0) > > +\end{lstlisting} > > + > > +The VIRTIO_CRYPTO_S_HW_READY flag is used to show whether the > hardware is ready to work or not. > > I do not like hardware here. > Sorry about that, but it has existed in both Qemu and Linux driver. :( > > + > > +The following driver-read-only fields include \field{max_dataqueues}, which > specifies the > > Why following? > Er, I used 'following' at here and there... It's just an auxiliary word. > > +maximum number of data virtqueues (dataq1\ldots dataqN), and > \field{crypto_services}, > > +which indicates the crypto services the virtio crypto supports. > > + > > +The following services are defined: > > + > > +\begin{lstlisting} > > +/* CIPHER service */ > > +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0 > > +/* HASH service */ > > +#define VIRTIO_CRYPTO_SERVICE_HASH 1 > > +/* MAC (Message Authentication Codes) service */ > > +#define VIRTIO_CRYPTO_SERVICE_MAC 2 > > +/* AEAD (Authenticated Encryption with Associated Data) service */ > > +#define VIRTIO_CRYPTO_SERVICE_AEAD 3 > > +\end{lstlisting} > > + > > +The last driver-read-only fields specify detailed algorithms masks > > +the device offers for corresponding services. The following CIPHER > algorithms > > +are defined: > > You do not establish an explicit relationship between the fields and the > macros for the flags. These are flags or? It seems quite common in the > spec to use _F_ in flag names. Would it be appropriate here? > > > + > > +\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 following HASH algorithms are defined: > > + > > +\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} > > + > > +The following MAC algorithms are defined: > > + > > +\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_KASUMI_F9 27 > > +#define VIRTIO_CRYPTO_MAC_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 following AEAD algorithms are defined: > > + > > +\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} > > + > > Would it make sense to interleave the flag definition > with the struct virtio_crypto_config? > > > +\begin{note} > > +Any other values are reserved for future use. > > Are these flags or values? I do not think values is appropriate here. > Please refer to my reply in another thread. > > +\end{note} > > + > > Some fields are missing here. This is inconsistent. Either > you should describe all or none (here). > Ok, will add other fields. > > +\devicenormative{\subsubsection}{Device configuration layout}{Device Types > / Crypto Device / Device configuration layout} > > + > > +\begin{itemize*} > > +\item The device MUST set \field{max_dataqueues} to between 1 and 65535 > inclusive. > > +\item The device MUST set \field{status} based on the status of the > hardware-backed implementation. > > What is a hardware-backend? > I meant the cryptodev backend, cryptodev-builtin, cryptodev-vhost-user for example. > > +\item The device MUST accept and handle requests after \field{status} is > > set > to VIRTIO_CRYPTO_S_HW_READY. > > Shouldn't this be the other way around (if VIRTIO_CRYPTO_S_HW_READY > then reject). Is a not well formed request or a backend failure considered? > What does handle mean? What should happen if requests are submitted > before VIRTIO_CRYPTO_S_HW_READY is set? > The requests MUST not be transmitted before VIRTIO_CRYPTO_S_HW_READY is set. Which is stated by the driver requirements. Otherwise the requests will be dropped. The handle means execute crypto operations. > > +\item The device MUST set \field{crypto_services} based on the crypto > services the device offers. > > +\item The device MUST set detailed algorithms masks based on the > \field{crypto_services} field. > > +\item The device MUST set \field{max_size} to show the maximum size of > crypto request the device supports. > > +\item The device MUST set \field{max_cipher_key_len} to show the > maximum length of cipher key if the device supports CIPHER service. > > +\item The device MUST set \field{max_auth_key_len} to show the maximum > length of authenticated key if the device supports MAC service. > > +\end{itemize*} > > + > > +\drivernormative{\subsubsection}{Device configuration layout}{Device Types > / Crypto Device / Device configuration layout} > > + > > +\begin{itemize*} > > +\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, and the driver MUST reread it after > the device reset. > > +\item The driver MUST NOT transmit any packets to the device if the ready > \field{status} is not set. > > +\item The driver MUST read \field{max_dataqueues} field to discover the > number of 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 the detailed algorithms fields based on > \field{crypto_services} field. > > +\item The driver SHOULD read \field{max_size} to discover the maximum > size of crypto request the device supports. > > +\item The driver SHOULD read \field{max_cipher_key_len} to discover the > maximum length of cipher key the device supports. > > +\item The driver SHOULD read \field{max_auth_key_len} to discover the > maximum length of authenticated key the device supports. > > +\end{itemize*} > > + > > +\subsection{Device Initialization}\label{sec:Device Types / Crypto Device / > Device Initialization} > > + > > +\drivernormative{\subsubsection}{Device Initialization}{Device Types / > Crypto Device / Device Initialization} > > + > > +\begin{itemize*} > > +\item The driver MUST identify and initialize the control virtqueue. > > But does not have to identify and initialize any data virtqueues? > Good catch. Both controlq and dataq. > > +\item The driver MUST read the supported crypto services from bits of > \field{crypto_services}. > > +\item The driver MUST read the supported algorithms based on > \field{crypto_services} field. > > +\end{itemize*} > > + > > +\devicenormative{\subsubsection}{Device Initialization}{Device Types / > Crypto Device / Device Initialization} > > + > > +\begin{itemize*} > > +\item The device MUST be configured with at least one accelerator which > executes backend crypto operations. > > What does configured mean here? Is this initialization requirement. > It means the virtio crypto device MUST attach cryptodev backend. Yes, it's a requirement. > > +\item The device MUST write the \field{crypto_services} field based on the > capacities of the backend accelerator. > > +\end{itemize*} > > + > > How do 'Initialization' and 'Configuration Layout' requirements relate to > each-other. > Basically they do the same things. But 'configuration layout' can't state the specific time. > > +\subsection{Device Operation}\label{sec:Device Types / Crypto Device / > Device Operation} > > + > > +Packets can be transmitted by placing them in both the controlq and dataq. > > +Packets consist of a general header and a service-specific request. > > Are packets and requests synonyms? > Yes, they are in virtio crypto spec. > > +Where 'general header' is for all crypto requests, and 'service specific > requests' > > From below it seems you have two types of 'general header', but up until this > point it seems there is a single definition. Of course, this does not > really matter. > I distinguish it based on controlq and dataq in following statement. > > +are composed of operation parameter + output data + input data in general. > > +Operation parameters are algorithm-specific parameters, output data is the > > +data that should be utilized in operations, and input data is equal to > > +"operation result + result data". > > + > > +The device can support both session mode (See \ref{sec:Device Types / > Crypto Device / Device Operation / Control Virtqueue / Session operation}) and > non-session mode, for example, > > +As VIRTIO_CRYPTO_F_CIPHER_NON_SESSION_MODE feature bit is > negotiated, the driver can use non-session mode for CIPHER service, otherwise > it can only use session mode. > > Grammar: Does not seem right to me. 'As' seems off and this could be two > sentences. > OK. Let me s/As/If the/ here. > > > + > > +\begin{note} > > +The basic unit of all data length the byte. > > +\end{note} > > + > > +The general header for controlq is as follows: > > + > > +\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; > > + le32 algo; > > + le32 flag; > > + /* data virtqueue id */ > > + le32 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; > > + /* session_id should be service-specific algorithms */ > > + le64 session_id; > > +#define VIRTIO_CRYPTO_FLAG_SESSION_MODE 1 > > +#define VIRTIO_CRYPTO_FLAG_NON_SESSION_MODE 2 > > Use _F_ istead of _FLAG_? > I'm afraid they are confused with the feature bits. > > + /* control flag to control the request */ > > + le32 flag; > > + le32 padding; > > +}; > > +\end{lstlisting} > > + > > +The device can set the operation status as follows: VIRTIO_CRYPTO_OK: > success; > > +VIRTIO_CRYPTO_ERR: failure or device error; VIRTIO_CRYPTO_NOTSUPP: > not supported; > > +VIRTIO_CRYPTO_INVSESS: invalid session ID when executing crypto > operations. > > + > > +\begin{lstlisting} > > +enum VIRITO_CRYPTO_STATUS { > > + VIRTIO_CRYPTO_OK = 0, > > + VIRTIO_CRYPTO_ERR = 1, > > + VIRTIO_CRYPTO_BADMSG = 2, > > + VIRTIO_CRYPTO_NOTSUPP = 3, > > + VIRTIO_CRYPTO_INVSESS = 4, > > + VIRTIO_CRYPTO_MAX > > +}; > > +\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 handles the non-data plane operations, such as session > > What is a 'non-data plane'? > I named controlling plane as the non-data plane. Maybe it's superfluous. > > +operations (See \ref{sec:Device Types / Crypto Device / Device Operation / > Control Virtqueue / Session operation}). > > Reviewed up until here. Depending on how things evolve will try to > review the rest too in the following days. > I knew it's a hard and time-consuming work to review such long spec. Great thanks for your reviewing. :) > A question and a remark as a closing word: > * Are there already some kernel and qemu patches for this 'non-session' stuff? THB currently I have a patch for virtio_crypto.h. Attaching it for better review. > * I think some proofreading (and eventually also touch-up) by a native speaker > would really benefit us. Sadly my grammar skills are very questionable, so > I can't help much. Nevertheless since it is a spec, I think we sould strive > for high standards in language usage too. > I agree and I actually asked a native speaker in my company to review it several months ago. When all comments are handled, I'll do it again. Thanks, -Gonglei
0001-virtio-crypto-update-header-file.patch
Description: 0001-virtio-crypto-update-header-file.patch