Re: [Qemu-devel] [PATCH v5] virtio-crypto: Add virtio crypto device specification

[Zeng, Xin]

On Friday, July 29, 2016 1:34 PM Michael S. Tsirkin Wrote:
> -----Original Message-----
> From: Michael S. Tsirkin [mailto:address@hidden
> Sent: Friday, July 29, 2016 1:34 PM
> To: Zeng, Xin
> Cc: Gonglei (Arei); address@hidden; address@hidden
> 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