[Qemu-devel] [PATCH v2 0/5] arm: microbit Non-Volatile Memory Controller

[Qemu-devel] [PATCH v2 0/5] arm: microbit Non-Volatile Memory Controller

[Stefan Hajnoczi]

v2:
 * Add Patch 2 to call memory_region_flush_rom_device() from pflash devices
   [Peter]

This series adds the Non-Volatile Memory Controller, which controls access to
the User Information Control Registers (UICR), Factory Information Control
Registers (FICR), and flash memory.

This is the last piece of microbit work needed to make basic programs like
Micropython "Hello world" work under QEMU.

Originally sent as part of Steffen's longer microbit device emulation series, I
extracted this and deferred it until later because cleanups were necessary:

 * Use memory_region_flush_rom_device() to dirty/invalidate memory [Peter]
   ^--- Paolo: I CCed you on this new memory API
 * Fix device-introspect-test segfault due to missing owner when initializing
   FICR and UICR memory regions [Peter]
 * Fix off-by-one assertion checks [Peter]
 * Fix missing whitespace at end of comment [Peter]
 * Clear UICR on reset - we'd need a block device for true non-volatility
   [Peter]

Stefan Hajnoczi (2):
  memory: add memory_region_flush_rom_device()
  pflash: flush rom device memory region

Steffen Görtz (3):
  hw/nvram/nrf51_nvm: Add nRF51 non-volatile memories
  arm: Instantiate NRF51 special NVM's and NVMC
  tests/microbit-test: Add tests for nRF51 NVMC

 hw/nvram/Makefile.objs       |   1 +
 include/exec/memory.h        |  18 ++
 include/hw/arm/nrf51_soc.h   |   2 +
 include/hw/nvram/nrf51_nvm.h |  64 ++++++
 exec.c                       |  12 ++
 hw/arm/nrf51_soc.c           |  41 ++--
 hw/block/pflash_cfi01.c      |   5 +
 hw/block/pflash_cfi02.c      |   8 +
 hw/nvram/nrf51_nvm.c         | 381 +++++++++++++++++++++++++++++++++++
 tests/microbit-test.c        |  97 +++++++++
 10 files changed, 617 insertions(+), 12 deletions(-)
 create mode 100644 include/hw/nvram/nrf51_nvm.h
 create mode 100644 hw/nvram/nrf51_nvm.c

-- 
2.20.1

[Qemu-devel] [PATCH v2 1/5] memory: add memory_region_flush_rom_device()

[Stefan Hajnoczi]

ROM devices go via MemoryRegionOps->write() callbacks for write
operations and do not dirty/invalidate that memory.  Device emulation
must be able to mark memory ranges that have been modified internally
(e.g. using memory_region_get_ram_ptr()).

Introduce the memory_region_flush_rom_device() API for this purpose.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/exec/memory.h | 18 ++++++++++++++++++
 exec.c                | 12 ++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index cd2f209b64..abe9cc79c0 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1344,6 +1344,24 @@ bool memory_region_snapshot_get_dirty(MemoryRegion *mr,
 void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr,
                                hwaddr size, unsigned client);
 
+/**
+ * memory_region_flush_rom_device: Mark a range of pages dirty and invalidate
+ *                                 TBs (for self-modifying code).
+ *
+ * The MemoryRegionOps->write() callback of a ROM device must use this function
+ * to mark byte ranges that have been modified internally, such as by directly
+ * accessing the memory returned by memory_region_get_ram_ptr().
+ *
+ * This function marks the range dirty and invalidates TBs so that TCG can
+ * detect self-modifying code.
+ *
+ * @mr: the region being flushed.
+ * @addr: the start, relative to the start of the region, of the range being
+ *        flushed.
+ * @size: the size, in bytes, of the range being flushed.
+ */
+void memory_region_flush_rom_device(MemoryRegion *mr, hwaddr addr, hwaddr size);
+
 /**
  * memory_region_set_readonly: Turn a memory region read-only (or read-write)
  *
diff --git a/exec.c b/exec.c
index 895449f926..105ff21e74 100644
--- a/exec.c
+++ b/exec.c
@@ -3162,6 +3162,18 @@ static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
     cpu_physical_memory_set_dirty_range(addr, length, dirty_log_mask);
 }
 
+void memory_region_flush_rom_device(MemoryRegion *mr, hwaddr addr, hwaddr size)
+{
+    /* In principle this function would work on other memory region types too,
+     * but the ROM device use case is the only one where this operation is
+     * necessary.  Other memory regions should use the
+     * address_space_read/write() APIs.
+     */
+    assert(memory_region_is_romd(mr));
+
+    invalidate_and_set_dirty(mr, addr, size);
+}
+
 static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
 {
     unsigned access_size_max = mr->ops->valid.max_access_size;
-- 
2.20.1

[Qemu-devel] [PATCH v2 2/5] pflash: flush rom device memory region

[Stefan Hajnoczi]

pflash devices should mark the memory region dirty and invalidate TBs
after directly writing to the RAM backing the ROM device.

Note that pflash_cfi01_get_memory() is used by several machine types to
populate ROM contents directly.  Callers are untouched by this patch
because they only modify memory before the guest is started.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 hw/block/pflash_cfi01.c | 5 +++++
 hw/block/pflash_cfi02.c | 8 ++++++++
 2 files changed, 13 insertions(+)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index bffb4c40e7..5301c11c18 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -446,6 +446,7 @@ static inline void pflash_data_write(pflash_t *pfl, hwaddr offset,
         break;
     }
 
+    memory_region_flush_rom_device(&pfl->mem, offset, width);
 }
 
 static void pflash_write(pflash_t *pfl, hwaddr offset,
@@ -482,6 +483,8 @@ static void pflash_write(pflash_t *pfl, hwaddr offset,
             if (!pfl->ro) {
                 memset(p + offset, 0xff, pfl->sector_len);
                 pflash_update(pfl, offset, pfl->sector_len);
+                memory_region_flush_rom_device(&pfl->mem, offset,
+                                               pfl->sector_len);
             } else {
                 pfl->status |= 0x20; /* Block erase error */
             }
@@ -763,6 +766,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
             error_setg(errp, "failed to read the initial flash content");
             return;
         }
+
+        memory_region_flush_rom_device(&pfl->mem, 0, total_len);
     }
 
     /* Default to devices being used at their maximum device width. This was
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 0f8b7b8c7b..d04572eca4 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -378,6 +378,8 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
                     pflash_update(pfl, offset, 4);
                     break;
                 }
+
+                memory_region_flush_rom_device(&pfl->orig_mem, offset, width);
             }
             pfl->status = 0x00 | ~(value & 0x80);
             /* Let's pretend write is immediate */
@@ -426,6 +428,8 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
             if (!pfl->ro) {
                 memset(pfl->storage, 0xFF, pfl->chip_len);
                 pflash_update(pfl, 0, pfl->chip_len);
+                memory_region_flush_rom_device(&pfl->orig_mem, 0,
+                                               pfl->chip_len);
             }
             pfl->status = 0x00;
             /* Let's wait 5 seconds before chip erase is done */
@@ -441,6 +445,8 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
             if (!pfl->ro) {
                 memset(p + offset, 0xFF, pfl->sector_len);
                 pflash_update(pfl, offset, pfl->sector_len);
+                memory_region_flush_rom_device(&pfl->orig_mem, offset,
+                                               pfl->sector_len);
             }
             pfl->status = 0x00;
             /* Let's wait 1/2 second before sector erase is done */
@@ -590,6 +596,8 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
             error_setg(errp, "failed to read the initial flash content");
             return;
         }
+
+        memory_region_flush_rom_device(&pfl->orig_mem, 0, chip_len);
     }
 
     pflash_setup_mappings(pfl);
-- 
2.20.1

[Qemu-devel] [PATCH v2 3/5] hw/nvram/nrf51_nvm: Add nRF51 non-volatile memories

[Stefan Hajnoczi]

From: Steffen Görtz <contrib@steffen-goertz.de>

The nRF51 contains three regions of non-volatile memory (NVM):
- CODE (R/W): contains code
- FICR (R): Factory information like code size, chip id etc.
- UICR (R/W): Changeable configuration data. Lock bits, Code
  protection configuration, Bootloader address, Nordic SoftRadio
  configuration, Firmware configuration.

Read and write access to the memories is managed by the
Non-volatile memory controller.

Memory schema:
 [ CPU ] -+- [ NVM, either FICR, UICR or CODE ]
          |      |
          \- [ NVMC ]

Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 * Fix device-introspect-test segfault due to missing owner when
   initializing FICR and UICR memory regions [Peter]
 * Fix off-by-one assertion checks [Peter]
 * Fix missing whitespace at end of comment [Peter]
 * Clear UICR on reset - we'd need a block device for true
   non-volatility [Peter]
 * Use memory_region_flush_rom_device() to dirty/invalidate memory
   [Peter]
---
 hw/nvram/Makefile.objs       |   1 +
 include/hw/nvram/nrf51_nvm.h |  64 ++++++
 hw/nvram/nrf51_nvm.c         | 381 +++++++++++++++++++++++++++++++++++
 3 files changed, 446 insertions(+)
 create mode 100644 include/hw/nvram/nrf51_nvm.h
 create mode 100644 hw/nvram/nrf51_nvm.c

diff --git a/hw/nvram/Makefile.objs b/hw/nvram/Makefile.objs
index b318e53a43..26f7b4ca35 100644
--- a/hw/nvram/Makefile.objs
+++ b/hw/nvram/Makefile.objs
@@ -5,3 +5,4 @@ common-obj-y += fw_cfg.o
 common-obj-y += chrp_nvram.o
 common-obj-$(CONFIG_MAC_NVRAM) += mac_nvram.o
 obj-$(CONFIG_PSERIES) += spapr_nvram.o
+obj-$(CONFIG_NRF51_SOC) += nrf51_nvm.o
diff --git a/include/hw/nvram/nrf51_nvm.h b/include/hw/nvram/nrf51_nvm.h
new file mode 100644
index 0000000000..0a8b41b358
--- /dev/null
+++ b/include/hw/nvram/nrf51_nvm.h
@@ -0,0 +1,64 @@
+/*
+ * Nordic Semiconductor nRF51 non-volatile memory
+ *
+ * It provides an interface to erase regions in flash memory.
+ * Furthermore it provides the user and factory information registers.
+ *
+ * QEMU interface:
+ * + sysbus MMIO regions 0: NVMC peripheral registers
+ * + sysbus MMIO regions 1: FICR peripheral registers
+ * + sysbus MMIO regions 2: UICR peripheral registers
+ * + flash-size property: flash size in bytes.
+ *
+ * Accuracy of the peripheral model:
+ * + Code regions (MPU configuration) are disregarded.
+ *
+ * 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.
+ *
+ */
+#ifndef NRF51_NVM_H
+#define NRF51_NVM_H
+
+#include "hw/sysbus.h"
+#define TYPE_NRF51_NVM "nrf51_soc.nvm"
+#define NRF51_NVM(obj) OBJECT_CHECK(NRF51NVMState, (obj), TYPE_NRF51_NVM)
+
+#define NRF51_UICR_FIXTURE_SIZE 64
+
+#define NRF51_NVMC_SIZE         0x1000
+
+#define NRF51_NVMC_READY        0x400
+#define NRF51_NVMC_READY_READY  0x01
+#define NRF51_NVMC_CONFIG       0x504
+#define NRF51_NVMC_CONFIG_MASK  0x03
+#define NRF51_NVMC_CONFIG_WEN   0x01
+#define NRF51_NVMC_CONFIG_EEN   0x02
+#define NRF51_NVMC_ERASEPCR1    0x508
+#define NRF51_NVMC_ERASEPCR0    0x510
+#define NRF51_NVMC_ERASEALL     0x50C
+#define NRF51_NVMC_ERASEUICR    0x514
+#define NRF51_NVMC_ERASE        0x01
+
+#define NRF51_UICR_SIZE         0x100
+
+typedef struct NRF51NVMState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion mmio;
+    MemoryRegion ficr;
+    MemoryRegion uicr;
+    MemoryRegion flash;
+
+    uint32_t uicr_content[NRF51_UICR_FIXTURE_SIZE];
+    uint32_t flash_size;
+    uint32_t *storage;
+
+    uint32_t config;
+
+} NRF51NVMState;
+
+
+#endif
diff --git a/hw/nvram/nrf51_nvm.c b/hw/nvram/nrf51_nvm.c
new file mode 100644
index 0000000000..e51228d669
--- /dev/null
+++ b/hw/nvram/nrf51_nvm.c
@@ -0,0 +1,381 @@
+/*
+ * Nordic Semiconductor nRF51 non-volatile memory
+ *
+ * It provides an interface to erase regions in flash memory.
+ * Furthermore it provides the user and factory information registers.
+ *
+ * Reference Manual: http://infocenter.nordicsemi.com/pdf/nRF51_RM_v3.0.pdf
+ *
+ * See nRF51 reference manual and product sheet sections:
+ * + Non-Volatile Memory Controller (NVMC)
+ * + Factory Information Configuration Registers (FICR)
+ * + User Information Configuration Registers (UICR)
+ *
+ * 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 "qapi/error.h"
+#include "qemu/log.h"
+#include "exec/address-spaces.h"
+#include "hw/arm/nrf51.h"
+#include "hw/nvram/nrf51_nvm.h"
+
+/*
+ * FICR Registers Assignments
+ * CODEPAGESIZE      0x010
+ * CODESIZE          0x014
+ * CLENR0            0x028
+ * PPFC              0x02C
+ * NUMRAMBLOCK       0x034
+ * SIZERAMBLOCKS     0x038
+ * SIZERAMBLOCK[0]   0x038
+ * SIZERAMBLOCK[1]   0x03C
+ * SIZERAMBLOCK[2]   0x040
+ * SIZERAMBLOCK[3]   0x044
+ * CONFIGID          0x05C
+ * DEVICEID[0]       0x060
+ * DEVICEID[1]       0x064
+ * ER[0]             0x080
+ * ER[1]             0x084
+ * ER[2]             0x088
+ * ER[3]             0x08C
+ * IR[0]             0x090
+ * IR[1]             0x094
+ * IR[2]             0x098
+ * IR[3]             0x09C
+ * DEVICEADDRTYPE    0x0A0
+ * DEVICEADDR[0]     0x0A4
+ * DEVICEADDR[1]     0x0A8
+ * OVERRIDEEN        0x0AC
+ * NRF_1MBIT[0]      0x0B0
+ * NRF_1MBIT[1]      0x0B4
+ * NRF_1MBIT[2]      0x0B8
+ * NRF_1MBIT[3]      0x0BC
+ * NRF_1MBIT[4]      0x0C0
+ * BLE_1MBIT[0]      0x0EC
+ * BLE_1MBIT[1]      0x0F0
+ * BLE_1MBIT[2]      0x0F4
+ * BLE_1MBIT[3]      0x0F8
+ * BLE_1MBIT[4]      0x0FC
+ */
+static const uint32_t ficr_content[64] = {
+    0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0x00000400,
+    0x00000100, 0xFFFFFFFF, 0xFFFFFFFF, 0x00000002, 0x00002000,
+    0x00002000, 0x00002000, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+    0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+    0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0x00000003,
+    0x12345678, 0x9ABCDEF1, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+    0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+    0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+    0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+    0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+    0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+    0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+    0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF
+};
+
+static uint64_t ficr_read(void *opaque, hwaddr offset, unsigned int size)
+{
+    assert(offset < sizeof(ficr_content));
+    return ficr_content[offset / 4];
+}
+
+static void ficr_write(void *opaque, hwaddr offset, uint64_t value,
+        unsigned int size)
+{
+    /* Intentionally do nothing */
+}
+
+static const MemoryRegionOps ficr_ops = {
+    .read = ficr_read,
+    .write = ficr_write,
+    .impl.min_access_size = 4,
+    .impl.max_access_size = 4,
+    .endianness = DEVICE_LITTLE_ENDIAN
+};
+
+/*
+ * UICR Registers Assignments
+ * CLENR0           0x000
+ * RBPCONF          0x004
+ * XTALFREQ         0x008
+ * FWID             0x010
+ * BOOTLOADERADDR   0x014
+ * NRFFW[0]         0x014
+ * NRFFW[1]         0x018
+ * NRFFW[2]         0x01C
+ * NRFFW[3]         0x020
+ * NRFFW[4]         0x024
+ * NRFFW[5]         0x028
+ * NRFFW[6]         0x02C
+ * NRFFW[7]         0x030
+ * NRFFW[8]         0x034
+ * NRFFW[9]         0x038
+ * NRFFW[10]        0x03C
+ * NRFFW[11]        0x040
+ * NRFFW[12]        0x044
+ * NRFFW[13]        0x048
+ * NRFFW[14]        0x04C
+ * NRFHW[0]         0x050
+ * NRFHW[1]         0x054
+ * NRFHW[2]         0x058
+ * NRFHW[3]         0x05C
+ * NRFHW[4]         0x060
+ * NRFHW[5]         0x064
+ * NRFHW[6]         0x068
+ * NRFHW[7]         0x06C
+ * NRFHW[8]         0x070
+ * NRFHW[9]         0x074
+ * NRFHW[10]        0x078
+ * NRFHW[11]        0x07C
+ * CUSTOMER[0]      0x080
+ * CUSTOMER[1]      0x084
+ * CUSTOMER[2]      0x088
+ * CUSTOMER[3]      0x08C
+ * CUSTOMER[4]      0x090
+ * CUSTOMER[5]      0x094
+ * CUSTOMER[6]      0x098
+ * CUSTOMER[7]      0x09C
+ * CUSTOMER[8]      0x0A0
+ * CUSTOMER[9]      0x0A4
+ * CUSTOMER[10]     0x0A8
+ * CUSTOMER[11]     0x0AC
+ * CUSTOMER[12]     0x0B0
+ * CUSTOMER[13]     0x0B4
+ * CUSTOMER[14]     0x0B8
+ * CUSTOMER[15]     0x0BC
+ * CUSTOMER[16]     0x0C0
+ * CUSTOMER[17]     0x0C4
+ * CUSTOMER[18]     0x0C8
+ * CUSTOMER[19]     0x0CC
+ * CUSTOMER[20]     0x0D0
+ * CUSTOMER[21]     0x0D4
+ * CUSTOMER[22]     0x0D8
+ * CUSTOMER[23]     0x0DC
+ * CUSTOMER[24]     0x0E0
+ * CUSTOMER[25]     0x0E4
+ * CUSTOMER[26]     0x0E8
+ * CUSTOMER[27]     0x0EC
+ * CUSTOMER[28]     0x0F0
+ * CUSTOMER[29]     0x0F4
+ * CUSTOMER[30]     0x0F8
+ * CUSTOMER[31]     0x0FC
+ */
+
+static uint64_t uicr_read(void *opaque, hwaddr offset, unsigned int size)
+{
+    NRF51NVMState *s = NRF51_NVM(opaque);
+
+    assert(offset < sizeof(s->uicr_content));
+    return s->uicr_content[offset / 4];
+}
+
+static void uicr_write(void *opaque, hwaddr offset, uint64_t value,
+        unsigned int size)
+{
+    NRF51NVMState *s = NRF51_NVM(opaque);
+
+    assert(offset < sizeof(s->uicr_content));
+    s->uicr_content[offset / 4] = value;
+}
+
+static const MemoryRegionOps uicr_ops = {
+    .read = uicr_read,
+    .write = uicr_write,
+    .impl.min_access_size = 4,
+    .impl.max_access_size = 4,
+    .endianness = DEVICE_LITTLE_ENDIAN
+};
+
+
+static uint64_t io_read(void *opaque, hwaddr offset, unsigned int size)
+{
+    NRF51NVMState *s = NRF51_NVM(opaque);
+    uint64_t r = 0;
+
+    switch (offset) {
+    case NRF51_NVMC_READY:
+        r = NRF51_NVMC_READY_READY;
+        break;
+    case NRF51_NVMC_CONFIG:
+        r = s->config;
+        break;
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                "%s: bad read offset 0x%" HWADDR_PRIx "\n", __func__, offset);
+        break;
+    }
+
+    return r;
+}
+
+static void io_write(void *opaque, hwaddr offset, uint64_t value,
+        unsigned int size)
+{
+    NRF51NVMState *s = NRF51_NVM(opaque);
+
+    switch (offset) {
+    case NRF51_NVMC_CONFIG:
+        s->config = value & NRF51_NVMC_CONFIG_MASK;
+        break;
+    case NRF51_NVMC_ERASEPCR0:
+    case NRF51_NVMC_ERASEPCR1:
+        if (s->config & NRF51_NVMC_CONFIG_EEN) {
+            /* Mask in-page sub address */
+            value &= ~(NRF51_PAGE_SIZE - 1);
+            if (value < (s->flash_size - NRF51_PAGE_SIZE)) {
+                memset(s->storage + value / 4, 0xFF, NRF51_PAGE_SIZE);
+                memory_region_flush_rom_device(&s->flash, value,
+                                               NRF51_PAGE_SIZE);
+            }
+        } else {
+            qemu_log_mask(LOG_GUEST_ERROR,
+            "%s: Flash erase at 0x%" HWADDR_PRIx" while flash not erasable.\n",
+            __func__, offset);
+        }
+        break;
+    case NRF51_NVMC_ERASEALL:
+        if (value == NRF51_NVMC_ERASE) {
+            if (s->config & NRF51_NVMC_CONFIG_EEN) {
+                memset(s->storage, 0xFF, s->flash_size);
+                memory_region_flush_rom_device(&s->flash, 0, s->flash_size);
+                memset(s->uicr_content, 0xFF, sizeof(s->uicr_content));
+            } else {
+                qemu_log_mask(LOG_GUEST_ERROR, "%s: Flash not erasable.\n",
+                              __func__);
+            }
+        }
+        break;
+    case NRF51_NVMC_ERASEUICR:
+        if (value == NRF51_NVMC_ERASE) {
+            memset(s->uicr_content, 0xFF, sizeof(s->uicr_content));
+        }
+        break;
+
+    default:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                "%s: bad write offset 0x%" HWADDR_PRIx "\n", __func__, offset);
+    }
+}
+
+static const MemoryRegionOps io_ops = {
+        .read = io_read,
+        .write = io_write,
+        .impl.min_access_size = 4,
+        .impl.max_access_size = 4,
+        .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+
+static void flash_write(void *opaque, hwaddr offset, uint64_t value,
+        unsigned int size)
+{
+    NRF51NVMState *s = NRF51_NVM(opaque);
+
+    if (s->config & NRF51_NVMC_CONFIG_WEN) {
+        assert(offset < s->flash_size);
+        /* NOR Flash only allows bits to be flipped from 1's to 0's on write */
+        s->storage[offset / 4] &= value;
+    } else {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                "%s: Flash write 0x%" HWADDR_PRIx" while flash not writable.\n",
+                __func__, offset);
+    }
+}
+
+
+
+static const MemoryRegionOps flash_ops = {
+    .write = flash_write,
+    .valid.min_access_size = 4,
+    .valid.max_access_size = 4,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void nrf51_nvm_init(Object *obj)
+{
+    NRF51NVMState *s = NRF51_NVM(obj);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+
+    memory_region_init_io(&s->mmio, obj, &io_ops, s, "nrf51_soc.nvmc",
+                          NRF51_NVMC_SIZE);
+    sysbus_init_mmio(sbd, &s->mmio);
+
+    memory_region_init_io(&s->ficr, obj, &ficr_ops, s, "nrf51_soc.ficr",
+                          sizeof(ficr_content));
+    sysbus_init_mmio(sbd, &s->ficr);
+
+    memory_region_init_io(&s->uicr, obj, &uicr_ops, s, "nrf51_soc.uicr",
+                          sizeof(s->uicr_content));
+    sysbus_init_mmio(sbd, &s->uicr);
+}
+
+static void nrf51_nvm_realize(DeviceState *dev, Error **errp)
+{
+    NRF51NVMState *s = NRF51_NVM(dev);
+    Error *err = NULL;
+
+    memory_region_init_rom_device(&s->flash, OBJECT(dev), &flash_ops, s,
+        "nrf51_soc.flash", s->flash_size, &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    s->storage = memory_region_get_ram_ptr(&s->flash);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->flash);
+}
+
+static void nrf51_nvm_reset(DeviceState *dev)
+{
+    NRF51NVMState *s = NRF51_NVM(dev);
+
+    s->config = 0x00;
+    memset(s->uicr_content, 0xFF, sizeof(s->uicr_content));
+}
+
+static Property nrf51_nvm_properties[] = {
+    DEFINE_PROP_UINT32("flash-size", NRF51NVMState, flash_size, 0x40000),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static const VMStateDescription vmstate_nvm = {
+    .name = "nrf51_soc.nvm",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(uicr_content, NRF51NVMState,
+                NRF51_UICR_FIXTURE_SIZE),
+        VMSTATE_UINT32(config, NRF51NVMState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void nrf51_nvm_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->props = nrf51_nvm_properties;
+    dc->vmsd = &vmstate_nvm;
+    dc->realize = nrf51_nvm_realize;
+    dc->reset = nrf51_nvm_reset;
+}
+
+static const TypeInfo nrf51_nvm_info = {
+    .name = TYPE_NRF51_NVM,
+    .parent = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(NRF51NVMState),
+    .instance_init = nrf51_nvm_init,
+    .class_init = nrf51_nvm_class_init
+};
+
+static void nrf51_nvm_register_types(void)
+{
+    type_register_static(&nrf51_nvm_info);
+}
+
+type_init(nrf51_nvm_register_types)
-- 
2.20.1

[Qemu-devel] [PATCH v2 4/5] arm: Instantiate NRF51 special NVM's and NVMC

[Stefan Hajnoczi]

From: Steffen Görtz <contrib@steffen-goertz.de>

Instantiates UICR, FICR, FLASH and NVMC in nRF51 SOC.

Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/hw/arm/nrf51_soc.h |  2 ++
 hw/arm/nrf51_soc.c         | 41 +++++++++++++++++++++++++++-----------
 2 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/include/hw/arm/nrf51_soc.h b/include/hw/arm/nrf51_soc.h
index e06f0304b4..c8c91cf9ff 100644
--- a/include/hw/arm/nrf51_soc.h
+++ b/include/hw/arm/nrf51_soc.h
@@ -15,6 +15,7 @@
 #include "hw/char/nrf51_uart.h"
 #include "hw/misc/nrf51_rng.h"
 #include "hw/gpio/nrf51_gpio.h"
+#include "hw/nvram/nrf51_nvm.h"
 #include "hw/timer/nrf51_timer.h"
 
 #define TYPE_NRF51_SOC "nrf51-soc"
@@ -32,6 +33,7 @@ typedef struct NRF51State {
 
     NRF51UARTState uart;
     NRF51RNGState rng;
+    NRF51NVMState nvm;
     NRF51GPIOState gpio;
     NRF51TimerState timer[NRF51_NUM_TIMERS];
 
diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index 1630c27594..b839daea8b 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -29,8 +29,10 @@
  * are supported in the future, add a sub-class of NRF51SoC for
  * the specific variants
  */
-#define NRF51822_FLASH_SIZE     (256 * NRF51_PAGE_SIZE)
-#define NRF51822_SRAM_SIZE      (16 * NRF51_PAGE_SIZE)
+#define NRF51822_FLASH_PAGES    256
+#define NRF51822_SRAM_PAGES     16
+#define NRF51822_FLASH_SIZE     (NRF51822_FLASH_PAGES * NRF51_PAGE_SIZE)
+#define NRF51822_SRAM_SIZE      (NRF51822_SRAM_PAGES * NRF51_PAGE_SIZE)
 
 #define BASE_TO_IRQ(base) ((base >> 12) & 0x1F)
 
@@ -81,14 +83,6 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
 
     memory_region_add_subregion_overlap(&s->container, 0, s->board_memory, -1);
 
-    memory_region_init_rom(&s->flash, OBJECT(s), "nrf51.flash", s->flash_size,
-            &err);
-    if (err) {
-        error_propagate(errp, err);
-        return;
-    }
-    memory_region_add_subregion(&s->container, NRF51_FLASH_BASE, &s->flash);
-
     memory_region_init_ram(&s->sram, NULL, "nrf51.sram", s->sram_size, &err);
     if (err) {
         error_propagate(errp, err);
@@ -121,6 +115,29 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
                        qdev_get_gpio_in(DEVICE(&s->cpu),
                        BASE_TO_IRQ(NRF51_RNG_BASE)));
 
+    /* UICR, FICR, NVMC, FLASH */
+    object_property_set_uint(OBJECT(&s->nvm), s->flash_size, "flash-size",
+                             &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    object_property_set_bool(OBJECT(&s->nvm), true, "realized", &err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->nvm), 0);
+    memory_region_add_subregion_overlap(&s->container, NRF51_NVMC_BASE, mr, 0);
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->nvm), 1);
+    memory_region_add_subregion_overlap(&s->container, NRF51_FICR_BASE, mr, 0);
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->nvm), 2);
+    memory_region_add_subregion_overlap(&s->container, NRF51_UICR_BASE, mr, 0);
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->nvm), 3);
+    memory_region_add_subregion_overlap(&s->container, NRF51_FLASH_BASE, mr, 0);
+
     /* GPIO */
     object_property_set_bool(OBJECT(&s->gpio), true, "realized", &err);
     if (err) {
@@ -158,8 +175,6 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
 
     create_unimplemented_device("nrf51_soc.io", NRF51_IOMEM_BASE,
                                 NRF51_IOMEM_SIZE);
-    create_unimplemented_device("nrf51_soc.ficr", NRF51_FICR_BASE,
-                                NRF51_FICR_SIZE);
     create_unimplemented_device("nrf51_soc.private",
                                 NRF51_PRIVATE_BASE, NRF51_PRIVATE_SIZE);
 }
@@ -186,6 +201,8 @@ static void nrf51_soc_init(Object *obj)
     sysbus_init_child_obj(obj, "rng", &s->rng, sizeof(s->rng),
                            TYPE_NRF51_RNG);
 
+    sysbus_init_child_obj(obj, "nvm", &s->nvm, sizeof(s->nvm), TYPE_NRF51_NVM);
+
     sysbus_init_child_obj(obj, "gpio", &s->gpio, sizeof(s->gpio),
                           TYPE_NRF51_GPIO);
 
-- 
2.20.1

[Qemu-devel] [PATCH v2 5/5] tests/microbit-test: Add tests for nRF51 NVMC

[Stefan Hajnoczi]

From: Steffen Görtz <contrib@steffen-goertz.de>

Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/microbit-test.c | 97 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/tests/microbit-test.c b/tests/microbit-test.c
index 0c125535f6..d3edd38643 100644
--- a/tests/microbit-test.c
+++ b/tests/microbit-test.c
@@ -20,8 +20,104 @@
 
 #include "hw/arm/nrf51.h"
 #include "hw/gpio/nrf51_gpio.h"
+#include "hw/nvram/nrf51_nvm.h"
 #include "hw/timer/nrf51_timer.h"
 
+#define FLASH_SIZE          (256 * NRF51_PAGE_SIZE)
+
+static void fill_and_erase(hwaddr base, hwaddr size, uint32_t address_reg)
+{
+    hwaddr i;
+
+    /* Erase Page */
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x02);
+    writel(NRF51_NVMC_BASE + address_reg, base);
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00);
+
+    /* Check memory */
+    for (i = 0; i < size / 4; i++) {
+        g_assert_cmpuint(readl(base + i * 4), ==, 0xFFFFFFFF);
+    }
+
+    /* Fill memory */
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x01);
+    for (i = 0; i < size / 4; i++) {
+        writel(base + i * 4, i);
+        g_assert_cmpuint(readl(base + i * 4), ==, i);
+    }
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00);
+}
+
+static void test_nrf51_nvmc(void)
+{
+    uint32_t value;
+    hwaddr i;
+
+    /* Test always ready */
+    value = readl(NRF51_NVMC_BASE + NRF51_NVMC_READY);
+    g_assert_cmpuint(value & 0x01, ==, 0x01);
+
+    /* Test write-read config register */
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x03);
+    g_assert_cmpuint(readl(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG), ==, 0x03);
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00);
+    g_assert_cmpuint(readl(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG), ==, 0x00);
+
+    /* Test PCR0 */
+    fill_and_erase(NRF51_FLASH_BASE, NRF51_PAGE_SIZE, NRF51_NVMC_ERASEPCR0);
+    fill_and_erase(NRF51_FLASH_BASE + NRF51_PAGE_SIZE,
+                   NRF51_PAGE_SIZE, NRF51_NVMC_ERASEPCR0);
+
+    /* Test PCR1 */
+    fill_and_erase(NRF51_FLASH_BASE, NRF51_PAGE_SIZE, NRF51_NVMC_ERASEPCR1);
+    fill_and_erase(NRF51_FLASH_BASE + NRF51_PAGE_SIZE,
+                   NRF51_PAGE_SIZE, NRF51_NVMC_ERASEPCR1);
+
+    /* Erase all */
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x02);
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_ERASEALL, 0x01);
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00);
+
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x01);
+    for (i = 0; i < FLASH_SIZE / 4; i++) {
+        writel(NRF51_FLASH_BASE + i * 4, i);
+        g_assert_cmpuint(readl(NRF51_FLASH_BASE + i * 4), ==, i);
+    }
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00);
+
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x02);
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_ERASEALL, 0x01);
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00);
+
+    for (i = 0; i < FLASH_SIZE / 4; i++) {
+        g_assert_cmpuint(readl(NRF51_FLASH_BASE + i * 4), ==, 0xFFFFFFFF);
+    }
+
+    /* Erase UICR */
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x02);
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_ERASEUICR, 0x01);
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00);
+
+    for (i = 0; i < NRF51_UICR_SIZE / 4; i++) {
+        g_assert_cmpuint(readl(NRF51_UICR_BASE + i * 4), ==, 0xFFFFFFFF);
+    }
+
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x01);
+    for (i = 0; i < NRF51_UICR_SIZE / 4; i++) {
+        writel(NRF51_UICR_BASE + i * 4, i);
+        g_assert_cmpuint(readl(NRF51_UICR_BASE + i * 4), ==, i);
+    }
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00);
+
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x02);
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_ERASEUICR, 0x01);
+    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00);
+
+    for (i = 0; i < NRF51_UICR_SIZE / 4; i++) {
+        g_assert_cmpuint(readl(NRF51_UICR_BASE + i * 4), ==, 0xFFFFFFFF);
+    }
+}
+
 static void test_nrf51_gpio(void)
 {
     size_t i;
@@ -246,6 +342,7 @@ int main(int argc, char **argv)
     global_qtest = qtest_initf("-machine microbit");
 
     qtest_add_func("/microbit/nrf51/gpio", test_nrf51_gpio);
+    qtest_add_func("/microbit/nrf51/nvmc", test_nrf51_nvmc);
     qtest_add_func("/microbit/nrf51/timer", test_nrf51_timer);
 
     ret = g_test_run();
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 5/5] tests/microbit-test: Add tests for nRF51 NVMC

[Thomas Huth]

On 2019-01-23 22:22, Stefan Hajnoczi wrote:
> From: Steffen Görtz <contrib@steffen-goertz.de>
> 
> Signed-off-by: Steffen Görtz <contrib@steffen-goertz.de>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  tests/microbit-test.c | 97 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/tests/microbit-test.c b/tests/microbit-test.c
> index 0c125535f6..d3edd38643 100644
> --- a/tests/microbit-test.c
> +++ b/tests/microbit-test.c
> @@ -20,8 +20,104 @@
>  
>  #include "hw/arm/nrf51.h"
>  #include "hw/gpio/nrf51_gpio.h"
> +#include "hw/nvram/nrf51_nvm.h"
>  #include "hw/timer/nrf51_timer.h"
>  
> +#define FLASH_SIZE          (256 * NRF51_PAGE_SIZE)
> +
> +static void fill_and_erase(hwaddr base, hwaddr size, uint32_t address_reg)
> +{
> +    hwaddr i;
> +
> +    /* Erase Page */
> +    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x02);
> +    writel(NRF51_NVMC_BASE + address_reg, base);
> +    writel(NRF51_NVMC_BASE + NRF51_NVMC_CONFIG, 0x00);

I think you might have a conflict here with Julia's patch
"tests/microbit-test: Make test independent of global_qtest". Could you
please have a look (and e.g. include Julia's patches in your series)?

 Thanks,
  Thomas

Re: [Qemu-devel] [PATCH v2 2/5] pflash: flush rom device memory region

[Philippe Mathieu-Daudé]

Hi Stefan,

On 1/23/19 10:22 PM, Stefan Hajnoczi wrote:
> pflash devices should mark the memory region dirty and invalidate TBs
> after directly writing to the RAM backing the ROM device.
> 
> Note that pflash_cfi01_get_memory() is used by several machine types to
> populate ROM contents directly.  Callers are untouched by this patch
> because they only modify memory before the guest is started.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 5 +++++
>  hw/block/pflash_cfi02.c | 8 ++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index bffb4c40e7..5301c11c18 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -446,6 +446,7 @@ static inline void pflash_data_write(pflash_t *pfl, hwaddr offset,
>          break;
>      }
>  
> +    memory_region_flush_rom_device(&pfl->mem, offset, width);
>  }
>  
>  static void pflash_write(pflash_t *pfl, hwaddr offset,
> @@ -482,6 +483,8 @@ static void pflash_write(pflash_t *pfl, hwaddr offset,
>              if (!pfl->ro) {
>                  memset(p + offset, 0xff, pfl->sector_len);
>                  pflash_update(pfl, offset, pfl->sector_len);
> +                memory_region_flush_rom_device(&pfl->mem, offset,
> +                                               pfl->sector_len);
>              } else {
>                  pfl->status |= 0x20; /* Block erase error */
>              }
> @@ -763,6 +766,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>              error_setg(errp, "failed to read the initial flash content");
>              return;
>          }
> +
> +        memory_region_flush_rom_device(&pfl->mem, 0, total_len);

The device realize() is also executed before the guest is started, is
this call really necessary?

>      }
>  
>      /* Default to devices being used at their maximum device width. This was
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 0f8b7b8c7b..d04572eca4 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -378,6 +378,8 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>                      pflash_update(pfl, offset, 4);
>                      break;
>                  }
> +
> +                memory_region_flush_rom_device(&pfl->orig_mem, offset, width);
>              }
>              pfl->status = 0x00 | ~(value & 0x80);
>              /* Let's pretend write is immediate */
> @@ -426,6 +428,8 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>              if (!pfl->ro) {
>                  memset(pfl->storage, 0xFF, pfl->chip_len);
>                  pflash_update(pfl, 0, pfl->chip_len);
> +                memory_region_flush_rom_device(&pfl->orig_mem, 0,
> +                                               pfl->chip_len);
>              }
>              pfl->status = 0x00;
>              /* Let's wait 5 seconds before chip erase is done */
> @@ -441,6 +445,8 @@ static void pflash_write (pflash_t *pfl, hwaddr offset,
>              if (!pfl->ro) {
>                  memset(p + offset, 0xFF, pfl->sector_len);
>                  pflash_update(pfl, offset, pfl->sector_len);
> +                memory_region_flush_rom_device(&pfl->orig_mem, offset,
> +                                               pfl->sector_len);
>              }
>              pfl->status = 0x00;
>              /* Let's wait 1/2 second before sector erase is done */
> @@ -590,6 +596,8 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>              error_setg(errp, "failed to read the initial flash content");
>              return;
>          }
> +
> +        memory_region_flush_rom_device(&pfl->orig_mem, 0, chip_len);

Ditto.

>      }
>  
>      pflash_setup_mappings(pfl);
> 

Re: [Qemu-devel] [PATCH v2 0/5] arm: microbit Non-Volatile Memory Controller

[Peter Maydell]

On Wed, 23 Jan 2019 at 21:23, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> v2:
>  * Add Patch 2 to call memory_region_flush_rom_device() from pflash devices
>    [Peter]
>
> This series adds the Non-Volatile Memory Controller, which controls access to
> the User Information Control Registers (UICR), Factory Information Control
> Registers (FICR), and flash memory.
>
> This is the last piece of microbit work needed to make basic programs like
> Micropython "Hello world" work under QEMU.

I've applied patches 1, 3, 4 and 5 to target-arm.next, thanks.
Philippe has review comments on 2 and it's not really microbit
related anyway.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 0/5] arm: microbit Non-Volatile Memory Controller

[Peter Maydell]

On Thu, 24 Jan 2019 at 13:38, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 23 Jan 2019 at 21:23, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > v2:
> >  * Add Patch 2 to call memory_region_flush_rom_device() from pflash devices
> >    [Peter]
> >
> > This series adds the Non-Volatile Memory Controller, which controls access to
> > the User Information Control Registers (UICR), Factory Information Control
> > Registers (FICR), and flash memory.
> >
> > This is the last piece of microbit work needed to make basic programs like
> > Micropython "Hello world" work under QEMU.
>
> I've applied patches 1, 3, 4 and 5 to target-arm.next, thanks.
> Philippe has review comments on 2 and it's not really microbit
> related anyway.

As discussed on IRC, I'm dropping patch 5, because it conflicts
with Julia's patch that drops the usage of global_qtest.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 2/5] pflash: flush rom device memory region

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1145 bytes --]

On Thu, Jan 24, 2019 at 12:11:55PM +0100, Philippe Mathieu-Daudé wrote:
> On 1/23/19 10:22 PM, Stefan Hajnoczi wrote:
> > pflash devices should mark the memory region dirty and invalidate TBs
> > after directly writing to the RAM backing the ROM device.
> > 
> > Note that pflash_cfi01_get_memory() is used by several machine types to
> > populate ROM contents directly.  Callers are untouched by this patch
> > because they only modify memory before the guest is started.
...
> > @@ -763,6 +766,8 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
> >              error_setg(errp, "failed to read the initial flash content");
> >              return;
> >          }
> > +
> > +        memory_region_flush_rom_device(&pfl->mem, 0, total_len);
> 
> The device realize() is also executed before the guest is started, is
> this call really necessary?

My rationale was that machine init only happens before the guest is
started while ->realize() is called by hotplug too.

That said, can pflash devices be hotplugged?

If you guys prefer not flushing from pflash ->realize() then I'll drop
it.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 2/5] pflash: flush rom device memory region

[Peter Maydell]

On Fri, 25 Jan 2019 at 10:28, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> On Thu, Jan 24, 2019 at 12:11:55PM +0100, Philippe Mathieu-Daudé wrote:
> > The device realize() is also executed before the guest is started, is
> > this call really necessary?
>
> My rationale was that machine init only happens before the guest is
> started while ->realize() is called by hotplug too.

Yes, but before realize the flash memory can't actually be
visible to the guest, so there's no need to invalidate anything.

> That said, can pflash devices be hotplugged?

I guess you could have one inside a hotplugged device in theory;
they can't be directly hotpluggged. (I bet that in practice
there are leaks though if you did do that, and we don't have
any devices that do so today.)

> If you guys prefer not flushing from pflash ->realize() then I'll drop
> it.

I think it makes more sense not to do a flush in the realize method.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 2/5] pflash: flush rom device memory region

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

On Fri, Jan 25, 2019 at 10:36:17AM +0000, Peter Maydell wrote:
> On Fri, 25 Jan 2019 at 10:28, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >
> > On Thu, Jan 24, 2019 at 12:11:55PM +0100, Philippe Mathieu-Daudé wrote:
> > > The device realize() is also executed before the guest is started, is
> > > this call really necessary?
> >
> > My rationale was that machine init only happens before the guest is
> > started while ->realize() is called by hotplug too.
> 
> Yes, but before realize the flash memory can't actually be
> visible to the guest, so there's no need to invalidate anything.
> 
> > That said, can pflash devices be hotplugged?
> 
> I guess you could have one inside a hotplugged device in theory;
> they can't be directly hotpluggged. (I bet that in practice
> there are leaks though if you did do that, and we don't have
> any devices that do so today.)
> 
> > If you guys prefer not flushing from pflash ->realize() then I'll drop
> > it.
> 
> I think it makes more sense not to do a flush in the realize method.

Okay, I'll resend this patch.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 2/5] pflash: flush rom device memory region

[Philippe Mathieu-Daudé]

[-- Attachment #1: Type: text/plain, Size: 1307 bytes --]

On 1/25/19 4:37 PM, Stefan Hajnoczi wrote:
> On Fri, Jan 25, 2019 at 10:36:17AM +0000, Peter Maydell wrote:
>> On Fri, 25 Jan 2019 at 10:28, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>>
>>> On Thu, Jan 24, 2019 at 12:11:55PM +0100, Philippe Mathieu-Daudé wrote:
>>>> The device realize() is also executed before the guest is started, is
>>>> this call really necessary?
>>>
>>> My rationale was that machine init only happens before the guest is
>>> started while ->realize() is called by hotplug too.
>>
>> Yes, but before realize the flash memory can't actually be
>> visible to the guest, so there's no need to invalidate anything.
>>
>>> That said, can pflash devices be hotplugged?
>>
>> I guess you could have one inside a hotplugged device in theory;
>> they can't be directly hotpluggged. (I bet that in practice
>> there are leaks though if you did do that, and we don't have
>> any devices that do so today.)

Can we assume pflash aren't hotpluggable until someone find an use case
and is willing to work on it? I can't find any practical example.

>>> If you guys prefer not flushing from pflash ->realize() then I'll drop
>>> it.
>>
>> I think it makes more sense not to do a flush in the realize method.
> 
> Okay, I'll resend this patch.

Thanks!

Phil.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]