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

Steffen Görtz posted 1 patch 5 years, 9 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180619105451.29163-1-contrib@steffen-goertz.de
Test checkpatch failed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test s390x failed
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
[Qemu-devel] [RFC] Add NRF51 RNG peripheral
Posted by Steffen Görtz 5 years, 9 months ago
Add a model of the NRF51 RNG peripheral.

Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
---
 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 <contrib@steffen-goertz.de>
+ *
+ * This code is licensed under the GPL version 2 or later.  See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/misc/nrf51_rng.h"
+#include "crypto/random.h"
+
+#define NRF51_RNG_SIZE         0x1000
+
+#define NRF51_RNG_TASK_START   0x000
+#define NRF51_RNG_TASK_STOP    0x004
+#define NRF51_RNG_EVENT_VALRDY 0x100
+#define NRF51_RNG_REG_SHORTS   0x200
+#define NRF51_RNG_REG_SHORTS_VALRDY_STOP 0
+#define NRF51_RNG_REG_INTEN    0x300
+#define NRF51_RNG_REG_INTEN_VALRDY 0
+#define NRF51_RNG_REG_INTENSET 0x304
+#define NRF51_RNG_REG_INTENCLR 0x308
+#define NRF51_RNG_REG_CONFIG   0x504
+#define NRF51_RNG_REG_CONFIG_DECEN 0
+#define NRF51_RNG_REG_VALUE    0x508
+
+#define NRF51_TRIGGER_TASK 0x01
+#define NRF51_EVENT_CLEAR  0x00
+
+
+static uint64_t rng_read(void *opaque, hwaddr offset, unsigned int size)
+{
+    Nrf51RNGState *s = NRF51_RNG(opaque);
+    uint64_t r = 0;
+
+    assert(size == 4);
+
+    switch (offset) {
+    case NRF51_RNG_EVENT_VALRDY:
+        r = s->state.event_valrdy;
+        break;
+    case NRF51_RNG_REG_SHORTS:
+        r = s->state.shortcut_stop_on_valrdy;
+        break;
+    case NRF51_RNG_REG_INTEN:
+    case NRF51_RNG_REG_INTENSET:
+    case NRF51_RNG_REG_INTENCLR:
+        r = s->state.interrupt_enabled;
+        break;
+    case NRF51_RNG_REG_CONFIG:
+        r = s->state.filter_enabled;
+        break;
+    case NRF51_RNG_REG_VALUE:
+        r = s->value;
+        break;
+
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: bad read offset 0x%" HWADDR_PRIx "\n",
+                      __func__, offset);
+    }
+
+    return r;
+}
+
+static int64_t calc_next_timeout(Nrf51RNGState *s) {
+    int64_t timeout = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL);
+    if(s->state.filter_enabled) {
+        timeout += s->period_filtered_us;
+    } else {
+        timeout += s->period_unfiltered_us;
+    }
+
+    return timeout;
+}
+
+
+static void rng_update_timer(Nrf51RNGState *s) {
+    if (s->state.active) {
+        timer_mod(&s->timer, calc_next_timeout(s));
+    } else {
+        timer_del(&s->timer);
+    }
+}
+
+
+static void rng_write(void *opaque, hwaddr offset,
+                       uint64_t value, unsigned int size)
+{
+    Nrf51RNGState *s = NRF51_RNG(opaque);
+
+    assert(size == 4);
+
+    switch (offset) {
+    case NRF51_RNG_TASK_START:
+        if(value == NRF51_TRIGGER_TASK)  {
+            s->state.active = 1;
+            rng_update_timer(s);
+        }
+        break;
+    case NRF51_RNG_TASK_STOP:
+        if(value == NRF51_TRIGGER_TASK)  {
+            s->state.active = 0;
+            rng_update_timer(s);
+        }
+        break;
+    case NRF51_RNG_EVENT_VALRDY:
+        if(value == NRF51_EVENT_CLEAR)  {
+            s->state.event_valrdy = 0;
+            qemu_set_irq(s->eep_valrdy, 0);
+        }
+        break;
+    case NRF51_RNG_REG_SHORTS:
+        s->state.shortcut_stop_on_valrdy =
+                (value & BIT_MASK(NRF51_RNG_REG_SHORTS_VALRDY_STOP)) ? 1 : 0;
+        break;
+    case NRF51_RNG_REG_INTEN:
+        s->state.interrupt_enabled =
+                (value & BIT_MASK(NRF51_RNG_REG_INTEN_VALRDY)) ? 1 : 0;
+        break;
+    case NRF51_RNG_REG_INTENSET:
+        if(value & BIT_MASK(NRF51_RNG_REG_INTEN_VALRDY)) {
+            s->state.interrupt_enabled = 1;
+        }
+        break;
+    case NRF51_RNG_REG_INTENCLR:
+        if(value & BIT_MASK(NRF51_RNG_REG_INTEN_VALRDY)) {
+            s->state.interrupt_enabled = 0;
+        }
+        break;
+    case NRF51_RNG_REG_CONFIG:
+        s->state.filter_enabled =
+                      (value & BIT_MASK(NRF51_RNG_REG_CONFIG_DECEN)) ? 1 : 0;
+        break;
+
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: bad write offset 0x%" HWADDR_PRIx "\n",
+                      __func__, offset);
+    }
+}
+
+static const MemoryRegionOps rng_ops = {
+    .read =  rng_read,
+    .write = rng_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+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);
+    }
+
+    if(s->state.shortcut_stop_on_valrdy) {
+        s->state.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->state.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->state.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 Property nrf51_rng_properties[] = {
+    DEFINE_PROP_UINT16("period_unfiltered_us", Nrf51RNGState,
+            period_unfiltered_us, 167),
+    DEFINE_PROP_UINT16("period_filtered_us", Nrf51RNGState,
+            period_filtered_us, 660),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void nrf51_rng_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->props = nrf51_rng_properties;
+}
+
+static const TypeInfo nrf51_rng_info = {
+    .name = TYPE_NRF51_RNG,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(Nrf51RNGState),
+    .instance_init = nrf51_rng_init,
+    .class_init = nrf51_rng_class_init
+};
+
+static void nrf51_rng_register_types(void)
+{
+    type_register_static(&nrf51_rng_info);
+}
+
+type_init(nrf51_rng_register_types)
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 <contrib@steffen-goertz.de>
+ *
+ * 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.
+ * + sysbus MMIO regions 0: Memory Region with tasks, events and registers
+ *   to be mapped to the peripherals instance address by the SOC.
+ * + Named GPIO output "irq": Interrupt line of the peripheral. Must be
+ *   connected to the associated peripheral interrupt line of the NVIC.
+ * + Named GPIO output "eep_valrdy": Event set when new random value is ready
+ *   to be read.
+ * + Named GPIO input "tep_start": Task that triggers start of continuous
+ *   generation of random values.
+ * + Named GPIO input "tep_stop": Task that ends continuous generation of
+ *   random values.
+ *
+ */
+#ifndef NRF51_RNG_H
+#define NRF51_RNG_H
+
+#include "hw/sysbus.h"
+#include "qemu/timer.h"
+#define TYPE_NRF51_RNG "nrf51_soc.rng"
+#define NRF51_RNG(obj) OBJECT_CHECK(Nrf51RNGState, (obj), TYPE_NRF51_RNG)
+
+typedef struct Nrf51RNGState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion mmio;
+    qemu_irq irq;
+
+    /* Event End Points */
+    qemu_irq eep_valrdy;
+
+    QEMUTimer timer;
+
+    /* Time between generation of successive unfiltered values in us */
+    uint16_t period_unfiltered_us;
+    /* Time between generation of successive filtered values in us */
+    uint16_t period_filtered_us;
+
+    uint8_t value;
+
+    struct {
+        uint32_t active: 1;
+        uint32_t event_valrdy: 1;
+        uint32_t shortcut_stop_on_valrdy: 1;
+        uint32_t interrupt_enabled: 1;
+        uint32_t filter_enabled: 1;
+    } state;
+
+} Nrf51RNGState;
+
+
+#endif /* NRF51_RNG_H_ */
-- 
2.17.1


Re: [Qemu-devel] [RFC] Add NRF51 RNG peripheral
Posted by no-reply@patchew.org 5 years, 9 months ago
Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180619105451.29163-1-contrib@steffen-goertz.de
Subject: [Qemu-devel] [RFC] Add NRF51 RNG peripheral

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/1529346580-25537-1-git-send-email-aleksandar.markovic@rt-rk.com -> patchew/1529346580-25537-1-git-send-email-aleksandar.markovic@rt-rk.com
 t [tag update]            patchew/20180618175958.29073-1-armbru@redhat.com -> patchew/20180618175958.29073-1-armbru@redhat.com
 * [new tag]               patchew/20180619105451.29163-1-contrib@steffen-goertz.de -> patchew/20180619105451.29163-1-contrib@steffen-goertz.de
Switched to a new branch 'test'
2a46903e1e Add NRF51 RNG peripheral

=== OUTPUT BEGIN ===
Checking PATCH 1/1: Add NRF51 RNG peripheral...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#24: 
new file mode 100644

ERROR: open brace '{' following function declarations go on the next line
#97: FILE: hw/misc/nrf51_rng.c:69:
+static int64_t calc_next_timeout(Nrf51RNGState *s) {

ERROR: space required before the open parenthesis '('
#99: FILE: hw/misc/nrf51_rng.c:71:
+    if(s->state.filter_enabled) {

ERROR: open brace '{' following function declarations go on the next line
#109: FILE: hw/misc/nrf51_rng.c:81:
+static void rng_update_timer(Nrf51RNGState *s) {

ERROR: space required before the open parenthesis '('
#127: FILE: hw/misc/nrf51_rng.c:99:
+        if(value == NRF51_TRIGGER_TASK)  {

ERROR: space required before the open parenthesis '('
#133: FILE: hw/misc/nrf51_rng.c:105:
+        if(value == NRF51_TRIGGER_TASK)  {

ERROR: space required before the open parenthesis '('
#139: FILE: hw/misc/nrf51_rng.c:111:
+        if(value == NRF51_EVENT_CLEAR)  {

ERROR: space required before the open parenthesis '('
#153: FILE: hw/misc/nrf51_rng.c:125:
+        if(value & BIT_MASK(NRF51_RNG_REG_INTEN_VALRDY)) {

ERROR: space required before the open parenthesis '('
#158: FILE: hw/misc/nrf51_rng.c:130:
+        if(value & BIT_MASK(NRF51_RNG_REG_INTEN_VALRDY)) {

ERROR: open brace '{' following function declarations go on the next line
#180: FILE: hw/misc/nrf51_rng.c:152:
+static void nrf51_rng_timer_expire(void *opaque) {

ERROR: space required before the open parenthesis '('
#188: FILE: hw/misc/nrf51_rng.c:160:
+    if(s->state.interrupt_enabled) {

ERROR: space required before the open parenthesis '('
#192: FILE: hw/misc/nrf51_rng.c:164:
+    if(s->state.shortcut_stop_on_valrdy) {

ERROR: space required before the open parenthesis '('
#203: FILE: hw/misc/nrf51_rng.c:175:
+    if(level) {

ERROR: suspect code indent for conditional statements (4, 7)
#213: FILE: hw/misc/nrf51_rng.c:185:
+    if(level) {
+       s->state.active = 0;

ERROR: space required before the open parenthesis '('
#213: FILE: hw/misc/nrf51_rng.c:185:
+    if(level) {

ERROR: spaces prohibited around that ':' (ctx:VxW)
#326: FILE: include/hw/misc/nrf51_rng.h:51:
+        uint32_t active: 1;
                        ^

ERROR: spaces prohibited around that ':' (ctx:VxW)
#327: FILE: include/hw/misc/nrf51_rng.h:52:
+        uint32_t event_valrdy: 1;
                              ^

ERROR: spaces prohibited around that ':' (ctx:VxW)
#328: FILE: include/hw/misc/nrf51_rng.h:53:
+        uint32_t shortcut_stop_on_valrdy: 1;
                                         ^

ERROR: spaces prohibited around that ':' (ctx:VxW)
#329: FILE: include/hw/misc/nrf51_rng.h:54:
+        uint32_t interrupt_enabled: 1;
                                   ^

ERROR: spaces prohibited around that ':' (ctx:VxW)
#330: FILE: include/hw/misc/nrf51_rng.h:55:
+        uint32_t filter_enabled: 1;
                                ^

total: 19 errors, 1 warnings, 306 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [Qemu-devel] [RFC] Add NRF51 RNG peripheral
Posted by Daniel P. Berrangé 5 years, 9 months ago
On Tue, Jun 19, 2018 at 06:54:51AM -0400, Steffen Görtz wrote:
> Add a model of the NRF51 RNG peripheral.
> 
> Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
> ---
>  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
> 

> +static void nrf51_rng_timer_expire(void *opaque) {
> +    Nrf51RNGState *s = NRF51_RNG(opaque);
> +
> +    qcrypto_random_bytes(&s->value, 1, NULL);

Passing NULL for the error object and ignoring return value is a
security flaw, because you'll be reporting non-random data to the
guest on failure. If you don't want to handle failure, then best
thing todo is pass  &error_abort so that QEMU terminates if the
RNG fails

> +
> +    s->state.event_valrdy = 1;
> +    qemu_set_irq(s->eep_valrdy, 1);
> +
> +    if(s->state.interrupt_enabled) {
> +        qemu_irq_pulse(s->irq);
> +    }
> +
> +    if(s->state.shortcut_stop_on_valrdy) {
> +        s->state.active = 0;
> +    }
> +
> +    rng_update_timer(s);
> +}

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC] Add NRF51 RNG peripheral
Posted by no-reply@patchew.org 5 years, 9 months ago
Hi,

This series failed build test on s390x host. Please find the details below.

N/A. Internal error while reading log file



---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [Patchew-devel] [Qemu-devel] [RFC] Add NRF51 RNG peripheral
Posted by Philippe Mathieu-Daudé 5 years, 9 months ago
Hi Fam,

On 06/19/2018 10:20 AM, no-reply@patchew.org wrote:
> Hi,
> 
> This series failed build test on s390x host. Please find the details below.
> 
> N/A. Internal error while reading log file

Did you noticed this error?

Maybe this kind of error should only be sent to patchew-devel@redhat.com

> 
> 
> 
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@redhat.com
> 

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel
Re: [Qemu-devel] [RFC] Add NRF51 RNG peripheral
Posted by Stefan Hajnoczi 5 years, 9 months ago
On Tue, Jun 19, 2018 at 11:54 AM, Steffen Görtz
<contrib@steffen-goertz.de> wrote:
> Add a model of the NRF51 RNG peripheral.
>
> Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
> ---
>  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 <contrib@steffen-goertz.de>
> + *
> + * 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 <contrib@steffen-goertz.de>
> + *
> + * 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.

Re: [Qemu-devel] [RFC] Add NRF51 RNG peripheral
Posted by Steffen Görtz 5 years, 9 months ago
Hi Stefan,>
>> +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.
>
The "real" ARM armv7m NVIC supports both level and pulsed interrupt
lines from connected devices (see ARM TRM cortex-m 8.3. Level versus
pulse interrupts). Level interrupts require (both in "real" and modeled
devices) some kind of interrupt flag held in the context of the device,
which is reflected in the device interrupt line.

The Nrf51 RNG peripheral does not have a peripheral interrupt status
register neither does it have a interrupt status flag. Furthermore the
nature of the timer expire tick is of a pulsed rather then a stateful
nature.

The model of the ARM armv7m NVIC (armv7m_nvic.c) also supports pulsed
interrupts sources, see set_irq_level:

>/* The pending status of an external interrupt is
> * latched on rising edge and exception handler return.
> *
> * Pulsing the IRQ will always run the handler
> * once, and the handler will re-run until the
> * level is low when the handler completes.
>*/

I used the pulsed implementation because it was more straightforward to
use in this situation.

Best,
Steffen