Re: [Qemu-devel] [virtio-dev] [PATCH v16 1/2] virtio-crypto: Add virtio crypto device specification

[Halil Pasic]

On 02/08/2017 04:46 AM, Gonglei (Arei) wrote:
> Hi Cornelia,
> 
>>
>> On Tue, 7 Feb 2017 12:39:44 +0100
>> Halil Pasic <address@hidden> wrote:
>>
>>> On 01/18/2017 09:22 AM, Gonglei wrote:
>>
>>>> +\section{Crypto Device}\label{sec:Device Types / Crypto Device}
>>>> +
>>>> +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 any of the data active queues and are
>> ultimately handled by the
>>>
>>> Am I the only one having a problem with 'data active queues'?
>>
>> No. I think it looks weird.
>>
>> (...)
>>
>>>> +The value of the \field{status} field is VIRTIO_CRYPTO_S_HW_READY or
>> ~VIRTIO_CRYPTO_S_HW_READY.
>>>
>>> Not entirely happy with this. What you want to say is reserved
>>> for future use, or? Would it make sense to have a general note
>>> -- in a similar fashion like for 'sizes are in bytes' -- for
>>> reserved for future use?
>>>
>>> One possible formulation would be:
>>>
>>> "In this specification, unless explicitly stated otherwise,
>>> fields and bits reserved for future use shall be zeroed out.
>>> Both the a device or a driver device and the driver should
>>> detect violations of this rule, and deny the requested
>>> operation in an appropriate way if possible."
>>
>> If we go with reserved-and-must-be-zero, we need to make rejecting
>> non-zero for reserved value a MUST, or we may run into problems later.
>>
>> In this case, I'd opt for a specific formulation, though; like
>>
>> "The \field{status} field can be either zero or have one or more flags
>> set. Valid flags are listed below."
>>
>> And then state that non-valid flags MUST NOT be set resp. MUST be
>> rejected in a normative statement.
>>
> Sounds good.
> 

This is not only about \field{status} but about the algo mask fields
too. If we go down this path we should make sure to have a specific statement
in each case we reserve something for future use.

The part about the 'normative statement' is very important in my opinion too.

>> (...)
>>
>>>> +The following services are defined:
>>>> +
>>>> +\begin{lstlisting}
>>>> +/* CIPHER service */
>>>> +#define VIRTIO_CRYPTO_SERVICE_CIPHER 0
>>>> +/* HASH service */
>>>> +#define VIRTIO_CRYPTO_SERVICE_HASH   1
>>>> +/* MAC (Message Authentication Codes) service */
>>>> +#define VIRTIO_CRYPTO_SERVICE_MAC    2
>>>> +/* AEAD (Authenticated Encryption with Associated Data) service */
>>>> +#define VIRTIO_CRYPTO_SERVICE_AEAD   3
>>>> +\end{lstlisting}
>>>> +
>>>> +The last driver-read-only fields specify detailed algorithms masks
>>>> +the device offers for corresponding services. The following CIPHER
>> algorithms
>>>> +are defined:
>>>
>>> You do not establish an explicit relationship between the fields and the
>>> macros for the flags. These are flags or? It seems quite common in the
>>> spec to use _F_ in flag names. Would it be appropriate here?
>>
>> The values as specified do not look very flaggy to me...
>>
>>>
>>>> +
>>>> +\begin{lstlisting}
>>>> +#define VIRTIO_CRYPTO_NO_CIPHER                 0
>>>> +#define VIRTIO_CRYPTO_CIPHER_ARC4               1
>>>> +#define VIRTIO_CRYPTO_CIPHER_AES_ECB            2
>>>> +#define VIRTIO_CRYPTO_CIPHER_AES_CBC            3
>>>> +#define VIRTIO_CRYPTO_CIPHER_AES_CTR            4
>>>> +#define VIRTIO_CRYPTO_CIPHER_DES_ECB            5
>>>> +#define VIRTIO_CRYPTO_CIPHER_DES_CBC            6
>>>> +#define VIRTIO_CRYPTO_CIPHER_3DES_ECB           7
>>>> +#define VIRTIO_CRYPTO_CIPHER_3DES_CBC           8
>>>> +#define VIRTIO_CRYPTO_CIPHER_3DES_CTR           9
>>>> +#define VIRTIO_CRYPTO_CIPHER_KASUMI_F8          10
>>>> +#define VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2        11
>>>> +#define VIRTIO_CRYPTO_CIPHER_AES_F8             12
>>>> +#define VIRTIO_CRYPTO_CIPHER_AES_XTS            13
>>>> +#define VIRTIO_CRYPTO_CIPHER_ZUC_EEA3           14
>>>> +\end{lstlisting}
>>>> +
>>>> +
>>>
>>> Would it make sense to interleave the flag definition
>>> with the struct virtio_crypto_config?
>>>
>>>> +\begin{note}
>>>> +Any other values are reserved for future use.
>>>
>>> Are these flags or values? I do not think values is appropriate here.
>>
>> These look like values to me and not flags.
>>
>> The more I stare at this, the more confused I become. Is the device
>> supposed to indicate exactly one algorithm only? Or are these supposed
>> to be bit numbers? (If yes, it would make sense to either call them
>> that or specify flags in the (1 << bit_num) notation.)
> 
> Actually these are both bit numbers and values.
> 
> On the one hand, the device can set algorithms by '1 << bit_num' notation to 
> tell the driver what
> algorithms are supported. On the other hand, the driver can set the value to 
> tell
> the device a virtio crypto request's algorithm in struct 
> virtio_crypto_op_header.algo.
> 
> Pls see cryptodev-builtin.c:: cryptodev_buitlin_init()
> 
> backend->conf.crypto_services =
>                          1u << VIRTIO_CRYPTO_SERVICE_CIPHER |
>                          1u << VIRTIO_CRYPTO_SERVICE_HASH |
>                          1u << VIRTIO_CRYPTO_SERVICE_MAC;
> backend->conf.cipher_algo_l = 1u << VIRTIO_CRYPTO_CIPHER_AES_CBC;
> backend->conf.hash_algo = 1u << VIRTIO_CRYPTO_HASH_SHA1;
> 
> Perhaps I should add a specific formulation about them.


Thanks for the clarification. I think a 'specific formulation' is a good idea.
I suggest to define the constants elsewhere, that is at the site where the
algorithms are defined, and only reference that (or those) definition(s).

Obviously we criteria for valid/invalid for the both usages.
so you will have to describe that separately for the distinct usages.

Concentrate on the fields you are describing and their semantic.

Cheers,
Halil