[PATCH 0/4] hwrng: virtio - add an internal buffer

[PATCH 0/4] hwrng: virtio - add an internal buffer

[Laurent Vivier]

hwrng core uses two buffers that can be mixed in the virtio-rng queue.

This series fixes the problem by adding an internal buffer in virtio-rng.

Once the internal buffer is added, we can fix two other problems:

- to be able to release the driver without waiting the device releases the
  buffer

- actually returns some data when wait=0 as we can have some already
  available data

It also tries to improve the performance by always having a buffer in
the queue of the device.

Laurent Vivier (4):
  hwrng: virtio - add an internal buffer
  hwrng: virtio - don't wait on cleanup
  hwrng: virtio - don't waste entropy
  hwrng: virtio - always add a pending request

 drivers/char/hw_random/virtio-rng.c | 84 +++++++++++++++++++++--------
 1 file changed, 63 insertions(+), 21 deletions(-)

-- 
2.31.1

[PATCH 0/4] hwrng: virtio - add an internal buffer

[Laurent Vivier]

hwrng core uses two buffers that can be mixed in the virtio-rng queue.

This series fixes the problem by adding an internal buffer in virtio-rng.

Once the internal buffer is added, we can fix two other problems:

- to be able to release the driver without waiting the device releases the
  buffer

- actually returns some data when wait=0 as we can have some already
  available data

It also tries to improve the performance by always having a buffer in
the queue of the device.

Laurent Vivier (4):
  hwrng: virtio - add an internal buffer
  hwrng: virtio - don't wait on cleanup
  hwrng: virtio - don't waste entropy
  hwrng: virtio - always add a pending request

 drivers/char/hw_random/virtio-rng.c | 84 +++++++++++++++++++++--------
 1 file changed, 63 insertions(+), 21 deletions(-)

-- 
2.31.1


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 1/4] hwrng: virtio - add an internal buffer

[Laurent Vivier]

hwrng core uses two buffers that can be mixed in the
virtio-rng queue.

If the buffer is provided with wait=0 it is enqueued in the
virtio-rng queue but unused by the caller.
On the next call, core provides another buffer but the
first one is filled instead and the new one queued.
And the caller reads the data from the new one that is not
updated, and the data in the first one are lost.

To avoid this mix, virtio-rng needs to use its own unique
internal buffer at a cost of a data copy to the caller buffer.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 drivers/char/hw_random/virtio-rng.c | 43 ++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index a90001e02bf7..208c547dcac1 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -18,13 +18,20 @@ static DEFINE_IDA(rng_index_ida);
 struct virtrng_info {
 	struct hwrng hwrng;
 	struct virtqueue *vq;
-	struct completion have_data;
 	char name[25];
-	unsigned int data_avail;
 	int index;
 	bool busy;
 	bool hwrng_register_done;
 	bool hwrng_removed;
+	/* data transfer */
+	struct completion have_data;
+	unsigned int data_avail;
+	/* minimal size returned by rng_buffer_size() */
+#if SMP_CACHE_BYTES < 32
+	u8 data[32];
+#else
+	u8 data[SMP_CACHE_BYTES];
+#endif
 };
 
 static void random_recv_done(struct virtqueue *vq)
@@ -39,14 +46,14 @@ static void random_recv_done(struct virtqueue *vq)
 }
 
 /* The host will fill any buffer we give it with sweet, sweet randomness. */
-static void register_buffer(struct virtrng_info *vi, u8 *buf, size_t size)
+static void register_buffer(struct virtrng_info *vi)
 {
 	struct scatterlist sg;
 
-	sg_init_one(&sg, buf, size);
+	sg_init_one(&sg, vi->data, sizeof(vi->data));
 
 	/* There should always be room for one buffer. */
-	virtqueue_add_inbuf(vi->vq, &sg, 1, buf, GFP_KERNEL);
+	virtqueue_add_inbuf(vi->vq, &sg, 1, vi->data, GFP_KERNEL);
 
 	virtqueue_kick(vi->vq);
 }
@@ -55,6 +62,8 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 {
 	int ret;
 	struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
+	unsigned int chunk;
+	size_t read;
 
 	if (vi->hwrng_removed)
 		return -ENODEV;
@@ -62,19 +71,33 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 	if (!vi->busy) {
 		vi->busy = true;
 		reinit_completion(&vi->have_data);
-		register_buffer(vi, buf, size);
+		register_buffer(vi);
 	}
 
 	if (!wait)
 		return 0;
 
-	ret = wait_for_completion_killable(&vi->have_data);
-	if (ret < 0)
-		return ret;
+	read = 0;
+	while (size != 0) {
+		ret = wait_for_completion_killable(&vi->have_data);
+		if (ret < 0)
+			return ret;
+
+		chunk = min_t(unsigned int, size, vi->data_avail);
+		memcpy(buf + read, vi->data, chunk);
+		read += chunk;
+		size -= chunk;
+		vi->data_avail = 0;
+
+		if (size != 0) {
+			reinit_completion(&vi->have_data);
+			register_buffer(vi);
+		}
+	}
 
 	vi->busy = false;
 
-	return vi->data_avail;
+	return read;
 }
 
 static void virtio_cleanup(struct hwrng *rng)
-- 
2.31.1

[PATCH 1/4] hwrng: virtio - add an internal buffer

[Laurent Vivier]

hwrng core uses two buffers that can be mixed in the
virtio-rng queue.

If the buffer is provided with wait=0 it is enqueued in the
virtio-rng queue but unused by the caller.
On the next call, core provides another buffer but the
first one is filled instead and the new one queued.
And the caller reads the data from the new one that is not
updated, and the data in the first one are lost.

To avoid this mix, virtio-rng needs to use its own unique
internal buffer at a cost of a data copy to the caller buffer.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 drivers/char/hw_random/virtio-rng.c | 43 ++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index a90001e02bf7..208c547dcac1 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -18,13 +18,20 @@ static DEFINE_IDA(rng_index_ida);
 struct virtrng_info {
 	struct hwrng hwrng;
 	struct virtqueue *vq;
-	struct completion have_data;
 	char name[25];
-	unsigned int data_avail;
 	int index;
 	bool busy;
 	bool hwrng_register_done;
 	bool hwrng_removed;
+	/* data transfer */
+	struct completion have_data;
+	unsigned int data_avail;
+	/* minimal size returned by rng_buffer_size() */
+#if SMP_CACHE_BYTES < 32
+	u8 data[32];
+#else
+	u8 data[SMP_CACHE_BYTES];
+#endif
 };
 
 static void random_recv_done(struct virtqueue *vq)
@@ -39,14 +46,14 @@ static void random_recv_done(struct virtqueue *vq)
 }
 
 /* The host will fill any buffer we give it with sweet, sweet randomness. */
-static void register_buffer(struct virtrng_info *vi, u8 *buf, size_t size)
+static void register_buffer(struct virtrng_info *vi)
 {
 	struct scatterlist sg;
 
-	sg_init_one(&sg, buf, size);
+	sg_init_one(&sg, vi->data, sizeof(vi->data));
 
 	/* There should always be room for one buffer. */
-	virtqueue_add_inbuf(vi->vq, &sg, 1, buf, GFP_KERNEL);
+	virtqueue_add_inbuf(vi->vq, &sg, 1, vi->data, GFP_KERNEL);
 
 	virtqueue_kick(vi->vq);
 }
@@ -55,6 +62,8 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 {
 	int ret;
 	struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
+	unsigned int chunk;
+	size_t read;
 
 	if (vi->hwrng_removed)
 		return -ENODEV;
@@ -62,19 +71,33 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 	if (!vi->busy) {
 		vi->busy = true;
 		reinit_completion(&vi->have_data);
-		register_buffer(vi, buf, size);
+		register_buffer(vi);
 	}
 
 	if (!wait)
 		return 0;
 
-	ret = wait_for_completion_killable(&vi->have_data);
-	if (ret < 0)
-		return ret;
+	read = 0;
+	while (size != 0) {
+		ret = wait_for_completion_killable(&vi->have_data);
+		if (ret < 0)
+			return ret;
+
+		chunk = min_t(unsigned int, size, vi->data_avail);
+		memcpy(buf + read, vi->data, chunk);
+		read += chunk;
+		size -= chunk;
+		vi->data_avail = 0;
+
+		if (size != 0) {
+			reinit_completion(&vi->have_data);
+			register_buffer(vi);
+		}
+	}
 
 	vi->busy = false;
 
-	return vi->data_avail;
+	return read;
 }
 
 static void virtio_cleanup(struct hwrng *rng)
-- 
2.31.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 2/4] hwrng: virtio - don't wait on cleanup

[Laurent Vivier]

When virtio-rng device was dropped by the hwrng core we were forced
to wait the buffer to come back from the device to not have
remaining ongoing operation that could spoil the buffer.

But now, as the buffer is internal to the virtio-rng we can release
the waiting loop immediately, the buffer will be retrieve and use
when the virtio-rng driver will be selected again.

This avoids to hang on an rng_current write command if the virtio-rng
device is blocked by a lack of entropy. This allows to select
another entropy source if the current one is empty.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 drivers/char/hw_random/virtio-rng.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index 208c547dcac1..173aeea835bb 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -82,6 +82,11 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 		ret = wait_for_completion_killable(&vi->have_data);
 		if (ret < 0)
 			return ret;
+		/* if vi->data_avail is 0, we have been interrupted
+		 * by a cleanup, but buffer stays in the queue
+		 */
+		if (vi->data_avail == 0)
+			return read;
 
 		chunk = min_t(unsigned int, size, vi->data_avail);
 		memcpy(buf + read, vi->data, chunk);
@@ -105,7 +110,7 @@ static void virtio_cleanup(struct hwrng *rng)
 	struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
 
 	if (vi->busy)
-		wait_for_completion(&vi->have_data);
+		complete(&vi->have_data);
 }
 
 static int probe_common(struct virtio_device *vdev)
-- 
2.31.1

[PATCH 2/4] hwrng: virtio - don't wait on cleanup

[Laurent Vivier]

When virtio-rng device was dropped by the hwrng core we were forced
to wait the buffer to come back from the device to not have
remaining ongoing operation that could spoil the buffer.

But now, as the buffer is internal to the virtio-rng we can release
the waiting loop immediately, the buffer will be retrieve and use
when the virtio-rng driver will be selected again.

This avoids to hang on an rng_current write command if the virtio-rng
device is blocked by a lack of entropy. This allows to select
another entropy source if the current one is empty.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 drivers/char/hw_random/virtio-rng.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index 208c547dcac1..173aeea835bb 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -82,6 +82,11 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 		ret = wait_for_completion_killable(&vi->have_data);
 		if (ret < 0)
 			return ret;
+		/* if vi->data_avail is 0, we have been interrupted
+		 * by a cleanup, but buffer stays in the queue
+		 */
+		if (vi->data_avail == 0)
+			return read;
 
 		chunk = min_t(unsigned int, size, vi->data_avail);
 		memcpy(buf + read, vi->data, chunk);
@@ -105,7 +110,7 @@ static void virtio_cleanup(struct hwrng *rng)
 	struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
 
 	if (vi->busy)
-		wait_for_completion(&vi->have_data);
+		complete(&vi->have_data);
 }
 
 static int probe_common(struct virtio_device *vdev)
-- 
2.31.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 3/4] hwrng: virtio - don't waste entropy

[Laurent Vivier]

if we don't use all the entropy available in the buffer, keep it
and use it later.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 drivers/char/hw_random/virtio-rng.c | 52 +++++++++++++++++++----------
 1 file changed, 35 insertions(+), 17 deletions(-)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index 173aeea835bb..8ba97cf4ca8f 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -26,6 +26,7 @@ struct virtrng_info {
 	/* data transfer */
 	struct completion have_data;
 	unsigned int data_avail;
+	unsigned int data_idx;
 	/* minimal size returned by rng_buffer_size() */
 #if SMP_CACHE_BYTES < 32
 	u8 data[32];
@@ -42,6 +43,9 @@ static void random_recv_done(struct virtqueue *vq)
 	if (!virtqueue_get_buf(vi->vq, &vi->data_avail))
 		return;
 
+	vi->data_idx = 0;
+	vi->busy = false;
+
 	complete(&vi->have_data);
 }
 
@@ -58,6 +62,16 @@ static void register_buffer(struct virtrng_info *vi)
 	virtqueue_kick(vi->vq);
 }
 
+static unsigned int copy_data(struct virtrng_info *vi, void *buf,
+			      unsigned int size)
+{
+	size = min_t(unsigned int, size, vi->data_avail);
+	memcpy(buf, vi->data + vi->data_idx, size);
+	vi->data_idx += size;
+	vi->data_avail -= size;
+	return size;
+}
+
 static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 {
 	int ret;
@@ -68,17 +82,29 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 	if (vi->hwrng_removed)
 		return -ENODEV;
 
-	if (!vi->busy) {
-		vi->busy = true;
-		reinit_completion(&vi->have_data);
-		register_buffer(vi);
+	read = 0;
+
+	/* copy available data */
+	if (vi->data_avail) {
+		chunk = copy_data(vi, buf, size);
+		size -= chunk;
+		read += chunk;
 	}
 
 	if (!wait)
-		return 0;
+		return read;
 
-	read = 0;
+	/* We have already copied available entropy,
+	 * so either size is 0 or data_avail is 0
+	 */
 	while (size != 0) {
+		/* data_avail is 0 */
+		if (!vi->busy) {
+			/* no pending request, ask for more */
+			vi->busy = true;
+			reinit_completion(&vi->have_data);
+			register_buffer(vi);
+		}
 		ret = wait_for_completion_killable(&vi->have_data);
 		if (ret < 0)
 			return ret;
@@ -88,20 +114,11 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 		if (vi->data_avail == 0)
 			return read;
 
-		chunk = min_t(unsigned int, size, vi->data_avail);
-		memcpy(buf + read, vi->data, chunk);
-		read += chunk;
+		chunk = copy_data(vi, buf + read, size);
 		size -= chunk;
-		vi->data_avail = 0;
-
-		if (size != 0) {
-			reinit_completion(&vi->have_data);
-			register_buffer(vi);
-		}
+		read += chunk;
 	}
 
-	vi->busy = false;
-
 	return read;
 }
 
@@ -161,6 +178,7 @@ static void remove_common(struct virtio_device *vdev)
 
 	vi->hwrng_removed = true;
 	vi->data_avail = 0;
+	vi->data_idx = 0;
 	complete(&vi->have_data);
 	vdev->config->reset(vdev);
 	vi->busy = false;
-- 
2.31.1

[PATCH 3/4] hwrng: virtio - don't waste entropy

[Laurent Vivier]

if we don't use all the entropy available in the buffer, keep it
and use it later.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 drivers/char/hw_random/virtio-rng.c | 52 +++++++++++++++++++----------
 1 file changed, 35 insertions(+), 17 deletions(-)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index 173aeea835bb..8ba97cf4ca8f 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -26,6 +26,7 @@ struct virtrng_info {
 	/* data transfer */
 	struct completion have_data;
 	unsigned int data_avail;
+	unsigned int data_idx;
 	/* minimal size returned by rng_buffer_size() */
 #if SMP_CACHE_BYTES < 32
 	u8 data[32];
@@ -42,6 +43,9 @@ static void random_recv_done(struct virtqueue *vq)
 	if (!virtqueue_get_buf(vi->vq, &vi->data_avail))
 		return;
 
+	vi->data_idx = 0;
+	vi->busy = false;
+
 	complete(&vi->have_data);
 }
 
@@ -58,6 +62,16 @@ static void register_buffer(struct virtrng_info *vi)
 	virtqueue_kick(vi->vq);
 }
 
+static unsigned int copy_data(struct virtrng_info *vi, void *buf,
+			      unsigned int size)
+{
+	size = min_t(unsigned int, size, vi->data_avail);
+	memcpy(buf, vi->data + vi->data_idx, size);
+	vi->data_idx += size;
+	vi->data_avail -= size;
+	return size;
+}
+
 static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 {
 	int ret;
@@ -68,17 +82,29 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 	if (vi->hwrng_removed)
 		return -ENODEV;
 
-	if (!vi->busy) {
-		vi->busy = true;
-		reinit_completion(&vi->have_data);
-		register_buffer(vi);
+	read = 0;
+
+	/* copy available data */
+	if (vi->data_avail) {
+		chunk = copy_data(vi, buf, size);
+		size -= chunk;
+		read += chunk;
 	}
 
 	if (!wait)
-		return 0;
+		return read;
 
-	read = 0;
+	/* We have already copied available entropy,
+	 * so either size is 0 or data_avail is 0
+	 */
 	while (size != 0) {
+		/* data_avail is 0 */
+		if (!vi->busy) {
+			/* no pending request, ask for more */
+			vi->busy = true;
+			reinit_completion(&vi->have_data);
+			register_buffer(vi);
+		}
 		ret = wait_for_completion_killable(&vi->have_data);
 		if (ret < 0)
 			return ret;
@@ -88,20 +114,11 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 		if (vi->data_avail == 0)
 			return read;
 
-		chunk = min_t(unsigned int, size, vi->data_avail);
-		memcpy(buf + read, vi->data, chunk);
-		read += chunk;
+		chunk = copy_data(vi, buf + read, size);
 		size -= chunk;
-		vi->data_avail = 0;
-
-		if (size != 0) {
-			reinit_completion(&vi->have_data);
-			register_buffer(vi);
-		}
+		read += chunk;
 	}
 
-	vi->busy = false;
-
 	return read;
 }
 
@@ -161,6 +178,7 @@ static void remove_common(struct virtio_device *vdev)
 
 	vi->hwrng_removed = true;
 	vi->data_avail = 0;
+	vi->data_idx = 0;
 	complete(&vi->have_data);
 	vdev->config->reset(vdev);
 	vi->busy = false;
-- 
2.31.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 4/4] hwrng: virtio - always add a pending request

[Laurent Vivier]

If we ensure we have already some data available by enqueuing
again the buffer once data are exhausted, we can return what we
have without waiting for the device answer.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 drivers/char/hw_random/virtio-rng.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index 8ba97cf4ca8f..0b3c9b643495 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -20,7 +20,6 @@ struct virtrng_info {
 	struct virtqueue *vq;
 	char name[25];
 	int index;
-	bool busy;
 	bool hwrng_register_done;
 	bool hwrng_removed;
 	/* data transfer */
@@ -44,16 +43,16 @@ static void random_recv_done(struct virtqueue *vq)
 		return;
 
 	vi->data_idx = 0;
-	vi->busy = false;
 
 	complete(&vi->have_data);
 }
 
-/* The host will fill any buffer we give it with sweet, sweet randomness. */
-static void register_buffer(struct virtrng_info *vi)
+static void request_entropy(struct virtrng_info *vi)
 {
 	struct scatterlist sg;
 
+	reinit_completion(&vi->have_data);
+
 	sg_init_one(&sg, vi->data, sizeof(vi->data));
 
 	/* There should always be room for one buffer. */
@@ -69,6 +68,8 @@ static unsigned int copy_data(struct virtrng_info *vi, void *buf,
 	memcpy(buf, vi->data + vi->data_idx, size);
 	vi->data_idx += size;
 	vi->data_avail -= size;
+	if (vi->data_avail == 0)
+		request_entropy(vi);
 	return size;
 }
 
@@ -98,13 +99,7 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 	 * so either size is 0 or data_avail is 0
 	 */
 	while (size != 0) {
-		/* data_avail is 0 */
-		if (!vi->busy) {
-			/* no pending request, ask for more */
-			vi->busy = true;
-			reinit_completion(&vi->have_data);
-			register_buffer(vi);
-		}
+		/* data_avail is 0 but a request is pending */
 		ret = wait_for_completion_killable(&vi->have_data);
 		if (ret < 0)
 			return ret;
@@ -126,8 +121,7 @@ static void virtio_cleanup(struct hwrng *rng)
 {
 	struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
 
-	if (vi->busy)
-		complete(&vi->have_data);
+	complete(&vi->have_data);
 }
 
 static int probe_common(struct virtio_device *vdev)
@@ -163,6 +157,9 @@ static int probe_common(struct virtio_device *vdev)
 		goto err_find;
 	}
 
+	/* we always have a pending entropy request */
+	request_entropy(vi);
+
 	return 0;
 
 err_find:
@@ -181,7 +178,6 @@ static void remove_common(struct virtio_device *vdev)
 	vi->data_idx = 0;
 	complete(&vi->have_data);
 	vdev->config->reset(vdev);
-	vi->busy = false;
 	if (vi->hwrng_register_done)
 		hwrng_unregister(&vi->hwrng);
 	vdev->config->del_vqs(vdev);
-- 
2.31.1

[PATCH 4/4] hwrng: virtio - always add a pending request

[Laurent Vivier]

If we ensure we have already some data available by enqueuing
again the buffer once data are exhausted, we can return what we
have without waiting for the device answer.

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
---
 drivers/char/hw_random/virtio-rng.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index 8ba97cf4ca8f..0b3c9b643495 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -20,7 +20,6 @@ struct virtrng_info {
 	struct virtqueue *vq;
 	char name[25];
 	int index;
-	bool busy;
 	bool hwrng_register_done;
 	bool hwrng_removed;
 	/* data transfer */
@@ -44,16 +43,16 @@ static void random_recv_done(struct virtqueue *vq)
 		return;
 
 	vi->data_idx = 0;
-	vi->busy = false;
 
 	complete(&vi->have_data);
 }
 
-/* The host will fill any buffer we give it with sweet, sweet randomness. */
-static void register_buffer(struct virtrng_info *vi)
+static void request_entropy(struct virtrng_info *vi)
 {
 	struct scatterlist sg;
 
+	reinit_completion(&vi->have_data);
+
 	sg_init_one(&sg, vi->data, sizeof(vi->data));
 
 	/* There should always be room for one buffer. */
@@ -69,6 +68,8 @@ static unsigned int copy_data(struct virtrng_info *vi, void *buf,
 	memcpy(buf, vi->data + vi->data_idx, size);
 	vi->data_idx += size;
 	vi->data_avail -= size;
+	if (vi->data_avail == 0)
+		request_entropy(vi);
 	return size;
 }
 
@@ -98,13 +99,7 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 	 * so either size is 0 or data_avail is 0
 	 */
 	while (size != 0) {
-		/* data_avail is 0 */
-		if (!vi->busy) {
-			/* no pending request, ask for more */
-			vi->busy = true;
-			reinit_completion(&vi->have_data);
-			register_buffer(vi);
-		}
+		/* data_avail is 0 but a request is pending */
 		ret = wait_for_completion_killable(&vi->have_data);
 		if (ret < 0)
 			return ret;
@@ -126,8 +121,7 @@ static void virtio_cleanup(struct hwrng *rng)
 {
 	struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
 
-	if (vi->busy)
-		complete(&vi->have_data);
+	complete(&vi->have_data);
 }
 
 static int probe_common(struct virtio_device *vdev)
@@ -163,6 +157,9 @@ static int probe_common(struct virtio_device *vdev)
 		goto err_find;
 	}
 
+	/* we always have a pending entropy request */
+	request_entropy(vi);
+
 	return 0;
 
 err_find:
@@ -181,7 +178,6 @@ static void remove_common(struct virtio_device *vdev)
 	vi->data_idx = 0;
 	complete(&vi->have_data);
 	vdev->config->reset(vdev);
-	vi->busy = false;
 	if (vi->hwrng_register_done)
 		hwrng_unregister(&vi->hwrng);
 	vdev->config->del_vqs(vdev);
-- 
2.31.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/4] hwrng: virtio - add an internal buffer

[Alexander Potapenko]

On Wed, Sep 22, 2021 at 7:09 PM Laurent Vivier <lvivier@redhat.com> wrote:
>
> hwrng core uses two buffers that can be mixed in the virtio-rng queue.
>
> This series fixes the problem by adding an internal buffer in virtio-rng.
>
> Once the internal buffer is added, we can fix two other problems:
>
> - to be able to release the driver without waiting the device releases the
>   buffer
>
> - actually returns some data when wait=0 as we can have some already
>   available data
>
> It also tries to improve the performance by always having a buffer in
> the queue of the device.

Tested-by: Alexander Potapenko <glider@google.com>
for the series

With these four patches applied, KMSAN stops reporting boot-time
errors in _mix_pool_bytes reported here:
https://www.spinics.net/lists/linux-virtualization/msg46310.html

> Laurent Vivier (4):
>   hwrng: virtio - add an internal buffer
>   hwrng: virtio - don't wait on cleanup
>   hwrng: virtio - don't waste entropy
>   hwrng: virtio - always add a pending request
>
>  drivers/char/hw_random/virtio-rng.c | 84 +++++++++++++++++++++--------
>  1 file changed, 63 insertions(+), 21 deletions(-)
>
> --
> 2.31.1
>
>


-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Re: [PATCH 0/4] hwrng: virtio - add an internal buffer

[Alexander Potapenko via Virtualization]

On Wed, Sep 22, 2021 at 7:09 PM Laurent Vivier <lvivier@redhat.com> wrote:
>
> hwrng core uses two buffers that can be mixed in the virtio-rng queue.
>
> This series fixes the problem by adding an internal buffer in virtio-rng.
>
> Once the internal buffer is added, we can fix two other problems:
>
> - to be able to release the driver without waiting the device releases the
>   buffer
>
> - actually returns some data when wait=0 as we can have some already
>   available data
>
> It also tries to improve the performance by always having a buffer in
> the queue of the device.

Tested-by: Alexander Potapenko <glider@google.com>
for the series

With these four patches applied, KMSAN stops reporting boot-time
errors in _mix_pool_bytes reported here:
https://www.spinics.net/lists/linux-virtualization/msg46310.html

> Laurent Vivier (4):
>   hwrng: virtio - add an internal buffer
>   hwrng: virtio - don't wait on cleanup
>   hwrng: virtio - don't waste entropy
>   hwrng: virtio - always add a pending request
>
>  drivers/char/hw_random/virtio-rng.c | 84 +++++++++++++++++++++--------
>  1 file changed, 63 insertions(+), 21 deletions(-)
>
> --
> 2.31.1
>
>


-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/4] hwrng: virtio - add an internal buffer

[Michael S. Tsirkin]

On Wed, Sep 22, 2021 at 07:09:00PM +0200, Laurent Vivier wrote:
> hwrng core uses two buffers that can be mixed in the
> virtio-rng queue.
> 
> If the buffer is provided with wait=0 it is enqueued in the
> virtio-rng queue but unused by the caller.
> On the next call, core provides another buffer but the
> first one is filled instead and the new one queued.
> And the caller reads the data from the new one that is not
> updated, and the data in the first one are lost.
> 
> To avoid this mix, virtio-rng needs to use its own unique
> internal buffer at a cost of a data copy to the caller buffer.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  drivers/char/hw_random/virtio-rng.c | 43 ++++++++++++++++++++++-------
>  1 file changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> index a90001e02bf7..208c547dcac1 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
> @@ -18,13 +18,20 @@ static DEFINE_IDA(rng_index_ida);
>  struct virtrng_info {
>  	struct hwrng hwrng;
>  	struct virtqueue *vq;
> -	struct completion have_data;
>  	char name[25];
> -	unsigned int data_avail;
>  	int index;
>  	bool busy;
>  	bool hwrng_register_done;
>  	bool hwrng_removed;
> +	/* data transfer */
> +	struct completion have_data;
> +	unsigned int data_avail;
> +	/* minimal size returned by rng_buffer_size() */
> +#if SMP_CACHE_BYTES < 32
> +	u8 data[32];
> +#else
> +	u8 data[SMP_CACHE_BYTES];
> +#endif

Let's move this logic to a macro in hw_random.h ?

>  };
>  
>  static void random_recv_done(struct virtqueue *vq)
> @@ -39,14 +46,14 @@ static void random_recv_done(struct virtqueue *vq)
>  }
>  
>  /* The host will fill any buffer we give it with sweet, sweet randomness. */
> -static void register_buffer(struct virtrng_info *vi, u8 *buf, size_t size)
> +static void register_buffer(struct virtrng_info *vi)
>  {
>  	struct scatterlist sg;
>  
> -	sg_init_one(&sg, buf, size);
> +	sg_init_one(&sg, vi->data, sizeof(vi->data));

Note that add_early_randomness requests less:
        size_t size = min_t(size_t, 16, rng_buffer_size());

maybe track how much was requested and grow up to sizeof(data)?

>  
>  	/* There should always be room for one buffer. */
> -	virtqueue_add_inbuf(vi->vq, &sg, 1, buf, GFP_KERNEL);
> +	virtqueue_add_inbuf(vi->vq, &sg, 1, vi->data, GFP_KERNEL);


BTW no longer true if DMA API is in use ... not easy to fix,
I think some changes to virtio API to allow pre-mapping
s/g for DMA might be needed ...

>  
>  	virtqueue_kick(vi->vq);
>  }
> @@ -55,6 +62,8 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
>  {
>  	int ret;
>  	struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
> +	unsigned int chunk;
> +	size_t read;
>  
>  	if (vi->hwrng_removed)
>  		return -ENODEV;
> @@ -62,19 +71,33 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
>  	if (!vi->busy) {
>  		vi->busy = true;
>  		reinit_completion(&vi->have_data);
> -		register_buffer(vi, buf, size);
> +		register_buffer(vi);
>  	}
>  
>  	if (!wait)
>  		return 0;
>  
> -	ret = wait_for_completion_killable(&vi->have_data);
> -	if (ret < 0)
> -		return ret;
> +	read = 0;
> +	while (size != 0) {
> +		ret = wait_for_completion_killable(&vi->have_data);
> +		if (ret < 0)
> +			return ret;
> +
> +		chunk = min_t(unsigned int, size, vi->data_avail);
> +		memcpy(buf + read, vi->data, chunk);
> +		read += chunk;
> +		size -= chunk;
> +		vi->data_avail = 0;
> +
> +		if (size != 0) {
> +			reinit_completion(&vi->have_data);
> +			register_buffer(vi);
> +		}
> +	}
>  
>  	vi->busy = false;
>  
> -	return vi->data_avail;
> +	return read;
>  }
>  
>  static void virtio_cleanup(struct hwrng *rng)
> -- 
> 2.31.1

Re: [PATCH 1/4] hwrng: virtio - add an internal buffer

[Michael S. Tsirkin]

On Wed, Sep 22, 2021 at 07:09:00PM +0200, Laurent Vivier wrote:
> hwrng core uses two buffers that can be mixed in the
> virtio-rng queue.
> 
> If the buffer is provided with wait=0 it is enqueued in the
> virtio-rng queue but unused by the caller.
> On the next call, core provides another buffer but the
> first one is filled instead and the new one queued.
> And the caller reads the data from the new one that is not
> updated, and the data in the first one are lost.
> 
> To avoid this mix, virtio-rng needs to use its own unique
> internal buffer at a cost of a data copy to the caller buffer.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  drivers/char/hw_random/virtio-rng.c | 43 ++++++++++++++++++++++-------
>  1 file changed, 33 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> index a90001e02bf7..208c547dcac1 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
> @@ -18,13 +18,20 @@ static DEFINE_IDA(rng_index_ida);
>  struct virtrng_info {
>  	struct hwrng hwrng;
>  	struct virtqueue *vq;
> -	struct completion have_data;
>  	char name[25];
> -	unsigned int data_avail;
>  	int index;
>  	bool busy;
>  	bool hwrng_register_done;
>  	bool hwrng_removed;
> +	/* data transfer */
> +	struct completion have_data;
> +	unsigned int data_avail;
> +	/* minimal size returned by rng_buffer_size() */
> +#if SMP_CACHE_BYTES < 32
> +	u8 data[32];
> +#else
> +	u8 data[SMP_CACHE_BYTES];
> +#endif

Let's move this logic to a macro in hw_random.h ?

>  };
>  
>  static void random_recv_done(struct virtqueue *vq)
> @@ -39,14 +46,14 @@ static void random_recv_done(struct virtqueue *vq)
>  }
>  
>  /* The host will fill any buffer we give it with sweet, sweet randomness. */
> -static void register_buffer(struct virtrng_info *vi, u8 *buf, size_t size)
> +static void register_buffer(struct virtrng_info *vi)
>  {
>  	struct scatterlist sg;
>  
> -	sg_init_one(&sg, buf, size);
> +	sg_init_one(&sg, vi->data, sizeof(vi->data));

Note that add_early_randomness requests less:
        size_t size = min_t(size_t, 16, rng_buffer_size());

maybe track how much was requested and grow up to sizeof(data)?

>  
>  	/* There should always be room for one buffer. */
> -	virtqueue_add_inbuf(vi->vq, &sg, 1, buf, GFP_KERNEL);
> +	virtqueue_add_inbuf(vi->vq, &sg, 1, vi->data, GFP_KERNEL);


BTW no longer true if DMA API is in use ... not easy to fix,
I think some changes to virtio API to allow pre-mapping
s/g for DMA might be needed ...

>  
>  	virtqueue_kick(vi->vq);
>  }
> @@ -55,6 +62,8 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
>  {
>  	int ret;
>  	struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
> +	unsigned int chunk;
> +	size_t read;
>  
>  	if (vi->hwrng_removed)
>  		return -ENODEV;
> @@ -62,19 +71,33 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
>  	if (!vi->busy) {
>  		vi->busy = true;
>  		reinit_completion(&vi->have_data);
> -		register_buffer(vi, buf, size);
> +		register_buffer(vi);
>  	}
>  
>  	if (!wait)
>  		return 0;
>  
> -	ret = wait_for_completion_killable(&vi->have_data);
> -	if (ret < 0)
> -		return ret;
> +	read = 0;
> +	while (size != 0) {
> +		ret = wait_for_completion_killable(&vi->have_data);
> +		if (ret < 0)
> +			return ret;
> +
> +		chunk = min_t(unsigned int, size, vi->data_avail);
> +		memcpy(buf + read, vi->data, chunk);
> +		read += chunk;
> +		size -= chunk;
> +		vi->data_avail = 0;
> +
> +		if (size != 0) {
> +			reinit_completion(&vi->have_data);
> +			register_buffer(vi);
> +		}
> +	}
>  
>  	vi->busy = false;
>  
> -	return vi->data_avail;
> +	return read;
>  }
>  
>  static void virtio_cleanup(struct hwrng *rng)
> -- 
> 2.31.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/4] hwrng: virtio - add an internal buffer

[Laurent Vivier]

On 22/09/2021 21:02, Michael S. Tsirkin wrote:
> On Wed, Sep 22, 2021 at 07:09:00PM +0200, Laurent Vivier wrote:
>> hwrng core uses two buffers that can be mixed in the
>> virtio-rng queue.
>>
>> If the buffer is provided with wait=0 it is enqueued in the
>> virtio-rng queue but unused by the caller.
>> On the next call, core provides another buffer but the
>> first one is filled instead and the new one queued.
>> And the caller reads the data from the new one that is not
>> updated, and the data in the first one are lost.
>>
>> To avoid this mix, virtio-rng needs to use its own unique
>> internal buffer at a cost of a data copy to the caller buffer.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>   drivers/char/hw_random/virtio-rng.c | 43 ++++++++++++++++++++++-------
>>   1 file changed, 33 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
>> index a90001e02bf7..208c547dcac1 100644
>> --- a/drivers/char/hw_random/virtio-rng.c
>> +++ b/drivers/char/hw_random/virtio-rng.c
>> @@ -18,13 +18,20 @@ static DEFINE_IDA(rng_index_ida);
>>   struct virtrng_info {
>>   	struct hwrng hwrng;
>>   	struct virtqueue *vq;
>> -	struct completion have_data;
>>   	char name[25];
>> -	unsigned int data_avail;
>>   	int index;
>>   	bool busy;
>>   	bool hwrng_register_done;
>>   	bool hwrng_removed;
>> +	/* data transfer */
>> +	struct completion have_data;
>> +	unsigned int data_avail;
>> +	/* minimal size returned by rng_buffer_size() */
>> +#if SMP_CACHE_BYTES < 32
>> +	u8 data[32];
>> +#else
>> +	u8 data[SMP_CACHE_BYTES];
>> +#endif
> 
> Let's move this logic to a macro in hw_random.h ?
> 
>>   };
>>   
>>   static void random_recv_done(struct virtqueue *vq)
>> @@ -39,14 +46,14 @@ static void random_recv_done(struct virtqueue *vq)
>>   }
>>   
>>   /* The host will fill any buffer we give it with sweet, sweet randomness. */
>> -static void register_buffer(struct virtrng_info *vi, u8 *buf, size_t size)
>> +static void register_buffer(struct virtrng_info *vi)
>>   {
>>   	struct scatterlist sg;
>>   
>> -	sg_init_one(&sg, buf, size);
>> +	sg_init_one(&sg, vi->data, sizeof(vi->data));
> 
> Note that add_early_randomness requests less:
>          size_t size = min_t(size_t, 16, rng_buffer_size());
> 
> maybe track how much was requested and grow up to sizeof(data)?

I think this problem is managed by PATCH 3/4 as we reuse unused data of the buffer.

> 
>>   
>>   	/* There should always be room for one buffer. */
>> -	virtqueue_add_inbuf(vi->vq, &sg, 1, buf, GFP_KERNEL);
>> +	virtqueue_add_inbuf(vi->vq, &sg, 1, vi->data, GFP_KERNEL);
> 
> 
> BTW no longer true if DMA API is in use ... not easy to fix,
> I think some changes to virtio API to allow pre-mapping
> s/g for DMA might be needed ...

Is there something I can do here?

Thanks,
Laurent

Re: [PATCH 1/4] hwrng: virtio - add an internal buffer

[Laurent Vivier]

On 22/09/2021 21:02, Michael S. Tsirkin wrote:
> On Wed, Sep 22, 2021 at 07:09:00PM +0200, Laurent Vivier wrote:
>> hwrng core uses two buffers that can be mixed in the
>> virtio-rng queue.
>>
>> If the buffer is provided with wait=0 it is enqueued in the
>> virtio-rng queue but unused by the caller.
>> On the next call, core provides another buffer but the
>> first one is filled instead and the new one queued.
>> And the caller reads the data from the new one that is not
>> updated, and the data in the first one are lost.
>>
>> To avoid this mix, virtio-rng needs to use its own unique
>> internal buffer at a cost of a data copy to the caller buffer.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>   drivers/char/hw_random/virtio-rng.c | 43 ++++++++++++++++++++++-------
>>   1 file changed, 33 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
>> index a90001e02bf7..208c547dcac1 100644
>> --- a/drivers/char/hw_random/virtio-rng.c
>> +++ b/drivers/char/hw_random/virtio-rng.c
>> @@ -18,13 +18,20 @@ static DEFINE_IDA(rng_index_ida);
>>   struct virtrng_info {
>>   	struct hwrng hwrng;
>>   	struct virtqueue *vq;
>> -	struct completion have_data;
>>   	char name[25];
>> -	unsigned int data_avail;
>>   	int index;
>>   	bool busy;
>>   	bool hwrng_register_done;
>>   	bool hwrng_removed;
>> +	/* data transfer */
>> +	struct completion have_data;
>> +	unsigned int data_avail;
>> +	/* minimal size returned by rng_buffer_size() */
>> +#if SMP_CACHE_BYTES < 32
>> +	u8 data[32];
>> +#else
>> +	u8 data[SMP_CACHE_BYTES];
>> +#endif
> 
> Let's move this logic to a macro in hw_random.h ?
> 
>>   };
>>   
>>   static void random_recv_done(struct virtqueue *vq)
>> @@ -39,14 +46,14 @@ static void random_recv_done(struct virtqueue *vq)
>>   }
>>   
>>   /* The host will fill any buffer we give it with sweet, sweet randomness. */
>> -static void register_buffer(struct virtrng_info *vi, u8 *buf, size_t size)
>> +static void register_buffer(struct virtrng_info *vi)
>>   {
>>   	struct scatterlist sg;
>>   
>> -	sg_init_one(&sg, buf, size);
>> +	sg_init_one(&sg, vi->data, sizeof(vi->data));
> 
> Note that add_early_randomness requests less:
>          size_t size = min_t(size_t, 16, rng_buffer_size());
> 
> maybe track how much was requested and grow up to sizeof(data)?

I think this problem is managed by PATCH 3/4 as we reuse unused data of the buffer.

> 
>>   
>>   	/* There should always be room for one buffer. */
>> -	virtqueue_add_inbuf(vi->vq, &sg, 1, buf, GFP_KERNEL);
>> +	virtqueue_add_inbuf(vi->vq, &sg, 1, vi->data, GFP_KERNEL);
> 
> 
> BTW no longer true if DMA API is in use ... not easy to fix,
> I think some changes to virtio API to allow pre-mapping
> s/g for DMA might be needed ...

Is there something I can do here?

Thanks,
Laurent

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/4] hwrng: virtio - add an internal buffer

[Michael S. Tsirkin]

On Thu, Sep 23, 2021 at 08:26:06AM +0200, Laurent Vivier wrote:
> On 22/09/2021 21:02, Michael S. Tsirkin wrote:
> > On Wed, Sep 22, 2021 at 07:09:00PM +0200, Laurent Vivier wrote:
> > > hwrng core uses two buffers that can be mixed in the
> > > virtio-rng queue.
> > > 
> > > If the buffer is provided with wait=0 it is enqueued in the
> > > virtio-rng queue but unused by the caller.
> > > On the next call, core provides another buffer but the
> > > first one is filled instead and the new one queued.
> > > And the caller reads the data from the new one that is not
> > > updated, and the data in the first one are lost.
> > > 
> > > To avoid this mix, virtio-rng needs to use its own unique
> > > internal buffer at a cost of a data copy to the caller buffer.
> > > 
> > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > ---
> > >   drivers/char/hw_random/virtio-rng.c | 43 ++++++++++++++++++++++-------
> > >   1 file changed, 33 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> > > index a90001e02bf7..208c547dcac1 100644
> > > --- a/drivers/char/hw_random/virtio-rng.c
> > > +++ b/drivers/char/hw_random/virtio-rng.c
> > > @@ -18,13 +18,20 @@ static DEFINE_IDA(rng_index_ida);
> > >   struct virtrng_info {
> > >   	struct hwrng hwrng;
> > >   	struct virtqueue *vq;
> > > -	struct completion have_data;
> > >   	char name[25];
> > > -	unsigned int data_avail;
> > >   	int index;
> > >   	bool busy;
> > >   	bool hwrng_register_done;
> > >   	bool hwrng_removed;
> > > +	/* data transfer */
> > > +	struct completion have_data;
> > > +	unsigned int data_avail;
> > > +	/* minimal size returned by rng_buffer_size() */
> > > +#if SMP_CACHE_BYTES < 32
> > > +	u8 data[32];
> > > +#else
> > > +	u8 data[SMP_CACHE_BYTES];
> > > +#endif
> > 
> > Let's move this logic to a macro in hw_random.h ?
> > 
> > >   };
> > >   static void random_recv_done(struct virtqueue *vq)
> > > @@ -39,14 +46,14 @@ static void random_recv_done(struct virtqueue *vq)
> > >   }
> > >   /* The host will fill any buffer we give it with sweet, sweet randomness. */
> > > -static void register_buffer(struct virtrng_info *vi, u8 *buf, size_t size)
> > > +static void register_buffer(struct virtrng_info *vi)
> > >   {
> > >   	struct scatterlist sg;
> > > -	sg_init_one(&sg, buf, size);
> > > +	sg_init_one(&sg, vi->data, sizeof(vi->data));
> > 
> > Note that add_early_randomness requests less:
> >          size_t size = min_t(size_t, 16, rng_buffer_size());
> > 
> > maybe track how much was requested and grow up to sizeof(data)?
> 
> I think this problem is managed by PATCH 3/4 as we reuse unused data of the buffer.

the issue I'm pointing out is that we are requesting too much
entropy from host - more than guest needs.

> > 
> > >   	/* There should always be room for one buffer. */
> > > -	virtqueue_add_inbuf(vi->vq, &sg, 1, buf, GFP_KERNEL);
> > > +	virtqueue_add_inbuf(vi->vq, &sg, 1, vi->data, GFP_KERNEL);
> > 
> > 
> > BTW no longer true if DMA API is in use ... not easy to fix,
> > I think some changes to virtio API to allow pre-mapping
> > s/g for DMA might be needed ...
> 
> Is there something I can do here?
> 
> Thanks,
> Laurent

We can let it be for now, but down the road I think we should
support a way to pre-map memory for DMA.

-- 
MST

Re: [PATCH 1/4] hwrng: virtio - add an internal buffer

[Michael S. Tsirkin]

On Thu, Sep 23, 2021 at 08:26:06AM +0200, Laurent Vivier wrote:
> On 22/09/2021 21:02, Michael S. Tsirkin wrote:
> > On Wed, Sep 22, 2021 at 07:09:00PM +0200, Laurent Vivier wrote:
> > > hwrng core uses two buffers that can be mixed in the
> > > virtio-rng queue.
> > > 
> > > If the buffer is provided with wait=0 it is enqueued in the
> > > virtio-rng queue but unused by the caller.
> > > On the next call, core provides another buffer but the
> > > first one is filled instead and the new one queued.
> > > And the caller reads the data from the new one that is not
> > > updated, and the data in the first one are lost.
> > > 
> > > To avoid this mix, virtio-rng needs to use its own unique
> > > internal buffer at a cost of a data copy to the caller buffer.
> > > 
> > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > ---
> > >   drivers/char/hw_random/virtio-rng.c | 43 ++++++++++++++++++++++-------
> > >   1 file changed, 33 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> > > index a90001e02bf7..208c547dcac1 100644
> > > --- a/drivers/char/hw_random/virtio-rng.c
> > > +++ b/drivers/char/hw_random/virtio-rng.c
> > > @@ -18,13 +18,20 @@ static DEFINE_IDA(rng_index_ida);
> > >   struct virtrng_info {
> > >   	struct hwrng hwrng;
> > >   	struct virtqueue *vq;
> > > -	struct completion have_data;
> > >   	char name[25];
> > > -	unsigned int data_avail;
> > >   	int index;
> > >   	bool busy;
> > >   	bool hwrng_register_done;
> > >   	bool hwrng_removed;
> > > +	/* data transfer */
> > > +	struct completion have_data;
> > > +	unsigned int data_avail;
> > > +	/* minimal size returned by rng_buffer_size() */
> > > +#if SMP_CACHE_BYTES < 32
> > > +	u8 data[32];
> > > +#else
> > > +	u8 data[SMP_CACHE_BYTES];
> > > +#endif
> > 
> > Let's move this logic to a macro in hw_random.h ?
> > 
> > >   };
> > >   static void random_recv_done(struct virtqueue *vq)
> > > @@ -39,14 +46,14 @@ static void random_recv_done(struct virtqueue *vq)
> > >   }
> > >   /* The host will fill any buffer we give it with sweet, sweet randomness. */
> > > -static void register_buffer(struct virtrng_info *vi, u8 *buf, size_t size)
> > > +static void register_buffer(struct virtrng_info *vi)
> > >   {
> > >   	struct scatterlist sg;
> > > -	sg_init_one(&sg, buf, size);
> > > +	sg_init_one(&sg, vi->data, sizeof(vi->data));
> > 
> > Note that add_early_randomness requests less:
> >          size_t size = min_t(size_t, 16, rng_buffer_size());
> > 
> > maybe track how much was requested and grow up to sizeof(data)?
> 
> I think this problem is managed by PATCH 3/4 as we reuse unused data of the buffer.

the issue I'm pointing out is that we are requesting too much
entropy from host - more than guest needs.

> > 
> > >   	/* There should always be room for one buffer. */
> > > -	virtqueue_add_inbuf(vi->vq, &sg, 1, buf, GFP_KERNEL);
> > > +	virtqueue_add_inbuf(vi->vq, &sg, 1, vi->data, GFP_KERNEL);
> > 
> > 
> > BTW no longer true if DMA API is in use ... not easy to fix,
> > I think some changes to virtio API to allow pre-mapping
> > s/g for DMA might be needed ...
> 
> Is there something I can do here?
> 
> Thanks,
> Laurent

We can let it be for now, but down the road I think we should
support a way to pre-map memory for DMA.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/4] hwrng: virtio - add an internal buffer

[Laurent Vivier]

On 23/09/2021 09:04, Michael S. Tsirkin wrote:
> On Thu, Sep 23, 2021 at 08:26:06AM +0200, Laurent Vivier wrote:
>> On 22/09/2021 21:02, Michael S. Tsirkin wrote:
>>> On Wed, Sep 22, 2021 at 07:09:00PM +0200, Laurent Vivier wrote:
>>>> hwrng core uses two buffers that can be mixed in the
>>>> virtio-rng queue.
>>>>
>>>> If the buffer is provided with wait=0 it is enqueued in the
>>>> virtio-rng queue but unused by the caller.
>>>> On the next call, core provides another buffer but the
>>>> first one is filled instead and the new one queued.
>>>> And the caller reads the data from the new one that is not
>>>> updated, and the data in the first one are lost.
>>>>
>>>> To avoid this mix, virtio-rng needs to use its own unique
>>>> internal buffer at a cost of a data copy to the caller buffer.
>>>>
>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>> ---
>>>>    drivers/char/hw_random/virtio-rng.c | 43 ++++++++++++++++++++++-------
>>>>    1 file changed, 33 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
>>>> index a90001e02bf7..208c547dcac1 100644
>>>> --- a/drivers/char/hw_random/virtio-rng.c
>>>> +++ b/drivers/char/hw_random/virtio-rng.c
>>>> @@ -18,13 +18,20 @@ static DEFINE_IDA(rng_index_ida);
>>>>    struct virtrng_info {
>>>>    	struct hwrng hwrng;
>>>>    	struct virtqueue *vq;
>>>> -	struct completion have_data;
>>>>    	char name[25];
>>>> -	unsigned int data_avail;
>>>>    	int index;
>>>>    	bool busy;
>>>>    	bool hwrng_register_done;
>>>>    	bool hwrng_removed;
>>>> +	/* data transfer */
>>>> +	struct completion have_data;
>>>> +	unsigned int data_avail;
>>>> +	/* minimal size returned by rng_buffer_size() */
>>>> +#if SMP_CACHE_BYTES < 32
>>>> +	u8 data[32];
>>>> +#else
>>>> +	u8 data[SMP_CACHE_BYTES];
>>>> +#endif
>>>
>>> Let's move this logic to a macro in hw_random.h ?
>>>
>>>>    };
>>>>    static void random_recv_done(struct virtqueue *vq)
>>>> @@ -39,14 +46,14 @@ static void random_recv_done(struct virtqueue *vq)
>>>>    }
>>>>    /* The host will fill any buffer we give it with sweet, sweet randomness. */
>>>> -static void register_buffer(struct virtrng_info *vi, u8 *buf, size_t size)
>>>> +static void register_buffer(struct virtrng_info *vi)
>>>>    {
>>>>    	struct scatterlist sg;
>>>> -	sg_init_one(&sg, buf, size);
>>>> +	sg_init_one(&sg, vi->data, sizeof(vi->data));
>>>
>>> Note that add_early_randomness requests less:
>>>           size_t size = min_t(size_t, 16, rng_buffer_size());
>>>
>>> maybe track how much was requested and grow up to sizeof(data)?
>>
>> I think this problem is managed by PATCH 3/4 as we reuse unused data of the buffer.
> 
> the issue I'm pointing out is that we are requesting too much
> entropy from host - more than guest needs.

Yes, guest asks for 16 bytes, but we request SMP_CACHE_BYTES (64 on x86_64), and these 16 
bytes are used with add_device_randomness(). With the following patches, the remaining 48 
bytes are used rapidly by hwgnd kthread or by the next virtio_read.

If there is no enough entropy the call is simply ignored as wait=0.

At this patch level the call is always simply ignored (because wait=0) and the data 
requested here are used by the next read that always asks for a SMP_CACHE_BYTES bytes data 
size.

Moreover in PATCH 4/4 we always have a pending request of size SMP_CACHE_BYTES, so driver 
always asks a block of this size and the guest takes what it needs.

Originally I used a 16 bytes block but performance are divided by 4.

Do you propose something else?

Thanks,
Laurent

Re: [PATCH 1/4] hwrng: virtio - add an internal buffer

[Laurent Vivier]

On 23/09/2021 09:04, Michael S. Tsirkin wrote:
> On Thu, Sep 23, 2021 at 08:26:06AM +0200, Laurent Vivier wrote:
>> On 22/09/2021 21:02, Michael S. Tsirkin wrote:
>>> On Wed, Sep 22, 2021 at 07:09:00PM +0200, Laurent Vivier wrote:
>>>> hwrng core uses two buffers that can be mixed in the
>>>> virtio-rng queue.
>>>>
>>>> If the buffer is provided with wait=0 it is enqueued in the
>>>> virtio-rng queue but unused by the caller.
>>>> On the next call, core provides another buffer but the
>>>> first one is filled instead and the new one queued.
>>>> And the caller reads the data from the new one that is not
>>>> updated, and the data in the first one are lost.
>>>>
>>>> To avoid this mix, virtio-rng needs to use its own unique
>>>> internal buffer at a cost of a data copy to the caller buffer.
>>>>
>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>> ---
>>>>    drivers/char/hw_random/virtio-rng.c | 43 ++++++++++++++++++++++-------
>>>>    1 file changed, 33 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
>>>> index a90001e02bf7..208c547dcac1 100644
>>>> --- a/drivers/char/hw_random/virtio-rng.c
>>>> +++ b/drivers/char/hw_random/virtio-rng.c
>>>> @@ -18,13 +18,20 @@ static DEFINE_IDA(rng_index_ida);
>>>>    struct virtrng_info {
>>>>    	struct hwrng hwrng;
>>>>    	struct virtqueue *vq;
>>>> -	struct completion have_data;
>>>>    	char name[25];
>>>> -	unsigned int data_avail;
>>>>    	int index;
>>>>    	bool busy;
>>>>    	bool hwrng_register_done;
>>>>    	bool hwrng_removed;
>>>> +	/* data transfer */
>>>> +	struct completion have_data;
>>>> +	unsigned int data_avail;
>>>> +	/* minimal size returned by rng_buffer_size() */
>>>> +#if SMP_CACHE_BYTES < 32
>>>> +	u8 data[32];
>>>> +#else
>>>> +	u8 data[SMP_CACHE_BYTES];
>>>> +#endif
>>>
>>> Let's move this logic to a macro in hw_random.h ?
>>>
>>>>    };
>>>>    static void random_recv_done(struct virtqueue *vq)
>>>> @@ -39,14 +46,14 @@ static void random_recv_done(struct virtqueue *vq)
>>>>    }
>>>>    /* The host will fill any buffer we give it with sweet, sweet randomness. */
>>>> -static void register_buffer(struct virtrng_info *vi, u8 *buf, size_t size)
>>>> +static void register_buffer(struct virtrng_info *vi)
>>>>    {
>>>>    	struct scatterlist sg;
>>>> -	sg_init_one(&sg, buf, size);
>>>> +	sg_init_one(&sg, vi->data, sizeof(vi->data));
>>>
>>> Note that add_early_randomness requests less:
>>>           size_t size = min_t(size_t, 16, rng_buffer_size());
>>>
>>> maybe track how much was requested and grow up to sizeof(data)?
>>
>> I think this problem is managed by PATCH 3/4 as we reuse unused data of the buffer.
> 
> the issue I'm pointing out is that we are requesting too much
> entropy from host - more than guest needs.

Yes, guest asks for 16 bytes, but we request SMP_CACHE_BYTES (64 on x86_64), and these 16 
bytes are used with add_device_randomness(). With the following patches, the remaining 48 
bytes are used rapidly by hwgnd kthread or by the next virtio_read.

If there is no enough entropy the call is simply ignored as wait=0.

At this patch level the call is always simply ignored (because wait=0) and the data 
requested here are used by the next read that always asks for a SMP_CACHE_BYTES bytes data 
size.

Moreover in PATCH 4/4 we always have a pending request of size SMP_CACHE_BYTES, so driver 
always asks a block of this size and the guest takes what it needs.

Originally I used a 16 bytes block but performance are divided by 4.

Do you propose something else?

Thanks,
Laurent

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/4] hwrng: virtio - add an internal buffer

[Laurent Vivier]

On 22/09/2021 19:08, Laurent Vivier wrote:
> hwrng core uses two buffers that can be mixed in the virtio-rng queue.
> 
> This series fixes the problem by adding an internal buffer in virtio-rng.
> 
> Once the internal buffer is added, we can fix two other problems:
> 
> - to be able to release the driver without waiting the device releases the
>    buffer
> 
> - actually returns some data when wait=0 as we can have some already
>    available data
> 
> It also tries to improve the performance by always having a buffer in
> the queue of the device.
> 
> Laurent Vivier (4):
>    hwrng: virtio - add an internal buffer
>    hwrng: virtio - don't wait on cleanup
>    hwrng: virtio - don't waste entropy
>    hwrng: virtio - always add a pending request
> 
>   drivers/char/hw_random/virtio-rng.c | 84 +++++++++++++++++++++--------
>   1 file changed, 63 insertions(+), 21 deletions(-)
> 

Any comment?

Do we need a v2?

Thanks,
Laurent

Re: [PATCH 0/4] hwrng: virtio - add an internal buffer

[Laurent Vivier]

On 22/09/2021 19:08, Laurent Vivier wrote:
> hwrng core uses two buffers that can be mixed in the virtio-rng queue.
> 
> This series fixes the problem by adding an internal buffer in virtio-rng.
> 
> Once the internal buffer is added, we can fix two other problems:
> 
> - to be able to release the driver without waiting the device releases the
>    buffer
> 
> - actually returns some data when wait=0 as we can have some already
>    available data
> 
> It also tries to improve the performance by always having a buffer in
> the queue of the device.
> 
> Laurent Vivier (4):
>    hwrng: virtio - add an internal buffer
>    hwrng: virtio - don't wait on cleanup
>    hwrng: virtio - don't waste entropy
>    hwrng: virtio - always add a pending request
> 
>   drivers/char/hw_random/virtio-rng.c | 84 +++++++++++++++++++++--------
>   1 file changed, 63 insertions(+), 21 deletions(-)
> 

Any comment?

Do we need a v2?

Thanks,
Laurent

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/4] hwrng: virtio - add an internal buffer

[Michael S. Tsirkin]

On Thu, Sep 23, 2021 at 09:34:18AM +0200, Laurent Vivier wrote:
> On 23/09/2021 09:04, Michael S. Tsirkin wrote:
> > On Thu, Sep 23, 2021 at 08:26:06AM +0200, Laurent Vivier wrote:
> > > On 22/09/2021 21:02, Michael S. Tsirkin wrote:
> > > > On Wed, Sep 22, 2021 at 07:09:00PM +0200, Laurent Vivier wrote:
> > > > > hwrng core uses two buffers that can be mixed in the
> > > > > virtio-rng queue.
> > > > > 
> > > > > If the buffer is provided with wait=0 it is enqueued in the
> > > > > virtio-rng queue but unused by the caller.
> > > > > On the next call, core provides another buffer but the
> > > > > first one is filled instead and the new one queued.
> > > > > And the caller reads the data from the new one that is not
> > > > > updated, and the data in the first one are lost.
> > > > > 
> > > > > To avoid this mix, virtio-rng needs to use its own unique
> > > > > internal buffer at a cost of a data copy to the caller buffer.
> > > > > 
> > > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > > > ---
> > > > >    drivers/char/hw_random/virtio-rng.c | 43 ++++++++++++++++++++++-------
> > > > >    1 file changed, 33 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> > > > > index a90001e02bf7..208c547dcac1 100644
> > > > > --- a/drivers/char/hw_random/virtio-rng.c
> > > > > +++ b/drivers/char/hw_random/virtio-rng.c
> > > > > @@ -18,13 +18,20 @@ static DEFINE_IDA(rng_index_ida);
> > > > >    struct virtrng_info {
> > > > >    	struct hwrng hwrng;
> > > > >    	struct virtqueue *vq;
> > > > > -	struct completion have_data;
> > > > >    	char name[25];
> > > > > -	unsigned int data_avail;
> > > > >    	int index;
> > > > >    	bool busy;
> > > > >    	bool hwrng_register_done;
> > > > >    	bool hwrng_removed;
> > > > > +	/* data transfer */
> > > > > +	struct completion have_data;
> > > > > +	unsigned int data_avail;
> > > > > +	/* minimal size returned by rng_buffer_size() */
> > > > > +#if SMP_CACHE_BYTES < 32
> > > > > +	u8 data[32];
> > > > > +#else
> > > > > +	u8 data[SMP_CACHE_BYTES];
> > > > > +#endif
> > > > 
> > > > Let's move this logic to a macro in hw_random.h ?
> > > > 
> > > > >    };
> > > > >    static void random_recv_done(struct virtqueue *vq)
> > > > > @@ -39,14 +46,14 @@ static void random_recv_done(struct virtqueue *vq)
> > > > >    }
> > > > >    /* The host will fill any buffer we give it with sweet, sweet randomness. */
> > > > > -static void register_buffer(struct virtrng_info *vi, u8 *buf, size_t size)
> > > > > +static void register_buffer(struct virtrng_info *vi)
> > > > >    {
> > > > >    	struct scatterlist sg;
> > > > > -	sg_init_one(&sg, buf, size);
> > > > > +	sg_init_one(&sg, vi->data, sizeof(vi->data));
> > > > 
> > > > Note that add_early_randomness requests less:
> > > >           size_t size = min_t(size_t, 16, rng_buffer_size());
> > > > 
> > > > maybe track how much was requested and grow up to sizeof(data)?
> > > 
> > > I think this problem is managed by PATCH 3/4 as we reuse unused data of the buffer.
> > 
> > the issue I'm pointing out is that we are requesting too much
> > entropy from host - more than guest needs.
> 
> Yes, guest asks for 16 bytes, but we request SMP_CACHE_BYTES (64 on x86_64),
> and these 16 bytes are used with add_device_randomness(). With the following
> patches, the remaining 48 bytes are used rapidly by hwgnd kthread or by the
> next virtio_read.
> 
> If there is no enough entropy the call is simply ignored as wait=0.
> 
> At this patch level the call is always simply ignored (because wait=0) and
> the data requested here are used by the next read that always asks for a
> SMP_CACHE_BYTES bytes data size.
> 
> Moreover in PATCH 4/4 we always have a pending request of size
> SMP_CACHE_BYTES, so driver always asks a block of this size and the guest
> takes what it needs.
> 
> Originally I used a 16 bytes block but performance are divided by 4.
> 
> Do you propose something else?
> 
> Thanks,
> Laurent

Maybe min(size, sizeof(vi->data))?

-- 
MST

Re: [PATCH 1/4] hwrng: virtio - add an internal buffer

[Michael S. Tsirkin]

On Thu, Sep 23, 2021 at 09:34:18AM +0200, Laurent Vivier wrote:
> On 23/09/2021 09:04, Michael S. Tsirkin wrote:
> > On Thu, Sep 23, 2021 at 08:26:06AM +0200, Laurent Vivier wrote:
> > > On 22/09/2021 21:02, Michael S. Tsirkin wrote:
> > > > On Wed, Sep 22, 2021 at 07:09:00PM +0200, Laurent Vivier wrote:
> > > > > hwrng core uses two buffers that can be mixed in the
> > > > > virtio-rng queue.
> > > > > 
> > > > > If the buffer is provided with wait=0 it is enqueued in the
> > > > > virtio-rng queue but unused by the caller.
> > > > > On the next call, core provides another buffer but the
> > > > > first one is filled instead and the new one queued.
> > > > > And the caller reads the data from the new one that is not
> > > > > updated, and the data in the first one are lost.
> > > > > 
> > > > > To avoid this mix, virtio-rng needs to use its own unique
> > > > > internal buffer at a cost of a data copy to the caller buffer.
> > > > > 
> > > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> > > > > ---
> > > > >    drivers/char/hw_random/virtio-rng.c | 43 ++++++++++++++++++++++-------
> > > > >    1 file changed, 33 insertions(+), 10 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> > > > > index a90001e02bf7..208c547dcac1 100644
> > > > > --- a/drivers/char/hw_random/virtio-rng.c
> > > > > +++ b/drivers/char/hw_random/virtio-rng.c
> > > > > @@ -18,13 +18,20 @@ static DEFINE_IDA(rng_index_ida);
> > > > >    struct virtrng_info {
> > > > >    	struct hwrng hwrng;
> > > > >    	struct virtqueue *vq;
> > > > > -	struct completion have_data;
> > > > >    	char name[25];
> > > > > -	unsigned int data_avail;
> > > > >    	int index;
> > > > >    	bool busy;
> > > > >    	bool hwrng_register_done;
> > > > >    	bool hwrng_removed;
> > > > > +	/* data transfer */
> > > > > +	struct completion have_data;
> > > > > +	unsigned int data_avail;
> > > > > +	/* minimal size returned by rng_buffer_size() */
> > > > > +#if SMP_CACHE_BYTES < 32
> > > > > +	u8 data[32];
> > > > > +#else
> > > > > +	u8 data[SMP_CACHE_BYTES];
> > > > > +#endif
> > > > 
> > > > Let's move this logic to a macro in hw_random.h ?
> > > > 
> > > > >    };
> > > > >    static void random_recv_done(struct virtqueue *vq)
> > > > > @@ -39,14 +46,14 @@ static void random_recv_done(struct virtqueue *vq)
> > > > >    }
> > > > >    /* The host will fill any buffer we give it with sweet, sweet randomness. */
> > > > > -static void register_buffer(struct virtrng_info *vi, u8 *buf, size_t size)
> > > > > +static void register_buffer(struct virtrng_info *vi)
> > > > >    {
> > > > >    	struct scatterlist sg;
> > > > > -	sg_init_one(&sg, buf, size);
> > > > > +	sg_init_one(&sg, vi->data, sizeof(vi->data));
> > > > 
> > > > Note that add_early_randomness requests less:
> > > >           size_t size = min_t(size_t, 16, rng_buffer_size());
> > > > 
> > > > maybe track how much was requested and grow up to sizeof(data)?
> > > 
> > > I think this problem is managed by PATCH 3/4 as we reuse unused data of the buffer.
> > 
> > the issue I'm pointing out is that we are requesting too much
> > entropy from host - more than guest needs.
> 
> Yes, guest asks for 16 bytes, but we request SMP_CACHE_BYTES (64 on x86_64),
> and these 16 bytes are used with add_device_randomness(). With the following
> patches, the remaining 48 bytes are used rapidly by hwgnd kthread or by the
> next virtio_read.
> 
> If there is no enough entropy the call is simply ignored as wait=0.
> 
> At this patch level the call is always simply ignored (because wait=0) and
> the data requested here are used by the next read that always asks for a
> SMP_CACHE_BYTES bytes data size.
> 
> Moreover in PATCH 4/4 we always have a pending request of size
> SMP_CACHE_BYTES, so driver always asks a block of this size and the guest
> takes what it needs.
> 
> Originally I used a 16 bytes block but performance are divided by 4.
> 
> Do you propose something else?
> 
> Thanks,
> Laurent

Maybe min(size, sizeof(vi->data))?

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/4] hwrng: virtio - add an internal buffer

[Laurent Vivier]

On 05/10/2021 13:55, Michael S. Tsirkin wrote:
> On Thu, Sep 23, 2021 at 09:34:18AM +0200, Laurent Vivier wrote:
>> On 23/09/2021 09:04, Michael S. Tsirkin wrote:
>>> On Thu, Sep 23, 2021 at 08:26:06AM +0200, Laurent Vivier wrote:
>>>> On 22/09/2021 21:02, Michael S. Tsirkin wrote:
>>>>> On Wed, Sep 22, 2021 at 07:09:00PM +0200, Laurent Vivier wrote:
>>>>>> hwrng core uses two buffers that can be mixed in the
>>>>>> virtio-rng queue.
>>>>>>
>>>>>> If the buffer is provided with wait=0 it is enqueued in the
>>>>>> virtio-rng queue but unused by the caller.
>>>>>> On the next call, core provides another buffer but the
>>>>>> first one is filled instead and the new one queued.
>>>>>> And the caller reads the data from the new one that is not
>>>>>> updated, and the data in the first one are lost.
>>>>>>
>>>>>> To avoid this mix, virtio-rng needs to use its own unique
>>>>>> internal buffer at a cost of a data copy to the caller buffer.
>>>>>>
>>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>>>> ---
>>>>>>     drivers/char/hw_random/virtio-rng.c | 43 ++++++++++++++++++++++-------
>>>>>>     1 file changed, 33 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
>>>>>> index a90001e02bf7..208c547dcac1 100644
>>>>>> --- a/drivers/char/hw_random/virtio-rng.c
>>>>>> +++ b/drivers/char/hw_random/virtio-rng.c
>>>>>> @@ -18,13 +18,20 @@ static DEFINE_IDA(rng_index_ida);
>>>>>>     struct virtrng_info {
>>>>>>     	struct hwrng hwrng;
>>>>>>     	struct virtqueue *vq;
>>>>>> -	struct completion have_data;
>>>>>>     	char name[25];
>>>>>> -	unsigned int data_avail;
>>>>>>     	int index;
>>>>>>     	bool busy;
>>>>>>     	bool hwrng_register_done;
>>>>>>     	bool hwrng_removed;
>>>>>> +	/* data transfer */
>>>>>> +	struct completion have_data;
>>>>>> +	unsigned int data_avail;
>>>>>> +	/* minimal size returned by rng_buffer_size() */
>>>>>> +#if SMP_CACHE_BYTES < 32
>>>>>> +	u8 data[32];
>>>>>> +#else
>>>>>> +	u8 data[SMP_CACHE_BYTES];
>>>>>> +#endif
>>>>>
>>>>> Let's move this logic to a macro in hw_random.h ?
>>>>>
>>>>>>     };
>>>>>>     static void random_recv_done(struct virtqueue *vq)
>>>>>> @@ -39,14 +46,14 @@ static void random_recv_done(struct virtqueue *vq)
>>>>>>     }
>>>>>>     /* The host will fill any buffer we give it with sweet, sweet randomness. */
>>>>>> -static void register_buffer(struct virtrng_info *vi, u8 *buf, size_t size)
>>>>>> +static void register_buffer(struct virtrng_info *vi)
>>>>>>     {
>>>>>>     	struct scatterlist sg;
>>>>>> -	sg_init_one(&sg, buf, size);
>>>>>> +	sg_init_one(&sg, vi->data, sizeof(vi->data));
>>>>>
>>>>> Note that add_early_randomness requests less:
>>>>>            size_t size = min_t(size_t, 16, rng_buffer_size());
>>>>>
>>>>> maybe track how much was requested and grow up to sizeof(data)?
>>>>
>>>> I think this problem is managed by PATCH 3/4 as we reuse unused data of the buffer.
>>>
>>> the issue I'm pointing out is that we are requesting too much
>>> entropy from host - more than guest needs.
>>
>> Yes, guest asks for 16 bytes, but we request SMP_CACHE_BYTES (64 on x86_64),
>> and these 16 bytes are used with add_device_randomness(). With the following
>> patches, the remaining 48 bytes are used rapidly by hwgnd kthread or by the
>> next virtio_read.
>>
>> If there is no enough entropy the call is simply ignored as wait=0.
>>
>> At this patch level the call is always simply ignored (because wait=0) and
>> the data requested here are used by the next read that always asks for a
>> SMP_CACHE_BYTES bytes data size.
>>
>> Moreover in PATCH 4/4 we always have a pending request of size
>> SMP_CACHE_BYTES, so driver always asks a block of this size and the guest
>> takes what it needs.
>>
>> Originally I used a 16 bytes block but performance are divided by 4.
>>
>> Do you propose something else?
>>
>> Thanks,
>> Laurent
> 
> Maybe min(size, sizeof(vi->data))?
> 
But it means, in the case of mixed buffers, we will ask 16 bytes on the first call, not 
use it, and  ask SMP_CACHE_BYTES bytes on the next call to get only 16:

- add_early_randomness() asks for 16 bytes but wait = 0 and thus the request is queued but 
not used. add_early_randomness() is called when we switch from one hw_random backend to 
another (so generally only once...)

- hwrng_fillfn() and rng_dev_read() always ask rng_buffer_size() (max(32, SMP_CACHE_BYTES)).

So we can say we use SMP_CACHE_BYTES in 99% of the cases.

Moreover, this will be discarded by patch 3 and 4 as we have a loop to ask more data in a 
fixed size buffer.

I'm not sure it's worth introducing this change in this patch.

Thanks,
Laurent

Re: [PATCH 1/4] hwrng: virtio - add an internal buffer

[Laurent Vivier]

On 05/10/2021 13:55, Michael S. Tsirkin wrote:
> On Thu, Sep 23, 2021 at 09:34:18AM +0200, Laurent Vivier wrote:
>> On 23/09/2021 09:04, Michael S. Tsirkin wrote:
>>> On Thu, Sep 23, 2021 at 08:26:06AM +0200, Laurent Vivier wrote:
>>>> On 22/09/2021 21:02, Michael S. Tsirkin wrote:
>>>>> On Wed, Sep 22, 2021 at 07:09:00PM +0200, Laurent Vivier wrote:
>>>>>> hwrng core uses two buffers that can be mixed in the
>>>>>> virtio-rng queue.
>>>>>>
>>>>>> If the buffer is provided with wait=0 it is enqueued in the
>>>>>> virtio-rng queue but unused by the caller.
>>>>>> On the next call, core provides another buffer but the
>>>>>> first one is filled instead and the new one queued.
>>>>>> And the caller reads the data from the new one that is not
>>>>>> updated, and the data in the first one are lost.
>>>>>>
>>>>>> To avoid this mix, virtio-rng needs to use its own unique
>>>>>> internal buffer at a cost of a data copy to the caller buffer.
>>>>>>
>>>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>>>> ---
>>>>>>     drivers/char/hw_random/virtio-rng.c | 43 ++++++++++++++++++++++-------
>>>>>>     1 file changed, 33 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
>>>>>> index a90001e02bf7..208c547dcac1 100644
>>>>>> --- a/drivers/char/hw_random/virtio-rng.c
>>>>>> +++ b/drivers/char/hw_random/virtio-rng.c
>>>>>> @@ -18,13 +18,20 @@ static DEFINE_IDA(rng_index_ida);
>>>>>>     struct virtrng_info {
>>>>>>     	struct hwrng hwrng;
>>>>>>     	struct virtqueue *vq;
>>>>>> -	struct completion have_data;
>>>>>>     	char name[25];
>>>>>> -	unsigned int data_avail;
>>>>>>     	int index;
>>>>>>     	bool busy;
>>>>>>     	bool hwrng_register_done;
>>>>>>     	bool hwrng_removed;
>>>>>> +	/* data transfer */
>>>>>> +	struct completion have_data;
>>>>>> +	unsigned int data_avail;
>>>>>> +	/* minimal size returned by rng_buffer_size() */
>>>>>> +#if SMP_CACHE_BYTES < 32
>>>>>> +	u8 data[32];
>>>>>> +#else
>>>>>> +	u8 data[SMP_CACHE_BYTES];
>>>>>> +#endif
>>>>>
>>>>> Let's move this logic to a macro in hw_random.h ?
>>>>>
>>>>>>     };
>>>>>>     static void random_recv_done(struct virtqueue *vq)
>>>>>> @@ -39,14 +46,14 @@ static void random_recv_done(struct virtqueue *vq)
>>>>>>     }
>>>>>>     /* The host will fill any buffer we give it with sweet, sweet randomness. */
>>>>>> -static void register_buffer(struct virtrng_info *vi, u8 *buf, size_t size)
>>>>>> +static void register_buffer(struct virtrng_info *vi)
>>>>>>     {
>>>>>>     	struct scatterlist sg;
>>>>>> -	sg_init_one(&sg, buf, size);
>>>>>> +	sg_init_one(&sg, vi->data, sizeof(vi->data));
>>>>>
>>>>> Note that add_early_randomness requests less:
>>>>>            size_t size = min_t(size_t, 16, rng_buffer_size());
>>>>>
>>>>> maybe track how much was requested and grow up to sizeof(data)?
>>>>
>>>> I think this problem is managed by PATCH 3/4 as we reuse unused data of the buffer.
>>>
>>> the issue I'm pointing out is that we are requesting too much
>>> entropy from host - more than guest needs.
>>
>> Yes, guest asks for 16 bytes, but we request SMP_CACHE_BYTES (64 on x86_64),
>> and these 16 bytes are used with add_device_randomness(). With the following
>> patches, the remaining 48 bytes are used rapidly by hwgnd kthread or by the
>> next virtio_read.
>>
>> If there is no enough entropy the call is simply ignored as wait=0.
>>
>> At this patch level the call is always simply ignored (because wait=0) and
>> the data requested here are used by the next read that always asks for a
>> SMP_CACHE_BYTES bytes data size.
>>
>> Moreover in PATCH 4/4 we always have a pending request of size
>> SMP_CACHE_BYTES, so driver always asks a block of this size and the guest
>> takes what it needs.
>>
>> Originally I used a 16 bytes block but performance are divided by 4.
>>
>> Do you propose something else?
>>
>> Thanks,
>> Laurent
> 
> Maybe min(size, sizeof(vi->data))?
> 
But it means, in the case of mixed buffers, we will ask 16 bytes on the first call, not 
use it, and  ask SMP_CACHE_BYTES bytes on the next call to get only 16:

- add_early_randomness() asks for 16 bytes but wait = 0 and thus the request is queued but 
not used. add_early_randomness() is called when we switch from one hw_random backend to 
another (so generally only once...)

- hwrng_fillfn() and rng_dev_read() always ask rng_buffer_size() (max(32, SMP_CACHE_BYTES)).

So we can say we use SMP_CACHE_BYTES in 99% of the cases.

Moreover, this will be discarded by patch 3 and 4 as we have a loop to ask more data in a 
fixed size buffer.

I'm not sure it's worth introducing this change in this patch.

Thanks,
Laurent

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization