Re: [Qemu-devel] [virtio-dev] Re: [PATCH v15 0/2] virtio-crypto: virtio crypto device specification

[Gonglei (Arei)]

> 
> On Thu, Jan 12, 2017 at 12:26:24PM +0000, Gonglei (Arei) wrote:
> > Hi,
> >
> >
> > >
> > > On 01/04/2017 11:10 AM, Gonglei (Arei) wrote:
> > > > Hi all,
> > > >
> > > > I attach the diff files between v14 and v15 for better review.
> > > >
> > > Hi,
> > >
> > > only had a quick look. Will try to come back to this later.
> > >
> > That's cool.
> >
> > > > diff --git a/virtio-crypto.tex b/virtio-crypto.tex
> > > > index 9f7faf0..884ee95 100644
> > > > --- a/virtio-crypto.tex
> > > > +++ b/virtio-crypto.tex
> > > > @@ -2,8 +2,8 @@
> > > >
> > > >  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 the data queue and are ultimately
> handled
> > > by the
> > > > -backend crypto accelerators. The second queue is the control queue used
> to
> > > create
> > > > +decryption requests are placed in any of the data active queues and are
> > > ultimately handled by the
> > > s/data active/active data/
> > > > +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.
> > >
> > > [..]
> > >
> > > > ===============The below diff shows the changes of add non-session
> mode
> > > support:
> > > >
> > > > diff --git a/virtio-crypto.tex b/virtio-crypto.tex
> > > > index 884ee95..44819f9 100644
> > > > --- a/virtio-crypto.tex
> > > > +++ b/virtio-crypto.tex
> > > > @@ -26,7 +26,10 @@ N is set by \field{max_dataqueues}.
> > > >
> > > >  \subsection{Feature bits}\label{sec:Device Types / Crypto Device /
> Feature
> > > bits}
> > > >
> > > > -None currently defined.
> > > > +VIRTIO_CRYPTO_F_CIPHER_SESSION_MODE (1) Session mode is
> available
> > > for CIPHER service.
> > > > +VIRTIO_CRYPTO_F_HASH_SESSION_MODE (2) Session mode is available
> for
> > > HASH service.
> > > > +VIRTIO_CRYPTO_F_MAC_SESSION_MODE (3) Session mode is available
> for
> > > MAC service.
> > > > +VIRTIO_CRYPTO_F_AEAD_SESSION_MODE (4) Session mode is available
> for
> > > AEAD service.
> > > >
> > > >  \subsection{Device configuration layout}\label{sec:Device Types /
> Crypto
> > > Device / Device configuration layout}
> > > >
> > > > @@ -208,6 +211,9 @@ 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_SESSION feature bit is negotiated, the
> driver
> > > can use session mode for CIPHER service, otherwise it can only use
> non-session
> > > mode.
> > > > +
> > >
> > > As far as I understand you are adding non-session mode to the mix but
> > > providing feature bits for session mode. Would this render the the current
> > > implementation non-compliant?
> > >
> > You are right, shall we use feature bits for non-session mode for 
> > compliancy?
> > Or because the spec is on the fly, and some structures in the 
> > virtio_crypto.h
> need to
> > be modified, can we keep the compliancy completely?
> >
> > Thanks,
> > -Gonglei
> 
> Since there's a linux driver upstream you must at least
> keep compatibility with that.
> 
Sure. We use feature bits for non-session mode then.
For structures modification, do we need to do some specific
actions for compatibility?  Take CIPHER service as an example:

The present structure:

struct virtio_crypto_cipher_para {
    /*
     * Byte Length of valid IV/Counter data pointed to by the below iv data.
     *
     * For block ciphers in CBC or F8 mode, or for Kasumi in F8 mode, or for
     *   SNOW3G in UEA2 mode, this is the length of the IV (which
     *   must be the same as the block length of the cipher).
     * For block ciphers in CTR mode, this is the length of the counter
     *   (which must be the same as the block length of the cipher).
     */
    le32 iv_len;
    /* length of source data */
    le32 src_data_len;
    /* length of destination data */
    le32 dst_data_len;
};

The future structure if supporting non-session based operations:

struct virtio_crypto_cipher_para {
    struct {
        /* See VIRTIO_CRYPTO_CIPHER* above */
        le32 algo;
        /* length of key */
        le32 keylen;

        /* See VIRTIO_CRYPTO_OP_* above */
        le32 op;
    } sess_para;

    /*
     * Byte Length of valid IV/Counter data pointed to by the below iv data.
     *
     * For block ciphers in CBC or F8 mode, or for Kasumi in F8 mode, or for
     *   SNOW3G in UEA2 mode, this is the length of the IV (which
     *   must be the same as the block length of the cipher).
     * For block ciphers in CTR mode, this is the length of the counter
     *   (which must be the same as the block length of the cipher).
     */
    le32 iv_len;
    /* length of source data */
    le32 src_data_len;
    /* length of destination data */
    le32 dst_data_len;
};

Thanks,
-Gonglei