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

[Gonglei (Arei)]

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

> >> (...)
> >>
> >>>> +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).
> 
Good idea.

> Obviously we criteria for valid/invalid for the both usages.
> so you will have to describe that separately for the distinct usages.
> 
Oh, so much work need to be done.

Halil, Would you mind work together with me to perfect the spec?
And feel free to add your signed-off-by tag. :)

TBH as a non-native English speaker, it's more difficult writing a
spec than coding. :(

Look forward to your reply.

Thanks,
-Gonglei

> Concentrate on the fields you are describing and their semantic.
> 
> Cheers,
> Halil