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

[Heinrich Schuchardt]

On 12/27/19 12:29 PM, Sughosh Ganu wrote:

On Fri, 27 Dec 2019 at 12:42, Heinrich Schuchardt <xypron.g...@gmx.de
<mailto:xypron.g...@gmx.de>> wrote:

    On 12/26/19 6:25 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.g...@linaro.org
    <mailto:sughosh.g...@linaro.org>>
     > ---
     > Changes since V4:
     > * Return number of bytes read on a successful read, and a -ve
    value on
     >    error.
     >
     >   drivers/virtio/Kconfig         |  6 +++
     >   drivers/virtio/Makefile        |  1 +
     >   drivers/virtio/virtio-uclass.c |  1 +
     >   drivers/virtio/virtio_rng.c    | 85
    ++++++++++++++++++++++++++++++++++++++++++
     >   include/virtio.h               |  4 +-
     >   5 files changed, 96 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..fda8d04
     > --- /dev/null
     > +++ b/drivers/virtio/virtio_rng.c
     > @@ -0,0 +1,85 @@
     > +// 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>
     > +
     > +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;
     > +     struct virtio_sg sg;
     > +     struct virtio_sg *sgs[1];
     > +     struct virtio_rng_priv *priv = dev_get_priv(dev);
     > +
     > +     /*
     > +      * Only INT_MAX number of bytes can be returned. If more
     > +      * bytes need to be read, the caller must do it in a loop.
     > +      */
     > +     if (len > INT_MAX)
     > +             len = INT_MAX;
     > +
     > +     sg.addr = data;
     > +     sg.length = len;
     > +     sgs[0] = &sg;
     > +
     > +     ret = virtqueue_add(priv->rng_vq, sgs, 0, 1);
     > +     if (ret)
     > +             return ret;
     > +
     > +     virtqueue_kick(priv->rng_vq);
     > +
     > +     while (!virtqueue_get_buf(priv->rng_vq, &rsize))

    This code assumes that data is 4 byte aligned and may lead to a crash
    due to missing alignment on ARM in function le32_to_cpu().


Are you referring to the virtqueue_get_buf function. Not sure I
understand. In any case, I will be sending an updated version which
moves the loop into the driver, as you had commented on my other patch.

virtqueue_get_buf() calls virtio32_to_cpu() which calls le32_to_cpu()
which assumes that the destination is properly aligned.

Best regards

Heinrich