Re: [Qemu-devel] [RFC] Add NRF51 RNG peripheral

[Stefan Hajnoczi]

On Tue, Jun 19, 2018 at 11:54 AM, Steffen Görtz
<address@hidden> wrote:
> Add a model of the NRF51 RNG peripheral.
>
> Signed-off-by: Steffen Görtz <address@hidden>
> ---
>  hw/misc/Makefile.objs       |   1 +
>  hw/misc/nrf51_rng.c         | 241 ++++++++++++++++++++++++++++++++++++
>  include/hw/misc/nrf51_rng.h |  61 +++++++++
>  3 files changed, 303 insertions(+)
>  create mode 100644 hw/misc/nrf51_rng.c
>  create mode 100644 include/hw/misc/nrf51_rng.h
>
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 00e834d0f0..fd8cc97249 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -70,3 +70,4 @@ obj-$(CONFIG_AUX) += auxbus.o
>  obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o aspeed_sdmc.o
>  obj-y += mmio_interface.o
>  obj-$(CONFIG_MSF2) += msf2-sysreg.o
> +obj-$(CONFIG_NRF51_SOC) += nrf51_rng.o
> diff --git a/hw/misc/nrf51_rng.c b/hw/misc/nrf51_rng.c
> new file mode 100644
> index 0000000000..6bf1e3efc9
> --- /dev/null
> +++ b/hw/misc/nrf51_rng.c
> @@ -0,0 +1,241 @@
> +/*
> + * nrf51_rng.c
> + *
> + * Copyright 2018 Steffen Görtz <address@hidden>
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.

Please include a reference to the datasheet for this device and
mention the full name "Random Number Generator" so the purpose of this
device is clear.

> +static void rng_write(void *opaque, hwaddr offset,
> +                       uint64_t value, unsigned int size)
> +{
> +    Nrf51RNGState *s = NRF51_RNG(opaque);
> +
> +    assert(size == 4);

The guest must not be able to crash QEMU via small/large store instructions.

Have you looked at the access size constraints in MemoryRegionOps?

This device must behave the way real hardware does.  What is supposed
to happen if the guest uses a STRB or STRH instruction?

The same applies to rng_read().

> +static void nrf51_rng_timer_expire(void *opaque) {
> +    Nrf51RNGState *s = NRF51_RNG(opaque);
> +
> +    qcrypto_random_bytes(&s->value, 1, NULL);
> +
> +    s->state.event_valrdy = 1;
> +    qemu_set_irq(s->eep_valrdy, 1);
> +
> +    if(s->state.interrupt_enabled) {
> +        qemu_irq_pulse(s->irq);
> +    }

Why does eep_valrdy use qemu_set_irq() while s->irq uses
qemu_irq_pulse()?  I don't see a many other qemu_irq_pulse() instances
in hw/arm/ so I'm curious why it was necessary.

> diff --git a/include/hw/misc/nrf51_rng.h b/include/hw/misc/nrf51_rng.h
> new file mode 100644
> index 0000000000..cec7d5f92c
> --- /dev/null
> +++ b/include/hw/misc/nrf51_rng.h
> @@ -0,0 +1,61 @@
> +/*
> + * nrf51_rng.h
> + *
> + * Copyright 2018 Steffen Görtz <address@hidden>
> + *
> + * This code is licensed under the GPL version 2 or later.  See
> + * the COPYING file in the top-level directory.
> + *
> + * * QEMU interface:
> + * + Property "time_between_values": Time between two biased values in
> + *   microseconds.

This is outdated.  The code uses period_unfiltered_us and period_filtered_us.