[Qemu-devel] [PATCH 0/3] Drop obsolete memory system request_ptr API

[Qemu-devel] [PATCH 0/3] Drop obsolete memory system request_ptr API

[Peter Maydell]

Now that support has hit master for direct execution from
arbitrary MMIO regions, we can remove the MMIO request_ptr
API, which required special case support in each device that
wanted to handle it, and also had bad race conditions that
resulted in crashes if you tried to use it heavily.

This API was only ever used in one device in the source
tree, the Xilinx SPIPS. These three patches remove the
now-unneeded code from the Xilinx device and then the
core memory subsystem code.

thanks
-- PMM

Peter Maydell (3):
  hw/ssi/xilinx_spips: Remove unneeded MMIO request_ptr code
  memory: Remove MMIO request_ptr APIs
  hw/misc: Remove mmio_interface device

 hw/misc/Makefile.objs            |   1 -
 include/exec/memory.h            |  35 --------
 include/hw/misc/mmio_interface.h |  49 -----------
 hw/misc/mmio_interface.c         | 135 -------------------------------
 hw/ssi/xilinx_spips.c            |  46 -----------
 memory.c                         | 110 -------------------------
 6 files changed, 376 deletions(-)
 delete mode 100644 include/hw/misc/mmio_interface.h
 delete mode 100644 hw/misc/mmio_interface.c

-- 
2.18.0

[Qemu-devel] [PATCH 1/3] hw/ssi/xilinx_spips: Remove unneeded MMIO request_ptr code

[Peter Maydell]

We now support direct execution from MMIO regions in the
core memory subsystem. This means that we don't need to
have device-specific support for it, and we can remove
the request_ptr handling from the Xilinx SPIPS device.
(It was broken anyway due to race conditions, and disabled
by default.)

This device is the only in-tree user of this API.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/ssi/xilinx_spips.c | 46 -------------------------------------------
 1 file changed, 46 deletions(-)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index c052bfc4b3c..16f88f74029 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -1031,14 +1031,6 @@ static const MemoryRegionOps spips_ops = {
 
 static void xilinx_qspips_invalidate_mmio_ptr(XilinxQSPIPS *q)
 {
-    XilinxSPIPS *s = &q->parent_obj;
-
-    if ((q->mmio_execution_enabled) && (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;
 }
 
@@ -1207,23 +1199,6 @@ static void lqspi_load_cache(void *opaque, hwaddr addr)
     }
 }
 
-static void *lqspi_request_mmio_ptr(void *opaque, hwaddr addr, unsigned *size,
-                                    unsigned *offset)
-{
-    XilinxQSPIPS *q = opaque;
-    hwaddr offset_within_the_region;
-
-    if (!q->mmio_execution_enabled) {
-        return NULL;
-    }
-
-    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)
 {
@@ -1245,7 +1220,6 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
 
 static const MemoryRegionOps lqspi_ops = {
     .read = lqspi_read,
-    .request_ptr = lqspi_request_mmio_ptr,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .valid = {
         .min_access_size = 1,
@@ -1322,15 +1296,6 @@ static void xilinx_qspips_realize(DeviceState *dev, Error **errp)
     sysbus_init_mmio(sbd, &s->mmlqspi);
 
     q->lqspi_cached_addr = ~0ULL;
-
-    /* mmio_execution breaks migration better aborting than having strange
-     * bugs.
-     */
-    if (q->mmio_execution_enabled) {
-        error_setg(&q->migration_blocker,
-                   "enabling mmio_execution breaks migration");
-        migrate_add_blocker(q->migration_blocker, &error_fatal);
-    }
 }
 
 static void xlnx_zynqmp_qspips_realize(DeviceState *dev, Error **errp)
@@ -1427,16 +1392,6 @@ static Property xilinx_zynqmp_qspips_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
-static Property xilinx_qspips_properties[] = {
-    /* We had to turn this off for 2.10 as it is not compatible with migration.
-     * It can be enabled but will prevent the device to be migrated.
-     * This will go aways when a fix will be released.
-     */
-    DEFINE_PROP_BOOL("x-mmio-exec", XilinxQSPIPS, mmio_execution_enabled,
-                     false),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
 static Property xilinx_spips_properties[] = {
     DEFINE_PROP_UINT8("num-busses", XilinxSPIPS, num_busses, 1),
     DEFINE_PROP_UINT8("num-ss-bits", XilinxSPIPS, num_cs, 4),
@@ -1450,7 +1405,6 @@ static void xilinx_qspips_class_init(ObjectClass *klass, void * data)
     XilinxSPIPSClass *xsc = XILINX_SPIPS_CLASS(klass);
 
     dc->realize = xilinx_qspips_realize;
-    dc->props = xilinx_qspips_properties;
     xsc->reg_ops = &qspips_ops;
     xsc->rx_fifo_size = RXFF_A_Q;
     xsc->tx_fifo_size = TXFF_A_Q;
-- 
2.18.0

[Qemu-devel] [PATCH 2/3] memory: Remove MMIO request_ptr APIs

[Peter Maydell]

Remove the obsolete MMIO request_ptr APIs; they have no
users now.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/memory.h |  35 --------------
 memory.c              | 110 ------------------------------------------
 2 files changed, 145 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 448d41a7529..68636561821 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -141,15 +141,6 @@ 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: */
@@ -1667,32 +1658,6 @@ void memory_global_dirty_log_stop(void);
 void mtree_info(fprintf_function mon_printf, void *f, bool flatview,
                 bool dispatch_tree, bool owner);
 
-/**
- * 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.
- * @offset: offset within the memory 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 2ea16e7bfb0..8b44672c132 100644
--- a/memory.c
+++ b/memory.c
@@ -29,7 +29,6 @@
 #include "exec/ram_addr.h"
 #include "sysemu/kvm.h"
 #include "sysemu/sysemu.h"
-#include "hw/misc/mmio_interface.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 
@@ -2680,115 +2679,6 @@ 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.18.0

[Qemu-devel] [PATCH 3/3] hw/misc: Remove mmio_interface device

[Peter Maydell]

The mmio_interface device was a purely internal artifact
of the implementation of the memory subsystem's request_ptr
APIs. Now that we have removed those APIs, we can remove
the mmio_interface device too.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/misc/Makefile.objs            |   1 -
 include/hw/misc/mmio_interface.h |  49 -----------
 hw/misc/mmio_interface.c         | 135 -------------------------------
 3 files changed, 185 deletions(-)
 delete mode 100644 include/hw/misc/mmio_interface.h
 delete mode 100644 hw/misc/mmio_interface.c

diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 51d27b3af1e..22714b08510 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -71,5 +71,4 @@ obj-$(CONFIG_PVPANIC) += pvpanic.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
 obj-$(CONFIG_MSF2) += msf2-sysreg.o
diff --git a/include/hw/misc/mmio_interface.h b/include/hw/misc/mmio_interface.h
deleted file mode 100644
index 90d34fb2286..00000000000
--- a/include/hw/misc/mmio_interface.h
+++ /dev/null
@@ -1,49 +0,0 @@
-/*
- * 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 */
diff --git a/hw/misc/mmio_interface.c b/hw/misc/mmio_interface.c
deleted file mode 100644
index 3b0e2039a36..00000000000
--- a/hw/misc/mmio_interface.c
+++ /dev/null
@@ -1,135 +0,0 @@
-/*
- * 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");
-        return;
-    }
-
-    if (!s->subregion) {
-        error_setg(errp, "subregion property must be set");
-        return;
-    }
-
-    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;
-    /* Reason: pointer property "host_ptr", and this device
-     * is an implementation detail of the memory subsystem,
-     * not intended to be created directly by the user.
-     */
-    dc->user_creatable = false;
-}
-
-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)
-- 
2.18.0

Re: [Qemu-devel] [PATCH 0/3] Drop obsolete memory system request_ptr API

[Paolo Bonzini]

On 17/08/2018 13:46, Peter Maydell wrote:
> Now that support has hit master for direct execution from
> arbitrary MMIO regions, we can remove the MMIO request_ptr
> API, which required special case support in each device that
> wanted to handle it, and also had bad race conditions that
> resulted in crashes if you tried to use it heavily.
> 
> This API was only ever used in one device in the source
> tree, the Xilinx SPIPS. These three patches remove the
> now-unneeded code from the Xilinx device and then the
> core memory subsystem code.
> 
> thanks
> -- PMM
> 
> Peter Maydell (3):
>   hw/ssi/xilinx_spips: Remove unneeded MMIO request_ptr code
>   memory: Remove MMIO request_ptr APIs
>   hw/misc: Remove mmio_interface device
> 
>  hw/misc/Makefile.objs            |   1 -
>  include/exec/memory.h            |  35 --------
>  include/hw/misc/mmio_interface.h |  49 -----------
>  hw/misc/mmio_interface.c         | 135 -------------------------------
>  hw/ssi/xilinx_spips.c            |  46 -----------
>  memory.c                         | 110 -------------------------
>  6 files changed, 376 deletions(-)
>  delete mode 100644 include/hw/misc/mmio_interface.h
>  delete mode 100644 hw/misc/mmio_interface.c
> 

The diffstat speaks for itself, go ahead and include it via the ARM tree.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 0/3] Drop obsolete memory system request_ptr API

[Edgar E. Iglesias]

On Fri, Aug 17, 2018 at 12:46:16PM +0100, Peter Maydell wrote:
> Now that support has hit master for direct execution from
> arbitrary MMIO regions, we can remove the MMIO request_ptr
> API, which required special case support in each device that
> wanted to handle it, and also had bad race conditions that
> resulted in crashes if you tried to use it heavily.
> 
> This API was only ever used in one device in the source
> tree, the Xilinx SPIPS. These three patches remove the
> now-unneeded code from the Xilinx device and then the
> core memory subsystem code.


Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> thanks
> -- PMM
> 
> Peter Maydell (3):
>   hw/ssi/xilinx_spips: Remove unneeded MMIO request_ptr code
>   memory: Remove MMIO request_ptr APIs
>   hw/misc: Remove mmio_interface device
> 
>  hw/misc/Makefile.objs            |   1 -
>  include/exec/memory.h            |  35 --------
>  include/hw/misc/mmio_interface.h |  49 -----------
>  hw/misc/mmio_interface.c         | 135 -------------------------------
>  hw/ssi/xilinx_spips.c            |  46 -----------
>  memory.c                         | 110 -------------------------
>  6 files changed, 376 deletions(-)
>  delete mode 100644 include/hw/misc/mmio_interface.h
>  delete mode 100644 hw/misc/mmio_interface.c
> 
> -- 
> 2.18.0
> 

Re: [Qemu-devel] [PATCH 1/3] hw/ssi/xilinx_spips: Remove unneeded MMIO request_ptr code

[Alistair Francis]

On Fri, Aug 17, 2018 at 4:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> We now support direct execution from MMIO regions in the
> core memory subsystem. This means that we don't need to
> have device-specific support for it, and we can remove
> the request_ptr handling from the Xilinx SPIPS device.
> (It was broken anyway due to race conditions, and disabled
> by default.)
>
> This device is the only in-tree user of this API.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/ssi/xilinx_spips.c | 46 -------------------------------------------
>  1 file changed, 46 deletions(-)
>
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index c052bfc4b3c..16f88f74029 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -1031,14 +1031,6 @@ static const MemoryRegionOps spips_ops = {
>
>  static void xilinx_qspips_invalidate_mmio_ptr(XilinxQSPIPS *q)
>  {
> -    XilinxSPIPS *s = &q->parent_obj;
> -
> -    if ((q->mmio_execution_enabled) && (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;
>  }
>
> @@ -1207,23 +1199,6 @@ static void lqspi_load_cache(void *opaque, hwaddr addr)
>      }
>  }
>
> -static void *lqspi_request_mmio_ptr(void *opaque, hwaddr addr, unsigned *size,
> -                                    unsigned *offset)
> -{
> -    XilinxQSPIPS *q = opaque;
> -    hwaddr offset_within_the_region;
> -
> -    if (!q->mmio_execution_enabled) {
> -        return NULL;
> -    }
> -
> -    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)
>  {
> @@ -1245,7 +1220,6 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
>
>  static const MemoryRegionOps lqspi_ops = {
>      .read = lqspi_read,
> -    .request_ptr = lqspi_request_mmio_ptr,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>      .valid = {
>          .min_access_size = 1,
> @@ -1322,15 +1296,6 @@ static void xilinx_qspips_realize(DeviceState *dev, Error **errp)
>      sysbus_init_mmio(sbd, &s->mmlqspi);
>
>      q->lqspi_cached_addr = ~0ULL;
> -
> -    /* mmio_execution breaks migration better aborting than having strange
> -     * bugs.
> -     */
> -    if (q->mmio_execution_enabled) {
> -        error_setg(&q->migration_blocker,
> -                   "enabling mmio_execution breaks migration");
> -        migrate_add_blocker(q->migration_blocker, &error_fatal);
> -    }
>  }
>
>  static void xlnx_zynqmp_qspips_realize(DeviceState *dev, Error **errp)
> @@ -1427,16 +1392,6 @@ static Property xilinx_zynqmp_qspips_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> -static Property xilinx_qspips_properties[] = {
> -    /* We had to turn this off for 2.10 as it is not compatible with migration.
> -     * It can be enabled but will prevent the device to be migrated.
> -     * This will go aways when a fix will be released.
> -     */
> -    DEFINE_PROP_BOOL("x-mmio-exec", XilinxQSPIPS, mmio_execution_enabled,
> -                     false),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
>  static Property xilinx_spips_properties[] = {
>      DEFINE_PROP_UINT8("num-busses", XilinxSPIPS, num_busses, 1),
>      DEFINE_PROP_UINT8("num-ss-bits", XilinxSPIPS, num_cs, 4),
> @@ -1450,7 +1405,6 @@ static void xilinx_qspips_class_init(ObjectClass *klass, void * data)
>      XilinxSPIPSClass *xsc = XILINX_SPIPS_CLASS(klass);
>
>      dc->realize = xilinx_qspips_realize;
> -    dc->props = xilinx_qspips_properties;
>      xsc->reg_ops = &qspips_ops;
>      xsc->rx_fifo_size = RXFF_A_Q;
>      xsc->tx_fifo_size = TXFF_A_Q;
> --
> 2.18.0
>
>

Re: [Qemu-devel] [PATCH 2/3] memory: Remove MMIO request_ptr APIs

[Alistair Francis]

On Fri, Aug 17, 2018 at 4:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Remove the obsolete MMIO request_ptr APIs; they have no
> users now.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/exec/memory.h |  35 --------------
>  memory.c              | 110 ------------------------------------------
>  2 files changed, 145 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 448d41a7529..68636561821 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -141,15 +141,6 @@ 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: */
> @@ -1667,32 +1658,6 @@ void memory_global_dirty_log_stop(void);
>  void mtree_info(fprintf_function mon_printf, void *f, bool flatview,
>                  bool dispatch_tree, bool owner);
>
> -/**
> - * 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.
> - * @offset: offset within the memory 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 2ea16e7bfb0..8b44672c132 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -29,7 +29,6 @@
>  #include "exec/ram_addr.h"
>  #include "sysemu/kvm.h"
>  #include "sysemu/sysemu.h"
> -#include "hw/misc/mmio_interface.h"
>  #include "hw/qdev-properties.h"
>  #include "migration/vmstate.h"
>
> @@ -2680,115 +2679,6 @@ 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.18.0
>
>

Re: [Qemu-devel] [PATCH 3/3] hw/misc: Remove mmio_interface device

[Alistair Francis]

On Fri, Aug 17, 2018 at 4:46 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> The mmio_interface device was a purely internal artifact
> of the implementation of the memory subsystem's request_ptr
> APIs. Now that we have removed those APIs, we can remove
> the mmio_interface device too.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/misc/Makefile.objs            |   1 -
>  include/hw/misc/mmio_interface.h |  49 -----------
>  hw/misc/mmio_interface.c         | 135 -------------------------------
>  3 files changed, 185 deletions(-)
>  delete mode 100644 include/hw/misc/mmio_interface.h
>  delete mode 100644 hw/misc/mmio_interface.c
>
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 51d27b3af1e..22714b08510 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -71,5 +71,4 @@ obj-$(CONFIG_PVPANIC) += pvpanic.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
>  obj-$(CONFIG_MSF2) += msf2-sysreg.o
> diff --git a/include/hw/misc/mmio_interface.h b/include/hw/misc/mmio_interface.h
> deleted file mode 100644
> index 90d34fb2286..00000000000
> --- a/include/hw/misc/mmio_interface.h
> +++ /dev/null
> @@ -1,49 +0,0 @@
> -/*
> - * 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 */
> diff --git a/hw/misc/mmio_interface.c b/hw/misc/mmio_interface.c
> deleted file mode 100644
> index 3b0e2039a36..00000000000
> --- a/hw/misc/mmio_interface.c
> +++ /dev/null
> @@ -1,135 +0,0 @@
> -/*
> - * 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");
> -        return;
> -    }
> -
> -    if (!s->subregion) {
> -        error_setg(errp, "subregion property must be set");
> -        return;
> -    }
> -
> -    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;
> -    /* Reason: pointer property "host_ptr", and this device
> -     * is an implementation detail of the memory subsystem,
> -     * not intended to be created directly by the user.
> -     */
> -    dc->user_creatable = false;
> -}
> -
> -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)
> --
> 2.18.0
>
>

Re: [Qemu-devel] [PATCH 1/3] hw/ssi/xilinx_spips: Remove unneeded MMIO request_ptr code

[KONRAD Frederic]

Le 08/17/2018 à 01:46 PM, Peter Maydell a écrit :
> We now support direct execution from MMIO regions in the
> core memory subsystem. This means that we don't need to
> have device-specific support for it, and we can remove
> the request_ptr handling from the Xilinx SPIPS device.
> (It was broken anyway due to race conditions, and disabled
> by default.)
> 
> This device is the only in-tree user of this API.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/ssi/xilinx_spips.c | 46 -------------------------------------------
>   1 file changed, 46 deletions(-)
> 
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index c052bfc4b3c..16f88f74029 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -1031,14 +1031,6 @@ static const MemoryRegionOps spips_ops = {
>   
>   static void xilinx_qspips_invalidate_mmio_ptr(XilinxQSPIPS *q)

I'd drop this function and replace the calls by
q->lqspi_cached_addr = ~0ULL;

Otherwise:

Reviewed-by: KONRAD Frederic <frederic.konrad@adacore.com>

>   {
> -    XilinxSPIPS *s = &q->parent_obj;
> -
> -    if ((q->mmio_execution_enabled) && (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;
>   }
>   
> @@ -1207,23 +1199,6 @@ static void lqspi_load_cache(void *opaque, hwaddr addr)
>       }
>   }
>   
> -static void *lqspi_request_mmio_ptr(void *opaque, hwaddr addr, unsigned *size,
> -                                    unsigned *offset)
> -{
> -    XilinxQSPIPS *q = opaque;
> -    hwaddr offset_within_the_region;
> -
> -    if (!q->mmio_execution_enabled) {
> -        return NULL;
> -    }
> -
> -    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)
>   {
> @@ -1245,7 +1220,6 @@ lqspi_read(void *opaque, hwaddr addr, unsigned int size)
>   
>   static const MemoryRegionOps lqspi_ops = {
>       .read = lqspi_read,
> -    .request_ptr = lqspi_request_mmio_ptr,
>       .endianness = DEVICE_NATIVE_ENDIAN,
>       .valid = {
>           .min_access_size = 1,
> @@ -1322,15 +1296,6 @@ static void xilinx_qspips_realize(DeviceState *dev, Error **errp)
>       sysbus_init_mmio(sbd, &s->mmlqspi);
>   
>       q->lqspi_cached_addr = ~0ULL;
> -
> -    /* mmio_execution breaks migration better aborting than having strange
> -     * bugs.
> -     */
> -    if (q->mmio_execution_enabled) {
> -        error_setg(&q->migration_blocker,
> -                   "enabling mmio_execution breaks migration");
> -        migrate_add_blocker(q->migration_blocker, &error_fatal);
> -    }
>   }
>   
>   static void xlnx_zynqmp_qspips_realize(DeviceState *dev, Error **errp)
> @@ -1427,16 +1392,6 @@ static Property xilinx_zynqmp_qspips_properties[] = {
>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> -static Property xilinx_qspips_properties[] = {
> -    /* We had to turn this off for 2.10 as it is not compatible with migration.
> -     * It can be enabled but will prevent the device to be migrated.
> -     * This will go aways when a fix will be released.
> -     */
> -    DEFINE_PROP_BOOL("x-mmio-exec", XilinxQSPIPS, mmio_execution_enabled,
> -                     false),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
>   static Property xilinx_spips_properties[] = {
>       DEFINE_PROP_UINT8("num-busses", XilinxSPIPS, num_busses, 1),
>       DEFINE_PROP_UINT8("num-ss-bits", XilinxSPIPS, num_cs, 4),
> @@ -1450,7 +1405,6 @@ static void xilinx_qspips_class_init(ObjectClass *klass, void * data)
>       XilinxSPIPSClass *xsc = XILINX_SPIPS_CLASS(klass);
>   
>       dc->realize = xilinx_qspips_realize;
> -    dc->props = xilinx_qspips_properties;
>       xsc->reg_ops = &qspips_ops;
>       xsc->rx_fifo_size = RXFF_A_Q;
>       xsc->tx_fifo_size = TXFF_A_Q;
> 

Re: [Qemu-devel] [PATCH 2/3] memory: Remove MMIO request_ptr APIs

[KONRAD Frederic]

Le 08/17/2018 à 01:46 PM, Peter Maydell a écrit :
> Remove the obsolete MMIO request_ptr APIs; they have no
> users now.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: KONRAD Frederic <frederic.konrad@adacore.com>

> ---
>   include/exec/memory.h |  35 --------------
>   memory.c              | 110 ------------------------------------------
>   2 files changed, 145 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 448d41a7529..68636561821 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -141,15 +141,6 @@ 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: */
> @@ -1667,32 +1658,6 @@ void memory_global_dirty_log_stop(void);
>   void mtree_info(fprintf_function mon_printf, void *f, bool flatview,
>                   bool dispatch_tree, bool owner);
>   
> -/**
> - * 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.
> - * @offset: offset within the memory 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 2ea16e7bfb0..8b44672c132 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -29,7 +29,6 @@
>   #include "exec/ram_addr.h"
>   #include "sysemu/kvm.h"
>   #include "sysemu/sysemu.h"
> -#include "hw/misc/mmio_interface.h"
>   #include "hw/qdev-properties.h"
>   #include "migration/vmstate.h"
>   
> @@ -2680,115 +2679,6 @@ 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);
> 

Re: [Qemu-devel] [PATCH 3/3] hw/misc: Remove mmio_interface device

[KONRAD Frederic]

Le 08/17/2018 à 01:46 PM, Peter Maydell a écrit :
> The mmio_interface device was a purely internal artifact
> of the implementation of the memory subsystem's request_ptr
> APIs. Now that we have removed those APIs, we can remove
> the mmio_interface device too.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: KONRAD Frederic <frederic.konrad@adacore.com>
> ---
>   hw/misc/Makefile.objs            |   1 -
>   include/hw/misc/mmio_interface.h |  49 -----------
>   hw/misc/mmio_interface.c         | 135 -------------------------------
>   3 files changed, 185 deletions(-)
>   delete mode 100644 include/hw/misc/mmio_interface.h
>   delete mode 100644 hw/misc/mmio_interface.c
> 
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 51d27b3af1e..22714b08510 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -71,5 +71,4 @@ obj-$(CONFIG_PVPANIC) += pvpanic.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
>   obj-$(CONFIG_MSF2) += msf2-sysreg.o
> diff --git a/include/hw/misc/mmio_interface.h b/include/hw/misc/mmio_interface.h
> deleted file mode 100644
> index 90d34fb2286..00000000000
> --- a/include/hw/misc/mmio_interface.h
> +++ /dev/null
> @@ -1,49 +0,0 @@
> -/*
> - * 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 */
> diff --git a/hw/misc/mmio_interface.c b/hw/misc/mmio_interface.c
> deleted file mode 100644
> index 3b0e2039a36..00000000000
> --- a/hw/misc/mmio_interface.c
> +++ /dev/null
> @@ -1,135 +0,0 @@
> -/*
> - * 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");
> -        return;
> -    }
> -
> -    if (!s->subregion) {
> -        error_setg(errp, "subregion property must be set");
> -        return;
> -    }
> -
> -    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;
> -    /* Reason: pointer property "host_ptr", and this device
> -     * is an implementation detail of the memory subsystem,
> -     * not intended to be created directly by the user.
> -     */
> -    dc->user_creatable = false;
> -}
> -
> -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)
> 

Re: [Qemu-devel] [PATCH 1/3] hw/ssi/xilinx_spips: Remove unneeded MMIO request_ptr code

[Peter Maydell]

On 20 August 2018 at 09:49, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
>
>
> Le 08/17/2018 à 01:46 PM, Peter Maydell a écrit :
>>
>> We now support direct execution from MMIO regions in the
>> core memory subsystem. This means that we don't need to
>> have device-specific support for it, and we can remove
>> the request_ptr handling from the Xilinx SPIPS device.
>> (It was broken anyway due to race conditions, and disabled
>> by default.)
>>
>> This device is the only in-tree user of this API.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>   hw/ssi/xilinx_spips.c | 46 -------------------------------------------
>>   1 file changed, 46 deletions(-)
>>
>> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
>> index c052bfc4b3c..16f88f74029 100644
>> --- a/hw/ssi/xilinx_spips.c
>> +++ b/hw/ssi/xilinx_spips.c
>> @@ -1031,14 +1031,6 @@ static const MemoryRegionOps spips_ops = {
>>     static void xilinx_qspips_invalidate_mmio_ptr(XilinxQSPIPS *q)
>
>
> I'd drop this function and replace the calls by
> q->lqspi_cached_addr = ~0ULL;

I thought about that but decided it was reasonable to keep
the function, because it matches with having a
lqspi_load_cache() function still.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/3] hw/ssi/xilinx_spips: Remove unneeded MMIO request_ptr code

[KONRAD Frederic]

Le 08/20/2018 à 10:53 AM, Peter Maydell a écrit :
> On 20 August 2018 at 09:49, KONRAD Frederic <frederic.konrad@adacore.com> wrote:
>>
>>
>> Le 08/17/2018 à 01:46 PM, Peter Maydell a écrit :
>>>
>>> We now support direct execution from MMIO regions in the
>>> core memory subsystem. This means that we don't need to
>>> have device-specific support for it, and we can remove
>>> the request_ptr handling from the Xilinx SPIPS device.
>>> (It was broken anyway due to race conditions, and disabled
>>> by default.)
>>>
>>> This device is the only in-tree user of this API.
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>>    hw/ssi/xilinx_spips.c | 46 -------------------------------------------
>>>    1 file changed, 46 deletions(-)
>>>
>>> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
>>> index c052bfc4b3c..16f88f74029 100644
>>> --- a/hw/ssi/xilinx_spips.c
>>> +++ b/hw/ssi/xilinx_spips.c
>>> @@ -1031,14 +1031,6 @@ static const MemoryRegionOps spips_ops = {
>>>      static void xilinx_qspips_invalidate_mmio_ptr(XilinxQSPIPS *q)
>>
>>
>> I'd drop this function and replace the calls by
>> q->lqspi_cached_addr = ~0ULL;
> 
> I thought about that but decided it was reasonable to keep
> the function, because it matches with having a
> lqspi_load_cache() function still.
> 

Right ok.

Thanks,
Fred

> thanks
> -- PMM
>