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

[Halil Pasic]

On 10/28/2016 07: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 <address@hidden>
> CC: Michael S. Tsirkin <address@hidden>
> CC: Cornelia Huck <address@hidden>
> CC: Stefan Hajnoczi <address@hidden>
> CC: Lingli Deng <address@hidden>
> CC: Jani Kokkonen <address@hidden>
> CC: Ola Liljedahl <address@hidden>
> CC: Varun Sethi <address@hidden>
> CC: Zeng Xin <address@hidden>
> CC: Keating Brian <address@hidden>
> CC: Ma Liang J <address@hidden>
> CC: Griffin John <address@hidden>
> CC: Hanweidong <address@hidden>
> CC: Mihai Claudiu Caraman <address@hidden>
> ---
[..]
> +The header is the general header and the union is of the algorithm-specific 
> type,
> +which is set by the driver. All properties in the union are shown as follows.
> +
> +There is a unified idata structure for all symmetric algorithms, including 
> CIPHER, HASH, MAC, and AEAD.
> +
> +The structure is defined as follows:
> +
> +\begin{lstlisting}
> +struct virtio_crypto_sym_input {
> +    /* Destination data guest address, it's useless for plain HASH and MAC */
> +    le64 dst_data_addr;
> +    /* Digest result guest address, it's useless for plain cipher algos */
> +    le64 digest_result_addr;
> +
> +    le32 status;
> +    le32 padding;
> +};
> +

This seems to be out of sync regarding the code (e.g. can't find it in 
virtio-crypto.h of the
series linked in the cover-letter). It seems to me this reflects v4 since the 
stuff is gone
since v5 (qemu code).

> +\end{lstlisting}
> +
> +\subsubsection{HASH Service Operation}\label{sec:Device Types / Crypto 
> Device / Device Operation / HASH Service Operation}
> +
> +\begin{lstlisting}
> +struct virtio_crypto_hash_para {
> +    /* length of source data */
> +    le32 src_data_len;
> +    /* hash result length */
> +    le32 hash_result_len;
> +};
> +
> +struct virtio_crypto_hash_input {
> +    struct virtio_crypto_sym_input input;
> +};
> +
> +struct virtio_crypto_hash_output {
> +    /* source data guest address */
> +    le64 src_data_addr;
> +};
> +
> +struct virtio_crypto_hash_data_req {
> +    /* Device-readable part */
> +    struct virtio_crypto_hash_para para;
> +    struct virtio_crypto_hash_output odata;
> +    /* Device-writable part */
> +    struct virtio_crypto_hash_input idata;
> +};
> +\end{lstlisting}
> +
> +Each data request uses virtio_crypto_hash_data_req structure to store 
> information
> +used to run the HASH operations. The request only occupies one entry
> +in the Vring Descriptor Table in the virtio crypto device's dataq, which 
> improves
> +the throughput of data transmitted for the HASH service, so that the virtio 
> crypto
> +device can be better accelerated.
> +
> +The information includes the source data guest physical address stored by 
> \field{odata}.\field{src_data_addr},
> +length of source data stored by \field{para}.\field{src_data_len}, and the 
> digest result guest physical address
> +stored by \field{digest_result_addr} used to save the results of the HASH 
> operations.
> +The address and length can determine exclusive content in the guest memory.
> +

Thus this does not make any sense to me. Furthermore the problem seems to 
persist
across the specification. Thus in my opinion there is no point in reviewing
this version. Or am I missing something here? In case I'm not missing anything
and the spec describes something quite outdated when should we expect a new
version of the spec?

Regards,
Halil

signature.asc
Description: OpenPGP digital signature