On Friday, July 29, 2016 1:34 PM Michael S. Tsirkin Wrote: > -----Original Message----- > From: Michael S. Tsirkin [mailto:m...@redhat.com] > Sent: Friday, July 29, 2016 1:34 PM > To: Zeng, Xin > Cc: Gonglei (Arei); qemu-devel@nongnu.org; virtio-dev@lists.oasis- > open.org; Ola Liljedahl; Keating, Brian A; Hanweidong (Randy); Luonengjun; > Huangpeng (Peter); Griffin, John; Ma, Liang J; Stefan Hajnoczi; Cornelia Huck; > Varun Sethi; Jani Kokkonen; Lingli Deng; Huangweidong (C) > Subject: Re: [Qemu-devel] [PATCH v5] virtio-crypto: Add virtio crypto device > specification > > On Thu, Jul 28, 2016 at 05:28:33AM +0000, Zeng, Xin wrote: > > On Thursday, July 28, 2016 10:51 AM Gonglei (Arei) Wrote: > > > > > Changes from v4: > > > > > - introduce crypto services into virtio crypto device. The services > > > > > currently defined are CIPHER, MAC, HASH, AEAD, KDF, ASYM, > > > > PRIMITIVE. > > > > > - 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". > > > > > - redefine the algorithms and structure based on above crypto > services. > > > > > - rearrange the title and subtitle > > > > > - Only support CIPHER, MAC, HASH and AEAD crypto services, and Xin > will > > > > > focus KDF, ASYM and PRIMITIVE services. > > > > > - Some other corresponding fixes. > > > > > - Make a formal patch using tex type. > > > > > > > > > > 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. > > > > > > > > OK I will let people who understand crypto comment on this. > > > > > > Excellently, thanks! > > > > > > > Down the road before we release this we'll need to link > > > > confirmance > > > clauses > > > > from confirmance section. Can be a patch on top, no big deal. > > > > > > > > > > Sorry, where is the confirmance section and what's confirmance clauses? > > I meant conformance :) The stuff in conformance.tex > > > > > > > > > > > > > --- > > > > > content.tex | 2 + > > > > > virtio-crypto.tex | 792 > > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > 2 files changed, 794 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: > > > > > > > > Generally we keep it all in a single file. But ok, let's > > > > experiment with this split layout and see whether it makes things > > > > easier or harder. We can always squash it later. > > > > > > > > > diff --git a/virtio-crypto.tex b/virtio-crypto.tex new file mode > > > > > 100644 index 0000000..0918f36 > > > > > --- /dev/null > > > > > +++ b/virtio-crypto.tex > > > > > @@ -0,0 +1,792 @@ > > > > > +\section{Crypto Device}\label{sec:Device Types / 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. 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). > > > > > + > > > > > +\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. > > > > > + > > > > > +\begin{lstlisting} > > > > > +struct virtio_crypto_config { > > > > > + le32 version; > > > > > + 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 driver-read-only field, \field{version} specifies the > > > > > +virtio > > > crypto??s > > > > > +version, which is reserved for back-compatibility in > > > > > +future.It??s currently defined for the version field: > > > > > + > > > > > +\begin{lstlisting} > > > > > +#define VIRTIO_CRYPTO_VERSION_1 (1) \end{lstlisting} > > > > > + > > > > > > > > We have feature bits so I think this should not be necessary. > > > > > > > Yes, you are right. > > > > > > > I think version field is useful in some cases. > > Considering a case, if virtio crypto device implementation has > > defects, and a new version fixes this defect, how can the driver know > > whether the behavior of the device is correct or not? > > Feature bit is mostly like a negotiable field between device and driver. > > You would add a feature bit meaning "bug is fixed". Allows fixing bugs in > driver as well - device looks at driver features and can work around bugs or > fail init. > > --
In this way, each release might need one additional feature bit meaning "some bugs are fixed"? Version field is somewhat redundant with feature bits, but in a real deployment environment, it might be useful for compatible check, especially for the case when backend device and frontend driver are not maintained by same people. > MST
