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

[Gonglei (Arei)]

Hi Halil,

> 
> On 01/16/2017 01:43 PM, Gonglei (Arei) wrote:
> > Hi Michael and others,
> >
> > I'd like to redefine struct virtio_crypto_op_data_req is as below:
> >
> > struct virtio_crypto_op_data_req {
> >     struct virtio_crypto_op_header header;
> >
> >     union {
> >         struct virtio_crypto_sym_data_req   sym_req;
> >         struct virtio_crypto_hash_data_req  hash_req;
> >         struct virtio_crypto_mac_data_req   mac_req;
> >         struct virtio_crypto_aead_data_req  aead_req;
> >         struct virtio_crypto_sym_data_req_non_sess
> sym_non_sess_req;
> >         struct virtio_crypto_hash_data_req_non_sess
> hash_non_sess_req;
> >         struct virtio_crypto_mac_data_req_non_sess
> mac_non_sess_req;
> >         struct virtio_crypto_aead_data_req_non_sess
> aead_non_sess_req;
> >             __u8 padding[72];
> >     } u;
> > };
> >
> > The length of union in the structure will be changed, which from current 48
> byte to 72 byte.
> >
> > We couldn't redefine a structure named
> virtio_crypto_op_data_req_non_sess,
> > Because the feature bits are for crypto services, not for the wrapper packet
> request.
> >
> 
> You mean virtio_crypto_op_data_req.virtio_crypto_op_header.flags
> are conceptually meant for something else and using that field woulb
> be misuse?
> 
Nope...
> 
> > It's impossible to mixed use struct virtio_crypto_op_data_req and
> > struct named virtio_crypto_op_data_req_non_sess for one data virtqueue.
> >
> 
> I do not understand what are you trying to say here. I think you
> should tell us what is the new feature and how it is guarded.
> 
> Would this mean that session or non-session mode will be tied
> to the whole device, or to one data-queue. If it's data-queue
> level how is it controlled (e.g. control queue)?
> 
... I'm so sorry for confusing explanation. Let me try to explain it more clear.

1 ) The first idea: For support non-session mode crypto operations, I introduce 
4 feature bits
for different crypto services, they are:

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.

but not only one feature bit, something like:

VIRTIO_CRYPTO_F_ NON_SESSION_MODE (1) non-session mode is available.

Meanwhile, I define 4 new non-session mode structures for different crypto
services in order to keep compatibility to pre-existing driver.

-*Advantages*:

a) We can support different modes for different crypto services
according to which features are negotiated.

b) The driver can still use the session mode when 
VIRTIO_CRYPTO_F_*_NON_SESSION_MODE are negotiated,
which is under control by 
virtio_crypto_op_data_req.virtio_crypto_op_header.flags.

- *Disadvantages*:

The current crypto packets of all
crypto services (CIPHER, HASH, MAC and AEAD) are wrapped in structure
virtio_crypto_op_data_req by an union in the data plane.

It will change the length of the union and break the pre-existing code
if still lay all non-session mode structures in strut virtio_crypto_op_data_req
like this:

struct virtio_crypto_op_data_req {
    struct virtio_crypto_op_header header;

    union {
        struct virtio_crypto_sym_data_req   sym_req;
        struct virtio_crypto_hash_data_req  hash_req;
        struct virtio_crypto_mac_data_req   mac_req;
        struct virtio_crypto_aead_data_req  aead_req;
        struct virtio_crypto_sym_data_req_non_sess   sym_non_sess_req;
        struct virtio_crypto_hash_data_req_non_sess  hash_non_sess_req;
        struct virtio_crypto_mac_data_req_non_sess   mac_non_sess_req;
        struct virtio_crypto_aead_data_req_non_sess  aead_non_sess_req;
                __u8 padding[72];
    } u;
};

So I should submit patches to fix them.

2) Another idea is define a non-session mode structure for strut 
virtio_crypto_op_data_req.

struct virtio_crypto_op_data_req_non_sess {
    struct virtio_crypto_op_header header;

    union {
        struct virtio_crypto_sym_data_req_non_sess   sym_non_sess_req;
        struct virtio_crypto_hash_data_req_non_sess  hash_non_sess_req;
        struct virtio_crypto_mac_data_req_non_sess   mac_non_sess_req;
        struct virtio_crypto_aead_data_req_non_sess  aead_non_sess_req;
                __u8 padding[72];
    } u;
};

And keep the pre-existing strut virtio_crypto_op_data_req:

struct virtio_crypto_op_data_req {
    struct virtio_crypto_op_header header;

    union {
        struct virtio_crypto_sym_data_req   sym_req;
        struct virtio_crypto_hash_data_req  hash_req;
        struct virtio_crypto_mac_data_req   mac_req;
        struct virtio_crypto_aead_data_req  aead_req;
                __u8 padding[48];
    } u;
};

- *Advantages*: 

Don't break the pre-existing code.

- *Disadvantages*: 

Can't support feature bits for different crypto services,
only the whole device. That means we should only use the below feature
bit:

VIRTIO_CRYPTO_F_ NON_SESSION_MODE (1) non-session mode is available.


3) The last idea is define a mixed structure for strut 
virtio_crypto_op_data_req.

struct virtio_crypto_op_data_req_mixed {
    struct virtio_crypto_op_header header;

    union {
        struct virtio_crypto_sym_data_req   sym_req;
        struct virtio_crypto_hash_data_req  hash_req;
        struct virtio_crypto_mac_data_req   mac_req;
        struct virtio_crypto_aead_data_req  aead_req;
        struct virtio_crypto_sym_data_req_non_sess   sym_non_sess_req;
        struct virtio_crypto_hash_data_req_non_sess  hash_non_sess_req;
        struct virtio_crypto_mac_data_req_non_sess   mac_non_sess_req;
        struct virtio_crypto_aead_data_req_non_sess  aead_non_sess_req;
                __u8 padding[72];
    } u;
};

And keep the pre-existing strut virtio_crypto_op_data_req:

struct virtio_crypto_op_data_req {
    struct virtio_crypto_op_header header;

    union {
        struct virtio_crypto_sym_data_req   sym_req;
        struct virtio_crypto_hash_data_req  hash_req;
        struct virtio_crypto_mac_data_req   mac_req;
        struct virtio_crypto_aead_data_req  aead_req;
                __u8 padding[48];
    } u;
};

That means we should use below five 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.

-*Advantages*:

Both idea 1) and 2).

-*Disadvantages*:

None.

What's your opinion? Thanks a lot!


> I guess virtio_crypto_op_data_req.virtio_crypto_op_header.session_id
> would become meaningless in case of non_sess?
> 
That's true.

> > For driver compabity, I can submit patches for linux dirver and Qemu to
> change the length
> > of struct virtio_crypto_op_data_req.u
> >
> > Is the approach available?
> >
> 
> In general and AFAIU any new behavior is possible, iff it
> is appropriately guarded by some negotiation mechanism and
> does not break per-existing code which knows nothing about
> the new stuff.
> 
> I would not mind seeing a spec re-spin with a proper
> proposal for session-less or stateless or whatever mode.
> 

Thanks,
-Gonglei

> Cheers,
> Halil
> 
> > Thanks,
> > -Gonglei
> >
> >
> >> -----Original Message-----
> >> From: Gonglei (Arei)
> >> Sent: Saturday, January 14, 2017 9:21 AM
> >> Subject: RE: [virtio-dev] Re: [Qemu-devel] [PATCH v15 0/2] virtio-crypto:
> virtio
> >> crypto device specification
> >>
> >>>
> >>> On Fri, Jan 13, 2017 at 02:54:29AM +0000, Gonglei (Arei) wrote:
> >>>>
> >>>>>
> >>>>> 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
> >>>
> >>> So you will have to maintain both structures for when non-session based
> >>> feature is and aren't present. You will have to give them different
> >>> names, too.
> >>>
> >> OK, I get your point. :)
> >>
> >> Thanks,
> >> -Gonglei
> >
> >