[Qemu-devel] [PULL v2 0/7] MMIO Exec pull request

[Qemu-devel] [PULL v2 0/7] MMIO Exec pull request

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Hi,

Paolo suggested offline that we send a pull request for this series.
Here it is, I've run it through my testsuite + tested the LQSPI testcase
on Zynq.

Cheers,
Edgar

ChangeLog:

v1 -> v2:
* Add EI RB line to patch #6
* Remove changelogs from commit messages
* Add EI SoB lines to all patches


The following changes since commit 054914f6461ce9d3427af6527ef3e5b07311c86b:

  Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2017-06-26 15:38:29 +0100)

are available in the git repository at:

  git@github.com:edgarigl/qemu.git tags/edgar/mmio-exec-v2.for-upstream

for you to fetch changes up to 252b99baeb9e7e86cc0ece8efcaddebf11d284f5:

  xilinx_spips: allow mmio execution (2017-06-27 15:09:15 +0200)

----------------------------------------------------------------
edgar/mmio-exec-v2.for-upstream

----------------------------------------------------------------
KONRAD Frederic (7):
      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
      qdev: add MemoryRegion property
      introduce mmio_interface
      exec: allow to get a pointer for some mmio memory region
      xilinx_spips: allow mmio execution

 accel/tcg/cputlb.c               |  82 ++++++++++++++-----------
 hw/misc/Makefile.objs            |   1 +
 hw/misc/mmio_interface.c         | 128 +++++++++++++++++++++++++++++++++++++++
 hw/ssi/xilinx_spips.c            |  74 ++++++++++++++++------
 include/exec/memory.h            |  35 +++++++++++
 include/hw/misc/mmio_interface.h |  49 +++++++++++++++
 include/hw/qdev-properties.h     |   2 +
 memory.c                         | 111 +++++++++++++++++++++++++++++++++
 8 files changed, 428 insertions(+), 54 deletions(-)
 create mode 100644 hw/misc/mmio_interface.c
 create mode 100644 include/hw/misc/mmio_interface.h

[Qemu-devel] [PULL v2 1/7] cputlb: cleanup get_page_addr_code to use VICTIM_TLB_HIT

[Edgar E. Iglesias]

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.

Reviewed-by: Richard Henderson <rth@twiddle.net>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 accel/tcg/cputlb.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 743776a..1cc382d 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -751,21 +751,21 @@ static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
  * 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)) {
@@ -777,7 +777,7 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, target_ulong addr)
         report_bad_exec(cpu, 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);
 }
 
-- 
2.7.4

[Qemu-devel] [PULL v2 2/7] cputlb: move get_page_addr_code

[Edgar E. Iglesias]

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

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

Reviewed-by: Richard Henderson <rth@twiddle.net>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 accel/tcg/cputlb.c | 70 +++++++++++++++++++++++++++---------------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 1cc382d..5d6c755 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -746,41 +746,6 @@ static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
     return ram_addr;
 }
 
-/* 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)) {
-        cpu_unassigned_access(cpu, addr, false, true, 0, 4);
-        /* The CPU's unassigned access hook might have longjumped out
-         * with an exception. If it didn't (or there was no hook) then
-         * we can't proceed further.
-         */
-        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)
 {
@@ -868,6 +833,41 @@ 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)) {
+        cpu_unassigned_access(cpu, addr, false, true, 0, 4);
+        /* The CPU's unassigned access hook might have longjumped out
+         * with an exception. If it didn't (or there was no hook) then
+         * we can't proceed further.
+         */
+        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).
-- 
2.7.4

[Qemu-devel] [PULL v2 3/7] cputlb: fix the way get_page_addr_code fills the tlb

[Edgar E. Iglesias]

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.

Reviewed-by: Richard Henderson <rth@twiddle.net>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 accel/tcg/cputlb.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 5d6c755..95265a0 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -849,8 +849,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;
-- 
2.7.4

[Qemu-devel] [PULL v2 4/7] qdev: add MemoryRegion property

[Edgar E. Iglesias]

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

We need to pass a pointer to a MemoryRegion for mmio_interface.
So this just adds that.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 include/hw/qdev-properties.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 1e5c928..39bf4b2 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -177,6 +177,8 @@ extern PropertyInfo qdev_prop_arraylen;
     DEFINE_PROP_UNSIGNED(_n, _s, _f, 0, qdev_prop_blocksize, uint16_t)
 #define DEFINE_PROP_PCI_HOST_DEVADDR(_n, _s, _f) \
     DEFINE_PROP(_n, _s, _f, qdev_prop_pci_host_devaddr, PCIHostDeviceAddress)
+#define DEFINE_PROP_MEMORY_REGION(_n, _s, _f)             \
+    DEFINE_PROP(_n, _s, _f, qdev_prop_ptr, MemoryRegion *)
 
 #define DEFINE_PROP_END_OF_LIST()               \
     {}
-- 
2.7.4

[Qemu-devel] [PULL v2 5/7] introduce mmio_interface

[Edgar E. Iglesias]

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

This introduces mmio_interface object which contains a MemoryRegion
and can be hotplugged/hotunplugged.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 hw/misc/Makefile.objs            |   1 +
 hw/misc/mmio_interface.c         | 128 +++++++++++++++++++++++++++++++++++++++
 include/hw/misc/mmio_interface.h |  49 +++++++++++++++
 3 files changed, 178 insertions(+)
 create mode 100644 hw/misc/mmio_interface.c
 create mode 100644 include/hw/misc/mmio_interface.h

diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 2019846..08a79c3 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -57,3 +57,4 @@ obj-$(CONFIG_EDU) += edu.o
 obj-$(CONFIG_HYPERV_TESTDEV) += hyperv_testdev.o
 obj-$(CONFIG_AUX) += auxbus.o
 obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o aspeed_sdmc.o
+obj-y += mmio_interface.o
diff --git a/hw/misc/mmio_interface.c b/hw/misc/mmio_interface.c
new file mode 100644
index 0000000..6f004d2
--- /dev/null
+++ b/hw/misc/mmio_interface.c
@@ -0,0 +1,128 @@
+/*
+ * mmio_interface.c
+ *
+ *  Copyright (C) 2017 : GreenSocs
+ *      http://www.greensocs.com/ , email: info@greensocs.com
+ *
+ *  Developed by :
+ *  Frederic Konrad   <fred.konrad@greensocs.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option)any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "trace.h"
+#include "hw/qdev-properties.h"
+#include "hw/misc/mmio_interface.h"
+#include "qapi/error.h"
+
+#ifndef DEBUG_MMIO_INTERFACE
+#define DEBUG_MMIO_INTERFACE 0
+#endif
+
+static uint64_t mmio_interface_counter;
+
+#define DPRINTF(fmt, ...) do {                                                 \
+    if (DEBUG_MMIO_INTERFACE) {                                                \
+        qemu_log("mmio_interface: 0x%" PRIX64 ": " fmt, s->id, ## __VA_ARGS__);\
+    }                                                                          \
+} while (0);
+
+static void mmio_interface_init(Object *obj)
+{
+    MMIOInterface *s = MMIO_INTERFACE(obj);
+
+    if (DEBUG_MMIO_INTERFACE) {
+        s->id = mmio_interface_counter++;
+    }
+
+    DPRINTF("interface created\n");
+    s->host_ptr = 0;
+    s->subregion = 0;
+}
+
+static void mmio_interface_realize(DeviceState *dev, Error **errp)
+{
+    MMIOInterface *s = MMIO_INTERFACE(dev);
+
+    DPRINTF("realize from 0x%" PRIX64 " to 0x%" PRIX64 " map host pointer"
+            " %p\n", s->start, s->end, s->host_ptr);
+
+    if (!s->host_ptr) {
+        error_setg(errp, "host_ptr property must be set");
+    }
+
+    if (!s->subregion) {
+        error_setg(errp, "subregion property must be set");
+    }
+
+    memory_region_init_ram_ptr(&s->ram_mem, OBJECT(s), "ram",
+                               s->end - s->start + 1, s->host_ptr);
+    memory_region_set_readonly(&s->ram_mem, s->ro);
+    memory_region_add_subregion(s->subregion, s->start, &s->ram_mem);
+}
+
+static void mmio_interface_unrealize(DeviceState *dev, Error **errp)
+{
+    MMIOInterface *s = MMIO_INTERFACE(dev);
+
+    DPRINTF("unrealize from 0x%" PRIX64 " to 0x%" PRIX64 " map host pointer"
+            " %p\n", s->start, s->end, s->host_ptr);
+    memory_region_del_subregion(s->subregion, &s->ram_mem);
+}
+
+static void mmio_interface_finalize(Object *obj)
+{
+    MMIOInterface *s = MMIO_INTERFACE(obj);
+
+    DPRINTF("finalize from 0x%" PRIX64 " to 0x%" PRIX64 " map host pointer"
+            " %p\n", s->start, s->end, s->host_ptr);
+    object_unparent(OBJECT(&s->ram_mem));
+}
+
+static Property mmio_interface_properties[] = {
+    DEFINE_PROP_UINT64("start", MMIOInterface, start, 0),
+    DEFINE_PROP_UINT64("end", MMIOInterface, end, 0),
+    DEFINE_PROP_PTR("host_ptr", MMIOInterface, host_ptr),
+    DEFINE_PROP_BOOL("ro", MMIOInterface, ro, false),
+    DEFINE_PROP_MEMORY_REGION("subregion", MMIOInterface, subregion),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void mmio_interface_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = mmio_interface_realize;
+    dc->unrealize = mmio_interface_unrealize;
+    dc->props = mmio_interface_properties;
+}
+
+static const TypeInfo mmio_interface_info = {
+    .name          = TYPE_MMIO_INTERFACE,
+    .parent        = TYPE_DEVICE,
+    .instance_size = sizeof(MMIOInterface),
+    .instance_init = mmio_interface_init,
+    .instance_finalize = mmio_interface_finalize,
+    .class_init    = mmio_interface_class_init,
+};
+
+static void mmio_interface_register_types(void)
+{
+    type_register_static(&mmio_interface_info);
+}
+
+type_init(mmio_interface_register_types)
diff --git a/include/hw/misc/mmio_interface.h b/include/hw/misc/mmio_interface.h
new file mode 100644
index 0000000..90d34fb
--- /dev/null
+++ b/include/hw/misc/mmio_interface.h
@@ -0,0 +1,49 @@
+/*
+ * mmio_interface.h
+ *
+ *  Copyright (C) 2017 : GreenSocs
+ *      http://www.greensocs.com/ , email: info@greensocs.com
+ *
+ *  Developed by :
+ *  Frederic Konrad   <fred.konrad@greensocs.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option)any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#ifndef MMIO_INTERFACE_H
+#define MMIO_INTERFACE_H
+
+#include "exec/memory.h"
+
+#define TYPE_MMIO_INTERFACE "mmio_interface"
+#define MMIO_INTERFACE(obj) OBJECT_CHECK(MMIOInterface, (obj),                 \
+                                         TYPE_MMIO_INTERFACE)
+
+typedef struct MMIOInterface {
+    DeviceState parent_obj;
+
+    MemoryRegion *subregion;
+    MemoryRegion ram_mem;
+    uint64_t start;
+    uint64_t end;
+    bool ro;
+    uint64_t id;
+    void *host_ptr;
+} MMIOInterface;
+
+void mmio_interface_map(MMIOInterface *s);
+void mmio_interface_unmap(MMIOInterface *s);
+
+#endif /* MMIO_INTERFACE_H */
-- 
2.7.4

[Qemu-devel] [PULL v2 6/7] exec: allow to get a pointer for some mmio memory region

[Edgar E. Iglesias]

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.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 accel/tcg/cputlb.c    |  10 +++++
 include/exec/memory.h |  35 ++++++++++++++++
 memory.c              | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 156 insertions(+)

diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 95265a0..1900936 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -858,6 +858,16 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr)
     pd = iotlbentry->addr & ~TARGET_PAGE_MASK;
     mr = iotlb_to_region(cpu, pd, iotlbentry->attrs);
     if (memory_region_is_unassigned(mr)) {
+        qemu_mutex_lock_iothread();
+        if (memory_region_request_mmio_ptr(mr, addr)) {
+            qemu_mutex_unlock_iothread();
+            /* A MemoryRegion is potentially added so re-run the
+             * get_page_addr_code.
+             */
+            return get_page_addr_code(env, addr);
+        }
+        qemu_mutex_unlock_iothread();
+
         cpu_unassigned_access(cpu, addr, false, true, 0, 4);
         /* The CPU's unassigned access hook might have longjumped out
          * with an exception. If it didn't (or there was no hook) then
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 37f8e78..8503685 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -137,6 +137,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: */
@@ -1363,6 +1372,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 e08fa0a..1044bba 100644
--- a/memory.c
+++ b/memory.c
@@ -30,6 +30,8 @@
 #include "exec/ram_addr.h"
 #include "sysemu/kvm.h"
 #include "sysemu/sysemu.h"
+#include "hw/misc/mmio_interface.h"
+#include "hw/qdev-properties.h"
 
 //#define DEBUG_UNASSIGNED
 
@@ -2430,6 +2432,115 @@ void memory_listener_unregister(MemoryListener *listener)
     listener->address_space = NULL;
 }
 
+bool memory_region_request_mmio_ptr(MemoryRegion *mr, hwaddr addr)
+{
+    void *host;
+    unsigned size = 0;
+    unsigned offset = 0;
+    Object *new_interface;
+
+    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;
+    }
+
+    new_interface = object_new("mmio_interface");
+    qdev_prop_set_uint64(DEVICE(new_interface), "start", offset);
+    qdev_prop_set_uint64(DEVICE(new_interface), "end", offset + size - 1);
+    qdev_prop_set_bit(DEVICE(new_interface), "ro", true);
+    qdev_prop_set_ptr(DEVICE(new_interface), "host_ptr", host);
+    qdev_prop_set_ptr(DEVICE(new_interface), "subregion", mr);
+    object_property_set_bool(OBJECT(new_interface), true, "realized", NULL);
+
+    memory_region_transaction_commit();
+    return true;
+}
+
+typedef struct MMIOPtrInvalidate {
+    MemoryRegion *mr;
+    hwaddr offset;
+    unsigned size;
+    int busy;
+    int allocated;
+} MMIOPtrInvalidate;
+
+#define MAX_MMIO_INVALIDATE 10
+static MMIOPtrInvalidate mmio_ptr_invalidate_list[MAX_MMIO_INVALIDATE];
+
+static void memory_region_do_invalidate_mmio_ptr(CPUState *cpu,
+                                                 run_on_cpu_data data)
+{
+    MMIOPtrInvalidate *invalidate_data = (MMIOPtrInvalidate *)data.host_ptr;
+    MemoryRegion *mr = invalidate_data->mr;
+    hwaddr offset = invalidate_data->offset;
+    unsigned size = invalidate_data->size;
+    MemoryRegionSection section = memory_region_find(mr, offset, size);
+
+    qemu_mutex_lock_iothread();
+
+    /* Reset dirty so this doesn't happen later. */
+    cpu_physical_memory_test_and_clear_dirty(offset, size, 1);
+
+    if (section.mr != mr) {
+        /* memory_region_find add a ref on section.mr */
+        memory_region_unref(section.mr);
+        if (MMIO_INTERFACE(section.mr->owner)) {
+            /* We found the interface just drop it. */
+            object_property_set_bool(section.mr->owner, false, "realized",
+                                     NULL);
+            object_unref(section.mr->owner);
+            object_unparent(section.mr->owner);
+        }
+    }
+
+    qemu_mutex_unlock_iothread();
+
+    if (invalidate_data->allocated) {
+        g_free(invalidate_data);
+    } else {
+        invalidate_data->busy = 0;
+    }
+}
+
+void memory_region_invalidate_mmio_ptr(MemoryRegion *mr, hwaddr offset,
+                                       unsigned size)
+{
+    size_t i;
+    MMIOPtrInvalidate *invalidate_data = NULL;
+
+    for (i = 0; i < MAX_MMIO_INVALIDATE; i++) {
+        if (atomic_cmpxchg(&(mmio_ptr_invalidate_list[i].busy), 0, 1) == 0) {
+            invalidate_data = &mmio_ptr_invalidate_list[i];
+            break;
+        }
+    }
+
+    if (!invalidate_data) {
+        invalidate_data = g_malloc0(sizeof(MMIOPtrInvalidate));
+        invalidate_data->allocated = 1;
+    }
+
+    invalidate_data->mr = mr;
+    invalidate_data->offset = offset;
+    invalidate_data->size = size;
+
+    async_safe_run_on_cpu(first_cpu, memory_region_do_invalidate_mmio_ptr,
+                          RUN_ON_CPU_HOST_PTR(invalidate_data));
+}
+
 void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
 {
     memory_region_ref(root);
-- 
2.7.4

[Qemu-devel] [PULL v2 7/7] xilinx_spips: allow mmio execution

[Edgar E. Iglesias]

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.

Tested-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.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,
-- 
2.7.4

Re: [Qemu-devel] [PULL v2 0/7] MMIO Exec pull request

[Peter Maydell]

On 27 June 2017 at 16:37, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Hi,
>
> Paolo suggested offline that we send a pull request for this series.
> Here it is, I've run it through my testsuite + tested the LQSPI testcase
> on Zynq.
>
> Cheers,
> Edgar
>
> ChangeLog:
>
> v1 -> v2:
> * Add EI RB line to patch #6
> * Remove changelogs from commit messages
> * Add EI SoB lines to all patches
>
>
> The following changes since commit 054914f6461ce9d3427af6527ef3e5b07311c86b:
>
>   Merge remote-tracking branch 'remotes/kevin/tags/for-upstream' into staging (2017-06-26 15:38:29 +0100)
>
> are available in the git repository at:
>
>   git@github.com:edgarigl/qemu.git tags/edgar/mmio-exec-v2.for-upstream
>
> for you to fetch changes up to 252b99baeb9e7e86cc0ece8efcaddebf11d284f5:
>
>   xilinx_spips: allow mmio execution (2017-06-27 15:09:15 +0200)
>
> ----------------------------------------------------------------
> edgar/mmio-exec-v2.for-upstream
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL v2 6/7] exec: allow to get a pointer for some mmio memory region

[Peter Maydell]

On 27 June 2017 at 16:37, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> 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.

> +static void memory_region_do_invalidate_mmio_ptr(CPUState *cpu,
> +                                                 run_on_cpu_data data)
> +{
> +    MMIOPtrInvalidate *invalidate_data = (MMIOPtrInvalidate *)data.host_ptr;
> +    MemoryRegion *mr = invalidate_data->mr;
> +    hwaddr offset = invalidate_data->offset;
> +    unsigned size = invalidate_data->size;
> +    MemoryRegionSection section = memory_region_find(mr, offset, size);
> +
> +    qemu_mutex_lock_iothread();
> +
> +    /* Reset dirty so this doesn't happen later. */
> +    cpu_physical_memory_test_and_clear_dirty(offset, size, 1);
> +
> +    if (section.mr != mr) {
> +        /* memory_region_find add a ref on section.mr */
> +        memory_region_unref(section.mr);
> +        if (MMIO_INTERFACE(section.mr->owner)) {

Could somebody explain why it's OK to unref section.mr here before
we go on to do things with it, rather than only unrefing it after
we've finished using it?

Also, by my reading memory_region_find() will always ref
ret.mr (if it's not NULL), whereas this code only unrefs it
if section.mr == mr. Does this leak a reference in the case
where section.mr != mr, or am I missing something ?

> +            /* We found the interface just drop it. */
> +            object_property_set_bool(section.mr->owner, false, "realized",
> +                                     NULL);
> +            object_unref(section.mr->owner);
> +            object_unparent(section.mr->owner);
> +        }
> +    }
> +
> +    qemu_mutex_unlock_iothread();
> +
> +    if (invalidate_data->allocated) {
> +        g_free(invalidate_data);
> +    } else {
> +        invalidate_data->busy = 0;
> +    }
> +}

thanks
-- PMM

Re: [Qemu-devel] [PULL v2 6/7] exec: allow to get a pointer for some mmio memory region

[Paolo Bonzini]

On 10/08/2017 14:56, Peter Maydell wrote:
>> +    qemu_mutex_lock_iothread();
>> +
>> +    /* Reset dirty so this doesn't happen later. */
>> +    cpu_physical_memory_test_and_clear_dirty(offset, size, 1);
>> +
>> +    if (section.mr != mr) {
>> +        /* memory_region_find add a ref on section.mr */
>> +        memory_region_unref(section.mr);
>> +        if (MMIO_INTERFACE(section.mr->owner)) {
> 
> Could somebody explain why it's OK to unref section.mr here before
> we go on to do things with it, rather than only unrefing it after
> we've finished using it?

The memory region won't disappear until you release the BQL and/or
RCU-read-lock, but yes it's cleaner to move it later, and yes there is a
leak.

Paolo

> Also, by my reading memory_region_find() will always ref
> ret.mr (if it's not NULL), whereas this code only unrefs it
> if section.mr == mr. Does this leak a reference in the case
> where section.mr != mr, or am I missing something ?
> 

Re: [Qemu-devel] [PULL v2 6/7] exec: allow to get a pointer for some mmio memory region

[KONRAD Frederic]

On 08/10/2017 02:59 PM, Paolo Bonzini wrote:
> On 10/08/2017 14:56, Peter Maydell wrote:
>>> +    qemu_mutex_lock_iothread();
>>> +
>>> +    /* Reset dirty so this doesn't happen later. */
>>> +    cpu_physical_memory_test_and_clear_dirty(offset, size, 1);
>>> +
>>> +    if (section.mr != mr) {
>>> +        /* memory_region_find add a ref on section.mr */
>>> +        memory_region_unref(section.mr);
>>> +        if (MMIO_INTERFACE(section.mr->owner)) {
>>
>> Could somebody explain why it's OK to unref section.mr here before
>> we go on to do things with it, rather than only unrefing it after
>> we've finished using it?
> 
> The memory region won't disappear until you release the BQL and/or
> RCU-read-lock, but yes it's cleaner to move it later, and yes there is a
> leak.

I missed the case here where section.mr != mr but this shouldn't
happen. Either we make it acceptable and fix the leak.. or just
trigger an error as it is a bogus device. I'd rather go for the
first.
This is the same for memory_region_unref(section.mr).
memory_region_find must hit a MMIO_INTERFACE. In this case the
reference of MMIO_INTERFACE can't be zero here.

Thanks,
Fred

> 
> Paolo
> 
>> Also, by my reading memory_region_find() will always ref
>> ret.mr (if it's not NULL), whereas this code only unrefs it
>> if section.mr == mr. Does this leak a reference in the case
>> where section.mr != mr, or am I missing something ?
>>
>