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

[Halil Pasic]

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?


> 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 guess virtio_crypto_op_data_req.virtio_crypto_op_header.session_id
would become meaningless in case of non_sess?

> 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.

Cheers,
Halil
 
> Thanks,
> -Gonglei
> 
> 
>> -----Original Message-----
>> From: Gonglei (Arei)
>> Sent: Saturday, January 14, 2017 9:21 AM
>> To: 'Michael S. Tsirkin'
>> Cc: Halil Pasic; address@hidden; address@hidden;
>> Huangweidong (C); address@hidden; address@hidden;
>> Zhoujian (jay, Euler); address@hidden; address@hidden;
>> address@hidden; address@hidden; address@hidden;
>> address@hidden; address@hidden; Luonengjun;
>> address@hidden; address@hidden; address@hidden; Shiqing Fan;
>> Jani Kokkonen; address@hidden; Claudio Fontana;
>> address@hidden; Wubin (H)
>> 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
> 
>