[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: RE: [Qemu-devel] [virtio-dev] Re: [virtio-dev] Re: [RFC 0/8] virtio-crypto: add multiplexing mode support
> -----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
>
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]