ROM devices go via MemoryRegionOps->write() callbacks for write
operations and do not dirty/invalidate that memory.  Device emulation
must be able to mark memory ranges that have been modified internally
(e.g. using memory_region_get_ram_ptr()).

Introduce the memory_region_flush_rom_device() API for this purpose.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/exec/memory.h | 18 ++++++++++++++++++
 exec.c                | 12 ++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index cd2f209b64..abe9cc79c0 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1344,6 +1344,24 @@ bool memory_region_snapshot_get_dirty(MemoryRegion *mr,
 void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr,
                                hwaddr size, unsigned client);
 
+/**
+ * memory_region_flush_rom_device: Mark a range of pages dirty and invalidate
+ *                                 TBs (for self-modifying code).
+ *
+ * The MemoryRegionOps->write() callback of a ROM device must use this function
+ * to mark byte ranges that have been modified internally, such as by directly
+ * accessing the memory returned by memory_region_get_ram_ptr().
+ *
+ * This function marks the range dirty and invalidates TBs so that TCG can
+ * detect self-modifying code.
+ *
+ * @mr: the region being flushed.
+ * @addr: the start, relative to the start of the region, of the range being
+ *        flushed.
+ * @size: the size, in bytes, of the range being flushed.
+ */
+void memory_region_flush_rom_device(MemoryRegion *mr, hwaddr addr, hwaddr size);
+
 /**
  * memory_region_set_readonly: Turn a memory region read-only (or read-write)
  *
diff --git a/exec.c b/exec.c
index 895449f926..105ff21e74 100644
--- a/exec.c
+++ b/exec.c
@@ -3162,6 +3162,18 @@ static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
     cpu_physical_memory_set_dirty_range(addr, length, dirty_log_mask);
 }
 
+void memory_region_flush_rom_device(MemoryRegion *mr, hwaddr addr, hwaddr size)
+{
+    /* In principle this function would work on other memory region types too,
+     * but the ROM device use case is the only one where this operation is
+     * necessary.  Other memory regions should use the
+     * address_space_read/write() APIs.
+     */
+    assert(memory_region_is_romd(mr));
+
+    invalidate_and_set_dirty(mr, addr, size);
+}
+
 static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
 {
     unsigned access_size_max = mr->ops->valid.max_access_size;
-- 
2.20.1

On Sun, 20 Jan 2019 at 14:35, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> ROM devices go via MemoryRegionOps->write() callbacks for write
> operations and do not dirty/invalidate that memory.  Device emulation
> must be able to mark memory ranges that have been modified internally
> (e.g. using memory_region_get_ram_ptr()).
>
> Introduce the memory_region_flush_rom_device() API for this purpose.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/exec/memory.h | 18 ++++++++++++++++++
>  exec.c                | 12 ++++++++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index cd2f209b64..abe9cc79c0 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1344,6 +1344,24 @@ bool memory_region_snapshot_get_dirty(MemoryRegion *mr,
>  void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr,
>                                 hwaddr size, unsigned client);
>
> +/**
> + * memory_region_flush_rom_device: Mark a range of pages dirty and invalidate
> + *                                 TBs (for self-modifying code).
> + *
> + * The MemoryRegionOps->write() callback of a ROM device must use this function
> + * to mark byte ranges that have been modified internally, such as by directly
> + * accessing the memory returned by memory_region_get_ram_ptr().
> + *
> + * This function marks the range dirty and invalidates TBs so that TCG can
> + * detect self-modifying code.
> + *
> + * @mr: the region being flushed.
> + * @addr: the start, relative to the start of the region, of the range being
> + *        flushed.
> + * @size: the size, in bytes, of the range being flushed.
> + */
> +void memory_region_flush_rom_device(MemoryRegion *mr, hwaddr addr, hwaddr size);
> +
>  /**
>   * memory_region_set_readonly: Turn a memory region read-only (or read-write)
>   *
> diff --git a/exec.c b/exec.c
> index 895449f926..105ff21e74 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3162,6 +3162,18 @@ static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
>      cpu_physical_memory_set_dirty_range(addr, length, dirty_log_mask);
>  }
>
> +void memory_region_flush_rom_device(MemoryRegion *mr, hwaddr addr, hwaddr size)
> +{
> +    /* In principle this function would work on other memory region types too,
> +     * but the ROM device use case is the only one where this operation is
> +     * necessary.  Other memory regions should use the
> +     * address_space_read/write() APIs.
> +     */
> +    assert(memory_region_is_romd(mr));
> +
> +    invalidate_and_set_dirty(mr, addr, size);
> +}
> +
>  static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
>  {
>      unsigned access_size_max = mr->ops->valid.max_access_size;

API and implementation make sense to me, but better that Paolo reviews
this I think. I guess we should add calls to this to the pflash device
models too...

thanks
-- PMM

On 22/01/19 17:36, Peter Maydell wrote:
>> +void memory_region_flush_rom_device(MemoryRegion *mr, hwaddr addr, hwaddr size)
>> +{
>> +    /* In principle this function would work on other memory region types too,
>> +     * but the ROM device use case is the only one where this operation is
>> +     * necessary.  Other memory regions should use the
>> +     * address_space_read/write() APIs.
>> +     */
>> +    assert(memory_region_is_romd(mr));
>> +
>> +    invalidate_and_set_dirty(mr, addr, size);
>> +}
>> +
>>  static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
>>  {
>>      unsigned access_size_max = mr->ops->valid.max_access_size;
> API and implementation make sense to me, but better that Paolo reviews
> this I think. I guess we should add calls to this to the pflash device
> models too...

Yes, I agree.   The implementation makes sense, though maybe we could
just rename invalidate_and_set_dirty to
memory_region_invalidate_and_set_dirty.  Whatever you guys prefer.

Paolo

On Tue, Jan 22, 2019 at 04:36:36PM +0000, Peter Maydell wrote:
> On Sun, 20 Jan 2019 at 14:35, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > ROM devices go via MemoryRegionOps->write() callbacks for write
> > operations and do not dirty/invalidate that memory.  Device emulation
> > must be able to mark memory ranges that have been modified internally
> > (e.g. using memory_region_get_ram_ptr()).
> >
> > Introduce the memory_region_flush_rom_device() API for this purpose.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  include/exec/memory.h | 18 ++++++++++++++++++
> >  exec.c                | 12 ++++++++++++
> >  2 files changed, 30 insertions(+)
> >
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index cd2f209b64..abe9cc79c0 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -1344,6 +1344,24 @@ bool memory_region_snapshot_get_dirty(MemoryRegion *mr,
> >  void memory_region_reset_dirty(MemoryRegion *mr, hwaddr addr,
> >                                 hwaddr size, unsigned client);
> >
> > +/**
> > + * memory_region_flush_rom_device: Mark a range of pages dirty and invalidate
> > + *                                 TBs (for self-modifying code).
> > + *
> > + * The MemoryRegionOps->write() callback of a ROM device must use this function
> > + * to mark byte ranges that have been modified internally, such as by directly
> > + * accessing the memory returned by memory_region_get_ram_ptr().
> > + *
> > + * This function marks the range dirty and invalidates TBs so that TCG can
> > + * detect self-modifying code.
> > + *
> > + * @mr: the region being flushed.
> > + * @addr: the start, relative to the start of the region, of the range being
> > + *        flushed.
> > + * @size: the size, in bytes, of the range being flushed.
> > + */
> > +void memory_region_flush_rom_device(MemoryRegion *mr, hwaddr addr, hwaddr size);
> > +
> >  /**
> >   * memory_region_set_readonly: Turn a memory region read-only (or read-write)
> >   *
> > diff --git a/exec.c b/exec.c
> > index 895449f926..105ff21e74 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -3162,6 +3162,18 @@ static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
> >      cpu_physical_memory_set_dirty_range(addr, length, dirty_log_mask);
> >  }
> >
> > +void memory_region_flush_rom_device(MemoryRegion *mr, hwaddr addr, hwaddr size)
> > +{
> > +    /* In principle this function would work on other memory region types too,
> > +     * but the ROM device use case is the only one where this operation is
> > +     * necessary.  Other memory regions should use the
> > +     * address_space_read/write() APIs.
> > +     */
> > +    assert(memory_region_is_romd(mr));
> > +
> > +    invalidate_and_set_dirty(mr, addr, size);
> > +}
> > +
> >  static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
> >  {
> >      unsigned access_size_max = mr->ops->valid.max_access_size;
> 
> API and implementation make sense to me, but better that Paolo reviews
> this I think. I guess we should add calls to this to the pflash device
> models too...

Okay, will cover pflash in the next revision.

Stefan

On Wed, 23 Jan 2019 at 21:07, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> On Tue, Jan 22, 2019 at 04:36:36PM +0000, Peter Maydell wrote:
> > API and implementation make sense to me, but better that Paolo reviews
> > this I think. I guess we should add calls to this to the pflash device
> > models too...
>
> Okay, will cover pflash in the next revision.

We can do that as a separate patch, since it's not related to
the microbit work. For this lot we just need to make a decision
about whether to do this this way or have as Paolo suggested
"memory_region_invalidate_and_set_dirty()". I don't have a
strong opinion, and it sounded like Paolo didn't either, so
let's go with the code you have here.

I can take this patchset via target-arm.next.

thanks
-- PMM