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

[Peter Maydell]

On Sun, 16 Dec 2018 at 06:20, Stefan Hajnoczi <address@hidden> wrote:
>
> On Mon, Nov 26, 2018 at 05:43:59PM +0000, Peter Maydell wrote:
> > On Mon, 26 Nov 2018 at 00:24, Steffen Görtz <address@hidden> 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