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

[Halil Pasic]

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

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.
What are the benefits of this approach?


Regards,
Halil