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

[Peter Maydell]

On Mon, 26 Nov 2018 at 00:24, Steffen Görtz <address@hidden> 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