[RFC 0/2] virtio-rng: add a control queue

[RFC 0/2] virtio-rng: add a control queue

[Laurent Vivier]

The kernel needs sometime to be able to cancel an ongoing command.

For instance, if the virtio-rng device uses the egd backend
and this backend doesn't provide data, the buffer provided by the
kernel is kept as long as it is needed.

On the kernel side, a read blocks until the buffer returns from QEMU.

As the read is done with a mutex held, all the hw_random interface
hangs and we cannot switch to another hw_random backend.

So this series adds a control queue to the virtio-rng device to allow
to flush the virtio-rng input queue to release the kernel mutex and
to allow to switch to another device.

The kernel side series can be found at:

https://github.com/vivier/linux/commits/virtio-rng-ctrl

Laurent Vivier (2):
  virtio-rng: prepare the introduction of a control queue
  virtio-rng: add a control queue

 hw/core/machine.c                           |  1 +
 hw/virtio/trace-events                      |  6 ++
 hw/virtio/virtio-rng.c                      | 99 ++++++++++++++++++---
 include/hw/virtio/virtio-rng.h              |  5 +-
 include/standard-headers/linux/virtio_rng.h | 14 +++
 5 files changed, 111 insertions(+), 14 deletions(-)

-- 
2.23.0

[RFC 1/2] virtio-rng: prepare the introduction of a control queue

[Laurent Vivier]

Rename the existing vq to input_vq and use virtio_delete_queue()
rather than virtio_del_queue().

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/virtio/virtio-rng.c         | 18 +++++++++---------
 include/hw/virtio/virtio-rng.h |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index b498a2033263..a0bca55bef55 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -25,7 +25,7 @@
 static bool is_guest_ready(VirtIORNG *vrng)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(vrng);
-    if (virtio_queue_ready(vrng->vq)
+    if (virtio_queue_ready(vrng->input_vq)
         && (vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
         return true;
     }
@@ -69,7 +69,7 @@ static void chr_read(void *opaque, const void *buf, size_t size)
 
     offset = 0;
     while (offset < size) {
-        elem = virtqueue_pop(vrng->vq, sizeof(VirtQueueElement));
+        elem = virtqueue_pop(vrng->input_vq, sizeof(VirtQueueElement));
         if (!elem) {
             break;
         }
@@ -78,13 +78,13 @@ static void chr_read(void *opaque, const void *buf, size_t size)
                            0, buf + offset, size - offset);
         offset += len;
 
-        virtqueue_push(vrng->vq, elem, len);
+        virtqueue_push(vrng->input_vq, elem, len);
         trace_virtio_rng_pushed(vrng, len);
         g_free(elem);
     }
-    virtio_notify(vdev, vrng->vq);
+    virtio_notify(vdev, vrng->input_vq);
 
-    if (!virtio_queue_empty(vrng->vq)) {
+    if (!virtio_queue_empty(vrng->input_vq)) {
         /* If we didn't drain the queue, call virtio_rng_process
          * to take care of asking for more data as appropriate.
          */
@@ -112,7 +112,7 @@ static void virtio_rng_process(VirtIORNG *vrng)
     } else {
         quota = MIN((uint64_t)vrng->quota_remaining, (uint64_t)UINT32_MAX);
     }
-    size = get_request_size(vrng->vq, quota);
+    size = get_request_size(vrng->input_vq, quota);
 
     trace_virtio_rng_request(vrng, size, quota);
 
@@ -122,7 +122,7 @@ static void virtio_rng_process(VirtIORNG *vrng)
     }
 }
 
-static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
+static void virtio_rng_handle_input(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIORNG *vrng = VIRTIO_RNG(vdev);
     virtio_rng_process(vrng);
@@ -220,7 +220,7 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
 
     virtio_init(vdev, "virtio-rng", VIRTIO_ID_RNG, 0);
 
-    vrng->vq = virtio_add_queue(vdev, 8, handle_input);
+    vrng->input_vq = virtio_add_queue(vdev, 8, virtio_rng_handle_input);
     vrng->quota_remaining = vrng->conf.max_bytes;
     vrng->rate_limit_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
                                                check_rate_limit, vrng);
@@ -238,7 +238,7 @@ static void virtio_rng_device_unrealize(DeviceState *dev, Error **errp)
     qemu_del_vm_change_state_handler(vrng->vmstate);
     timer_del(vrng->rate_limit_timer);
     timer_free(vrng->rate_limit_timer);
-    virtio_del_queue(vdev, 0);
+    virtio_delete_queue(vrng->input_vq);
     virtio_cleanup(vdev);
 }
 
diff --git a/include/hw/virtio/virtio-rng.h b/include/hw/virtio/virtio-rng.h
index bd05d734b87d..d77daf126828 100644
--- a/include/hw/virtio/virtio-rng.h
+++ b/include/hw/virtio/virtio-rng.h
@@ -32,7 +32,7 @@ typedef struct VirtIORNG {
     VirtIODevice parent_obj;
 
     /* Only one vq - guest puts buffer(s) on it when it needs entropy */
-    VirtQueue *vq;
+    VirtQueue *input_vq;
 
     VirtIORNGConf conf;
 
-- 
2.23.0

[RFC 2/2] virtio-rng: add a control queue

[Laurent Vivier]

When the linux kernel wants to switch to another device it needs
to be able to flush the on-fly requests.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 hw/core/machine.c                           |  1 +
 hw/virtio/trace-events                      |  6 ++
 hw/virtio/virtio-rng.c                      | 81 ++++++++++++++++++++-
 include/hw/virtio/virtio-rng.h              |  3 +-
 include/standard-headers/linux/virtio_rng.h | 14 ++++
 5 files changed, 101 insertions(+), 4 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 3e288bfceb7f..6c417dbdc02a 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -34,6 +34,7 @@ GlobalProperty hw_compat_4_2[] = {
     { "vhost-blk-device", "seg_max_adjust", "off"},
     { "usb-host", "suppress-remote-wake", "off" },
     { "usb-redir", "suppress-remote-wake", "off" },
+    { "virtio-rng", "ctrl-queue", "off" },
 };
 const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2);
 
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index e28ba48da621..95b77f3a3056 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -38,6 +38,12 @@ virtio_rng_popped(void *rng) "rng %p: elem popped"
 virtio_rng_pushed(void *rng, size_t len) "rng %p: %zd bytes pushed"
 virtio_rng_request(void *rng, size_t size, unsigned quota) "rng %p: %zd bytes requested, %u bytes quota left"
 virtio_rng_vm_state_change(void *rng, int running, int state) "rng %p: state change to running %d state %d"
+virtio_rng_ctrl(void *rng) "rng %p"
+virtio_rng_ctrl_popped(void *rng) "rng %p"
+virtio_rng_ctrl_pushed(void *rng) "rng %p"
+virtio_rng_flush(void *rng) "rng %p"
+virtio_rng_flush_popped(void *rng) "rng %p"
+virtio_rng_flush_pushed(void *rng) "rng %p"
 
 # virtio-balloon.c
 #
diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index a0bca55bef55..389aa8997f3d 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -128,9 +128,76 @@ static void virtio_rng_handle_input(VirtIODevice *vdev, VirtQueue *vq)
     virtio_rng_process(vrng);
 }
 
-static uint64_t get_features(VirtIODevice *vdev, uint64_t f, Error **errp)
+static virtio_rng_ctrl_ack virtio_rng_flush(VirtIORNG *vrng)
 {
-    return f;
+    VirtQueueElement *elem;
+    VirtIODevice *vdev = VIRTIO_DEVICE(vrng);
+
+    trace_virtio_rng_flush(vrng);
+    while (!virtio_queue_empty(vrng->input_vq)) {
+        elem = virtqueue_pop(vrng->input_vq, sizeof(VirtQueueElement));
+        if (!elem) {
+            break;
+        }
+        trace_virtio_rng_flush_popped(vrng);
+        virtqueue_push(vrng->input_vq, elem, 0);
+        trace_virtio_rng_flush_pushed(vrng);
+        g_free(elem);
+    }
+    virtio_notify(vdev, vrng->input_vq);
+
+    return VIRTIO_RNG_OK;
+}
+
+static void virtio_rng_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
+{
+    VirtIORNG *vrng = VIRTIO_RNG(vdev);
+    VirtQueueElement *elem;
+    virtio_rng_ctrl_ack status = VIRTIO_RNG_ERR;
+    struct virtio_rng_ctrl_hdr ctrl;
+    size_t s;
+
+    trace_virtio_rng_ctrl(vrng);
+    for (;;) {
+        elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
+        if (!elem) {
+            break;
+        }
+        trace_virtio_rng_ctrl_popped(vrng);
+
+        if (iov_size(elem->in_sg, elem->in_num) < sizeof(status) ||
+            iov_size(elem->out_sg, elem->out_num) < sizeof(ctrl)) {
+            virtio_error(vdev, "virtio-rng ctrl missing headers");
+            virtqueue_detach_element(vq, elem, 0);
+            g_free(elem);
+            break;
+        }
+
+        s = iov_to_buf(elem->out_sg, elem->out_num, 0, &ctrl, sizeof(ctrl));
+        if (s != sizeof(ctrl)) {
+            status = VIRTIO_RNG_ERR;
+        } else if (ctrl.cmd == VIRTIO_RNG_CMD_FLUSH) {
+            status = virtio_rng_flush(vrng);
+        }
+
+        s = iov_from_buf(elem->in_sg, elem->in_num, 0, &status, sizeof(status));
+        assert(s == sizeof(status));
+
+        virtqueue_push(vq, elem, sizeof(status));
+        trace_virtio_rng_ctrl_pushed(vrng);
+        virtio_notify(vdev, vq);
+        g_free(elem);
+    }
+}
+
+static uint64_t virtio_rng_get_features(VirtIODevice *vdev, uint64_t features,
+                                        Error **errp)
+{
+    VirtIORNG *vrng = VIRTIO_RNG(vdev);
+
+    features |= vrng->conf.host_features;
+
+    return features;
 }
 
 static void virtio_rng_vm_state_change(void *opaque, int running,
@@ -221,6 +288,9 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
     virtio_init(vdev, "virtio-rng", VIRTIO_ID_RNG, 0);
 
     vrng->input_vq = virtio_add_queue(vdev, 8, virtio_rng_handle_input);
+    if (virtio_has_feature(vrng->conf.host_features, VIRTIO_RNG_F_CTRL_VQ)) {
+        vrng->ctrl_vq = virtio_add_queue(vdev, 8, virtio_rng_handle_ctrl);
+    }
     vrng->quota_remaining = vrng->conf.max_bytes;
     vrng->rate_limit_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
                                                check_rate_limit, vrng);
@@ -238,6 +308,9 @@ static void virtio_rng_device_unrealize(DeviceState *dev, Error **errp)
     qemu_del_vm_change_state_handler(vrng->vmstate);
     timer_del(vrng->rate_limit_timer);
     timer_free(vrng->rate_limit_timer);
+    if (virtio_has_feature(vrng->conf.host_features, VIRTIO_RNG_F_CTRL_VQ)) {
+        virtio_delete_queue(vrng->ctrl_vq);
+    }
     virtio_delete_queue(vrng->input_vq);
     virtio_cleanup(vdev);
 }
@@ -261,6 +334,8 @@ static Property virtio_rng_properties[] = {
     DEFINE_PROP_UINT64("max-bytes", VirtIORNG, conf.max_bytes, INT64_MAX),
     DEFINE_PROP_UINT32("period", VirtIORNG, conf.period_ms, 1 << 16),
     DEFINE_PROP_LINK("rng", VirtIORNG, conf.rng, TYPE_RNG_BACKEND, RngBackend *),
+    DEFINE_PROP_BIT64("ctrl-queue", VirtIORNG, conf.host_features,
+                      VIRTIO_RNG_F_CTRL_VQ, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -274,7 +349,7 @@ static void virtio_rng_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_MISC, dc->categories);
     vdc->realize = virtio_rng_device_realize;
     vdc->unrealize = virtio_rng_device_unrealize;
-    vdc->get_features = get_features;
+    vdc->get_features = virtio_rng_get_features;
     vdc->set_status = virtio_rng_set_status;
 }
 
diff --git a/include/hw/virtio/virtio-rng.h b/include/hw/virtio/virtio-rng.h
index d77daf126828..abecec7b244a 100644
--- a/include/hw/virtio/virtio-rng.h
+++ b/include/hw/virtio/virtio-rng.h
@@ -26,13 +26,14 @@ struct VirtIORNGConf {
     RngBackend *rng;
     uint64_t max_bytes;
     uint32_t period_ms;
+    uint64_t host_features;
 };
 
 typedef struct VirtIORNG {
     VirtIODevice parent_obj;
 
-    /* Only one vq - guest puts buffer(s) on it when it needs entropy */
     VirtQueue *input_vq;
+    VirtQueue *ctrl_vq;
 
     VirtIORNGConf conf;
 
diff --git a/include/standard-headers/linux/virtio_rng.h b/include/standard-headers/linux/virtio_rng.h
index 60fc798bde18..b80e9298817e 100644
--- a/include/standard-headers/linux/virtio_rng.h
+++ b/include/standard-headers/linux/virtio_rng.h
@@ -5,4 +5,18 @@
 #include "standard-headers/linux/virtio_ids.h"
 #include "standard-headers/linux/virtio_config.h"
 
+/* The features bitmap for virtuio rng */
+#define VIRTIO_RNG_F_CTRL_VQ		0	/* Device has control queue */
+
+struct virtio_rng_ctrl_hdr {
+	uint8_t cmd;
+} QEMU_PACKED;
+
+#define VIRTIO_RNG_CMD_FLUSH 0
+
+typedef uint8_t virtio_rng_ctrl_ack;
+
+#define VIRTIO_RNG_OK	0
+#define VIRTIO_RNG_ERR	1
+
 #endif /* _LINUX_VIRTIO_RNG_H */
-- 
2.23.0

Re: [RFC 0/2] virtio-rng: add a control queue

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1396 bytes --]

On Thu, Jan 23, 2020 at 04:16:58PM +0100, Laurent Vivier wrote:
> The kernel needs sometime to be able to cancel an ongoing command.
> 
> For instance, if the virtio-rng device uses the egd backend
> and this backend doesn't provide data, the buffer provided by the
> kernel is kept as long as it is needed.
> 
> On the kernel side, a read blocks until the buffer returns from QEMU.
> 
> As the read is done with a mutex held, all the hw_random interface
> hangs and we cannot switch to another hw_random backend.
> 
> So this series adds a control queue to the virtio-rng device to allow
> to flush the virtio-rng input queue to release the kernel mutex and
> to allow to switch to another device.
> 
> The kernel side series can be found at:
> 
> https://github.com/vivier/linux/commits/virtio-rng-ctrl
> 
> Laurent Vivier (2):
>   virtio-rng: prepare the introduction of a control queue
>   virtio-rng: add a control queue
> 
>  hw/core/machine.c                           |  1 +
>  hw/virtio/trace-events                      |  6 ++
>  hw/virtio/virtio-rng.c                      | 99 ++++++++++++++++++---
>  include/hw/virtio/virtio-rng.h              |  5 +-
>  include/standard-headers/linux/virtio_rng.h | 14 +++
>  5 files changed, 111 insertions(+), 14 deletions(-)

Where can I find the VIRTIO specification for this new virtqueue?

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 0/2] virtio-rng: add a control queue

[Laurent Vivier]

On 24/01/2020 12:02, Stefan Hajnoczi wrote:
> On Thu, Jan 23, 2020 at 04:16:58PM +0100, Laurent Vivier wrote:
>> The kernel needs sometime to be able to cancel an ongoing command.
>>
>> For instance, if the virtio-rng device uses the egd backend
>> and this backend doesn't provide data, the buffer provided by the
>> kernel is kept as long as it is needed.
>>
>> On the kernel side, a read blocks until the buffer returns from QEMU.
>>
>> As the read is done with a mutex held, all the hw_random interface
>> hangs and we cannot switch to another hw_random backend.
>>
>> So this series adds a control queue to the virtio-rng device to allow
>> to flush the virtio-rng input queue to release the kernel mutex and
>> to allow to switch to another device.
>>
>> The kernel side series can be found at:
>>
>> https://github.com/vivier/linux/commits/virtio-rng-ctrl
>>
>> Laurent Vivier (2):
>>   virtio-rng: prepare the introduction of a control queue
>>   virtio-rng: add a control queue
>>
>>  hw/core/machine.c                           |  1 +
>>  hw/virtio/trace-events                      |  6 ++
>>  hw/virtio/virtio-rng.c                      | 99 ++++++++++++++++++---
>>  include/hw/virtio/virtio-rng.h              |  5 +-
>>  include/standard-headers/linux/virtio_rng.h | 14 +++
>>  5 files changed, 111 insertions(+), 14 deletions(-)
> 
> Where can I find the VIRTIO specification for this new virtqueue?

I didn't update the specs.

Is https://github.com/oasis-tcs/virtio-spec.git the document to update?

Thanks,
Laurent

Re: [RFC 0/2] virtio-rng: add a control queue

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1719 bytes --]

On Fri, Jan 24, 2020 at 03:05:18PM +0100, Laurent Vivier wrote:
> On 24/01/2020 12:02, Stefan Hajnoczi wrote:
> > On Thu, Jan 23, 2020 at 04:16:58PM +0100, Laurent Vivier wrote:
> >> The kernel needs sometime to be able to cancel an ongoing command.
> >>
> >> For instance, if the virtio-rng device uses the egd backend
> >> and this backend doesn't provide data, the buffer provided by the
> >> kernel is kept as long as it is needed.
> >>
> >> On the kernel side, a read blocks until the buffer returns from QEMU.
> >>
> >> As the read is done with a mutex held, all the hw_random interface
> >> hangs and we cannot switch to another hw_random backend.
> >>
> >> So this series adds a control queue to the virtio-rng device to allow
> >> to flush the virtio-rng input queue to release the kernel mutex and
> >> to allow to switch to another device.
> >>
> >> The kernel side series can be found at:
> >>
> >> https://github.com/vivier/linux/commits/virtio-rng-ctrl
> >>
> >> Laurent Vivier (2):
> >>   virtio-rng: prepare the introduction of a control queue
> >>   virtio-rng: add a control queue
> >>
> >>  hw/core/machine.c                           |  1 +
> >>  hw/virtio/trace-events                      |  6 ++
> >>  hw/virtio/virtio-rng.c                      | 99 ++++++++++++++++++---
> >>  include/hw/virtio/virtio-rng.h              |  5 +-
> >>  include/standard-headers/linux/virtio_rng.h | 14 +++
> >>  5 files changed, 111 insertions(+), 14 deletions(-)
> > 
> > Where can I find the VIRTIO specification for this new virtqueue?
> 
> I didn't update the specs.
> 
> Is https://github.com/oasis-tcs/virtio-spec.git the document to update?

Yes, please.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 0/2] virtio-rng: add a control queue

[Laurent Vivier]

On 29/01/2020 16:43, Stefan Hajnoczi wrote:
> On Fri, Jan 24, 2020 at 03:05:18PM +0100, Laurent Vivier wrote:
>> On 24/01/2020 12:02, Stefan Hajnoczi wrote:
>>> On Thu, Jan 23, 2020 at 04:16:58PM +0100, Laurent Vivier wrote:
>>>> The kernel needs sometime to be able to cancel an ongoing command.
>>>>
>>>> For instance, if the virtio-rng device uses the egd backend
>>>> and this backend doesn't provide data, the buffer provided by the
>>>> kernel is kept as long as it is needed.
>>>>
>>>> On the kernel side, a read blocks until the buffer returns from QEMU.
>>>>
>>>> As the read is done with a mutex held, all the hw_random interface
>>>> hangs and we cannot switch to another hw_random backend.
>>>>
>>>> So this series adds a control queue to the virtio-rng device to allow
>>>> to flush the virtio-rng input queue to release the kernel mutex and
>>>> to allow to switch to another device.
>>>>
>>>> The kernel side series can be found at:
>>>>
>>>> https://github.com/vivier/linux/commits/virtio-rng-ctrl
>>>>
>>>> Laurent Vivier (2):
>>>>   virtio-rng: prepare the introduction of a control queue
>>>>   virtio-rng: add a control queue
>>>>
>>>>  hw/core/machine.c                           |  1 +
>>>>  hw/virtio/trace-events                      |  6 ++
>>>>  hw/virtio/virtio-rng.c                      | 99 ++++++++++++++++++---
>>>>  include/hw/virtio/virtio-rng.h              |  5 +-
>>>>  include/standard-headers/linux/virtio_rng.h | 14 +++
>>>>  5 files changed, 111 insertions(+), 14 deletions(-)
>>>
>>> Where can I find the VIRTIO specification for this new virtqueue?
>>
>> I didn't update the specs.
>>
>> Is https://github.com/oasis-tcs/virtio-spec.git the document to update?
> 
> Yes, please.

I've updated the specs,

Following
https://github.com/oasis-tcs/virtio-spec/blob/master/CONTRIBUTING.md,
I've opened an issue:

https://github.com/oasis-tcs/virtio-spec/issues/83

Is this the good process?

Thanks,
Laurent

Re: [RFC 0/2] virtio-rng: add a control queue

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2313 bytes --]

On Tue, Jul 28, 2020 at 03:45:26PM +0200, Laurent Vivier wrote:
> On 29/01/2020 16:43, Stefan Hajnoczi wrote:
> > On Fri, Jan 24, 2020 at 03:05:18PM +0100, Laurent Vivier wrote:
> >> On 24/01/2020 12:02, Stefan Hajnoczi wrote:
> >>> On Thu, Jan 23, 2020 at 04:16:58PM +0100, Laurent Vivier wrote:
> >>>> The kernel needs sometime to be able to cancel an ongoing command.
> >>>>
> >>>> For instance, if the virtio-rng device uses the egd backend
> >>>> and this backend doesn't provide data, the buffer provided by the
> >>>> kernel is kept as long as it is needed.
> >>>>
> >>>> On the kernel side, a read blocks until the buffer returns from QEMU.
> >>>>
> >>>> As the read is done with a mutex held, all the hw_random interface
> >>>> hangs and we cannot switch to another hw_random backend.
> >>>>
> >>>> So this series adds a control queue to the virtio-rng device to allow
> >>>> to flush the virtio-rng input queue to release the kernel mutex and
> >>>> to allow to switch to another device.
> >>>>
> >>>> The kernel side series can be found at:
> >>>>
> >>>> https://github.com/vivier/linux/commits/virtio-rng-ctrl
> >>>>
> >>>> Laurent Vivier (2):
> >>>>   virtio-rng: prepare the introduction of a control queue
> >>>>   virtio-rng: add a control queue
> >>>>
> >>>>  hw/core/machine.c                           |  1 +
> >>>>  hw/virtio/trace-events                      |  6 ++
> >>>>  hw/virtio/virtio-rng.c                      | 99 ++++++++++++++++++---
> >>>>  include/hw/virtio/virtio-rng.h              |  5 +-
> >>>>  include/standard-headers/linux/virtio_rng.h | 14 +++
> >>>>  5 files changed, 111 insertions(+), 14 deletions(-)
> >>>
> >>> Where can I find the VIRTIO specification for this new virtqueue?
> >>
> >> I didn't update the specs.
> >>
> >> Is https://github.com/oasis-tcs/virtio-spec.git the document to update?
> > 
> > Yes, please.
> 
> I've updated the specs,
> 
> Following
> https://github.com/oasis-tcs/virtio-spec/blob/master/CONTRIBUTING.md,
> I've opened an issue:
> 
> https://github.com/oasis-tcs/virtio-spec/issues/83
> 
> Is this the good process?

Please post spec changes to the virtio-comment@lists.oasis-open.org
mailing list:
https://github.com/oasis-tcs/virtio-spec/#providing-feedback

Thanks,
Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 0/2] virtio-rng: add a control queue

[Michael S. Tsirkin]

On Tue, Jul 28, 2020 at 03:45:26PM +0200, Laurent Vivier wrote:
> On 29/01/2020 16:43, Stefan Hajnoczi wrote:
> > On Fri, Jan 24, 2020 at 03:05:18PM +0100, Laurent Vivier wrote:
> >> On 24/01/2020 12:02, Stefan Hajnoczi wrote:
> >>> On Thu, Jan 23, 2020 at 04:16:58PM +0100, Laurent Vivier wrote:
> >>>> The kernel needs sometime to be able to cancel an ongoing command.
> >>>>
> >>>> For instance, if the virtio-rng device uses the egd backend
> >>>> and this backend doesn't provide data, the buffer provided by the
> >>>> kernel is kept as long as it is needed.
> >>>>
> >>>> On the kernel side, a read blocks until the buffer returns from QEMU.
> >>>>
> >>>> As the read is done with a mutex held, all the hw_random interface
> >>>> hangs and we cannot switch to another hw_random backend.
> >>>>
> >>>> So this series adds a control queue to the virtio-rng device to allow
> >>>> to flush the virtio-rng input queue to release the kernel mutex and
> >>>> to allow to switch to another device.
> >>>>
> >>>> The kernel side series can be found at:
> >>>>
> >>>> https://github.com/vivier/linux/commits/virtio-rng-ctrl
> >>>>
> >>>> Laurent Vivier (2):
> >>>>   virtio-rng: prepare the introduction of a control queue
> >>>>   virtio-rng: add a control queue
> >>>>
> >>>>  hw/core/machine.c                           |  1 +
> >>>>  hw/virtio/trace-events                      |  6 ++
> >>>>  hw/virtio/virtio-rng.c                      | 99 ++++++++++++++++++---
> >>>>  include/hw/virtio/virtio-rng.h              |  5 +-
> >>>>  include/standard-headers/linux/virtio_rng.h | 14 +++
> >>>>  5 files changed, 111 insertions(+), 14 deletions(-)
> >>>
> >>> Where can I find the VIRTIO specification for this new virtqueue?
> >>
> >> I didn't update the specs.
> >>
> >> Is https://github.com/oasis-tcs/virtio-spec.git the document to update?
> > 
> > Yes, please.
> 
> I've updated the specs,
> 
> Following
> https://github.com/oasis-tcs/virtio-spec/blob/master/CONTRIBUTING.md,
> I've opened an issue:
> 
> https://github.com/oasis-tcs/virtio-spec/issues/83
> 
> Is this the good process?
> 
> Thanks,
> Laurent


It's ok but you also need to send spec patches by mail so
people can review.

Re: [RFC 0/2] virtio-rng: add a control queue

[Amit Shah]

On Thu, 2020-01-23 at 16:16 +0100, Laurent Vivier wrote:
> The kernel needs sometime to be able to cancel an ongoing command.
> 
> For instance, if the virtio-rng device uses the egd backend
> and this backend doesn't provide data, the buffer provided by the
> kernel is kept as long as it is needed.
> 
> On the kernel side, a read blocks until the buffer returns from QEMU.
> 
> As the read is done with a mutex held, all the hw_random interface
> hangs and we cannot switch to another hw_random backend.
> 
> So this series adds a control queue to the virtio-rng device to allow
> to flush the virtio-rng input queue to release the kernel mutex and
> to allow to switch to another device.
> 
> The kernel side series can be found at:
> 
> https://github.com/vivier/linux/commits/virtio-rng-ctrl

Did you submit the kernel series too?  Can you please CC me to it?

This will need spec changes as well, can you please point me to them
too?

I also recall a previous discussion about this, but my search-fu is
failing to find it...

		Amit

Re: [RFC 0/2] virtio-rng: add a control queue

[Laurent Vivier]

On 24/01/2020 13:43, Amit Shah wrote:
> On Thu, 2020-01-23 at 16:16 +0100, Laurent Vivier wrote:
>> The kernel needs sometime to be able to cancel an ongoing command.
>>
>> For instance, if the virtio-rng device uses the egd backend
>> and this backend doesn't provide data, the buffer provided by the
>> kernel is kept as long as it is needed.
>>
>> On the kernel side, a read blocks until the buffer returns from QEMU.
>>
>> As the read is done with a mutex held, all the hw_random interface
>> hangs and we cannot switch to another hw_random backend.
>>
>> So this series adds a control queue to the virtio-rng device to allow
>> to flush the virtio-rng input queue to release the kernel mutex and
>> to allow to switch to another device.
>>
>> The kernel side series can be found at:
>>
>> https://github.com/vivier/linux/commits/virtio-rng-ctrl
> 
> Did you submit the kernel series too?  Can you please CC me to it?

No, I didn't. I'd like to have some comments on the QEMU side first.
QEMU list is generally more responsive than kernel one.

It's why I put the link to my linux branch here.

> This will need spec changes as well, can you please point me to them
> too?

The same here. I didn't update the specs, I'd like to have some comments
before.

BTW, where can I find the specs to update?
Is this https://github.com/oasis-tcs/virtio-spec ?

> I also recall a previous discussion about this, but my search-fu is
> failing to find it...

See

[RFC] virtio-rng: add a watchdog
https://patchwork.kernel.org/patch/10987983/

Thanks,
Laurent