Re: [Qemu-devel] [PATCH v5 05/14] hw/nvram/nrf51_nvm: Add nRF51 non-volatile memories

[Peter Maydell <peter.maydell@linaro.org>]

On 12 November 2018 at 21:42, Steffen Görtz <contrib@steffen-goertz.de> wrote:
> 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>

Hi; I have some comments on this patch, but they're mostly
fairly minor.

> ---
>  hw/nvram/Makefile.objs       |   1 +
>  hw/nvram/nrf51_nvm.c         | 378 +++++++++++++++++++++++++++++++++++
>  include/hw/nvram/nrf51_nvm.h |  64 ++++++
>  3 files changed, 443 insertions(+)
>  create mode 100644 hw/nvram/nrf51_nvm.c
>  create mode 100644 include/hw/nvram/nrf51_nvm.h
>
> diff --git a/hw/nvram/Makefile.objs b/hw/nvram/Makefile.objs
> index a912d25391..3f978e6212 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/hw/nvram/nrf51_nvm.c b/hw/nvram/nrf51_nvm.c
> new file mode 100644
> index 0000000000..b3702ebe5e
> --- /dev/null
> +++ b/hw/nvram/nrf51_nvm.c
> @@ -0,0 +1,378 @@
> +/*
> + * 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));

This should be "<", not "<=". If the offset is equal to
the size of the array then the access will be off the end.

> +    return ficr_content[offset / 4];
> +}
> +
> +static void ficr_write(void *opaque, hwaddr offset, uint64_t value,
> +        unsigned int size)
> +{
> +    /* Intentionally do nothing */
> +}

> +static uint64_t uicr_read(void *opaque, hwaddr offset, unsigned int size)
> +{
> +    NRF51NVMState *s = NRF51_NVM(opaque);
> +
> +    assert(offset <= sizeof(s->uicr_content));

Also "<".

> +    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));

Ditto.

> +    s->uicr_content[offset / 4] = value;
> +}
> +



> +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*/

Missing space before "*/".

> +            value &= ~(NRF51_PAGE_SIZE - 1);
> +            if (value < (s->flash_size - NRF51_PAGE_SIZE)) {
> +                memset(s->storage + value / 4, 0xFF, NRF51_PAGE_SIZE);

Can the guest try to execute from the flash storage? If so
then just updating the backing storage directly like this is
not sufficient to ensure that QEMU discards any now-stale
translated code blocks from the affected memory.

> +            }
> +        } 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);
> +                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, NULL, &ficr_ops, s, "nrf51_soc.ficr",
> +                          sizeof(ficr_content));

This call and the one below are the ones where the NULL second
argument should be "obj", as it is in the first call in
this function. This is what causes the "make check" failures
I mentioned in my reply to the series cover letter.

> +    sysbus_init_mmio(sbd, &s->ficr);
> +
> +    memset(s->uicr_content, 0xFF, sizeof(s->uicr_content));
> +    memory_region_init_io(&s->uicr, NULL, &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;

Shouldn't uicr_content[] and storage be reset too ?

> +}

thanks
-- PMM