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

[Gonglei (Arei)]

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

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

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


Thanks,
-Gonglei