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

[Stefan Hajnoczi <stefanha@gmail.com>]

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

On Mon, Nov 26, 2018 at 05:43:59PM +0000, Peter Maydell wrote:
> On Mon, 26 Nov 2018 at 00:24, Steffen Görtz <mail@steffen-goertz.de> 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 = 0x5555565ee350, free = 0x0, Python Exception <class 'gdb.error'> There is no member named keys.:
properties = 0x55555672f860, ref = 1, parent = 0x5555566620f0}, 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 = 0x555556768b40,
  owner = 0x5555566620f0, ops = 0x55555615d360 <flash_ops>, opaque = 0x5555566620f0, container = 0x0, size = 262144, addr = 0, destructor = 0x555555893f00 <memory_region_destructor_ram>,
  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 = 0x555556662778}, subregions_link = {tqe_next = 0x0, tqe_prev = 0x0}, coalesced = {tqh_first = 0x0, tqh_last = 0x555556662798},
  name = 0x5555568033d0 "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()?

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