On 2016/11/17 2:11, Halil Pasic wrote:
> On 11/11/2016 10:23 AM, Gonglei wrote:
>> The virtio crypto device is a virtual crypto device (ie. hardware
>> crypto accelerator card). Currently, the virtio crypto device provides
>> the following crypto services: CIPHER, MAC, HASH, and AEAD.
>>
>> In this patch, CIPHER, MAC, HASH, AEAD services are introduced.
>>
>> VIRTIO-153
>>
>> Signed-off-by: Gonglei<arei.gong...@huawei.com>
>> CC: Michael S. Tsirkin<m...@redhat.com>
>> CC: Cornelia Huck<cornelia.h...@de.ibm.com>
>> CC: Stefan Hajnoczi<stefa...@redhat.com>
>> CC: Lingli Deng<denglin...@chinamobile.com>
>> CC: Jani Kokkonen<jani.kokko...@huawei.com>
>> CC: Ola Liljedahl<ola.liljed...@arm.com>
>> CC: Varun Sethi<varun.se...@freescale.com>
>> CC: Zeng Xin<xin.z...@intel.com>
>> CC: Keating Brian<brian.a.keat...@intel.com>
>> CC: Ma Liang J<liang.j...@intel.com>
>> CC: Griffin John<john.grif...@intel.com>
>> CC: Hanweidong<hanweid...@huawei.com>
>> CC: Mihai Claudiu Caraman<mike.cara...@nxp.com>
>> ---
>> content.tex | 2 +
>> virtio-crypto.tex | 945
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 947 insertions(+)
>> create mode 100644 virtio-crypto.tex
>>
>> diff --git a/content.tex b/content.tex
>> index 4b45678..ab75f78 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -5750,6 +5750,8 @@ descriptor for the \field{sense_len}, \field{residual},
>> \field{status_qualifier}, \field{status}, \field{response} and
>> \field{sense} fields.
>>
>> +\input{virtio-crypto.tex}
>> +
>> \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>
>> Currently there are three device-independent feature bits defined:
>> diff --git a/virtio-crypto.tex b/virtio-crypto.tex
>> new file mode 100644
>> index 0000000..9f7faf0
>> --- /dev/null
>> +++ b/virtio-crypto.tex
>> @@ -0,0 +1,945 @@
>> +\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 the data queue and are ultimately handled
>> by the
> ~~~~~~~~~~~~~~
> The data queue can be misleading since its rather any of the data active
> queues.
Ok.
>> +backend crypto accelerators. The second queue is the control queue used to
>> create
> ~~~~~~~~~~~~
> This could be confusing since it is a second type or kind of queue but
> not necessarily the queue with index 1.
Will change it.
>
>> +or destroy sessions for symmetric algorithms and will control some advanced
>> +features in the future. The virtio crypto device provides the following
>> crypto
> Promising future advanced features seems to be out of scope for this
> specification.
That's true, but I'd like to keep this statement so that people can know
extend functions for controlq.
>> +services: CIPHER, MAC, HASH, and AEAD.
>> +
>> +
>> +\subsection{Device ID}\label{sec:Device Types / Crypto Device / Device ID}
>> +
>> +20
>> +
>> +\subsection{Virtqueues}\label{sec:Device Types / Crypto Device / Virtqueues}
>> +
>> +\begin{description}
>> +\item[0] dataq1
>> +\item[\ldots]
>> +\item[N-1] dataqN
>> +\item[N] controlq
>> +\end{description}
>> +
>> +N is set by \field{max_dataqueues}.
>> +
>> +\subsection{Feature bits}\label{sec:Device Types / Crypto Device / Feature
>> bits}
>> +
>> +Undefined currently.
> Could use "None currently defined." like entropy device.
OK.
>> +
>> +\subsection{Device configuration layout}\label{sec:Device Types / Crypto
>> Device / Device configuration layout}
>> +
>> +The following driver-read-only configuration fields are defined:
>> +
>> +\begin{lstlisting}
>> +struct virtio_crypto_config {
>> + le32 status;
>> + le32 max_dataqueues;
>> + le32 crypto_services;
>> + /* Detailed algorithms mask */
>> + le32 cipher_algo_l;
>> + le32 cipher_algo_h;
>> + le32 hash_algo;
>> + le32 mac_algo_l;
>> + le32 mac_algo_h;
>> + le32 aead_algo;
>> + /* Maximum length of cipher key */
>> + le32 max_cipher_key_len;
>> + /* Maximum length of authenticated key */
>> + le32 max_auth_key_len;
>> + le32 reserve;
>> + /* Maximum size of each crypto request's content */
>> + le64 max_size;
>> +};
>> +\end{lstlisting}
>> +
>> +The value of the \field{status} field is VIRTIO_CRYPTO_S_HW_READY or
>> VIRTIO_CRYPTO_S_STARTED.
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_CRYPTO_S_HW_READY (1 << 0)
>> +#define VIRTIO_CRYPTO_S_STARTED (1 << 1)
>> +\end{lstlisting}
>> +
> Could not really figure out what this status actually does and how does
> it relate to the device status field if at all.
>
> Furthermore I see no mention of VIRTIO_CRYPTO_S_STARTED except for this
> one, so the only thing I can think of is that it's the initial value and
> means hardware not ready (you state these are the only two values).
Good catch. I set VIRTIO_CRYPTO_S_STARTED in the driver if the
virtio-crypto driver is ready to
work in the guest (registing to the Linux Crypto Framework and the
device is ready), vice versa.
> This however does not seem consistent with what your QEMU reference
> implementation does. Another thing is your implementations seem to
> use VIRTIO_CRYPTO_S_HW_READY as flag but your specification would
> (prohibit combining flags because you get another value).
The QEMU side doesn't use VIRTIO_CRYPTO_S_STARTED, so maybe I can remove
it from
the spec and define it in the driver. Does it make sense?
> There are more comments on this topic below.
>
>> +The following driver-read-only fields include \field{max_dataqueues}, which
>> specifies the
>> +maximum number of data virtqueues (dataq1\ldots dataqN), and
>> \field{crypto_services},
>> +which indicates the crypto services the virtio crypto supports.
>> +
>> +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:
>> +
>> +\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}
>> +
> You could clarify that these values have double meaning. Each value
> uniquely identifies an cipher algorithm and a bit in a 'algorithm mask'
> bitmap represented as le32 cipher_algo_l and le32 cipher_algo_h so that
> availability could be checked like this:
>
> bool is_avail(uint16_t flag)
> {
> return flag < 32 ? (le32_to_cpu(config.chiper_algo_l) & (1UL << flag)) :
> (flag < 64 ? (le32_to_cpu(config.chiper_algo_h) & (1UL << (flag -
> 32))): false);
> }
Good point.
> I'm curious what is the purpose of VIRTIO_CRYPTO_NO_CIPHER?
This macro is kept for the packets doesn't need to execute CIPHER operations
if it exists those requirements in some situations.
>> +The following HASH algorithms are defined:
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_CRYPTO_NO_HASH 0
>> +#define VIRTIO_CRYPTO_HASH_MD5 1
>> +#define VIRTIO_CRYPTO_HASH_SHA1 2
>> +#define VIRTIO_CRYPTO_HASH_SHA_224 3
>> +#define VIRTIO_CRYPTO_HASH_SHA_256 4
>> +#define VIRTIO_CRYPTO_HASH_SHA_384 5
>> +#define VIRTIO_CRYPTO_HASH_SHA_512 6
>> +#define VIRTIO_CRYPTO_HASH_SHA3_224 7
>> +#define VIRTIO_CRYPTO_HASH_SHA3_256 8
>> +#define VIRTIO_CRYPTO_HASH_SHA3_384 9
>> +#define VIRTIO_CRYPTO_HASH_SHA3_512 10
>> +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE128 11
>> +#define VIRTIO_CRYPTO_HASH_SHA3_SHAKE256 12
>> +\end{lstlisting}
>> +
>> +The following MAC algorithms are defined:
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_CRYPTO_NO_MAC 0
>> +#define VIRTIO_CRYPTO_MAC_HMAC_MD5 1
>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA1 2
>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_224 3
>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_256 4
>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_384 5
>> +#define VIRTIO_CRYPTO_MAC_HMAC_SHA_512 6
>> +#define VIRTIO_CRYPTO_MAC_CMAC_3DES 25
>> +#define VIRTIO_CRYPTO_MAC_CMAC_AES 26
>> +#define VIRTIO_CRYPTO_MAC_KASUMI_F9 27
>> +#define VIRTIO_CRYPTO_MAC_SNOW3G_UIA2 28
>> +#define VIRTIO_CRYPTO_MAC_GMAC_AES 41
>> +#define VIRTIO_CRYPTO_MAC_GMAC_TWOFISH 42
>> +#define VIRTIO_CRYPTO_MAC_CBCMAC_AES 49
>> +#define VIRTIO_CRYPTO_MAC_CBCMAC_KASUMI_F9 50
>> +#define VIRTIO_CRYPTO_MAC_XCBC_AES 53
>> +#define VIRTIO_CRYPTO_MAC_ZUC_EIA3 54
>> +\end{lstlisting}
>> +
>> +The following AEAD algorithms are defined:
>> +
>> +\begin{lstlisting}
>> +#define VIRTIO_CRYPTO_NO_AEAD 0
>> +#define VIRTIO_CRYPTO_AEAD_GCM 1
>> +#define VIRTIO_CRYPTO_AEAD_CCM 2
>> +#define VIRTIO_CRYPTO_AEAD_CHACHA20_POLY1305 3
>> +\end{lstlisting}
>> +
>> +\begin{note}
>> +Any other value is reserved for future use.
>> +\end{note}
>> +
>> +\devicenormative{\subsubsection}{Device configuration layout}{Device Types
>> / Crypto Device / Device configuration layout}
>> +
>> +\begin{itemize*}
>> +\item The device MUST set \field{max_dataqueues} to between 1 and 65535
>> inclusive.
>> +\item The device MUST set \field{status} based on the status of the
>> hardware-backed implementation.
>> +\item The device MUST accept and handle requests after \field{status} is
>> set to VIRTIO_CRYPTO_S_HW_READY.
> Not sure this is a configuration layout requirement.
>
>> +\item The device MUST set \field{crypto_services} based on the crypto
>> services the device offers.
>> +\item The device MUST set detailed algorithms masks based on the
>> \field{crypto_services} field.
>> +\item The device MUST set \field{max_size} to show the maximum size of
>> crypto request the device supports.
>> +\item The device MUST set \field{max_cipher_key_len} to show the maximum
>> length of cipher key if the device supports CIPHER service.
>> +\item The device MUST set \field{max_auth_key_len} to show the maximum
>> length of authenticated key if the device supports MAC service.
>> +\end{itemize*}
>> +
>> +\drivernormative{\subsubsection}{Device configuration layout}{Device Types
>> / Crypto Device / Device configuration layout}
>> +
>> +\begin{itemize*}
>> +\item The driver MUST read the ready \field{status} from the bottom bit of
>> status to check whether the hardware-backed
>> + implementation is ready or not, and the driver MUST reread it after
>> the device reset.
>> +\item The driver MUST NOT transmit any packets to the device if the ready
>> \field{status} is not set.
> Not sure this is a configuration layout requirement (read: I think it is
> not). I would also rather opt for SHOULD NOT if the think is that this
> can change on the fly (it might be difficult to say when is the ready
> set and when not: e.g. the driver changes to not ready and the interrupt
> for the configuration change is in flight). Well as I said I need some
> clarification regarding this whole status thing.
Acutally I refered to the virtio-net spec, whose status is also located
in configuration
layout requirement. The ready bit is only set by the device, as I
mentioned the driver set/clear
VIRTIO_CRYPTO_S_STARTED to show whether is ready to work or not.
A
> An other thing you probably should consider: when you establish a
> contract between the driver and the device and state a requirement
> regarding one party X I think it is always a good idea to think about
> what is the other party Y supposed to do if X violates the contract (of
> course doing noting about it can be an alternative but the the question
> of the associated risk becomes even more prominent.)
>
>> +\item The driver MAY read \field{max_dataqueues} field to discover the
>> number of data queues the device supports.
> Ain't this MAY contradictory with "The driver MUST identify and initialize
> the control virtqueue"? If that is a MUST, MUST is implied here too, or?
Yes, it should be a MUST.
>> +\item The driver MUST read \field{crypto_services} field to discover which
>> services the device is able to offer.
>> +\item The driver MUST read the detailed algorithms fields based on
>> \field{crypto_services} field.
>> +\item The driver SHOULD read \field{max_size} to discover the maximum size
>> of crypto request the device supports.
>> +\item The driver SHOULD read \field{max_cipher_key_len} to discover the
>> maximum length of cipher key the device supports.
>> +\item The driver SHOULD read \field{max_auth_key_len} to discover the
>> maximum length of authenticated key the device supports.
>> +\end{itemize*}
>> +
> Stopped reviewing here.
Thanks a lot for your feedback!
--
Regards,
-Gonglei
