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

[Halil Pasic]

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

Regards,
Haill


----------------------------------8<-------------------------------------------
From: Halil Pasic <address@hidden>
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 <address@hidden>
---
 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

0001-wip-decouple-test-from-implemetation.patch
Description: Text Data