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

[Gonglei (Arei)]

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