Re: [Qemu-devel] [virtio-dev] Re: [virtio-dev] Re: [RFC 0/8] virtio-crypto: add multiplexing mode support

["Gonglei (Arei)" <arei.gonglei@huawei.com>]

> -----Original Message-----
> From: Halil Pasic [mailto:pasic@linux.vnet.ibm.com]
> Sent: Friday, October 06, 2017 10:25 PM
> On 09/18/2017 03:17 AM, Longpeng (Mike) wrote:
> >
> >
> > On 2017/9/16 1:33, Halil Pasic wrote:
> >
> >>
> >>
> >> On 09/14/2017 02:58 AM, Longpeng (Mike) wrote:
> >>>
> >>>
> >>> On 2017/9/14 2:14, Halil Pasic wrote:
> >>>
> >>>>
> >>>>
> >>>> On 09/11/2017 03:10 AM, Longpeng(Mike) wrote:
> >>>>> *NOTE*
> >>>>> The code realization is based on the latest virtio crypto spec:
> >>>>>  [PATCH v19 0/2] virtio-crypto: virtio crypto device specification
> >>>>>
> https://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg05217.html
> >>>>>
> >>>>> In session mode, the process of create/close a session
> >>>>> makes we have a least one full round-trip cost from guest to host to
> guest
> >>>>> to be able to send any data for symmetric algorithms. It gets ourself into
> >>>>> synchronization troubles in some scenarios like a web server handling
> lots
> >>>>> of small requests whose algorithms and keys are different.
> >>>>>
> >>>>> We can support one-blob request (no sessions) as well for symmetric
> >>>>> algorithms, including HASH, MAC services. The benefit is obvious for
> >>>>> HASH service because it's usually a one-blob operation.
> >>>>>
> >>>>
> >>>> Hi!
> >>>>
> >>>> I've just started looking at this. Patch #1 modifies linux/virtio_crypto.h
> >>>> which if I compare with the (almost) latest linux master is different. Thus
> >>>> I would expect a corresponding kernel patch set too, but I haven't
> received
> >>>> one, nor did I find a reference in the cover letter.
> >>>>
> >>>> I think if I want to test the new features I need the kernel counter-part
> >>>> too, or?
> >>>>
> >>>> Could you point me to the kernel counterpart?
> >>>>
> >>>
> >>>
> >>> Hi Halil,
> >>>
> >>> We haven't implemented the kernel frontend part yet, but there's a
> testcase
> >>> based on qtest, you can use it.
> >>>
> >>> Please see the attachment.
> >>>
> >>
> >> Thanks Longpeng! I have two problems with this: first I can't use this on
> s390x
> >> and as you may have noticed I'm working mostly on s390x (that's what I'm
> payed
> >> for). OK, my laptop is amd64 so I was able to try it out, and that leads to the
> >> next problem. I can't test before/after and cross version stuff with this. That
> >> hurts me because I have a feeling things can be done simpler but that
> feeling has
> >> failed me before, so I tend to try out first and then start a discussion.
> >>
> >> Is some kernel patch series already in the pipeline?
> >>
> >
> >
> > Hi Halil，
> >
> > Thank for your comments about the v19 spec first, we'll close look at them
> recently.
> >
> > I'm so sorry that the kernel frontend driver isn't in the pipeline, so maybe you
> > can start a x86/tcg VM on your s390x machine or amd64 laptop and then
> debug this
> > feature with the testcase.
> >
> > If it's not convenient to you, I'll wrote an experimental version of the kernel
> > frontend driver these days. :)
> >
> 
> I've managed to do some experiments on my laptop using your testcase. Based
> on that, I think the code presented here can be significantly simplified, and
> same goes for the spec. I would like to share my experiment with you, and
> maybe
> the rest of the people too, but I'm not sure what is the best way to do it.
> 
> I did my experimenting on top of this patch set plus your test. The first thing
> I did is to decouple the virtio-crypto.h used by the test from the one used
> for the qemu executable. Then the next patch refactors the control queue
> handling.
> 

The next patch refactors make sense to me, 
but why do we need to decouple the virtio-crypto.h?


> The basic idea behind the whole thing is that tinging about the requests put
> on the virtqueues in terms of just complicates things unnecessarily.
> 
> I could guess I will post the interesting part as a reply to this and the less
> interesting part (decoupling) as an attachment. You are supposed to apply first
> the attachment then the part after the scissors line.
> 
> Of course should you could respin the series preferably with the test
> included I can rebase my stuff.
> 
> Please let me know about your opinion.
> 

Thanks for your work, Halil. What's your opinion about virtio crypto spec v20?

Thanks,
-Gonglei

> Regards,
> Haill
> 
> 
> ----------------------------------8<-------------------------------------------
> From: Halil Pasic <pasic@linux.vnet.ibm.com>
> Date: Thu, 5 Oct 2017 20:10:56 +0200
> Subject: [PATCH 2/2] wip: refactor ctrl qeue handling
> 
> Not meant for inclusion, but as a demonstrator for an alternative
> approach of handling/introducing mux mode. The changes to
> include/standard-headers/linux/virtio_crypto.h aren't necessary,
> but I think making them here is good fro sparking a discussion.
> For instance struct virtio_crypto_op_ctrl_req_mux is very weird,
> as it does not describe/represent the whole request, but just
> a header. The idea is to rewrite the hwole mux handling in this
> fashion.
> 
> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> ---
>  hw/virtio/virtio-crypto.c                      |   84
> +++++++++---------------
>  include/standard-headers/linux/virtio_crypto.h |   24 +-------
>  2 files changed, 33 insertions(+), 75 deletions(-)
> 
> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> index 69c5ad5..153712d 100644
> --- a/hw/virtio/virtio-crypto.c
> +++ b/hw/virtio/virtio-crypto.c
> @@ -239,11 +239,7 @@ static void virtio_crypto_handle_ctrl(VirtIODevice
> *vdev, VirtQueue *vq)
>      VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(vdev);
>      VirtQueueElement *elem;
>      struct virtio_crypto_session_input input;
> -    struct virtio_crypto_ctrl_header *generic_hdr;
> -    union {
> -        struct virtio_crypto_op_ctrl_req ctrl;
> -        struct virtio_crypto_op_ctrl_req_mux mux_ctrl;
> -    } req;
> +    struct virtio_crypto_ctrl_header hdr;
> 
>      struct iovec *in_iov;
>      struct iovec *out_iov;
> @@ -253,9 +249,10 @@ static void virtio_crypto_handle_ctrl(VirtIODevice
> *vdev, VirtQueue *vq)
>      uint32_t opcode;
>      int64_t session_id;
>      uint8_t status;
> -    size_t s, exp_len;
> -    void *sess;
> +    size_t s;
> 
> +#define payload_size(vdev, req) (virtio_crypto_in_mux_mode((vdev)) \
> +        ? sizeof((req)) :
> VIRTIO_CRYPTO_CTRL_REQ_PAYLOAD_SIZE_NONMUX)
>      for (;;) {
>          elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
>          if (!elem) {
> @@ -273,47 +270,34 @@ static void virtio_crypto_handle_ctrl(VirtIODevice
> *vdev, VirtQueue *vq)
>          in_num = elem->in_num;
>          in_iov = elem->in_sg;
> 
> -        if (virtio_crypto_in_mux_mode(vdev)) {
> -            exp_len = sizeof(req.mux_ctrl);
> -            generic_hdr = (struct virtio_crypto_ctrl_header
> *)(&req.mux_ctrl);
> -        } else {
> -            exp_len = sizeof(req.ctrl);
> -            generic_hdr = (struct virtio_crypto_ctrl_header *)(&req.ctrl);
> -        }
> -
> -        s = iov_to_buf(out_iov, out_num, 0, generic_hdr, exp_len);
> -        if (unlikely(s != exp_len)) {
> +        s =  sizeof(hdr);
> +        iov_to_buf(out_iov, out_num, 0, &hdr, s);
> +        if (unlikely(s != iov_discard_front(&out_iov, &out_num, s))) {
>              virtio_error(vdev, "virtio-crypto request ctrl_hdr too short");
>              virtqueue_detach_element(vq, elem, 0);
>              g_free(elem);
>              break;
>          }
> 
> -        iov_discard_front(&out_iov, &out_num, exp_len);
> -
> -        opcode = ldl_le_p(&generic_hdr->opcode);
> -        queue_id = ldl_le_p(&generic_hdr->queue_id);
> 
> +        opcode = ldl_le_p(&hdr.opcode);
> +        queue_id = ldl_le_p(&hdr.queue_id);
>          switch (opcode) {
>          case VIRTIO_CRYPTO_CIPHER_CREATE_SESSION:
> -            if (virtio_crypto_in_mux_mode(vdev)) {
> -                sess = g_new0(struct
> virtio_crypto_sym_create_session_req, 1);
> -                exp_len = sizeof(struct
> virtio_crypto_sym_create_session_req);
> -                s = iov_to_buf(out_iov, out_num, 0, sess, exp_len);
> -                if (unlikely(s != exp_len)) {
> -                    virtio_error(vdev, "virtio-crypto request additional "
> -                                 "parameters too short");
> -                    virtqueue_detach_element(vq, elem, 0);
> -                    break;
> -                }
> -                iov_discard_front(&out_iov, &out_num, exp_len);
> -            } else {
> -                sess = &req.ctrl.u.sym_create_session;
> +        {
> +            struct virtio_crypto_sym_create_session_req req;
> +            iov_to_buf(out_iov, out_num, 0, &req, sizeof(req));
> +            s = payload_size(vdev, req);
> +            if (unlikely(s != iov_discard_front(&out_iov, &out_num, s))) {
> +                virtio_error(vdev, "virtio-crypto request additional "
> +                             "parameters too short");
> +                virtqueue_detach_element(vq, elem, 0);
> +                break;
>              }
> 
>              memset(&input, 0, sizeof(input));
> 
> -            session_id = virtio_crypto_create_sym_session(vcrypto, sess,
> +            session_id = virtio_crypto_create_sym_session(vcrypto, &req,
>                                      queue_id, opcode, out_iov,
> out_num);
>              /* Serious errors, need to reset virtio crypto device */
>              if (session_id == -EFAULT) {
> @@ -338,27 +322,24 @@ static void virtio_crypto_handle_ctrl(VirtIODevice
> *vdev, VirtQueue *vq)
>              virtqueue_push(vq, elem, sizeof(input));
>              virtio_notify(vdev, vq);
>              break;
> +        }
>          case VIRTIO_CRYPTO_CIPHER_DESTROY_SESSION:
>          case VIRTIO_CRYPTO_HASH_DESTROY_SESSION:
>          case VIRTIO_CRYPTO_MAC_DESTROY_SESSION:
>          case VIRTIO_CRYPTO_AEAD_DESTROY_SESSION:
> -            if (virtio_crypto_in_mux_mode(vdev)) {
> -                sess = g_new0(struct virtio_crypto_destroy_session_req,
> 1);
> -                exp_len = sizeof(struct
> virtio_crypto_destroy_session_req);
> -                s = iov_to_buf(out_iov, out_num, 0, sess, exp_len);
> -                if (unlikely(s != exp_len)) {
> -                    virtio_error(vdev, "virtio-crypto request additional "
> -                                 "parameters too short");
> -                    virtqueue_detach_element(vq, elem, 0);
> -                    break;
> -                }
> -                iov_discard_front(&out_iov, &out_num, exp_len);
> -            } else {
> -                sess = &req.ctrl.u.destroy_session;
> +        {
> +            struct virtio_crypto_destroy_session_req req;
> +            iov_to_buf(out_iov, out_num, 0, &req, sizeof(req));
> +            s = payload_size(vdev, req);
> +            if (unlikely(s != iov_discard_front(&out_iov, &out_num, s))) {
> +                virtio_error(vdev, "virtio-crypto request additional "
> +                             "parameters too short");
> +                virtqueue_detach_element(vq, elem, 0);
> +                break;
>              }
> 
>              status = virtio_crypto_handle_close_session(vcrypto,
> -                                                sess, queue_id);
> +                                                &req, queue_id);
>              /* The status only occupy one byte, we can directly use it */
>              s = iov_from_buf(in_iov, in_num, 0, &status, sizeof(status));
>              if (unlikely(s != sizeof(status))) {
> @@ -369,6 +350,7 @@ static void virtio_crypto_handle_ctrl(VirtIODevice
> *vdev, VirtQueue *vq)
>              virtqueue_push(vq, elem, sizeof(status));
>              virtio_notify(vdev, vq);
>              break;
> +        }
>          case VIRTIO_CRYPTO_HASH_CREATE_SESSION:
>          case VIRTIO_CRYPTO_MAC_CREATE_SESSION:
>          case VIRTIO_CRYPTO_AEAD_CREATE_SESSION:
> @@ -388,11 +370,9 @@ static void virtio_crypto_handle_ctrl(VirtIODevice
> *vdev, VirtQueue *vq)
>              break;
>          } /* end switch case */
> 
> -        if (virtio_crypto_in_mux_mode(vdev)) {
> -            g_free(sess);
> -        }
>          g_free(elem);
>      } /* end for loop */
> +#undef payload_size
>  }
> 
>  static void virtio_crypto_init_request(VirtIOCrypto *vcrypto, VirtQueue *vq,
> diff --git a/include/standard-headers/linux/virtio_crypto.h
> b/include/standard-headers/linux/virtio_crypto.h
> index 0ea61b2..7d53c22 100644
> --- a/include/standard-headers/linux/virtio_crypto.h
> +++ b/include/standard-headers/linux/virtio_crypto.h
> @@ -241,29 +241,7 @@ struct virtio_crypto_destroy_session_req {
>  	uint8_t padding[48];
>  };
> 
> -/* The request of the control virtqueue's packet for non-MUX mode */
> -struct virtio_crypto_op_ctrl_req {
> -	struct virtio_crypto_ctrl_header header;
> -
> -	union {
> -		struct virtio_crypto_sym_create_session_req
> -			sym_create_session;
> -		struct virtio_crypto_hash_create_session_req
> -			hash_create_session;
> -		struct virtio_crypto_mac_create_session_req
> -			mac_create_session;
> -		struct virtio_crypto_aead_create_session_req
> -			aead_create_session;
> -		struct virtio_crypto_destroy_session_req
> -			destroy_session;
> -		uint8_t padding[56];
> -	} u;
> -};
> -
> -/* The request of the control virtqueue's packet for MUX mode */
> -struct virtio_crypto_op_ctrl_req_mux {
> -	struct virtio_crypto_ctrl_header header;
> -};
> +#define VIRTIO_CRYPTO_CTRL_REQ_PAYLOAD_SIZE_NONMUX 56
> 
>  struct virtio_crypto_op_header {
>  #define VIRTIO_CRYPTO_CIPHER_ENCRYPT \
> --
> 1.7.1
>