[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle
From: |
Halil Pasic |
Subject: |
Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request |
Date: |
Mon, 15 May 2017 18:15:50 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 |
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
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.
>> 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.
By the way, I'm having a hard time understing how is the requirement form
http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-260004
(2.4.4 Message Framing) satisfied by this code. Could you explain this
to me please?
>> What are the benefits of this approach?
>>
> We could unify the request for all algorithms, both symmetric algos and
> asymmetric algos,
> which is very convenient for handling tens of hundreds of different algorithm
> requests.
>
AFAIU the reason is ease of implementation. If everybody else is fine
with this, I won't object either.
>
> Thanks,
> -Gonglei
>
- [Qemu-devel] [RFC v1 0/9] virtio-crypto: add stateless mode support, Gonglei, 2017/05/08
- [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request, Gonglei, 2017/05/08
- Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request, Halil Pasic, 2017/05/12
- Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request, Gonglei (Arei), 2017/05/12
- Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request,
Halil Pasic <=
- Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request, Gonglei (Arei), 2017/05/15
- Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request, Halil Pasic, 2017/05/16
- Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request, Gonglei (Arei), 2017/05/17
- Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request, Halil Pasic, 2017/05/17
- Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request, Gonglei (Arei), 2017/05/17
- Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request, Halil Pasic, 2017/05/17
- Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request, Cornelia Huck, 2017/05/17
- Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request, Halil Pasic, 2017/05/17
- Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request, Halil Pasic, 2017/05/18
- Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request, Gonglei (Arei), 2017/05/18