[Qemu-devel] [RFC 0/5] execute code from mmio area

[Qemu-devel] [RFC 0/5] execute code from mmio area

[fred.konrad]

From: KONRAD Frederic <fred.konrad@greensocs.com>

This patch-set allows to execute code from mmio areas.
The main goal of this is to be able to run code for example from an SPI device.

The three first patch fixes the way get_page_addr_code fills the TLB.

The fourth patch implements the mmio execution helpers: the device must
implement the request_ptr callback of the MemoryRegion and will be notified when
the guest wants to execute code from it.

The fifth patch implements the execution from the SPI memories in the
xilinx_spips model.

Thanks,
Fred

KONRAD Frederic (5):
  cputlb: cleanup get_page_addr_code to use VICTIM_TLB_HIT
  cputlb: move get_page_addr_code
  cputlb: fix the way get_page_addr_code fills the tlb
  exec: allow to get a pointer for some mmio memory region
  xilinx_spips: allow mmio execution

 cputlb.c              | 81 ++++++++++++++++++++++++++++-----------------------
 hw/ssi/xilinx_spips.c | 74 ++++++++++++++++++++++++++++++++++------------
 include/exec/memory.h | 35 ++++++++++++++++++++++
 memory.c              | 45 ++++++++++++++++++++++++++++
 4 files changed, 180 insertions(+), 55 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [RFC 1/5] cputlb: cleanup get_page_addr_code to use VICTIM_TLB_HIT

[fred.konrad]

From: KONRAD Frederic <fred.konrad@greensocs.com>

This replaces env1 and page_index variables by env and index
so we can use VICTIM_TLB_HIT macro later.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 cputlb.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/cputlb.c b/cputlb.c
index 6c39927..665caea 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -457,21 +457,21 @@ static void report_bad_exec(CPUState *cpu, target_ulong addr)
  * is actually a ram_addr_t (in system mode; the user mode emulation
  * version of this function returns a guest virtual address).
  */
-tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
+tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
 {
-    int mmu_idx, page_index, pd;
+    int mmu_idx, index, pd;
     void *p;
     MemoryRegion *mr;
-    CPUState *cpu = ENV_GET_CPU(env1);
+    CPUState *cpu = ENV_GET_CPU(env);
     CPUIOTLBEntry *iotlbentry;
 
-    page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
-    mmu_idx = cpu_mmu_index(env1, true);
-    if (unlikely(env1->tlb_table[mmu_idx][page_index].addr_code !=
+    index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
+    mmu_idx = cpu_mmu_index(env, true);
+    if (unlikely(env->tlb_table[mmu_idx][index].addr_code !=
                  (addr & TARGET_PAGE_MASK))) {
-        cpu_ldub_code(env1, addr);
+        cpu_ldub_code(env, addr);
     }
-    iotlbentry = &env1->iotlb[mmu_idx][page_index];
+    iotlbentry = &env->iotlb[mmu_idx][index];
     pd = iotlbentry->addr & ~TARGET_PAGE_MASK;
     mr = iotlb_to_region(cpu, pd, iotlbentry->attrs);
     if (memory_region_is_unassigned(mr)) {
@@ -484,7 +484,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
             exit(1);
         }
     }
-    p = (void *)((uintptr_t)addr + env1->tlb_table[mmu_idx][page_index].addend);
+    p = (void *)((uintptr_t)addr + env->tlb_table[mmu_idx][index].addend);
     return qemu_ram_addr_from_host_nofail(p);
 }
 
-- 
1.8.3.1

[Qemu-devel] [RFC 2/5] cputlb: move get_page_addr_code

[fred.konrad]

From: KONRAD Frederic <fred.konrad@greensocs.com>

This just moves the code before VICTIM_TLB_HIT macro definition
so we can use it.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 cputlb.c | 72 ++++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/cputlb.c b/cputlb.c
index 665caea..b3a5f47 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -452,42 +452,6 @@ static void report_bad_exec(CPUState *cpu, target_ulong addr)
     log_cpu_state_mask(LOG_GUEST_ERROR, cpu, CPU_DUMP_FPU | CPU_DUMP_CCOP);
 }
 
-/* NOTE: this function can trigger an exception */
-/* NOTE2: the returned address is not exactly the physical address: it
- * is actually a ram_addr_t (in system mode; the user mode emulation
- * version of this function returns a guest virtual address).
- */
-tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
-{
-    int mmu_idx, index, pd;
-    void *p;
-    MemoryRegion *mr;
-    CPUState *cpu = ENV_GET_CPU(env);
-    CPUIOTLBEntry *iotlbentry;
-
-    index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
-    mmu_idx = cpu_mmu_index(env, true);
-    if (unlikely(env->tlb_table[mmu_idx][index].addr_code !=
-                 (addr & TARGET_PAGE_MASK))) {
-        cpu_ldub_code(env, addr);
-    }
-    iotlbentry = &env->iotlb[mmu_idx][index];
-    pd = iotlbentry->addr & ~TARGET_PAGE_MASK;
-    mr = iotlb_to_region(cpu, pd, iotlbentry->attrs);
-    if (memory_region_is_unassigned(mr)) {
-        CPUClass *cc = CPU_GET_CLASS(cpu);
-
-        if (cc->do_unassigned_access) {
-            cc->do_unassigned_access(cpu, addr, false, true, 0, 4);
-        } else {
-            report_bad_exec(cpu, addr);
-            exit(1);
-        }
-    }
-    p = (void *)((uintptr_t)addr + env->tlb_table[mmu_idx][index].addend);
-    return qemu_ram_addr_from_host_nofail(p);
-}
-
 static uint64_t io_readx(CPUArchState *env, CPUIOTLBEntry *iotlbentry,
                          target_ulong addr, uintptr_t retaddr, int size)
 {
@@ -554,6 +518,42 @@ static bool victim_tlb_hit(CPUArchState *env, size_t mmu_idx, size_t index,
   victim_tlb_hit(env, mmu_idx, index, offsetof(CPUTLBEntry, TY), \
                  (ADDR) & TARGET_PAGE_MASK)
 
+/* NOTE: this function can trigger an exception */
+/* NOTE2: the returned address is not exactly the physical address: it
+ * is actually a ram_addr_t (in system mode; the user mode emulation
+ * version of this function returns a guest virtual address).
+ */
+tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
+{
+    int mmu_idx, index, pd;
+    void *p;
+    MemoryRegion *mr;
+    CPUState *cpu = ENV_GET_CPU(env);
+    CPUIOTLBEntry *iotlbentry;
+
+    index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
+    mmu_idx = cpu_mmu_index(env, true);
+    if (unlikely(env->tlb_table[mmu_idx][index].addr_code !=
+                 (addr & TARGET_PAGE_MASK))) {
+        cpu_ldub_code(env, addr);
+    }
+    iotlbentry = &env->iotlb[mmu_idx][index];
+    pd = iotlbentry->addr & ~TARGET_PAGE_MASK;
+    mr = iotlb_to_region(cpu, pd, iotlbentry->attrs);
+    if (memory_region_is_unassigned(mr)) {
+        CPUClass *cc = CPU_GET_CLASS(cpu);
+
+        if (cc->do_unassigned_access) {
+            cc->do_unassigned_access(cpu, addr, false, true, 0, 4);
+        } else {
+            report_bad_exec(cpu, addr);
+            exit(1);
+        }
+    }
+    p = (void *)((uintptr_t)addr + env->tlb_table[mmu_idx][index].addend);
+    return qemu_ram_addr_from_host_nofail(p);
+}
+
 /* Probe for whether the specified guest write access is permitted.
  * If it is not permitted then an exception will be taken in the same
  * way as if this were a real write access (and we will not return).
-- 
1.8.3.1

[Qemu-devel] [RFC 3/5] cputlb: fix the way get_page_addr_code fills the tlb

[fred.konrad]

From: KONRAD Frederic <fred.konrad@greensocs.com>

get_page_addr_code(..) does a cpu_ldub_code to fill the tlb:
This can lead to some side effects if a device is mapped at this address.

So this patch replaces the cpu_memory_ld by a tlb_fill.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 cputlb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/cputlb.c b/cputlb.c
index b3a5f47..846341e 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -534,8 +534,10 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
     index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
     mmu_idx = cpu_mmu_index(env, true);
     if (unlikely(env->tlb_table[mmu_idx][index].addr_code !=
-                 (addr & TARGET_PAGE_MASK))) {
-        cpu_ldub_code(env, addr);
+                 (addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK)))) {
+        if (!VICTIM_TLB_HIT(addr_read, addr)) {
+            tlb_fill(ENV_GET_CPU(env), addr, MMU_INST_FETCH, mmu_idx, 0);
+        }
     }
     iotlbentry = &env->iotlb[mmu_idx][index];
     pd = iotlbentry->addr & ~TARGET_PAGE_MASK;
-- 
1.8.3.1

[Qemu-devel] [RFC 4/5] exec: allow to get a pointer for some mmio memory region

[fred.konrad]

From: KONRAD Frederic <fred.konrad@greensocs.com>

This introduces a special callback which allows to run code from some MMIO
devices.

SysBusDevice with a MemoryRegion which implements the request_ptr callback will
be notified when the guest try to execute code from their offset. Then it will
be able to eg: pre-load some code from an SPI device or ask a pointer from an
external simulator, etc..

When the pointer or the data in it are no longer valid the device has to
invalidate it.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 cputlb.c              |  7 +++++++
 include/exec/memory.h | 35 +++++++++++++++++++++++++++++++++++
 memory.c              | 45 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 87 insertions(+)

diff --git a/cputlb.c b/cputlb.c
index 846341e..9077247 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -545,6 +545,13 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
     if (memory_region_is_unassigned(mr)) {
         CPUClass *cc = CPU_GET_CLASS(cpu);
 
+        if (memory_region_request_mmio_ptr(mr, addr)) {
+            /* A MemoryRegion is potentially added so re-run the
+             * get_page_addr_code.
+             */
+            return get_page_addr_code(env, addr);
+        }
+
         if (cc->do_unassigned_access) {
             cc->do_unassigned_access(cpu, addr, false, true, 0, 4);
         } else {
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 987f925..36b0eec 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -120,6 +120,15 @@ struct MemoryRegionOps {
                                     uint64_t data,
                                     unsigned size,
                                     MemTxAttrs attrs);
+    /* Instruction execution pre-callback:
+     * @addr is the address of the access relative to the @mr.
+     * @size is the size of the area returned by the callback.
+     * @offset is the location of the pointer inside @mr.
+     *
+     * Returns a pointer to a location which contains guest code.
+     */
+    void *(*request_ptr)(void *opaque, hwaddr addr, unsigned *size,
+                         unsigned *offset);
 
     enum device_endian endianness;
     /* Guest-visible constraints: */
@@ -1253,6 +1262,32 @@ void memory_global_dirty_log_stop(void);
 void mtree_info(fprintf_function mon_printf, void *f, bool flatview);
 
 /**
+ * memory_region_request_mmio_ptr: request a pointer to an mmio
+ * MemoryRegion. If it is possible map a RAM MemoryRegion with this pointer.
+ * When the device wants to invalidate the pointer it will call
+ * memory_region_invalidate_mmio_ptr.
+ *
+ * @mr: #MemoryRegion to check
+ * @addr: address within that region
+ *
+ * Returns true on success, false otherwise.
+ */
+bool memory_region_request_mmio_ptr(MemoryRegion *mr, hwaddr addr);
+
+/**
+ * memory_region_invalidate_mmio_ptr: invalidate the pointer to an mmio
+ * previously requested.
+ * In the end that means that if something wants to execute from this area it
+ * will need to request the pointer again.
+ *
+ * @mr: #MemoryRegion associated to the pointer.
+ * @addr: address within that region
+ * @size: size of that area.
+ */
+void memory_region_invalidate_mmio_ptr(MemoryRegion *mr, hwaddr offset,
+                                       unsigned size);
+
+/**
  * memory_region_dispatch_read: perform a read directly to the specified
  * MemoryRegion.
  *
diff --git a/memory.c b/memory.c
index 6c58373..eb3e8ec 100644
--- a/memory.c
+++ b/memory.c
@@ -2375,6 +2375,51 @@ void memory_listener_unregister(MemoryListener *listener)
     QTAILQ_REMOVE(&listener->address_space->listeners, listener, link_as);
 }
 
+bool memory_region_request_mmio_ptr(MemoryRegion *mr, hwaddr addr)
+{
+    void *host;
+    unsigned size = 0;
+    unsigned offset = 0;
+    MemoryRegion *sub;
+
+    if (!mr || !mr->ops->request_ptr) {
+        return false;
+    }
+
+    /*
+     * Avoid an update if the request_ptr call
+     * memory_region_invalidate_mmio_ptr which seems to be likely when we use
+     * a cache.
+     */
+    memory_region_transaction_begin();
+
+    host = mr->ops->request_ptr(mr->opaque, addr - mr->addr, &size, &offset);
+
+    if (!host || !size) {
+        memory_region_transaction_commit();
+        return false;
+    }
+
+    sub = g_new(MemoryRegion, 1);
+    memory_region_init_ram_ptr(sub, OBJECT(mr), "mmio-map", size, host);
+    memory_region_add_subregion(mr, offset, sub);
+    memory_region_transaction_commit();
+    return true;
+}
+
+void memory_region_invalidate_mmio_ptr(MemoryRegion *mr, hwaddr offset,
+                                       unsigned size)
+{
+    MemoryRegionSection section = memory_region_find(mr, offset, size);
+
+    if (section.mr != mr) {
+        memory_region_del_subregion(mr, section.mr);
+        /* memory_region_find add a ref on section.mr */
+        memory_region_unref(section.mr);
+        object_unparent(OBJECT(section.mr));
+    }
+}
+
 void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
 {
     memory_region_ref(root);
-- 
1.8.3.1

[Qemu-devel] [RFC 5/5] xilinx_spips: allow mmio execution

[fred.konrad]

From: KONRAD Frederic <fred.konrad@greensocs.com>

This allows to execute from the lqspi area.

When the request_ptr is called the device loads 1024bytes from the SPI device.
Then this code can be executed by the guest.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
---
 hw/ssi/xilinx_spips.c | 74 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 55 insertions(+), 19 deletions(-)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index da8adfa..e833028 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -496,6 +496,18 @@ static const MemoryRegionOps spips_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+static void xilinx_qspips_invalidate_mmio_ptr(XilinxQSPIPS *q)
+{
+    XilinxSPIPS *s = &q->parent_obj;
+
+    if (q->lqspi_cached_addr != ~0ULL) {
+        /* Invalidate the current mapped mmio */
+        memory_region_invalidate_mmio_ptr(&s->mmlqspi, q->lqspi_cached_addr,
+                                          LQSPI_CACHE_SIZE);
+        q->lqspi_cached_addr = ~0ULL;
+    }
+}
+
 static void xilinx_qspips_write(void *opaque, hwaddr addr,
                                 uint64_t value, unsigned size)
 {
@@ -505,7 +517,7 @@ static void xilinx_qspips_write(void *opaque, hwaddr addr,
     addr >>= 2;
 
     if (addr == R_LQSPI_CFG) {
-        q->lqspi_cached_addr = ~0ULL;
+        xilinx_qspips_invalidate_mmio_ptr(q);
     }
 }
 
@@ -517,27 +529,20 @@ static const MemoryRegionOps qspips_ops = {
 
 #define LQSPI_CACHE_SIZE 1024
 
-static uint64_t
-lqspi_read(void *opaque, hwaddr addr, unsigned int size)
+static void lqspi_load_cache(void *opaque, hwaddr addr)
 {
-    int i;
     XilinxQSPIPS *q = opaque;
     XilinxSPIPS *s = opaque;
-    uint32_t ret;
-
-    if (addr >= q->lqspi_cached_addr &&
-            addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {
-        uint8_t *retp = &q->lqspi_buf[addr - q->lqspi_cached_addr];
-        ret = cpu_to_le32(*(uint32_t *)retp);
-        DB_PRINT_L(1, "addr: %08x, data: %08x\n", (unsigned)addr,
-                   (unsigned)ret);
-        return ret;
-    } else {
-        int flash_addr = (addr / num_effective_busses(s));
-        int slave = flash_addr >> LQSPI_ADDRESS_BITS;
-        int cache_entry = 0;
-        uint32_t u_page_save = s->regs[R_LQSPI_STS] & ~LQSPI_CFG_U_PAGE;
-
+    int i;
+    int flash_addr = ((addr & ~(LQSPI_CACHE_SIZE - 1))
+                   / num_effective_busses(s));
+    int slave = flash_addr >> LQSPI_ADDRESS_BITS;
+    int cache_entry = 0;
+    uint32_t u_page_save = s->regs[R_LQSPI_STS] & ~LQSPI_CFG_U_PAGE;
+
+    if (addr < q->lqspi_cached_addr ||
+            addr > q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {
+        xilinx_qspips_invalidate_mmio_ptr(q);
         s->regs[R_LQSPI_STS] &= ~LQSPI_CFG_U_PAGE;
         s->regs[R_LQSPI_STS] |= slave ? LQSPI_CFG_U_PAGE : 0;
 
@@ -589,12 +594,43 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
         xilinx_spips_update_cs_lines(s);
 
         q->lqspi_cached_addr = flash_addr * num_effective_busses(s);
+    }
+}
+
+static void *lqspi_request_mmio_ptr(void *opaque, hwaddr addr, unsigned *size,
+                                    unsigned *offset)
+{
+    XilinxQSPIPS *q = opaque;
+    hwaddr offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1);
+
+    lqspi_load_cache(opaque, offset_within_the_region);
+    *size = LQSPI_CACHE_SIZE;
+    *offset = offset_within_the_region;
+    return q->lqspi_buf;
+}
+
+static uint64_t
+lqspi_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    XilinxQSPIPS *q = opaque;
+    uint32_t ret;
+
+    if (addr >= q->lqspi_cached_addr &&
+            addr <= q->lqspi_cached_addr + LQSPI_CACHE_SIZE - 4) {
+        uint8_t *retp = &q->lqspi_buf[addr - q->lqspi_cached_addr];
+        ret = cpu_to_le32(*(uint32_t *)retp);
+        DB_PRINT_L(1, "addr: %08x, data: %08x\n", (unsigned)addr,
+                   (unsigned)ret);
+        return ret;
+    } else {
+        lqspi_load_cache(opaque, addr);
         return lqspi_read(opaque, addr, size);
     }
 }
 
 static const MemoryRegionOps lqspi_ops = {
     .read = lqspi_read,
+    .request_ptr = lqspi_request_mmio_ptr,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .valid = {
         .min_access_size = 1,
-- 
1.8.3.1

Re: [Qemu-devel] [RFC 4/5] exec: allow to get a pointer for some mmio memory region

[Paolo Bonzini]

On 03/02/2017 09:06, fred.konrad@greensocs.com wrote:
> +    host = mr->ops->request_ptr(mr->opaque, addr - mr->addr, &size, &offset);
> +
> +    if (!host || !size) {
> +        memory_region_transaction_commit();
> +        return false;
> +    }
> +
> +    sub = g_new(MemoryRegion, 1);
> +    memory_region_init_ram_ptr(sub, OBJECT(mr), "mmio-map", size, host);
> +    memory_region_add_subregion(mr, offset, sub);
> +    memory_region_transaction_commit();
> +    return true;
> +}
> +
> +void memory_region_invalidate_mmio_ptr(MemoryRegion *mr, hwaddr offset,
> +                                       unsigned size)
> +{
> +    MemoryRegionSection section = memory_region_find(mr, offset, size);
> +
> +    if (section.mr != mr) {
> +        memory_region_del_subregion(mr, section.mr);
> +        /* memory_region_find add a ref on section.mr */
> +        memory_region_unref(section.mr);
> +        object_unparent(OBJECT(section.mr));

I think this would cause a use-after-free when using MTTCG.  In general,
creating and dropping MemoryRegions dynamically can cause bugs that are
nondeterministic and hard to fix without rewriting everything.

An alternative design could be:

- memory_region_request_mmio_ptr returns a MemoryRegionCache instead of
a pointer, so that the device can map a subset of the device (e.g. a
single page)

- memory_region_request_mmio_ptr and MemoryRegionOps.request_ptr accept
a Notifier

- the device adds the Notifier to a NotifierList.  Before invalidating,
it invokes the Notifier and empties the NotifierList.

- for the TLB case, the Notifier calls tlb_flush_page.

I like the general idea though!

Paolo

> +    }
> +}

Re: [Qemu-devel] [RFC 4/5] exec: allow to get a pointer for some mmio memory region

[Frederic Konrad]

On 02/03/2017 06:26 PM, Paolo Bonzini wrote:
> 
> 
> On 03/02/2017 09:06, fred.konrad@greensocs.com wrote:
>> +    host = mr->ops->request_ptr(mr->opaque, addr - mr->addr, &size, &offset);
>> +
>> +    if (!host || !size) {
>> +        memory_region_transaction_commit();
>> +        return false;
>> +    }
>> +
>> +    sub = g_new(MemoryRegion, 1);
>> +    memory_region_init_ram_ptr(sub, OBJECT(mr), "mmio-map", size, host);
>> +    memory_region_add_subregion(mr, offset, sub);
>> +    memory_region_transaction_commit();
>> +    return true;
>> +}
>> +
>> +void memory_region_invalidate_mmio_ptr(MemoryRegion *mr, hwaddr offset,
>> +                                       unsigned size)
>> +{
>> +    MemoryRegionSection section = memory_region_find(mr, offset, size);
>> +
>> +    if (section.mr != mr) {
>> +        memory_region_del_subregion(mr, section.mr);
>> +        /* memory_region_find add a ref on section.mr */
>> +        memory_region_unref(section.mr);
>> +        object_unparent(OBJECT(section.mr));
> 
> I think this would cause a use-after-free when using MTTCG.  In general,
> creating and dropping MemoryRegions dynamically can cause bugs that are
> nondeterministic and hard to fix without rewriting everything.

Hi Paolo,

Thanks for your comment.
Yes, I read in the docs that dynamically dropping MemoryRegions is badly
broken when we use NULL as an owner because the machine owns it.
But it seems nothing said this is the case with an owner.

But I think I see your point here:
  * memory_region_unref will unref the owner.
  * object_unparent will unref the memory region (which should be 1).
  => the region will be dropped immediately.

Doesn't hotplug use dynamic MemoryRegion? In which case we better
make that work with MTTCG. I wonder if we can't simply handle that
with a safe_work for this case?

BTW the tests I have seems to pass without issues.

> 
> An alternative design could be:
> 
> - memory_region_request_mmio_ptr returns a MemoryRegionCache instead of
> a pointer, so that the device can map a subset of the device (e.g. a
> single page)

I'm not aware of this MemoryRegionCache yet, it seems pretty new.
I'll take a look.

Thanks,
Fred

> 
> - memory_region_request_mmio_ptr and MemoryRegionOps.request_ptr accept
> a Notifier
> 
> - the device adds the Notifier to a NotifierList.  Before invalidating,
> it invokes the Notifier and empties the NotifierList.
> 
> - for the TLB case, the Notifier calls tlb_flush_page.
> 
> I like the general idea though!
> 
> Paolo
> 
>> +    }
>> +}

Re: [Qemu-devel] [RFC 1/5] cputlb: cleanup get_page_addr_code to use VICTIM_TLB_HIT

[Edgar E. Iglesias]

On Fri, Feb 03, 2017 at 06:06:33PM +0100, fred.konrad@greensocs.com wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
> 
> This replaces env1 and page_index variables by env and index
> so we can use VICTIM_TLB_HIT macro later.
>

Hi Fred,

A question, wouldn't it be more readable to add env and index arguments to VICTIM_TLB_HIT?
VICTIM_TLB_HIT could perhaps even be made a static inline?

Cheers,
Edgar


 
> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
> ---
>  cputlb.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/cputlb.c b/cputlb.c
> index 6c39927..665caea 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -457,21 +457,21 @@ static void report_bad_exec(CPUState *cpu, target_ulong addr)
>   * is actually a ram_addr_t (in system mode; the user mode emulation
>   * version of this function returns a guest virtual address).
>   */
> -tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
> +tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
>  {
> -    int mmu_idx, page_index, pd;
> +    int mmu_idx, index, pd;
>      void *p;
>      MemoryRegion *mr;
> -    CPUState *cpu = ENV_GET_CPU(env1);
> +    CPUState *cpu = ENV_GET_CPU(env);
>      CPUIOTLBEntry *iotlbentry;
>  
> -    page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
> -    mmu_idx = cpu_mmu_index(env1, true);
> -    if (unlikely(env1->tlb_table[mmu_idx][page_index].addr_code !=
> +    index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
> +    mmu_idx = cpu_mmu_index(env, true);
> +    if (unlikely(env->tlb_table[mmu_idx][index].addr_code !=
>                   (addr & TARGET_PAGE_MASK))) {
> -        cpu_ldub_code(env1, addr);
> +        cpu_ldub_code(env, addr);
>      }
> -    iotlbentry = &env1->iotlb[mmu_idx][page_index];
> +    iotlbentry = &env->iotlb[mmu_idx][index];
>      pd = iotlbentry->addr & ~TARGET_PAGE_MASK;
>      mr = iotlb_to_region(cpu, pd, iotlbentry->attrs);
>      if (memory_region_is_unassigned(mr)) {
> @@ -484,7 +484,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
>              exit(1);
>          }
>      }
> -    p = (void *)((uintptr_t)addr + env1->tlb_table[mmu_idx][page_index].addend);
> +    p = (void *)((uintptr_t)addr + env->tlb_table[mmu_idx][index].addend);
>      return qemu_ram_addr_from_host_nofail(p);
>  }
>  
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [RFC 4/5] exec: allow to get a pointer for some mmio memory region

[Edgar E. Iglesias]

On Fri, Feb 03, 2017 at 09:26:19AM -0800, Paolo Bonzini wrote:
> 
> 
> On 03/02/2017 09:06, fred.konrad@greensocs.com wrote:
> > +    host = mr->ops->request_ptr(mr->opaque, addr - mr->addr, &size, &offset);
> > +
> > +    if (!host || !size) {
> > +        memory_region_transaction_commit();
> > +        return false;
> > +    }
> > +
> > +    sub = g_new(MemoryRegion, 1);
> > +    memory_region_init_ram_ptr(sub, OBJECT(mr), "mmio-map", size, host);
> > +    memory_region_add_subregion(mr, offset, sub);
> > +    memory_region_transaction_commit();
> > +    return true;
> > +}
> > +
> > +void memory_region_invalidate_mmio_ptr(MemoryRegion *mr, hwaddr offset,
> > +                                       unsigned size)
> > +{
> > +    MemoryRegionSection section = memory_region_find(mr, offset, size);
> > +
> > +    if (section.mr != mr) {
> > +        memory_region_del_subregion(mr, section.mr);
> > +        /* memory_region_find add a ref on section.mr */
> > +        memory_region_unref(section.mr);
> > +        object_unparent(OBJECT(section.mr));
> 
> I think this would cause a use-after-free when using MTTCG.  In general,
> creating and dropping MemoryRegions dynamically can cause bugs that are
> nondeterministic and hard to fix without rewriting everything.
> 
> An alternative design could be:
> 
> - memory_region_request_mmio_ptr returns a MemoryRegionCache instead of
> a pointer, so that the device can map a subset of the device (e.g. a
> single page)
> 
> - memory_region_request_mmio_ptr and MemoryRegionOps.request_ptr accept
> a Notifier
> 
> - the device adds the Notifier to a NotifierList.  Before invalidating,
> it invokes the Notifier and empties the NotifierList.
> 
> - for the TLB case, the Notifier calls tlb_flush_page.

Interesting! I totally missed the MemoryRegionCache patches. Cool concept.
I few lines about it in docs/memory.txt would have been nice too :-)


> I like the general idea though!

I think it's genial. I was expecting a solution for this to get ugly...
It solves some existing issues for us (like the QSPI one addressed in this series).
It also paves the way for certain use-cases when co-simulating with SystemC.
Nice one Fred!

Cheers,
Edgar



> 
> Paolo
> 
> > +    }
> > +}

Re: [Qemu-devel] [RFC 1/5] cputlb: cleanup get_page_addr_code to use VICTIM_TLB_HIT

[Frederic Konrad]

On 02/04/2017 12:30 PM, Edgar E. Iglesias wrote:
> On Fri, Feb 03, 2017 at 06:06:33PM +0100, fred.konrad@greensocs.com wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> This replaces env1 and page_index variables by env and index
>> so we can use VICTIM_TLB_HIT macro later.
>>
> 
> Hi Fred,
> 
> A question, wouldn't it be more readable to add env and index arguments to VICTIM_TLB_HIT?
> VICTIM_TLB_HIT could perhaps even be made a static inline?
> 
> Cheers,
> Edgar

Hi Edgar,

Well it seems the VICTIM_TLB_HIT macro is just here to hide those
variables actually.

in cputlb.c:
static bool victim_tlb_hit(CPUArchState *env, size_t mmu_idx,
                           size_t index, size_t elt_ofs,
                           target_ulong page)

and then:
/* Macro to call the above, with local variables from the use context.  */
#define VICTIM_TLB_HIT(TY, ADDR) \
  victim_tlb_hit(env, mmu_idx, index, offsetof(CPUTLBEntry, TY), \
                 (ADDR) & TARGET_PAGE_MASK)

My point of view is clearly that it makes more difficult to read.
So if everybody agrees I can drop the macro and call directly
victim_tlb_hit.

Fred


> 
> 
>  
>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
>> ---
>>  cputlb.c | 18 +++++++++---------
>>  1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/cputlb.c b/cputlb.c
>> index 6c39927..665caea 100644
>> --- a/cputlb.c
>> +++ b/cputlb.c
>> @@ -457,21 +457,21 @@ static void report_bad_exec(CPUState *cpu, target_ulong addr)
>>   * is actually a ram_addr_t (in system mode; the user mode emulation
>>   * version of this function returns a guest virtual address).
>>   */
>> -tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
>> +tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
>>  {
>> -    int mmu_idx, page_index, pd;
>> +    int mmu_idx, index, pd;
>>      void *p;
>>      MemoryRegion *mr;
>> -    CPUState *cpu = ENV_GET_CPU(env1);
>> +    CPUState *cpu = ENV_GET_CPU(env);
>>      CPUIOTLBEntry *iotlbentry;
>>  
>> -    page_index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
>> -    mmu_idx = cpu_mmu_index(env1, true);
>> -    if (unlikely(env1->tlb_table[mmu_idx][page_index].addr_code !=
>> +    index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
>> +    mmu_idx = cpu_mmu_index(env, true);
>> +    if (unlikely(env->tlb_table[mmu_idx][index].addr_code !=
>>                   (addr & TARGET_PAGE_MASK))) {
>> -        cpu_ldub_code(env1, addr);
>> +        cpu_ldub_code(env, addr);
>>      }
>> -    iotlbentry = &env1->iotlb[mmu_idx][page_index];
>> +    iotlbentry = &env->iotlb[mmu_idx][index];
>>      pd = iotlbentry->addr & ~TARGET_PAGE_MASK;
>>      mr = iotlb_to_region(cpu, pd, iotlbentry->attrs);
>>      if (memory_region_is_unassigned(mr)) {
>> @@ -484,7 +484,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
>>              exit(1);
>>          }
>>      }
>> -    p = (void *)((uintptr_t)addr + env1->tlb_table[mmu_idx][page_index].addend);
>> +    p = (void *)((uintptr_t)addr + env->tlb_table[mmu_idx][index].addend);
>>      return qemu_ram_addr_from_host_nofail(p);
>>  }
>>  
>> -- 
>> 1.8.3.1
>>

Re: [Qemu-devel] [RFC 0/5] execute code from mmio area

[Peter Maydell]

On 3 February 2017 at 17:06,  <fred.konrad@greensocs.com> wrote:
> From: KONRAD Frederic <fred.konrad@greensocs.com>
>
> This patch-set allows to execute code from mmio areas.
> The main goal of this is to be able to run code for example from an SPI device.
>
> The three first patch fixes the way get_page_addr_code fills the TLB.
>
> The fourth patch implements the mmio execution helpers: the device must
> implement the request_ptr callback of the MemoryRegion and will be notified when
> the guest wants to execute code from it.
>
> The fifth patch implements the execution from the SPI memories in the
> xilinx_spips model.

I like the general idea, but there's an awkward issue:
at the moment our translation system assumes that when we're
translating code then if the first instruction in the TB
can be read OK then we won't ever get a fault trying to
read subsequent bytes up to the end of the page. If we
move from "we only translate code out of whole pages of
RAM" to "we might translate code out of devices that
are in subpages" then this assumption gets broken.
(The symptom would be that we would report the fault
in the wrong place, for the PC at the start of the TB.)

thanks
-- PMM

Re: [Qemu-devel] [RFC 4/5] exec: allow to get a pointer for some mmio memory region

[Paolo Bonzini]

On 03/02/2017 13:09, Frederic Konrad wrote:
> On 02/03/2017 06:26 PM, Paolo Bonzini wrote:
>>
>>
>> On 03/02/2017 09:06, fred.konrad@greensocs.com wrote:
>>> +    host = mr->ops->request_ptr(mr->opaque, addr - mr->addr, &size, &offset);
>>> +
>>> +    if (!host || !size) {
>>> +        memory_region_transaction_commit();
>>> +        return false;
>>> +    }
>>> +
>>> +    sub = g_new(MemoryRegion, 1);
>>> +    memory_region_init_ram_ptr(sub, OBJECT(mr), "mmio-map", size, host);
>>> +    memory_region_add_subregion(mr, offset, sub);
>>> +    memory_region_transaction_commit();
>>> +    return true;
>>> +}
>>> +
>>> +void memory_region_invalidate_mmio_ptr(MemoryRegion *mr, hwaddr offset,
>>> +                                       unsigned size)
>>> +{
>>> +    MemoryRegionSection section = memory_region_find(mr, offset, size);
>>> +
>>> +    if (section.mr != mr) {
>>> +        memory_region_del_subregion(mr, section.mr);
>>> +        /* memory_region_find add a ref on section.mr */
>>> +        memory_region_unref(section.mr);
>>> +        object_unparent(OBJECT(section.mr));
>>
>> I think this would cause a use-after-free when using MTTCG.  In general,
>> creating and dropping MemoryRegions dynamically can cause bugs that are
>> nondeterministic and hard to fix without rewriting everything.
> 
> Hi Paolo,
> 
> Thanks for your comment.
> Yes, I read in the docs that dynamically dropping MemoryRegions is badly
> broken when we use NULL as an owner because the machine owns it.
> But it seems nothing said this is the case with an owner.
> 
> But I think I see your point here:
>   * memory_region_unref will unref the owner.
>   * object_unparent will unref the memory region (which should be 1).
>   => the region will be dropped immediately.
> 
> Doesn't hotplug use dynamic MemoryRegion? In which case we better
> make that work with MTTCG. I wonder if we can't simply handle that
> with a safe_work for this case?

Hot-unplug works because the backing memory is only freed when the
device gets instance_finalize, so at that point the region cannot have
any references.

What can go wrong is the following (from the docs):

- the memory region's owner had a reference taken via memory_region_ref
  (for example by address_space_map)

- the region is unparented, and has no owner anymore

- when address_space_unmap is called, the reference to the memory
  region's owner is leaked.

In your case you have phys_section_add/phys_section_destroy instead of
address_space_map/unmap, but the problem is the same.

I am a bit unsure about using the same lqspi_buf for caching different
addresses, and about using the same buffer for MMIO execution and read.
What if you allocate a different buffer every time
lqspi_request_mmio_ptr is called (adding a char* argument to
lqspi_load_cache) and free the old one from the safe_work item, after a
global TLB flush?  Then you don't need the Notifiers which were the
complicated and handwavy part of my proposal.

Paolo

> 
> BTW the tests I have seems to pass without issues.
> 
>>
>> An alternative design could be:
>>
>> - memory_region_request_mmio_ptr returns a MemoryRegionCache instead of
>> a pointer, so that the device can map a subset of the device (e.g. a
>> single page)
> 
> I'm not aware of this MemoryRegionCache yet, it seems pretty new.
> I'll take a look.
> 
> Thanks,
> Fred
> 
>>
>> - memory_region_request_mmio_ptr and MemoryRegionOps.request_ptr accept
>> a Notifier
>>
>> - the device adds the Notifier to a NotifierList.  Before invalidating,
>> it invokes the Notifier and empties the NotifierList.
>>
>> - for the TLB case, the Notifier calls tlb_flush_page.
>>
>> I like the general idea though!
>>
>> Paolo
>>
>>> +    }
>>> +}
> 
> 
> 

Re: [Qemu-devel] [RFC 0/5] execute code from mmio area

[Frederic Konrad]

On 02/04/2017 01:33 PM, Peter Maydell wrote:
> On 3 February 2017 at 17:06,  <fred.konrad@greensocs.com> wrote:
>> From: KONRAD Frederic <fred.konrad@greensocs.com>
>>
>> This patch-set allows to execute code from mmio areas.
>> The main goal of this is to be able to run code for example from an SPI device.
>>
>> The three first patch fixes the way get_page_addr_code fills the TLB.
>>
>> The fourth patch implements the mmio execution helpers: the device must
>> implement the request_ptr callback of the MemoryRegion and will be notified when
>> the guest wants to execute code from it.
>>
>> The fifth patch implements the execution from the SPI memories in the
>> xilinx_spips model.
> 
> I like the general idea, but there's an awkward issue:
> at the moment our translation system assumes that when we're
> translating code then if the first instruction in the TB
> can be read OK then we won't ever get a fault trying to
> read subsequent bytes up to the end of the page. If we
> move from "we only translate code out of whole pages of
> RAM" to "we might translate code out of devices that
> are in subpages" then this assumption gets broken.
> (The symptom would be that we would report the fault
> in the wrong place, for the PC at the start of the TB.)
> 
> thanks
> -- PMM
> 

Hi Peter,

I think I see your point.
Is that the case that we might get a Bad RAM address error or some such
if we are not on a page boundary (or too small as you say)?
I guess this is a limitation. Mapping on a page boundary shouldn't be
too much restrictive.

Thanks,
Fred

Re: [Qemu-devel] [RFC 0/5] execute code from mmio area

[Peter Maydell]

On 4 February 2017 at 12:52, Frederic Konrad <fred.konrad@greensocs.com> wrote:
> Is that the case that we might get a Bad RAM address error or some such
> if we are not on a page boundary (or too small as you say)?
> I guess this is a limitation. Mapping on a page boundary shouldn't be
> too much restrictive.

Yeah. I really ought to look more closely at what the flow of
execution is here, because I think how it works right now
is a bit weird and works as much by luck as by judgement
(we can longjump out of the middle of translating code
right back to the cpu-exec.c loop, and in some cases
I think what happens is that we try to translate code,
and as part of the "load didn't work" code path we
nestedly try to translate the same thing again which
of course fails again, only the second time around we
realize and longjump out.

(At the moment for linux-user mode this is causing us to
assert about taking the tb lock twice, because we hold
the tb lock during translation and then try to grab it
again to do the cpu_restore_state() in the signal handler.)

thanks
-- PMM

Re: [Qemu-devel] [RFC 4/5] exec: allow to get a pointer for some mmio memory region

[Frederic Konrad]

On 02/04/2017 01:41 PM, Paolo Bonzini wrote:
> 
...
>>
>> Doesn't hotplug use dynamic MemoryRegion? In which case we better
>> make that work with MTTCG. I wonder if we can't simply handle that
>> with a safe_work for this case?
> 
> Hot-unplug works because the backing memory is only freed when the
> device gets instance_finalize, so at that point the region cannot have
> any references.
> 
> What can go wrong is the following (from the docs):
> 
> - the memory region's owner had a reference taken via memory_region_ref
>   (for example by address_space_map)
> 
> - the region is unparented, and has no owner anymore
> 
> - when address_space_unmap is called, the reference to the memory
>   region's owner is leaked.

true.

> 
> In your case you have phys_section_add/phys_section_destroy instead of
> address_space_map/unmap, but the problem is the same.
> 
> I am a bit unsure about using the same lqspi_buf for caching different
> addresses, and about using the same buffer for MMIO execution and read.
> What if you allocate a different buffer every time
> lqspi_request_mmio_ptr is called (adding a char* argument to
> lqspi_load_cache) and free the old one from the safe_work item, after a
> global TLB flush?  Then you don't need the Notifiers which were the
> complicated and handwavy part of my proposal.

Actually this was just because it was the way xilinx_qspi worked.
It load 1K of SPI data and fill the buffer.

In some other case we don't want to free the pointer at all, just make
it not accessible for execution / read.
(BTW I'll add the read-only property to this).

What about a simple Object eg: mmio-execution-interface which has a
MemoryRegion? Instead of creating the MemoryRegion in request_pointer,
We create a mmio-execution-interface object which create and map the
region on the subregion.
Then in invalidate: we unplug the mmio-execution-interface, unref it and
it is freed all by magic when the map doesn't need it.

This avoid the reference issue and probably the bug with MTTCG.

Fred

> 
> Paolo
> 
>>
>> BTW the tests I have seems to pass without issues.
>>
>>>
>>> An alternative design could be:
>>>
>>> - memory_region_request_mmio_ptr returns a MemoryRegionCache instead of
>>> a pointer, so that the device can map a subset of the device (e.g. a
>>> single page)
>>
>> I'm not aware of this MemoryRegionCache yet, it seems pretty new.
>> I'll take a look.
>>
>> Thanks,
>> Fred
>>
>>>
>>> - memory_region_request_mmio_ptr and MemoryRegionOps.request_ptr accept
>>> a Notifier
>>>
>>> - the device adds the Notifier to a NotifierList.  Before invalidating,
>>> it invokes the Notifier and empties the NotifierList.
>>>
>>> - for the TLB case, the Notifier calls tlb_flush_page.
>>>
>>> I like the general idea though!
>>>
>>> Paolo
>>>
>>>> +    }
>>>> +}
>>
>>
>>

Re: [Qemu-devel] [RFC 0/5] execute code from mmio area

[Frederic Konrad]

On 02/04/2017 02:17 PM, Peter Maydell wrote:
> On 4 February 2017 at 12:52, Frederic Konrad <fred.konrad@greensocs.com> wrote:
>> Is that the case that we might get a Bad RAM address error or some such
>> if we are not on a page boundary (or too small as you say)?
>> I guess this is a limitation. Mapping on a page boundary shouldn't be
>> too much restrictive.
> 
> Yeah. I really ought to look more closely at what the flow of
> execution is here, because I think how it works right now
> is a bit weird and works as much by luck as by judgement
> (we can longjump out of the middle of translating code
> right back to the cpu-exec.c loop, and in some cases
> I think what happens is that we try to translate code,
> and as part of the "load didn't work" code path we
> nestedly try to translate the same thing again which
> of course fails again, only the second time around we
> realize and longjump out.
> 
> (At the moment for linux-user mode this is causing us to
> assert about taking the tb lock twice, because we hold
> the tb lock during translation and then try to grab it
> again to do the cpu_restore_state() in the signal handler.)
> 

Yes it seems there are some scary things happening there.

Fred

> thanks
> -- PMM
> 

Re: [Qemu-devel] [RFC 4/5] exec: allow to get a pointer for some mmio memory region

[Frederic Konrad]

On 02/04/2017 02:59 PM, Frederic Konrad wrote:
> On 02/04/2017 01:41 PM, Paolo Bonzini wrote:
>>
> ...
>>>
>>> Doesn't hotplug use dynamic MemoryRegion? In which case we better
>>> make that work with MTTCG. I wonder if we can't simply handle that
>>> with a safe_work for this case?
>>
>> Hot-unplug works because the backing memory is only freed when the
>> device gets instance_finalize, so at that point the region cannot have
>> any references.
>>
>> What can go wrong is the following (from the docs):
>>
>> - the memory region's owner had a reference taken via memory_region_ref
>>   (for example by address_space_map)
>>
>> - the region is unparented, and has no owner anymore
>>
>> - when address_space_unmap is called, the reference to the memory
>>   region's owner is leaked.
> 
> true.
> 
>>
>> In your case you have phys_section_add/phys_section_destroy instead of
>> address_space_map/unmap, but the problem is the same.
>>
>> I am a bit unsure about using the same lqspi_buf for caching different
>> addresses, and about using the same buffer for MMIO execution and read.
>> What if you allocate a different buffer every time
>> lqspi_request_mmio_ptr is called (adding a char* argument to
>> lqspi_load_cache) and free the old one from the safe_work item, after a
>> global TLB flush?  Then you don't need the Notifiers which were the
>> complicated and handwavy part of my proposal.
> 
> Actually this was just because it was the way xilinx_qspi worked.
> It load 1K of SPI data and fill the buffer.
> 
> In some other case we don't want to free the pointer at all, just make
> it not accessible for execution / read.
> (BTW I'll add the read-only property to this).
> 
> What about a simple Object eg: mmio-execution-interface which has a
> MemoryRegion? Instead of creating the MemoryRegion in request_pointer,
> We create a mmio-execution-interface object which create and map the
> region on the subregion.
> Then in invalidate: we unplug the mmio-execution-interface, unref it and
> it is freed all by magic when the map doesn't need it.
> 
> This avoid the reference issue and probably the bug with MTTCG.

Does that make sense?

Thanks,
Fred

> 
> Fred
> 
>>
>> Paolo
>>
>>>
>>> BTW the tests I have seems to pass without issues.
>>>
>>>>
>>>> An alternative design could be:
>>>>
>>>> - memory_region_request_mmio_ptr returns a MemoryRegionCache instead of
>>>> a pointer, so that the device can map a subset of the device (e.g. a
>>>> single page)
>>>
>>> I'm not aware of this MemoryRegionCache yet, it seems pretty new.
>>> I'll take a look.
>>>
>>> Thanks,
>>> Fred
>>>
>>>>
>>>> - memory_region_request_mmio_ptr and MemoryRegionOps.request_ptr accept
>>>> a Notifier
>>>>
>>>> - the device adds the Notifier to a NotifierList.  Before invalidating,
>>>> it invokes the Notifier and empties the NotifierList.
>>>>
>>>> - for the TLB case, the Notifier calls tlb_flush_page.
>>>>
>>>> I like the general idea though!
>>>>
>>>> Paolo
>>>>
>>>>> +    }
>>>>> +}
>>>
>>>
>>>
> 
>