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

[Halil Pasic]

On 05/17/2017 11:12 AM, Gonglei (Arei) wrote:
>> From: Halil Pasic [mailto:address@hidden
>> Sent: Wednesday, May 17, 2017 6:18 AM
>>
>>
>> On 05/16/2017 04:52 AM, Gonglei (Arei) wrote:
>>>> On 05/13/2017 03:16 AM, Gonglei (Arei) wrote:
>>>>>> From: Halil Pasic [mailto:address@hidden
>>>>>> Sent: Friday, May 12, 2017 7:02 PM
>>>>>>
>>>>>>
>>>>>> On 05/08/2017 01:38 PM, Gonglei wrote:
>>>>>>> According to the new spec, we should use different
>>>>>>> requst structure to store the data request based
>>>>>>> on whether VIRTIO_CRYPTO_F_MUX_MODE feature bit is
>>>>>>> negotiated or not.
>>>>>>>
>>>>>>> In this patch, we havn't supported stateless mode
>>>>>>> yet. The device reportes an error if both
>>>>>>> VIRTIO_CRYPTO_F_MUX_MODE and
>>>>>> VIRTIO_CRYPTO_F_CIPHER_STATELESS_MODE
>>>>>>> are negotiated, meanwhile the header.flag doesn't set
>>>>>>> to VIRTIO_CRYPTO_FLAG_SESSION_MODE.
>>>>>>>
>>>>>>> Let's handle this scenario in the following patches.
>>>>>>>
>>>>>>> Signed-off-by: Gonglei <address@hidden>
>>>>>>> ---
>>>>>>>  hw/virtio/virtio-crypto.c | 83
>>>>>> ++++++++++++++++++++++++++++++++++++++++-------
>>>>>>>  1 file changed, 71 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
>>>>>>> index 0353eb6..c4b8a2c 100644
>>>>>>> --- a/hw/virtio/virtio-crypto.c
>>>>>>> +++ b/hw/virtio/virtio-crypto.c
>>>>>>> @@ -577,6 +577,7 @@ virtio_crypto_handle_request(VirtIOCryptoReq
>>>>>> *request)
>>>>>>>      VirtQueueElement *elem = &request->elem;
>>>>>>>      int queue_index =
>>>>>> virtio_crypto_vq2q(virtio_get_queue_index(request->vq));
>>>>>>>      struct virtio_crypto_op_data_req req;
>>>>>>> +    struct virtio_crypto_op_data_req_mux req_mux;
>>>>>>>      int ret;
>>>>>>>      struct iovec *in_iov;
>>>>>>>      struct iovec *out_iov;
>>>>>>> @@ -587,6 +588,9 @@ virtio_crypto_handle_request(VirtIOCryptoReq
>>>>>> *request)
>>>>>>>      uint64_t session_id;
>>>>>>>      CryptoDevBackendSymOpInfo *sym_op_info = NULL;
>>>>>>>      Error *local_err = NULL;
>>>>>>> +    bool mux_mode_is_negotiated;
>>>>>>> +    struct virtio_crypto_op_header *header;
>>>>>>> +    bool is_stateless_req = false;
>>>>>>>
>>>>>>>      if (elem->out_num < 1 || elem->in_num < 1) {
>>>>>>>          virtio_error(vdev, "virtio-crypto dataq missing headers");
>>>>>>> @@ -597,12 +601,28 @@
>> virtio_crypto_handle_request(VirtIOCryptoReq
>>>>>> *request)
>>>>>>>      out_iov = elem->out_sg;
>>>>>>>      in_num = elem->in_num;
>>>>>>>      in_iov = elem->in_sg;
>>>>>>> -    if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
>>>>>>> -                != sizeof(req))) {
>>>>>>> -        virtio_error(vdev, "virtio-crypto request outhdr too short");
>>>>>>> -        return -1;
>>>>>>> +
>>>>>>> +    mux_mode_is_negotiated =
>>>>>>> +        virtio_vdev_has_feature(vdev,
>>>> VIRTIO_CRYPTO_F_MUX_MODE);
>>>>>>> +    if (!mux_mode_is_negotiated) {
>>>>>>> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req))
>>>>>>> +                    != sizeof(req))) {
>>>>>>> +            virtio_error(vdev, "virtio-crypto request outhdr too
>> short");
>>>>>>> +            return -1;
>>>>>>> +        }
>>>>>>> +        iov_discard_front(&out_iov, &out_num, sizeof(req));
>>>>>>> +
>>>>>>> +        header = &req.header;
>>>>>>> +    } else {
>>>>>>> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &req_mux,
>>>>>>> +                sizeof(req_mux)) != sizeof(req_mux))) {
>>>>>>> +            virtio_error(vdev, "virtio-crypto request outhdr too
>> short");
>>>>>>> +            return -1;
>>>>>>> +        }
>>>>>>> +        iov_discard_front(&out_iov, &out_num, sizeof(req_mux));
>>>>>>> +
>>>>>>> +        header = &req_mux.header;
>>>>>> I wonder if this request length checking logic is conform to the
>>>>>> most recent spec draft on the list ("[PATCH v18 0/2] virtio-crypto:
>>>>>> virtio crypto device specification").
>>>>>>
>>>>> Sure. Please see below normative formulation:
>>>>>
>>>>> '''
>>>>> \drivernormative{\paragraph}{Symmetric algorithms Operation}{Device
>> Types
>>>> / Crypto Device / Device Operation / Symmetric algorithms Operation}
>>>>> ...
>>>>> \item If the VIRTIO_CRYPTO_F_MUX_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.
>>>>> ...
>>>>> '''
>>>>>
>>>> As far as I can remember, we have already agreed that in terms of the
>>>> spec sizeof(struct virtio_crypto_op_data_req) makes no sense! In your
>>> Sorry, I don't think so. :(
>>>
>>>> code you have a substantially different struct virtio_crypto_op_data_req
>>>> than in your spec! For instance in the spec virtio_crypto_op_data_req is
>>>> the full request and contains the data buffers (src_data and the
>>>> dest_data), while in your code it's effectively just a header and does
>>>> not contain any data buffers.
>>>>
>>> I said struct virtio_crypto_op_data_req in the spec is just a symbol.
>>> I didn't find a better way to express the src_data and dst_data etc. So
>>> I used u8[len] xxx_data to occupy a sit in the request.
>>>
>> OK, tell me how is the reader/implementer of the spec supposed to figure
>> out that a 124 byte padded "header" needs to be precede any "data"?
>>
> If those people use the asked request based on the spec, 
> they don't need to worry about the pad IMHO.
> 

Is this comment of yours outdated? I have described below why I think
there are problems, and below you seem to agree...

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

>>>>>> AFAIU here you allow only requests of two sizes: one fixed size
>>>>>> for VIRTIO_CRYPTO_F_MUX_MODE and one without that feature. This
>>>>>> means that some requests need quite some padding between what
>>>>>> you call the 'request' and the actual data on which the crypto
>>>>>> operation dictated by the 'request' needs to be performed.
>>>>> Yes, that's true.
>>>>>
>>>> This implies that should we need more than 128 bytes for a request,
>>>> we will need to introduce jet another request format and negotiate it
>>>> via feature bits.
>>>>
>>> Why do we need other feature bits?
>> Because assume you have something that needs more that 128 bytes for
>> its request, how do you solve the problem without new format end new
>> feature bit?
>>
> Oh, maybe I get your point now. You mean the future use for some algorithm 
> requests
> use more than 128 bytes? If so, we have to introduce new feature bits.
> AFAICT the 128 bytes is enough for sym and asym algorithms currently. Xin 
> Zeng? Any thoughts?
> 

That's what I was trying to say.