[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: |
Gonglei (Arei) |
Subject: |
Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request |
Date: |
Tue, 16 May 2017 02:52:20 +0000 |
>
> 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.
>
> >> 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?
> 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-260
> 004
> (2.4.4 Message Framing) satisfied by this code. Could you explain this
> to me please?
>
Sorry for my bad English,
I don't know which normative formulation the code violates?
Thanks
-Gonglei
>
> >> 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, 2017/05/15
- Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request,
Gonglei (Arei) <=
- 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
- Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request, Halil Pasic, 2017/05/29