[PATCH v7 8/8] virtio: rng: Add a random number generator(rng) driver

[Sughosh Ganu]

On Sun, 29 Dec 2019 at 02:59, Heinrich Schuchardt <xypron.glpk at gmx.de>
wrote:

> On 12/28/19 7:28 PM, Sughosh Ganu wrote:
> > Add a driver for the virtio-rng device on the qemu platform. The
> > device uses pci as a transport medium. The driver can be enabled with
> > the following configs
> >
> > CONFIG_VIRTIO
> > CONFIG_DM_RNG
> > CONFIG_VIRTIO_PCI
> > CONFIG_VIRTIO_RNG
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > ---
> > Changes since V6:
> > * Handle review comments from Heinrich Schuchardt to handle the
> >    scenario of data buffer being passed to the virtio rng driver not
> >    being aligned on 4 bytes.
> >
> >   drivers/virtio/Kconfig         |  6 +++
> >   drivers/virtio/Makefile        |  1 +
> >   drivers/virtio/virtio-uclass.c |  1 +
> >   drivers/virtio/virtio_rng.c    | 92
> ++++++++++++++++++++++++++++++++++++++++++
> >   include/virtio.h               |  4 +-
> >   5 files changed, 103 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/virtio/virtio_rng.c
>

<snip>


> > diff --git a/drivers/virtio/virtio_rng.c b/drivers/virtio/virtio_rng.c
> > new file mode 100644
> > index 0000000..ffb6835
> > --- /dev/null
> > +++ b/drivers/virtio/virtio_rng.c
> > @@ -0,0 +1,92 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2019, Linaro Limited
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <rng.h>
> > +#include <virtio_types.h>
> > +#include <virtio.h>
> > +#include <virtio_ring.h>
> > +
> > +#define BUFFER_SIZE  16UL
> > +
> > +struct virtio_rng_priv {
> > +     struct virtqueue *rng_vq;
> > +};
> > +
> > +static int virtio_rng_read(struct udevice *dev, void *data, size_t len)
> > +{
> > +     int ret;
> > +     unsigned int rsize = 0;
> > +     size_t nbytes = len;
> > +     unsigned char buf[BUFFER_SIZE] __aligned(4);
> > +     unsigned char *ptr = data;
> > +     struct virtio_sg sg;
> > +     struct virtio_sg *sgs[1];
> > +     struct virtio_rng_priv *priv = dev_get_priv(dev);
> > +
> > +     if (!len)
> > +             return 0;
> > +
> > +     do {
>
> If you use
>
>         while(nbytes) {
>

Ok.


>
> here you can go without the if above. This will save 4 bytes of code on
> ARMv8. In U-Boot we have tight code size constraints.
>
> > +             sg.addr = buf;
> > +             sg.length = nbytes;
>
> We must avoid buffer overruns for len > 16:
>
> sg.length = min(nbytes, BUFFER_SIZE);
>

Oops. Forgot to take care of this. Will incorporate this in the next
version. Also, I had sent you a unicast a couple of days back asking for
the code in virtqueue_get_buf which requires a 4 byte alignment for the
data pointer. The virtio32_to_cpu calls in virtqueue_get_buf are being used
to reference 'id' and 'len' members of structs. I don't see the 'addr'
member being used in this function. But i do see the data pointer being
used in virtqueue_add function, which actually casts this to a u64 value.
Can you point me to the function which actually dereferences the data
pointer as a u32 pointer. Thanks a lot for your in depth review.

-sughosh