2021-10-28 10:12:39

by Laurent Vivier

[permalink] [raw]
Subject: [PATCH v2 0/4] hwrng: virtio - add an internal buffer

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.



v2:

fixes issue reported by [email protected]

by reseting data_idx to 0 when the buffer is submitted not when it is

received as the consumer checks for data_avail, not only for the completion.



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 | 86 ++++++++++++++++++++++-------

1 file changed, 65 insertions(+), 21 deletions(-)



--

2.31.1





2021-10-28 10:13:39

by Laurent Vivier

[permalink] [raw]
Subject: [PATCH v2 4/4] hwrng: virtio - always add a pending request

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 <[email protected]>
---
drivers/char/hw_random/virtio-rng.c | 26 ++++++++++++--------------
1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index 8ba97cf4ca8f..0a7dde135db1 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,18 @@ 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);
+ vi->data_avail = 0;
+ vi->data_idx = 0;
+
sg_init_one(&sg, vi->data, sizeof(vi->data));

/* There should always be room for one buffer. */
@@ -69,6 +70,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 +101,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 +123,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 +159,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 +180,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

2021-10-28 10:14:05

by Laurent Vivier

[permalink] [raw]
Subject: [PATCH v2 3/4] hwrng: virtio - don't waste entropy

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

Signed-off-by: Laurent Vivier <[email protected]>
---
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

2022-08-02 12:50:56

by Vladimir Murzin

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] hwrng: virtio - always add a pending request

Hi Laurent,

On 10/28/21 11:11, Laurent Vivier wrote:
> 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 <[email protected]>
> ---
> drivers/char/hw_random/virtio-rng.c | 26 ++++++++++++--------------
> 1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> index 8ba97cf4ca8f..0a7dde135db1 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,18 @@ 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);
> + vi->data_avail = 0;
> + vi->data_idx = 0;
> +
> sg_init_one(&sg, vi->data, sizeof(vi->data));
>
> /* There should always be room for one buffer. */
> @@ -69,6 +70,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 +101,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 +123,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 +159,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 +180,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);

We observed that after this commit virtio-rng implementation in FVP doesn't
work

INFO: bp.virtio_rng: Selected Random Generator Device: XORSHIFT DEVICE
INFO: bp.virtio_rng: Using seed value: 0x5674bba8
Error: FVP_Base_AEMvA: bp.virtio_rng: <vq0-requestq> Found invalid descriptor index
In file: (unknown):0
In process: FVP_Base_AEMvA.thread_p_12 @ 935500020 ns
Info: FVP_Base_AEMvA: bp.virtio_rng: Could not extract buffer

while basic baremetal test works as expected

INFO: bp.virtio_rng: Selected Random Generator Device: XORSHIFT DEVICE
INFO: bp.virtio_rng: Using seed value: 0x541c142e
Info: FVP_Base_AEMv8A: bp.virtio_rng: Generated Number: 0x4b098991ceb377e6
Info: FVP_Base_AEMv8A: bp.virtio_rng: Generated Number: 0xbdcbe3f765ba62f7

We are trying to get an idea what is missing and where, yet none of us familiar
with the driver :(

I'm looping Kevin who originally reported that and Mauricio who is looking form
the FVP side.

Cheers
Vladimir

2022-08-03 08:59:38

by Vladimir Murzin

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] hwrng: virtio - always add a pending request

On 8/2/22 13:49, Vladimir Murzin wrote:
> Hi Laurent,
>
> On 10/28/21 11:11, Laurent Vivier wrote:
>> 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 <[email protected]>
>> ---
>> drivers/char/hw_random/virtio-rng.c | 26 ++++++++++++--------------
>> 1 file changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
>> index 8ba97cf4ca8f..0a7dde135db1 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,18 @@ 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);
>> + vi->data_avail = 0;
>> + vi->data_idx = 0;
>> +
>> sg_init_one(&sg, vi->data, sizeof(vi->data));
>>
>> /* There should always be room for one buffer. */
>> @@ -69,6 +70,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 +101,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 +123,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 +159,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 +180,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);
>
> We observed that after this commit virtio-rng implementation in FVP doesn't
> work
>
> INFO: bp.virtio_rng: Selected Random Generator Device: XORSHIFT DEVICE
> INFO: bp.virtio_rng: Using seed value: 0x5674bba8
> Error: FVP_Base_AEMvA: bp.virtio_rng: <vq0-requestq> Found invalid descriptor index
> In file: (unknown):0
> In process: FVP_Base_AEMvA.thread_p_12 @ 935500020 ns
> Info: FVP_Base_AEMvA: bp.virtio_rng: Could not extract buffer
>
> while basic baremetal test works as expected
>
> INFO: bp.virtio_rng: Selected Random Generator Device: XORSHIFT DEVICE
> INFO: bp.virtio_rng: Using seed value: 0x541c142e
> Info: FVP_Base_AEMv8A: bp.virtio_rng: Generated Number: 0x4b098991ceb377e6
> Info: FVP_Base_AEMv8A: bp.virtio_rng: Generated Number: 0xbdcbe3f765ba62f7
>
> We are trying to get an idea what is missing and where, yet none of us familiar
> with the driver :(
>
> I'm looping Kevin who originally reported that and Mauricio who is looking form
> the FVP side.

With the following diff FVP works agin

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index a6f3a8a2ac..042503ad6c 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -54,6 +54,7 @@ static void request_entropy(struct virtrng_info *vi)
reinit_completion(&vi->have_data);
vi->data_avail = 0;
vi->data_idx = 0;
+ smp_mb();

sg_init_one(&sg, vi->data, sizeof(vi->data));


What do you reckon?

Cheers
Vladimir

>
> Cheers
> Vladimir


2022-08-03 11:47:23

by Michael S. Tsirkin

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] hwrng: virtio - always add a pending request

On Wed, Aug 03, 2022 at 09:57:35AM +0100, Vladimir Murzin wrote:
> On 8/2/22 13:49, Vladimir Murzin wrote:
> > Hi Laurent,
> >
> > On 10/28/21 11:11, Laurent Vivier wrote:
> >> 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 <[email protected]>
> >> ---
> >> drivers/char/hw_random/virtio-rng.c | 26 ++++++++++++--------------
> >> 1 file changed, 12 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> >> index 8ba97cf4ca8f..0a7dde135db1 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,18 @@ 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);
> >> + vi->data_avail = 0;
> >> + vi->data_idx = 0;
> >> +
> >> sg_init_one(&sg, vi->data, sizeof(vi->data));
> >>
> >> /* There should always be room for one buffer. */
> >> @@ -69,6 +70,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 +101,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 +123,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 +159,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 +180,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);
> >
> > We observed that after this commit virtio-rng implementation in FVP doesn't
> > work
> >
> > INFO: bp.virtio_rng: Selected Random Generator Device: XORSHIFT DEVICE
> > INFO: bp.virtio_rng: Using seed value: 0x5674bba8
> > Error: FVP_Base_AEMvA: bp.virtio_rng: <vq0-requestq> Found invalid descriptor index
> > In file: (unknown):0
> > In process: FVP_Base_AEMvA.thread_p_12 @ 935500020 ns
> > Info: FVP_Base_AEMvA: bp.virtio_rng: Could not extract buffer
> >
> > while basic baremetal test works as expected
> >
> > INFO: bp.virtio_rng: Selected Random Generator Device: XORSHIFT DEVICE
> > INFO: bp.virtio_rng: Using seed value: 0x541c142e
> > Info: FVP_Base_AEMv8A: bp.virtio_rng: Generated Number: 0x4b098991ceb377e6
> > Info: FVP_Base_AEMv8A: bp.virtio_rng: Generated Number: 0xbdcbe3f765ba62f7
> >
> > We are trying to get an idea what is missing and where, yet none of us familiar
> > with the driver :(
> >
> > I'm looping Kevin who originally reported that and Mauricio who is looking form
> > the FVP side.
>
> With the following diff FVP works agin
>
> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> index a6f3a8a2ac..042503ad6c 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
> @@ -54,6 +54,7 @@ static void request_entropy(struct virtrng_info *vi)
> reinit_completion(&vi->have_data);
> vi->data_avail = 0;
> vi->data_idx = 0;
> + smp_mb();
>
> sg_init_one(&sg, vi->data, sizeof(vi->data));
>
>
> What do you reckon?
>
> Cheers
> Vladimir

Thanks for debugging this!

OK, interesting.

data_idx and data_avail are accessed from virtio_read.

Which as far as I can tell is invoked just with reading_mutex.


But, request_entropy is called from probe when device is registered
this time without locks
so it can trigger while another thread is calling virtio_read.

Second request_entropy is called from a callback random_recv_done
also without locks.

So it's great that smp_mb helped here but I suspect in fact we
need locking. Laurent?

--
MST


2022-08-03 12:26:31

by Vladimir Murzin

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] hwrng: virtio - always add a pending request

On 8/3/22 12:39, Michael S. Tsirkin wrote:
> On Wed, Aug 03, 2022 at 09:57:35AM +0100, Vladimir Murzin wrote:
>> On 8/2/22 13:49, Vladimir Murzin wrote:
>>> Hi Laurent,
>>>
>>> On 10/28/21 11:11, Laurent Vivier wrote:
>>>> 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 <[email protected]>
>>>> ---
>>>> drivers/char/hw_random/virtio-rng.c | 26 ++++++++++++--------------
>>>> 1 file changed, 12 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
>>>> index 8ba97cf4ca8f..0a7dde135db1 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,18 @@ 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);
>>>> + vi->data_avail = 0;
>>>> + vi->data_idx = 0;
>>>> +
>>>> sg_init_one(&sg, vi->data, sizeof(vi->data));
>>>>
>>>> /* There should always be room for one buffer. */
>>>> @@ -69,6 +70,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 +101,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 +123,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 +159,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 +180,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);
>>>
>>> We observed that after this commit virtio-rng implementation in FVP doesn't
>>> work
>>>
>>> INFO: bp.virtio_rng: Selected Random Generator Device: XORSHIFT DEVICE
>>> INFO: bp.virtio_rng: Using seed value: 0x5674bba8
>>> Error: FVP_Base_AEMvA: bp.virtio_rng: <vq0-requestq> Found invalid descriptor index
>>> In file: (unknown):0
>>> In process: FVP_Base_AEMvA.thread_p_12 @ 935500020 ns
>>> Info: FVP_Base_AEMvA: bp.virtio_rng: Could not extract buffer
>>>
>>> while basic baremetal test works as expected
>>>
>>> INFO: bp.virtio_rng: Selected Random Generator Device: XORSHIFT DEVICE
>>> INFO: bp.virtio_rng: Using seed value: 0x541c142e
>>> Info: FVP_Base_AEMv8A: bp.virtio_rng: Generated Number: 0x4b098991ceb377e6
>>> Info: FVP_Base_AEMv8A: bp.virtio_rng: Generated Number: 0xbdcbe3f765ba62f7
>>>
>>> We are trying to get an idea what is missing and where, yet none of us familiar
>>> with the driver :(
>>>
>>> I'm looping Kevin who originally reported that and Mauricio who is looking form
>>> the FVP side.
>>
>> With the following diff FVP works agin
>>
>> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
>> index a6f3a8a2ac..042503ad6c 100644
>> --- a/drivers/char/hw_random/virtio-rng.c
>> +++ b/drivers/char/hw_random/virtio-rng.c
>> @@ -54,6 +54,7 @@ static void request_entropy(struct virtrng_info *vi)
>> reinit_completion(&vi->have_data);
>> vi->data_avail = 0;
>> vi->data_idx = 0;
>> + smp_mb();
>>
>> sg_init_one(&sg, vi->data, sizeof(vi->data));
>>
>>
>> What do you reckon?
>>
>> Cheers
>> Vladimir
>
> Thanks for debugging this!
>
> OK, interesting.
>
> data_idx and data_avail are accessed from virtio_read.
>
> Which as far as I can tell is invoked just with reading_mutex.
>
>
> But, request_entropy is called from probe when device is registered
> this time without locks
> so it can trigger while another thread is calling virtio_read.
>
> Second request_entropy is called from a callback random_recv_done
> also without locks.
>
> So it's great that smp_mb helped here but I suspect in fact we
> need locking. Laurent?
>

I'm sorry for the noise, but it looks like I'm seeing issue for some different reasons.
I manage to reproduce issue even with smb_mb() in place. The reason I though it helped
is because I changed both environment and added smb_mb().

Anyway, thank you for your time!

Cheers
Vladimir



2022-08-03 13:03:42

by Michael S. Tsirkin

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] hwrng: virtio - always add a pending request

On Wed, Aug 03, 2022 at 01:25:15PM +0100, Vladimir Murzin wrote:
> On 8/3/22 12:39, Michael S. Tsirkin wrote:
> > On Wed, Aug 03, 2022 at 09:57:35AM +0100, Vladimir Murzin wrote:
> >> On 8/2/22 13:49, Vladimir Murzin wrote:
> >>> Hi Laurent,
> >>>
> >>> On 10/28/21 11:11, Laurent Vivier wrote:
> >>>> 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 <[email protected]>
> >>>> ---
> >>>> drivers/char/hw_random/virtio-rng.c | 26 ++++++++++++--------------
> >>>> 1 file changed, 12 insertions(+), 14 deletions(-)
> >>>>
> >>>> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> >>>> index 8ba97cf4ca8f..0a7dde135db1 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,18 @@ 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);
> >>>> + vi->data_avail = 0;
> >>>> + vi->data_idx = 0;
> >>>> +
> >>>> sg_init_one(&sg, vi->data, sizeof(vi->data));
> >>>>
> >>>> /* There should always be room for one buffer. */
> >>>> @@ -69,6 +70,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 +101,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 +123,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 +159,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 +180,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);
> >>>
> >>> We observed that after this commit virtio-rng implementation in FVP doesn't
> >>> work
> >>>
> >>> INFO: bp.virtio_rng: Selected Random Generator Device: XORSHIFT DEVICE
> >>> INFO: bp.virtio_rng: Using seed value: 0x5674bba8
> >>> Error: FVP_Base_AEMvA: bp.virtio_rng: <vq0-requestq> Found invalid descriptor index
> >>> In file: (unknown):0
> >>> In process: FVP_Base_AEMvA.thread_p_12 @ 935500020 ns
> >>> Info: FVP_Base_AEMvA: bp.virtio_rng: Could not extract buffer
> >>>
> >>> while basic baremetal test works as expected
> >>>
> >>> INFO: bp.virtio_rng: Selected Random Generator Device: XORSHIFT DEVICE
> >>> INFO: bp.virtio_rng: Using seed value: 0x541c142e
> >>> Info: FVP_Base_AEMv8A: bp.virtio_rng: Generated Number: 0x4b098991ceb377e6
> >>> Info: FVP_Base_AEMv8A: bp.virtio_rng: Generated Number: 0xbdcbe3f765ba62f7
> >>>
> >>> We are trying to get an idea what is missing and where, yet none of us familiar
> >>> with the driver :(
> >>>
> >>> I'm looping Kevin who originally reported that and Mauricio who is looking form
> >>> the FVP side.
> >>
> >> With the following diff FVP works agin
> >>
> >> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> >> index a6f3a8a2ac..042503ad6c 100644
> >> --- a/drivers/char/hw_random/virtio-rng.c
> >> +++ b/drivers/char/hw_random/virtio-rng.c
> >> @@ -54,6 +54,7 @@ static void request_entropy(struct virtrng_info *vi)
> >> reinit_completion(&vi->have_data);
> >> vi->data_avail = 0;
> >> vi->data_idx = 0;
> >> + smp_mb();
> >>
> >> sg_init_one(&sg, vi->data, sizeof(vi->data));
> >>
> >>
> >> What do you reckon?
> >>
> >> Cheers
> >> Vladimir
> >
> > Thanks for debugging this!
> >
> > OK, interesting.
> >
> > data_idx and data_avail are accessed from virtio_read.
> >
> > Which as far as I can tell is invoked just with reading_mutex.
> >
> >
> > But, request_entropy is called from probe when device is registered
> > this time without locks
> > so it can trigger while another thread is calling virtio_read.
> >
> > Second request_entropy is called from a callback random_recv_done
> > also without locks.
> >
> > So it's great that smp_mb helped here but I suspect in fact we
> > need locking. Laurent?
> >
>
> I'm sorry for the noise, but it looks like I'm seeing issue for some different reasons.
> I manage to reproduce issue even with smb_mb() in place. The reason I though it helped
> is because I changed both environment and added smb_mb().
>
> Anyway, thank you for your time!
>
> Cheers
> Vladimir

Well we at least have a race condition found by code review here. Here's
a quick hack attempting to fix it. I don't like it much since
it adds buffers with GFP_ATOMIC and kicks under a spinlock, but
for now we can at least test it. I did a quick build but that's
all, a bit rushed now sorry. Would appreciate knowing whether
this addresses the issue for you.


diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index a6f3a8a2aca6..36121c8d0315 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -23,6 +23,7 @@ struct virtrng_info {
bool hwrng_register_done;
bool hwrng_removed;
/* data transfer */
+ spinlock_t lock;
struct completion have_data;
unsigned int data_avail;
unsigned int data_idx;
@@ -37,6 +38,9 @@ struct virtrng_info {
static void random_recv_done(struct virtqueue *vq)
{
struct virtrng_info *vi = vq->vdev->priv;
+ unsigned long flags;
+
+ spin_lock_irqsave(&vi->lock, flags);

/* We can get spurious callbacks, e.g. shared IRQs + virtio_pci. */
if (!virtqueue_get_buf(vi->vq, &vi->data_avail))
@@ -45,20 +49,20 @@ static void random_recv_done(struct virtqueue *vq)
vi->data_idx = 0;

complete(&vi->have_data);
+ spin_unlock_irqrestore(&vi->lock, flags);
}

static void request_entropy(struct virtrng_info *vi)
{
struct scatterlist sg;

- reinit_completion(&vi->have_data);
- vi->data_avail = 0;
+ BUG_ON(vi->data_avail != 0);
vi->data_idx = 0;

sg_init_one(&sg, vi->data, sizeof(vi->data));

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

virtqueue_kick(vi->vq);
}
@@ -70,8 +74,10 @@ 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)
+ if (vi->data_avail == 0) {
+ reinit_completion(&vi->have_data);
request_entropy(vi);
+ }
return size;
}

@@ -81,18 +87,21 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
unsigned int chunk;
size_t read;
+ unsigned long flags;

if (vi->hwrng_removed)
return -ENODEV;

read = 0;

+ spin_lock_irqsave(&vi->lock, flags);
/* copy available data */
if (vi->data_avail) {
chunk = copy_data(vi, buf, size);
size -= chunk;
read += chunk;
}
+ spin_unlock_irqrestore(&vi->lock, flags);

if (!wait)
return read;
@@ -108,12 +117,14 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
/* if vi->data_avail is 0, we have been interrupted
* by a cleanup, but buffer stays in the queue
*/
+ spin_lock_irqsave(&vi->lock, flags);
if (vi->data_avail == 0)
return read;

chunk = copy_data(vi, buf + read, size);
size -= chunk;
read += chunk;
+ spin_unlock_irqrestore(&vi->lock, flags);
}

return read;
@@ -122,19 +133,25 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
static void virtio_cleanup(struct hwrng *rng)
{
struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
+ unsigned long flags;

+ spin_lock_irqsave(&vi->lock, flags);
complete(&vi->have_data);
+ spin_unlock_irqrestore(&vi->lock, flags);
}

static int probe_common(struct virtio_device *vdev)
{
int err, index;
struct virtrng_info *vi = NULL;
+ unsigned long flags;

vi = kzalloc(sizeof(struct virtrng_info), GFP_KERNEL);
if (!vi)
return -ENOMEM;

+ spin_lock_init(&vi->lock);
+
vi->index = index = ida_simple_get(&rng_index_ida, 0, 0, GFP_KERNEL);
if (index < 0) {
err = index;
@@ -162,7 +179,10 @@ static int probe_common(struct virtio_device *vdev)
virtio_device_ready(vdev);

/* we always have a pending entropy request */
- request_entropy(vi);
+ spin_lock_irqsave(&vi->lock, flags);
+ if (vi->data_avail == 0)
+ request_entropy(vi);
+ spin_unlock_irqrestore(&vi->lock, flags);

return 0;



2022-08-03 13:14:04

by Vladimir Murzin

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] hwrng: virtio - always add a pending request

On 8/3/22 13:55, Michael S. Tsirkin wrote:
> On Wed, Aug 03, 2022 at 01:25:15PM +0100, Vladimir Murzin wrote:
>> On 8/3/22 12:39, Michael S. Tsirkin wrote:
>>> On Wed, Aug 03, 2022 at 09:57:35AM +0100, Vladimir Murzin wrote:
>>>> On 8/2/22 13:49, Vladimir Murzin wrote:
>>>>> Hi Laurent,
>>>>>
>>>>> On 10/28/21 11:11, Laurent Vivier wrote:
>>>>>> 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 <[email protected]>
>>>>>> ---
>>>>>> drivers/char/hw_random/virtio-rng.c | 26 ++++++++++++--------------
>>>>>> 1 file changed, 12 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
>>>>>> index 8ba97cf4ca8f..0a7dde135db1 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,18 @@ 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);
>>>>>> + vi->data_avail = 0;
>>>>>> + vi->data_idx = 0;
>>>>>> +
>>>>>> sg_init_one(&sg, vi->data, sizeof(vi->data));
>>>>>>
>>>>>> /* There should always be room for one buffer. */
>>>>>> @@ -69,6 +70,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 +101,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 +123,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 +159,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 +180,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);
>>>>>
>>>>> We observed that after this commit virtio-rng implementation in FVP doesn't
>>>>> work
>>>>>
>>>>> INFO: bp.virtio_rng: Selected Random Generator Device: XORSHIFT DEVICE
>>>>> INFO: bp.virtio_rng: Using seed value: 0x5674bba8
>>>>> Error: FVP_Base_AEMvA: bp.virtio_rng: <vq0-requestq> Found invalid descriptor index
>>>>> In file: (unknown):0
>>>>> In process: FVP_Base_AEMvA.thread_p_12 @ 935500020 ns
>>>>> Info: FVP_Base_AEMvA: bp.virtio_rng: Could not extract buffer
>>>>>
>>>>> while basic baremetal test works as expected
>>>>>
>>>>> INFO: bp.virtio_rng: Selected Random Generator Device: XORSHIFT DEVICE
>>>>> INFO: bp.virtio_rng: Using seed value: 0x541c142e
>>>>> Info: FVP_Base_AEMv8A: bp.virtio_rng: Generated Number: 0x4b098991ceb377e6
>>>>> Info: FVP_Base_AEMv8A: bp.virtio_rng: Generated Number: 0xbdcbe3f765ba62f7
>>>>>
>>>>> We are trying to get an idea what is missing and where, yet none of us familiar
>>>>> with the driver :(
>>>>>
>>>>> I'm looping Kevin who originally reported that and Mauricio who is looking form
>>>>> the FVP side.
>>>>
>>>> With the following diff FVP works agin
>>>>
>>>> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
>>>> index a6f3a8a2ac..042503ad6c 100644
>>>> --- a/drivers/char/hw_random/virtio-rng.c
>>>> +++ b/drivers/char/hw_random/virtio-rng.c
>>>> @@ -54,6 +54,7 @@ static void request_entropy(struct virtrng_info *vi)
>>>> reinit_completion(&vi->have_data);
>>>> vi->data_avail = 0;
>>>> vi->data_idx = 0;
>>>> + smp_mb();
>>>>
>>>> sg_init_one(&sg, vi->data, sizeof(vi->data));
>>>>
>>>>
>>>> What do you reckon?
>>>>
>>>> Cheers
>>>> Vladimir
>>>
>>> Thanks for debugging this!
>>>
>>> OK, interesting.
>>>
>>> data_idx and data_avail are accessed from virtio_read.
>>>
>>> Which as far as I can tell is invoked just with reading_mutex.
>>>
>>>
>>> But, request_entropy is called from probe when device is registered
>>> this time without locks
>>> so it can trigger while another thread is calling virtio_read.
>>>
>>> Second request_entropy is called from a callback random_recv_done
>>> also without locks.
>>>
>>> So it's great that smp_mb helped here but I suspect in fact we
>>> need locking. Laurent?
>>>
>>
>> I'm sorry for the noise, but it looks like I'm seeing issue for some different reasons.
>> I manage to reproduce issue even with smb_mb() in place. The reason I though it helped
>> is because I changed both environment and added smb_mb().
>>
>> Anyway, thank you for your time!
>>
>> Cheers
>> Vladimir
>
> Well we at least have a race condition found by code review here. Here's
> a quick hack attempting to fix it. I don't like it much since
> it adds buffers with GFP_ATOMIC and kicks under a spinlock, but
> for now we can at least test it. I did a quick build but that's
> all, a bit rushed now sorry. Would appreciate knowing whether
> this addresses the issue for you.
>
>
> diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
> index a6f3a8a2aca6..36121c8d0315 100644
> --- a/drivers/char/hw_random/virtio-rng.c
> +++ b/drivers/char/hw_random/virtio-rng.c
> @@ -23,6 +23,7 @@ struct virtrng_info {
> bool hwrng_register_done;
> bool hwrng_removed;
> /* data transfer */
> + spinlock_t lock;
> struct completion have_data;
> unsigned int data_avail;
> unsigned int data_idx;
> @@ -37,6 +38,9 @@ struct virtrng_info {
> static void random_recv_done(struct virtqueue *vq)
> {
> struct virtrng_info *vi = vq->vdev->priv;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&vi->lock, flags);
>
> /* We can get spurious callbacks, e.g. shared IRQs + virtio_pci. */
> if (!virtqueue_get_buf(vi->vq, &vi->data_avail))
> @@ -45,20 +49,20 @@ static void random_recv_done(struct virtqueue *vq)
> vi->data_idx = 0;
>
> complete(&vi->have_data);
> + spin_unlock_irqrestore(&vi->lock, flags);
> }
>
> static void request_entropy(struct virtrng_info *vi)
> {
> struct scatterlist sg;
>
> - reinit_completion(&vi->have_data);
> - vi->data_avail = 0;
> + BUG_ON(vi->data_avail != 0);
> vi->data_idx = 0;
>
> sg_init_one(&sg, vi->data, sizeof(vi->data));
>
> /* There should always be room for one buffer. */
> - virtqueue_add_inbuf(vi->vq, &sg, 1, vi->data, GFP_KERNEL);
> + virtqueue_add_inbuf(vi->vq, &sg, 1, vi->data, GFP_ATOMIC);
>
> virtqueue_kick(vi->vq);
> }
> @@ -70,8 +74,10 @@ 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)
> + if (vi->data_avail == 0) {
> + reinit_completion(&vi->have_data);
> request_entropy(vi);
> + }
> return size;
> }
>
> @@ -81,18 +87,21 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
> struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
> unsigned int chunk;
> size_t read;
> + unsigned long flags;
>
> if (vi->hwrng_removed)
> return -ENODEV;
>
> read = 0;
>
> + spin_lock_irqsave(&vi->lock, flags);
> /* copy available data */
> if (vi->data_avail) {
> chunk = copy_data(vi, buf, size);
> size -= chunk;
> read += chunk;
> }
> + spin_unlock_irqrestore(&vi->lock, flags);
>
> if (!wait)
> return read;
> @@ -108,12 +117,14 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
> /* if vi->data_avail is 0, we have been interrupted
> * by a cleanup, but buffer stays in the queue
> */
> + spin_lock_irqsave(&vi->lock, flags);
> if (vi->data_avail == 0)
> return read;
>
> chunk = copy_data(vi, buf + read, size);
> size -= chunk;
> read += chunk;
> + spin_unlock_irqrestore(&vi->lock, flags);
> }
>
> return read;
> @@ -122,19 +133,25 @@ static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
> static void virtio_cleanup(struct hwrng *rng)
> {
> struct virtrng_info *vi = (struct virtrng_info *)rng->priv;
> + unsigned long flags;
>
> + spin_lock_irqsave(&vi->lock, flags);
> complete(&vi->have_data);
> + spin_unlock_irqrestore(&vi->lock, flags);
> }
>
> static int probe_common(struct virtio_device *vdev)
> {
> int err, index;
> struct virtrng_info *vi = NULL;
> + unsigned long flags;
>
> vi = kzalloc(sizeof(struct virtrng_info), GFP_KERNEL);
> if (!vi)
> return -ENOMEM;
>
> + spin_lock_init(&vi->lock);
> +
> vi->index = index = ida_simple_get(&rng_index_ida, 0, 0, GFP_KERNEL);
> if (index < 0) {
> err = index;
> @@ -162,7 +179,10 @@ static int probe_common(struct virtio_device *vdev)
> virtio_device_ready(vdev);
>
> /* we always have a pending entropy request */
> - request_entropy(vi);
> + spin_lock_irqsave(&vi->lock, flags);
> + if (vi->data_avail == 0)
> + request_entropy(vi);
> + spin_unlock_irqrestore(&vi->lock, flags);
>
> return 0;
>
>

Thanks a lot! I gave it a go and it did not help. I think I need to find out how exactly
my environment affected... it not necessary need to be kernel related.

Cheers
Vladimir
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.