Re: [Qemu-devel] [RFC v3] arm: Add NRF51 random number generator peripheral

[Peter Maydell]

On 13 July 2018 at 16:40, Steffen Görtz <address@hidden> wrote:
> Add a model of the NRF51 random number generator peripheral.
>
> Signed-off-by: Steffen Görtz <address@hidden>
> ---
> Changes since v3:
>   - Replace bitfields
>   - Add VMState / reset
>   - Add reference to reference manual
>
> Changes since v2:
>   - Add missing 'qapi/error.h' for error_abort
>
> Changes since v1:
>   - Add implementation access size hints to MemoryRegionOps
>   - Fail on error if qcrypto_random_bytes fails
>   - Add references to Nrf51 datasheets
>
>
>  hw/misc/Makefile.objs       |   1 +
>  hw/misc/nrf51_rng.c         | 269 ++++++++++++++++++++++++++++++++++++
>  include/hw/misc/nrf51_rng.h |  71 ++++++++++
>  3 files changed, 341 insertions(+)
>  create mode 100644 hw/misc/nrf51_rng.c
>  create mode 100644 include/hw/misc/nrf51_rng.h
>

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

Is this definitely a pulse interrupt ? Usually
devices raise an irq line and keep it high until
the guest checks them and tells them to drop it.

> +    }
> +
> +    if (s->shortcut_stop_on_valrdy) {
> +        s->active = 0;
> +    }
> +
> +    rng_update_timer(s);
> +}
> +
> +static void nrf51_rng_tep_start(void *opaque, int n, int level)
> +{
> +    Nrf51RNGState *s = NRF51_RNG(opaque);
> +
> +    if (level) {
> +        s->active = 1;
> +        rng_update_timer(s);
> +    }
> +}
> +
> +static void nrf51_rng_tep_stop(void *opaque, int n, int level)
> +{
> +    Nrf51RNGState *s = NRF51_RNG(opaque);
> +
> +    if (level) {
> +        s->active = 0;
> +        rng_update_timer(s);
> +    }
> +}
> +
> +
> +static void nrf51_rng_init(Object *obj)
> +{
> +    Nrf51RNGState *s = NRF51_RNG(obj);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +
> +    memory_region_init_io(&s->mmio, obj, &rng_ops, s,
> +            TYPE_NRF51_RNG, NRF51_RNG_SIZE);
> +    sysbus_init_mmio(sbd, &s->mmio);
> +
> +    timer_init_us(&s->timer, QEMU_CLOCK_VIRTUAL, nrf51_rng_timer_expire, s);
> +
> +    qdev_init_gpio_out_named(DEVICE(s), &s->irq, "irq", 1);
> +
> +    /* Tasks */
> +    qdev_init_gpio_in_named(DEVICE(s), nrf51_rng_tep_start, "tep_start", 1);
> +    qdev_init_gpio_in_named(DEVICE(s), nrf51_rng_tep_stop, "tep_stop", 1);
> +
> +    /* Events */
> +    qdev_init_gpio_out_named(DEVICE(s), &s->eep_valrdy, "eep_valrdy", 1);
> +}
> +
> +static void nrf51_rng_reset(DeviceState *dev)
> +{
> +    Nrf51RNGState *s = NRF51_RNG(dev);
> +
> +    rng_update_timer(s);

Isn't this reset function missing some code to reset the
various bits of guest-modifiable state (eg s->active,
s->event_valrdy) ?

> +}

thanks
-- PMM