subject:"\[Qemu\-devel\] \[PATCH v5 05\/14\] hw\/nvram\/nrf51

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

2018-12-16 Thread Peter Maydell

On Sun, 16 Dec 2018 at 06:20, Stefan Hajnoczi  wrote:
>
> On Mon, Nov 26, 2018 at 05:43:59PM +, Peter Maydell wrote:
> > On Mon, 26 Nov 2018 at 00:24, Steffen Görtz  wrote:
> > > What else is necessary to invalidate stale blocks?
> >
> > You need an AddressSpace that points to the MemoryRegion(s)
> > you're altering, and you need to use the operations on
> > the AddressSpace like address_space_write(). These will
> > under the hood do the right thing with TB invalidation.
>
> I'm not sure about this.

> I see nothing that invalidates TBs in the address_space_write() code for
> MMIO memory regions (not RAM).  Only the RAM case calls
> invalidate_and_set_dirty().

Hmm, I think I see what you mean -- the problem
is because your ram backed region is created with
memory_region_init_rom_device(), which means that
if you try to write to the MR via address_space_write()
you get the guest access path which ends up in the
MemoryRegionOps write function, whereas what we wanted
was to access the MR via the write-host-RAM code path
which updates the dirty state.

> There are a few complications with this device:
>
> 1. Stores from the CPU are only honored when the NRF51_NVMC_CONFIG_WEN
>write enable bit is set.  NRF51_NVMC_ERASEPCRx and
>NRF51_NVMC_ERASEALL commands use a separate erase enable bit
>(NRF51_NVMC_CONFIG_EEN) and are therefore different from normal
>writes.

i.e. we want guest writes to the region to go
via a write function so we can impose the right semantics.

> 2. Stores from the CPU can only flip 1s to 0s (this is NOR flash).  When
>we erase a page of flash memory it must be set to 0xff (i.e. flip
>0s to 1s).

i.e. we have some other operations which need to perform
writes directly on the underlying storage.

> 3. nrf51_nvm.c:flash_write() does not mark the page dirty for live
>migration.

...and this is the bug we need to avoid: any write to the
underlying storage must update dirty status, both for
migration and for TCG cached code invalidation.

> My questions:
>
> 1. Is the current rom+mmio device approach okay or should it be modelled
>differently?
>
> 2. Erase operations cannot use ordinary address_space_write() for the
>reasons mentioned above.  Should this device directly call
>cpu_physical_memory_set_dirty_range() and tb_invalidate_phys_range()?

I think that these functions are too low level for devices to be
calling directly, and we should have something at the MemoryRegion or
AddressSpace level to do what we need. Perhaps some API for
"this is a rom_device MemoryRegion, create me a new MemoryRegion
which is a pure RAM MemoryRegion view of the underlying storage".
(The device could then put that into a private AddressSpace and
use that for when it wants to do updates directly to storage.)
Or perhaps we could just say "practically all uses of rom_device
MemoryRegions will want to be able to write to the underlying
storage, we should provide an API that handles flushing for them".

Paolo, do you have a view?

(Our only other users of rom_device are hw/block/pflash_cfi0[12].c;
I think they get away with this because they mostly flip the device
out of romd mode before updating the backing storage. That works
but is very heavyweight, and is almost working by accident rather
than design.)

thanks
-- PMM

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

2018-12-15 Thread Stefan Hajnoczi

On Mon, Nov 26, 2018 at 05:43:59PM +, Peter Maydell wrote:
> On Mon, 26 Nov 2018 at 00:24, Steffen Görtz  wrote:
> >
> > Hi Peter,
> >
> > thank you for your remarks!
> >
> > >> +};
> > >> +
> > >> +static uint64_t ficr_read(void *opaque, hwaddr offset
> > >
> > >> +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.
> >
> > What else is necessary to invalidate stale blocks?
> 
> You need an AddressSpace that points to the MemoryRegion(s)
> you're altering, and you need to use the operations on
> the AddressSpace like address_space_write(). These will
> under the hood do the right thing with TB invalidation.

I'm not sure about this.  The memory region looks like this:

{parent_obj = {class = 0x565ee350, free = 0x0, Python Exception  There is no member named keys.:
properties = 0x5672f860, ref = 1, parent = 0x566620f0}, romd_mode = 
true, ram = false, subpage = false, readonly = false,
  nonvolatile = false, rom_device = true, flush_coalesced_mmio = false, 
global_locking = true, dirty_log_mask = 0 '\000', is_iommu = false, ram_block = 
0x56768b40,
  owner = 0x566620f0, ops = 0x5615d360 , opaque = 
0x566620f0, container = 0x0, size = 262144, addr = 0, destructor = 
0x55893f00 ,
  align = 2097152, terminates = true, ram_device = false, enabled = true, 
warning_printed = false, vga_logging_count = 0 '\000', alias = 0x0, 
alias_offset = 0, priority = 0, subregions = {
tqh_first = 0x0, tqh_last = 0x56662778}, subregions_link = {tqe_next = 
0x0, tqe_prev = 0x0}, coalesced = {tqh_first = 0x0, tqh_last = 0x56662798},
  name = 0x568033d0 "nrf51_soc.flash", ioeventfd_nb = 0, ioeventfds = 0x0}

I see nothing that invalidates TBs in the address_space_write() code for
MMIO memory regions (not RAM).  Only the RAM case calls
invalidate_and_set_dirty().

There are a few complications with this device:

1. Stores from the CPU are only honored when the NRF51_NVMC_CONFIG_WEN
   write enable bit is set.  NRF51_NVMC_ERASEPCRx and
   NRF51_NVMC_ERASEALL commands use a separate erase enable bit
   (NRF51_NVMC_CONFIG_EEN) and are therefore different from normal
   writes.

2. Stores from the CPU can only flip 1s to 0s (this is NOR flash).  When
   we erase a page of flash memory it must be set to 0xff (i.e. flip
   0s to 1s).

3. nrf51_nvm.c:flash_write() does not mark the page dirty for live
   migration.

My questions:

1. Is the current rom+mmio device approach okay or should it be modelled
   differently?

2. Erase operations cannot use ordinary address_space_write() for the
   reasons mentioned above.  Should this device directly call
   cpu_physical_memory_set_dirty_range() and tb_invalidate_phys_range()?

signature.asc
Description: PGP signature

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

2018-11-26 Thread Peter Maydell

On Mon, 26 Nov 2018 at 00:24, Steffen Görtz  wrote:
>
> Hi Peter,
>
> thank you for your remarks!
>
> >> +};
> >> +
> >> +static uint64_t ficr_read(void *opaque, hwaddr offset
> >
> >> +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.
>
> What else is necessary to invalidate stale blocks?

You need an AddressSpace that points to the MemoryRegion(s)
you're altering, and you need to use the operations on
the AddressSpace like address_space_write(). These will
under the hood do the right thing with TB invalidation.

> >> +static void nrf51_nvm_reset(DeviceState *dev)
> >> +{
> >> +NRF51NVMState *s = NRF51_NVM(dev);
> >> +
> >> +s->config = 0x00;
> >
> > Shouldn't uicr_content[] and storage be reset too ?
>
>
> Storage and uicr_content should be retained during a device reset.

If you want storage that lives over system reset, that is
painful, because it means you need to back it with a
block device (like we do for disks and for flash memory).
(This is because QEMU's system reset is a hard-power-cycle;
it should be equivalent to stopping QEMU entirely and
then restarting it, so any persistent storage needs to be
in a block device somewhere.)

thanks
-- PMM

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

2018-11-25 Thread Steffen Görtz

Hi Peter,

thank you for your remarks!

>> +};
>> +
>> +static uint64_t ficr_read(void *opaque, hwaddr offset
> 
>> +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.

What else is necessary to invalidate stale blocks?


>> +
>> +static void nrf51_nvm_reset(DeviceState *dev)
>> +{
>> +NRF51NVMState *s = NRF51_NVM(dev);
>> +
>> +s->config = 0x00;
> 
> Shouldn't uicr_content[] and storage be reset too ?


Storage and uicr_content should be retained during a device reset. 

Best,
Steffen

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

2018-11-16 Thread Peter Maydell

On 12 November 2018 at 21:42, Steffen Görtz  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 

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 00..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 
> + *
> + * 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
> + * CLENR00x028
> + * 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
> + * DEVICEADDRTYPE0x0A0
> + * DEVICEADDR[0] 0x0A4
> + * DEVICEADDR[1] 0x0A8
> + * OVERRIDEEN0x0AC
> + * 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] = {
> +0x, 0x, 0x, 0x, 0x0400,
> +0x0100, 0x, 0x, 0x0002, 0x2000,
> +0x2000, 0x2000, 0x, 0x, 0x,
> +0x, 0x, 0x, 0x, 0x,
> +0x, 0x, 0x, 0x, 0x0003,
> +0x12345678, 0x9ABCDEF1, 0x, 0x, 0x,
> +0x, 0x, 0x, 0x, 0x,
> +0x, 0x, 0x, 0x, 0x,
> +0x, 0x, 0x, 0x, 0x,
> +0x, 0x, 0x, 0x, 0x,
> +0x, 0x, 0x, 0x, 0x,
> +0x, 0x, 0x, 0x, 0x,
> +0x, 0x, 0x, 0x
> +};
> +
> +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 =

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

2018-11-12 Thread Steffen Görtz

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 
---
 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 00..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 
+ *
+ * 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
+ * CLENR00x028
+ * 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
+ * DEVICEADDRTYPE0x0A0
+ * DEVICEADDR[0] 0x0A4
+ * DEVICEADDR[1] 0x0A8
+ * OVERRIDEEN0x0AC
+ * 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] = {
+0x, 0x, 0x, 0x, 0x0400,
+0x0100, 0x, 0x, 0x0002, 0x2000,
+0x2000, 0x2000, 0x, 0x, 0x,
+0x, 0x, 0x, 0x, 0x,
+0x, 0x, 0x, 0x, 0x0003,
+0x12345678, 0x9ABCDEF1, 0x, 0x, 0x,
+0x, 0x, 0x, 0x, 0x,
+0x, 0x, 0x, 0x, 0x,
+0x, 0x, 0x, 0x, 0x,
+0x, 0x, 0x, 0x, 0x,
+0x, 0x, 0x, 0x, 0x,
+0x, 0x, 0x, 0x, 0x,
+0x, 0x, 0x, 0x
+};
+
+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
+

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

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

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

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

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

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

6 matches

Site Navigation

Mail list logo

Footer information