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

[Michael S. Tsirkin]

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.

-- 
MST