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

[Halil Pasic]

On 05/04/2017 04:13 PM, Gonglei (Arei) wrote:
>>
>>
>> On 05/04/2017 03:33 PM, Gonglei (Arei) wrote:
>>>>> +\begin{description}
>>>>> +\item[VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE] Requires
>>>> VIRTIO_CRYPTO_F_STATELESS_MODE.
>>>>> +\item[VIRTIO_CRYPTO_F_HASH_STATELESS_MODE] Requires
>>>> VIRTIO_CRYPTO_F_STATELESS_MODE.
>>>>> +\item[VIRTIO_CRYPTO_F_MAC_STATELESS_MODE] Requires
>>>> VIRTIO_CRYPTO_F_STATELESS_MODE.
>>>>> +\item[VIRTIO_CRYPTO_F_AEAD_STATELESS_MODE] Requires
>>>> VIRTIO_CRYPTO_F_STATELESS_MODE.
>>>>> +\end{description}
>>>>
>>>> I find feature bit 0 redundant and bit confusing. We had a discussion
>>>> in v15 and v16.
>>>>
>>>> Could you answer:
>>>> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg03214.html
>>>> (Message-ID:
>> <address@hidden>)
>>>>
>>>>
>>> Please see my reply:
>>> https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg03186.html
>>>
>>> The main reason is we should keep compatibility to pre-existing driver and
>>> support some function that different services have different modes.
>>> We have only one unique crypto request named structure
>> virtio_crypto_op_data_req_mux.
>>> Please continue to see the sepc, you'll find the truth.
>>>
>>
>> Sorry, I still do not understand why can't we drop
>> VIRTIO_CRYPTO_F_STATELESS_MODE
>> and just keep the four service specific modes.
>>
>> Can you point me to the (published) code where
>> VIRTIO_CRYPTO_F_STATELESS_MODE is
>> used (that's what I'm missing -- preferably state the repository, the commit
>> a file and a line using VIRTIO_CRYPTO_F_STATELESS_MODE)?
>>
> No no no, there isn't existing code use VIRTIO_CRYPTO_F_STATELESS_MODE yet,
> It's just for future use.
> 

Thanks, that's a crucial piece of information. In the thread I referenced
this was not cleared (at least for me). It would be really nice to have
some working code before doing the spec, because I find it very easy to miss
a detail which renders the whole thing useless if one works solely on
theoretical level and does not try to create at least a working prototype.

> Please the blow description:
> """
> \drivernormative{\paragraph}{HASH Service Operation}{Device Types / Crypto 
> Device / Device Operation / HASH Service Operation}
> 
> \begin{itemize*}
> \item If the driver uses the session mode, then the driver MUST set 
> \field{session_id} in struct virtio_crypto_op_header
>       to a valid value assigned by the device when the session was created.
> \item If the VIRTIO_CRYPTO_F_STATELESS_MODE feature bit is negotiated, the 
> driver MUST use struct virtio_crypto_op_data_req_mux to wrap crypto requests. 
> Otherwise, the driver MUST use struct virtio_crypto_op_data_req.
> \item If the VIRTIO_CRYPTO_F_HASH_STATELESS_MODE feature bit is negotiated, 
> 1) if the driver uses the stateless mode, then the driver MUST set the 
> \field{flag} field in struct virtio_crypto_op_header
>       to VIRTIO_CRYPTO_FLAG_STATELESS_MODE and MUST set the fields in struct 
> virtio_crypto_hash_para_statelession.sess_para, 2) if the driver uses the 
> session mode, then the driver MUST set the \field{flag} field in struct 
> virtio_crypto_op_header to VIRTIO_CRYPTO_FLAG_STATE_MODE.
> \item The driver MUST set \field{opcode} in struct virtio_crypto_op_header to 
> VIRTIO_CRYPTO_HASH.
> \end{itemize*}
> """
> 
> If we drop the VIRTIO_CRYPTO_F_STATELESS_MODE feature bit, and the 
> VIRTIO_CRYPTO_F_HASH_STATELESS_MODE feature bit is not negotiated, 
> then the driver doesn't to know use which structure to store the crypto 
> request: 
> is struct virtio_crypto_op_data_req_mux ? or struct virtio_crypto_op_data_req.

Thanks for the detailed explanation.

Let's clarify some things first:
1) struct virtio_crypto_op_data_req_mux IS NOT a C language struct but
a somewhat informal desciption using C-like syntax. If you don't follow
try to reason about the size of struct virtio_crypto_op_data_req_mux.
For instance look at:
+struct virtio_crypto_cipher_data_req_stateless {
+    /* Device-readable part */
+    struct virtio_crypto_cipher_para_stateless para;
+    /* The cipher key */
+    u8 cipher_key[keylen];
+
+    /* Initialization Vector or Counter data. */
+    u8 iv[iv_len];
+    /* Source data */
+    u8 src_data[src_data_len];           <==

The  isrc_data_len is not a compile time constant!!

+
+    /* Device-writable part */
+    /* Destination data */
+    u8 dst_data[dst_data_len];
+    struct virtio_crypto_inhdr inhdr;
+};

Using something like BNF to describe the requests would be
less ambiguous but likely more difficult to read and reason about
for most of us (and also inconsistent with the rest of the spec).

2) Each struct virtio_crypto_op_data_req is also a struct 
virtio_crypto_op_data_req_mux
in a sense of 1).

3) The header struct virtio_crypto_op_header is a common prefix for 
struct virtio_crypto_op_data_req and a struct virtio_crypto_op_data_req_mux.


> 
> We assume the driver uses struct virtio_crypto_op_data_req, what about the 
> device?
> The device doesn't know how to parse the request in the 
> virtio_crypto_handle_request()
> of virito-crypto.c in Qemu.
> 
> static int
> virtio_crypto_handle_request(VirtIOCryptoReq *request)
> {
>     VirtIOCrypto *vcrypto = request->vcrypto;
>     VirtIODevice *vdev = VIRTIO_DEVICE(vcrypto);
>     VirtQueueElement *elem = &request->elem;
>     int queue_index = virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
>     struct virtio_crypto_op_data_req req; ???
> 
>     ///or struct virtio_crypto_op_data_req_mux req; ???
> 

Because of 3) and because the service associated with the request can be 
inferred
form virtio_crypto_op_header one can answer the question you as, and decide
if what comes after the header is a stateless or a session variant based
on virtio_crypto_op_header.flag -- maybe it does not even need opcode
to do that.

Thus (IMHO) dropping VIRTIO_CRYPTO_F_STATELESS_MODE is possible.

Or is there a flaw in my reasoning?

Halil

> Thanks,
> -Gonglei
>