Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Halil Pasic]

On 05/18/2017 03:21 PM, Gonglei (Arei) wrote:
>>>> Besides if you look at
>>>>
>>>> +Stateless mode HASH service requests are as follows:
>>>> +
>>>> +\begin{lstlisting}
>>>> +struct virtio_crypto_hash_para_statelesss {
>>>> +    struct {
>>>> +        /* See VIRTIO_CRYPTO_HASH_* above */
>>>> +        le32 algo;
>>>> +    } sess_para;
>>>> +
>>>> +    /* length of source data */
>>>> +    le32 src_data_len;
>>>> +    /* hash result length */
>>>> +    le32 hash_result_len;
>>>> +    le32 reserved;
>>>> +};
>>>> +struct virtio_crypto_hash_data_req_stateless {
>>>> +    /* Device-readable part */
>>>> +    struct virtio_crypto_hash_para_stateless para;
>>>> +    /* Source data */
>>>> +    u8 src_data[src_data_len];
>>>> +
>>>> +    /* Device-writable part */
>>>> +    /* Hash result data */
>>>> +    u8 hash_result[hash_result_len];
>>>> +    struct virtio_crypto_inhdr inhdr;
>>>> +};
>>>> +\end{lstlisting}
>>>> +
>>>>
>>>> from the "[PATCH v18 1/2] virtio-crypto: Add virtio crypto device
>>>> specification". You need the padding to 128 bytes after
>>>> virtio_crypto_hash_para_stateless para, but before src_data. But there is
>>>> no indication in the definition of virtio_crypto_hash_data_req_stateless
>>>> nor somewhere in the spec about that. On the contrary based on this
>>>> one would expect para.reserved and src_data being pretty adjecent.
>>>>
>>>> Thus the normative statement you quoted is (IMHO) ill suited and
>>>> insufficient to express what you have been trying to express.
>>>>
>>> That's indeed a problem. I can't find a good way to express my thoughts
>>> in the spec. Make me sad.~
>>>
>> Can't really tell if we are in an agreement based on your reply above.
>> If we are not, please tell.
>>
>> If we are we have two paths:
>> 1) Give up on the concept of same 'header' length. You could easily
>> branch on the common header and do everything else algorithm specific.
>> 2) Find a way to clean up the mess:
>>    * Bring to expression that the  struct virtio_crypto_op_data_req
>>      from the code ain't the full request (e.g. by postfix-ing _header).
>>      Same for mux.
>>    * Update the description in the spec so that it's compatible with
>>      what your implementations are doing.
>>
> Could you pls explain more about those two ways? Oh give me an example
> for each other. Which way do you like better?

Sorry, I'm quite busy at the time and it does not look like I will be
able to find the time for providing a more detailed explanation. Regarding
my preferences I think 1) is the more straight-forward approach at least
conceptually. OTOH I would need to implement 1) to verify that...

Regards,
Halil