[Qemu-devel] [patch v5 0/8] push mmio dispatch out of big lock

[Qemu-devel] [patch v5 0/8] push mmio dispatch out of big lock

[Liu Ping Fan]

v1:
https://lists.gnu.org/archive/html/qemu-devel/2012-07/msg03312.html

v2:
http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg01275.html

v3:
http://lists.nongnu.org/archive/html/qemu-devel/2012-09/msg01474.html

v4:
http://lists.gnu.org/archive/html/qemu-devel/2012-10/msg03857.html

changes v4->v5:
  Peel away the example of e1000.
  Rebase to Avi's patch "Integrate DMA into the memory API"



Liu Ping Fan (8):
  atomic: introduce atomic operations
  qom: apply atomic on object's refcount
  hotplug: introduce qdev_unplug_complete() to remove device from views
  pci: remove pci device from mem view when unplug
  memory: introduce local lock for address space
  memory: make mmio dispatch able to be out of biglock
  memory: introduce tls context to record nested dma
  vcpu: push mmio dispatcher out of big lock

 cpus.c                |    3 +
 docs/memory.txt       |    4 +
 exec.c                |  228 +++++++++++++++++++++++++++++++++++++++++++++----
 hw/acpi_piix4.c       |    2 +-
 hw/pci.c              |   16 +++-
 hw/pci.h              |    1 +
 hw/qdev.c             |   26 ++++++
 hw/qdev.h             |    3 +-
 include/qemu/atomic.h |   63 ++++++++++++++
 include/qemu/object.h |    3 +-
 kvm-all.c             |    4 +
 memory-internal.h     |    1 +
 memory.c              |   11 ++-
 memory.h              |    7 ++-
 qemu-thread.h         |    8 ++
 qom/object.c          |   11 +--
 vl.c                  |    1 +
 17 files changed, 362 insertions(+), 30 deletions(-)
 create mode 100644 include/qemu/atomic.h

-- 
1.7.4.4

[Qemu-devel] [patch v5 1/8] atomic: introduce atomic operations

[Liu Ping Fan]

If out of global lock, we will be challenged by SMP in low level,
so need atomic ops.

This file is a wrapper of GCC atomic builtin.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 include/qemu/atomic.h |   63 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 63 insertions(+), 0 deletions(-)
 create mode 100644 include/qemu/atomic.h

diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
new file mode 100644
index 0000000..a9e6d35
--- /dev/null
+++ b/include/qemu/atomic.h
@@ -0,0 +1,63 @@
+/*
+ * Simple wrapper of gcc Atomic-Builtins
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+#ifndef __QEMU_ATOMIC_H
+#define __QEMU_ATOMIC_H
+
+typedef struct Atomic {
+    volatile int counter;
+} Atomic;
+
+static inline void atomic_set(Atomic *v, int i)
+{
+    v->counter = i;
+}
+
+static inline int atomic_read(Atomic *v)
+{
+    return v->counter;
+}
+
+static inline int atomic_return_and_add(int i, Atomic *v)
+{
+    int ret;
+
+    ret = __sync_fetch_and_add(&v->counter, i);
+    return ret;
+}
+
+static inline int atomic_return_and_sub(int i, Atomic *v)
+{
+    int ret;
+
+    ret = __sync_fetch_and_sub(&v->counter, i);
+    return ret;
+}
+
+/**
+ *  * atomic_inc - increment atomic variable
+ *   * @v: pointer of type Atomic
+ *    *
+ *     * Atomically increments @v by 1.
+ *      */
+static inline void atomic_inc(Atomic *v)
+{
+    __sync_fetch_and_add(&v->counter, 1);
+}
+
+/**
+ *  * atomic_dec - decrement atomic variable
+ *   * @v: pointer of type Atomic
+ *    *
+ *     * Atomically decrements @v by 1.
+ *      */
+static inline void atomic_dec(Atomic *v)
+{
+    __sync_fetch_and_sub(&v->counter, 1);
+}
+
+#endif
-- 
1.7.4.4

[Qemu-devel] [patch v5 2/8] qom: apply atomic on object's refcount

[Liu Ping Fan]

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 include/qemu/object.h |    3 ++-
 qom/object.c          |   11 +++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index cc75fee..0c02614 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -18,6 +18,7 @@
 #include <stdint.h>
 #include <stdbool.h>
 #include "qemu-queue.h"
+#include "qemu/atomic.h"
 
 struct Visitor;
 struct Error;
@@ -262,7 +263,7 @@ struct Object
     /*< private >*/
     ObjectClass *class;
     QTAILQ_HEAD(, ObjectProperty) properties;
-    uint32_t ref;
+    Atomic ref;
     Object *parent;
 };
 
diff --git a/qom/object.c b/qom/object.c
index e3e9242..34ec2a1 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -383,7 +383,7 @@ void object_finalize(void *data)
     object_deinit(obj, ti);
     object_property_del_all(obj);
 
-    g_assert(obj->ref == 0);
+    g_assert(atomic_read(&obj->ref) == 0);
 }
 
 Object *object_new_with_type(Type type)
@@ -410,7 +410,7 @@ Object *object_new(const char *typename)
 void object_delete(Object *obj)
 {
     object_unparent(obj);
-    g_assert(obj->ref == 1);
+    g_assert(atomic_read(&obj->ref) == 1);
     object_unref(obj);
     g_free(obj);
 }
@@ -600,16 +600,15 @@ GSList *object_class_get_list(const char *implements_type,
 
 void object_ref(Object *obj)
 {
-    obj->ref++;
+    atomic_inc(&obj->ref);
 }
 
 void object_unref(Object *obj)
 {
-    g_assert(obj->ref > 0);
-    obj->ref--;
+    g_assert(atomic_read(&obj->ref) > 0);
 
     /* parent always holds a reference to its children */
-    if (obj->ref == 0) {
+    if (atomic_return_and_sub(1, &obj->ref) == 1) {
         object_finalize(obj);
     }
 }
-- 
1.7.4.4

[Qemu-devel] [patch v5 3/8] hotplug: introduce qdev_unplug_complete() to remove device from views

[Liu Ping Fan]

When device unplug has been ack by guest, we first remove it from memory
to prevent incoming access from dispatcher. Then we isolate it from
device composition tree

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/qdev.c |   26 ++++++++++++++++++++++++++
 hw/qdev.h |    3 ++-
 2 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 9b9aba3..681e133 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -98,6 +98,14 @@ void qdev_set_parent_bus(DeviceState *dev, BusState *bus)
     bus_add_child(bus, dev);
 }
 
+static void qdev_unset_parent(DeviceState *dev)
+{
+    BusState *b = dev->parent_bus;
+
+    object_unparent(OBJECT(dev));
+    bus_remove_child(b, dev);
+}
+
 /* Create a new device.  This only initializes the device state structure
    and allows properties to be set.  qdev_init should be called to
    initialize the actual device emulation.  */
@@ -187,6 +195,24 @@ void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
     dev->alias_required_for_version = required_for_version;
 }
 
+static int qdev_unmap(DeviceState *dev)
+{
+    DeviceClass *dc =  DEVICE_GET_CLASS(dev);
+    if (dc->unmap) {
+        dc->unmap(dev);
+    }
+    return 0;
+}
+
+void qdev_unplug_complete(DeviceState *dev, Error **errp)
+{
+    /* isolate from mem view */
+    qdev_unmap(dev);
+    /* isolate from device tree */
+    qdev_unset_parent(dev);
+    object_unref(OBJECT(dev));
+}
+
 void qdev_unplug(DeviceState *dev, Error **errp)
 {
     DeviceClass *dc = DEVICE_GET_CLASS(dev);
diff --git a/hw/qdev.h b/hw/qdev.h
index c6ac636..71eb9ca 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -47,7 +47,7 @@ typedef struct DeviceClass {
 
     /* callbacks */
     void (*reset)(DeviceState *dev);
-
+    void (*unmap)(DeviceState *dev);
     /* device state */
     const VMStateDescription *vmsd;
 
@@ -160,6 +160,7 @@ void qdev_init_nofail(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
 void qdev_unplug(DeviceState *dev, Error **errp);
+void qdev_unplug_complete(DeviceState *dev, Error **errp);
 void qdev_free(DeviceState *dev);
 int qdev_simple_unplug_cb(DeviceState *dev);
 void qdev_machine_creation_done(void);
-- 
1.7.4.4

[Qemu-devel] [patch v5 4/8] pci: remove pci device from mem view when unplug

[Liu Ping Fan]

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 hw/acpi_piix4.c |    2 +-
 hw/pci.c        |   13 ++++++++++++-
 hw/pci.h        |    1 +
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 15275cf..b45a016 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -306,7 +306,7 @@ static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots)
             if (pc->no_hotplug) {
                 slot_free = false;
             } else {
-                qdev_free(qdev);
+                qdev_unplug_complete(qdev, NULL);
             }
         }
     }
diff --git a/hw/pci.c b/hw/pci.c
index 7eeaac0..9ba589e 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -869,7 +869,6 @@ static int pci_unregister_device(DeviceState *dev)
     PCIDevice *pci_dev = PCI_DEVICE(dev);
     PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
 
-    pci_unregister_io_regions(pci_dev);
     pci_del_option_rom(pci_dev);
 
     if (pc->exit) {
@@ -880,6 +879,17 @@ static int pci_unregister_device(DeviceState *dev)
     return 0;
 }
 
+static void pci_unmap_device(DeviceState *dev)
+{
+    PCIDevice *pci_dev = PCI_DEVICE(dev);
+    PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
+
+    pci_unregister_io_regions(pci_dev);
+    if (pc->unmap) {
+        pc->unmap(pci_dev);
+    }
+}
+
 void pci_register_bar(PCIDevice *pci_dev, int region_num,
                       uint8_t type, MemoryRegion *memory)
 {
@@ -2105,6 +2115,7 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
     DeviceClass *k = DEVICE_CLASS(klass);
     k->init = pci_qdev_init;
     k->unplug = pci_unplug_device;
+    k->unmap = pci_unmap_device;
     k->exit = pci_unregister_device;
     k->bus_type = TYPE_PCI_BUS;
     k->props = pci_props;
diff --git a/hw/pci.h b/hw/pci.h
index 1f902f5..898cc5e 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -154,6 +154,7 @@ typedef struct PCIDeviceClass {
     DeviceClass parent_class;
 
     int (*init)(PCIDevice *dev);
+    void (*unmap)(PCIDevice *dev);
     PCIUnregisterFunc *exit;
     PCIConfigReadFunc *config_read;
     PCIConfigWriteFunc *config_write;
-- 
1.7.4.4

[Qemu-devel] [patch v5 5/8] memory: introduce local lock for address space

[Liu Ping Fan]

For those address spaces which want to be able out of big lock, they
will be protected by their own local.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 memory.c |   11 ++++++++++-
 memory.h |    5 ++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/memory.c b/memory.c
index 2f68d67..ff34aed 100644
--- a/memory.c
+++ b/memory.c
@@ -1532,9 +1532,15 @@ void memory_listener_unregister(MemoryListener *listener)
     QTAILQ_REMOVE(&memory_listeners, listener, link);
 }
 
-void address_space_init(AddressSpace *as, MemoryRegion *root)
+void address_space_init(AddressSpace *as, MemoryRegion *root, bool lock)
 {
     memory_region_transaction_begin();
+    if (lock) {
+        as->lock = g_new(QemuMutex, 1);
+        qemu_mutex_init(as->lock);
+    } else {
+        as->lock = NULL;
+    }
     as->root = root;
     as->current_map = g_new(FlatView, 1);
     flatview_init(as->current_map);
@@ -1553,6 +1559,9 @@ void address_space_destroy(AddressSpace *as)
     QTAILQ_REMOVE(&address_spaces, as, address_spaces_link);
     address_space_destroy_dispatch(as);
     flatview_destroy(as->current_map);
+    if (as->lock) {
+        g_free(as->lock);
+    }
     g_free(as->current_map);
 }
 
diff --git a/memory.h b/memory.h
index 79393f1..12d1c56 100644
--- a/memory.h
+++ b/memory.h
@@ -22,6 +22,7 @@
 #include "cpu-common.h"
 #include "targphys.h"
 #include "qemu-queue.h"
+#include "qemu-thread.h"
 #include "iorange.h"
 #include "ioport.h"
 #include "int128.h"
@@ -164,6 +165,7 @@ typedef struct AddressSpace AddressSpace;
  */
 struct AddressSpace {
     /* All fields are private. */
+    QemuMutex *lock;
     const char *name;
     MemoryRegion *root;
     struct FlatView *current_map;
@@ -801,8 +803,9 @@ void mtree_info(fprintf_function mon_printf, void *f);
  *
  * @as: an uninitialized #AddressSpace
  * @root: a #MemoryRegion that routes addesses for the address space
+ * @lock: if true, the physmap protected by local lock, otherwise big lock
  */
-void address_space_init(AddressSpace *as, MemoryRegion *root);
+void address_space_init(AddressSpace *as, MemoryRegion *root, bool lock);
 
 
 /**
-- 
1.7.4.4

[Qemu-devel] [patch v5 6/8] memory: make mmio dispatch able to be out of biglock

[Liu Ping Fan]

Without biglock, we try to protect the mr by increase refcnt.
If we can inc refcnt, go backward and resort to biglock.

Another point is memory radix-tree can be flushed by another
thread, so we should get the copy of terminal mr to survive
from such issue.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 docs/memory.txt   |    4 +
 exec.c            |  170 +++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/pci.c          |    3 +-
 memory-internal.h |    1 +
 memory.h          |    2 +
 5 files changed, 162 insertions(+), 18 deletions(-)

diff --git a/docs/memory.txt b/docs/memory.txt
index 5bbee8e..6b3d15a 100644
--- a/docs/memory.txt
+++ b/docs/memory.txt
@@ -170,3 +170,7 @@ various constraints can be supplied to control how these callbacks are called:
  - .old_portio and .old_mmio can be used to ease porting from code using
    cpu_register_io_memory() and register_ioport().  They should not be used
    in new code.
+
+MMIO regions are provided with ->ref() and ->unref() callbacks; This pair callbacks
+are optional. When ref() return non-zero, Both MemoryRegion and its opaque are
+safe to use.
diff --git a/exec.c b/exec.c
index 750008c..46da08c 100644
--- a/exec.c
+++ b/exec.c
@@ -2280,7 +2280,7 @@ static void register_multipage(AddressSpaceDispatch *d, MemoryRegionSection *sec
                   section_index);
 }
 
-static void mem_add(MemoryListener *listener, MemoryRegionSection *section)
+static void mem_nop(MemoryListener *listener, MemoryRegionSection *section)
 {
     AddressSpaceDispatch *d = container_of(listener, AddressSpaceDispatch, listener);
     MemoryRegionSection now = *section, remain = *section;
@@ -2314,6 +2314,26 @@ static void mem_add(MemoryListener *listener, MemoryRegionSection *section)
     }
 }
 
+static void mem_add(MemoryListener *listener, MemoryRegionSection *section)
+{
+    MemoryRegion *mr = section->mr;
+
+    if (mr->ops && mr->ops->ref) {
+        mr->ops->ref(mr);
+    }
+    mem_nop(listener, section);
+}
+
+static void mem_del(MemoryListener *listener,
+                            MemoryRegionSection *section)
+{
+    MemoryRegion *mr = section->mr;
+
+    if (mr->ops && mr->ops->unref) {
+        mr->ops->unref(mr);
+    }
+}
+
 void qemu_flush_coalesced_mmio_buffer(void)
 {
     if (kvm_enabled())
@@ -3165,11 +3185,27 @@ static void io_mem_init(void)
 static void mem_begin(MemoryListener *listener)
 {
     AddressSpaceDispatch *d = container_of(listener, AddressSpaceDispatch, listener);
+    AddressSpace *as = d->as;
 
+    /* protect the updating process of mrs in memory core agaist readers */
+    if (as->lock) {
+        qemu_mutex_lock(as->lock);
+    }
     destroy_all_mappings(d);
     d->phys_map.ptr = PHYS_MAP_NODE_NIL;
 }
 
+static void mem_commit(MemoryListener *listener)
+{
+    AddressSpaceDispatch *d = container_of(listener, AddressSpaceDispatch,
+                listener);
+    AddressSpace *as = d->as;
+
+    if (as->lock) {
+        qemu_mutex_unlock(as->lock);
+    }
+}
+
 static void core_begin(MemoryListener *listener)
 {
     phys_sections_clear();
@@ -3243,11 +3279,14 @@ void address_space_init_dispatch(AddressSpace *as)
     d->phys_map  = (PhysPageEntry) { .ptr = PHYS_MAP_NODE_NIL, .is_leaf = 0 };
     d->listener = (MemoryListener) {
         .begin = mem_begin,
+        .commit = mem_commit,
         .region_add = mem_add,
-        .region_nop = mem_add,
+        .region_del = mem_del,
+        .region_nop = mem_nop,
         .priority = 0,
     };
     as->dispatch = d;
+    as->dispatch->as = as;
     memory_listener_register(&d->listener, as);
 }
 
@@ -3265,12 +3304,12 @@ static void memory_map_init(void)
 {
     system_memory = g_malloc(sizeof(*system_memory));
     memory_region_init(system_memory, "system", INT64_MAX);
-    address_space_init(&address_space_memory, system_memory);
+    address_space_init(&address_space_memory, system_memory, true);
     address_space_memory.name = "memory";
 
     system_io = g_malloc(sizeof(*system_io));
     memory_region_init(system_io, "io", 65536);
-    address_space_init(&address_space_io, system_io);
+    address_space_init(&address_space_io, system_io, false);
     address_space_io.name = "I/O";
 
     memory_listener_register(&core_memory_listener, &address_space_memory);
@@ -3345,6 +3384,71 @@ static void invalidate_and_set_dirty(target_phys_addr_t addr,
     xen_modified_memory(addr, length);
 }
 
+static MemoryRegionSection *subpage_get_terminal(subpage_t *mmio,
+    target_phys_addr_t addr)
+{
+    MemoryRegionSection *section;
+    unsigned int idx = SUBPAGE_IDX(addr);
+
+    section = &phys_sections[mmio->sub_section[idx]];
+    return section;
+}
+
+static bool memory_region_section_ref(MemoryRegionSection *mrs)
+{
+    MemoryRegion *mr;
+    bool ret = false;
+
+    mr = mrs->mr;
+    if (mr->ops && mr->ops->ref) {
+        ret = mr->ops->ref(mr);
+    }
+    return ret;
+}
+
+static void memory_region_section_unref(MemoryRegionSection *mrs)
+{
+    MemoryRegion *mr;
+
+    mr = mrs->mr;
+    if (mr->ops && mr->ops->unref) {
+        mr->ops->unref(mr);
+    }
+}
+
+static bool memory_region_section_lookup_ref(AddressSpaceDispatch *d,
+    target_phys_addr_t addr, MemoryRegionSection *mrs)
+{
+    MemoryRegionSection *section;
+    bool ret;
+
+    section = phys_page_find(d, addr >> TARGET_PAGE_BITS);
+    if (section->mr->subpage) {
+        section = subpage_get_terminal(section->mr->opaque, addr);
+    }
+    *mrs = *section;
+    ret = memory_region_section_ref(mrs);
+
+    return ret;
+}
+
+static bool address_space_section_lookup_ref(AddressSpace *as,
+    target_phys_addr_t page, MemoryRegionSection *mrs)
+{
+    bool safe_ref;
+    AddressSpaceDispatch *d = as->dispatch;
+
+    if (as->lock) {
+        qemu_mutex_lock(as->lock);
+        safe_ref = memory_region_section_lookup_ref(d, page, mrs);
+        qemu_mutex_unlock(as->lock);
+    } else {
+        safe_ref = memory_region_section_lookup_ref(d, page, mrs);
+    }
+
+    return safe_ref;
+}
+
 void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
                       int len, bool is_write)
 {
@@ -3353,14 +3457,34 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
     uint8_t *ptr;
     uint32_t val;
     target_phys_addr_t page;
-    MemoryRegionSection *section;
+    bool safe_ref = false;
+    MemoryRegionSection *section, obj_mrs;
 
     while (len > 0) {
         page = addr & TARGET_PAGE_MASK;
         l = (page + TARGET_PAGE_SIZE) - addr;
         if (l > len)
             l = len;
-        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
+
+        if (as->lock) {
+            qemu_mutex_lock(as->lock);
+            safe_ref = memory_region_section_lookup_ref(d, page, &obj_mrs);
+            qemu_mutex_unlock(as->lock);
+            if (!safe_ref) {
+                qemu_mutex_lock_iothread();
+                qemu_mutex_lock(as->lock);
+                /* when 2nd try, mem map can change, need to judge it again */
+                safe_ref = memory_region_section_lookup_ref(d, page, &obj_mrs);
+                qemu_mutex_unlock(as->lock);
+                if (safe_ref) {
+                    qemu_mutex_unlock_iothread();
+                }
+            }
+        } else {
+            /* Caller hold the big lock */
+            memory_region_section_lookup_ref(d, page, &obj_mrs);
+        }
+        section = &obj_mrs;
 
         if (is_write) {
             if (!memory_region_is_ram(section->mr)) {
@@ -3425,9 +3549,13 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
                 qemu_put_ram_ptr(ptr);
             }
         }
+        memory_region_section_unref(&obj_mrs);
         len -= l;
         buf += l;
         addr += l;
+        if (!safe_ref && as->lock) {
+            qemu_mutex_unlock_iothread();
+        }
     }
 }
 
@@ -3460,18 +3588,19 @@ void cpu_physical_memory_rw(target_phys_addr_t addr, uint8_t *buf,
 void cpu_physical_memory_write_rom(target_phys_addr_t addr,
                                    const uint8_t *buf, int len)
 {
-    AddressSpaceDispatch *d = address_space_memory.dispatch;
     int l;
     uint8_t *ptr;
     target_phys_addr_t page;
-    MemoryRegionSection *section;
+    MemoryRegionSection *section, mr_obj;
 
     while (len > 0) {
         page = addr & TARGET_PAGE_MASK;
         l = (page + TARGET_PAGE_SIZE) - addr;
         if (l > len)
             l = len;
-        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
+        address_space_section_lookup_ref(&address_space_memory,
+                page >> TARGET_PAGE_BITS, &mr_obj);
+        section = &mr_obj;
 
         if (!(memory_region_is_ram(section->mr) ||
               memory_region_is_romd(section->mr))) {
@@ -3489,6 +3618,7 @@ void cpu_physical_memory_write_rom(target_phys_addr_t addr,
         len -= l;
         buf += l;
         addr += l;
+        memory_region_section_unref(&mr_obj);
     }
 }
 
@@ -3550,12 +3680,11 @@ void *address_space_map(AddressSpace *as,
                         target_phys_addr_t *plen,
                         bool is_write)
 {
-    AddressSpaceDispatch *d = as->dispatch;
     target_phys_addr_t len = *plen;
     target_phys_addr_t todo = 0;
     int l;
     target_phys_addr_t page;
-    MemoryRegionSection *section;
+    MemoryRegionSection *section, mr_obj;
     ram_addr_t raddr = RAM_ADDR_MAX;
     ram_addr_t rlen;
     void *ret;
@@ -3565,7 +3694,8 @@ void *address_space_map(AddressSpace *as,
         l = (page + TARGET_PAGE_SIZE) - addr;
         if (l > len)
             l = len;
-        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
+        address_space_section_lookup_ref(as, page >> TARGET_PAGE_BITS, &mr_obj);
+        section = &mr_obj;
 
         if (!(memory_region_is_ram(section->mr) && !section->readonly)) {
             if (todo || bounce.buffer) {
@@ -3579,6 +3709,7 @@ void *address_space_map(AddressSpace *as,
             }
 
             *plen = l;
+            memory_region_section_unref(&mr_obj);
             return bounce.buffer;
         }
         if (!todo) {
@@ -3589,6 +3720,7 @@ void *address_space_map(AddressSpace *as,
         len -= l;
         addr += l;
         todo += l;
+        memory_region_section_unref(&mr_obj);
     }
     rlen = todo;
     ret = qemu_ram_ptr_length(raddr, &rlen);
@@ -4197,12 +4329,16 @@ bool virtio_is_big_endian(void)
 #ifndef CONFIG_USER_ONLY
 bool cpu_physical_memory_is_io(target_phys_addr_t phys_addr)
 {
-    MemoryRegionSection *section;
-
-    section = phys_page_find(address_space_memory.dispatch,
-                             phys_addr >> TARGET_PAGE_BITS);
+    MemoryRegionSection *section, mr_obj;
+    bool ret;
 
-    return !(memory_region_is_ram(section->mr) ||
+    address_space_section_lookup_ref(&address_space_memory,
+                             phys_addr >> TARGET_PAGE_BITS, &mr_obj);
+    section = &mr_obj;
+    ret = !(memory_region_is_ram(section->mr) ||
              memory_region_is_romd(section->mr));
+    memory_region_section_unref(&mr_obj);
+
+    return ret;
 }
 #endif
diff --git a/hw/pci.c b/hw/pci.c
index 9ba589e..64206e6 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -786,7 +786,8 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
                                  get_system_memory(), 0,
                                  memory_region_size(get_system_memory()));
         memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);
-        address_space_init(&pci_dev->bus_master_as, &pci_dev->bus_master_enable_region);
+        address_space_init(&pci_dev->bus_master_as,
+                        &pci_dev->bus_master_enable_region, true);
         pci_dev->dma = g_new(DMAContext, 1);
         dma_context_init(pci_dev->dma, &pci_dev->bus_master_as, NULL, NULL, NULL);
     }
diff --git a/memory-internal.h b/memory-internal.h
index b33a99d..ea06bfb 100644
--- a/memory-internal.h
+++ b/memory-internal.h
@@ -38,6 +38,7 @@ struct AddressSpaceDispatch {
      */
     PhysPageEntry phys_map;
     MemoryListener listener;
+    AddressSpace *as;
 };
 
 void address_space_init_dispatch(AddressSpace *as);
diff --git a/memory.h b/memory.h
index 12d1c56..8ca9bcc 100644
--- a/memory.h
+++ b/memory.h
@@ -67,6 +67,8 @@ struct MemoryRegionOps {
                   target_phys_addr_t addr,
                   uint64_t data,
                   unsigned size);
+    bool (*ref)(MemoryRegion *mr);
+    void (*unref)(MemoryRegion *mr);
 
     enum device_endian endianness;
     /* Guest-visible constraints: */
-- 
1.7.4.4

[Qemu-devel] [patch v5 7/8] memory: introduce tls context to record nested dma

[Liu Ping Fan]

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 cpus.c        |    3 ++
 exec.c        |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-thread.h |    8 +++++++
 vl.c          |    1 +
 4 files changed, 70 insertions(+), 0 deletions(-)

diff --git a/cpus.c b/cpus.c
index 191cbf5..e67d80f 100644
--- a/cpus.c
+++ b/cpus.c
@@ -733,6 +733,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
 
     qemu_mutex_lock(&qemu_global_mutex);
     qemu_thread_get_self(cpu->thread);
+    qemu_thread_init_context();
     env->thread_id = qemu_get_thread_id();
     cpu_single_env = env;
 
@@ -774,6 +775,7 @@ static void *qemu_dummy_cpu_thread_fn(void *arg)
 
     qemu_mutex_lock_iothread();
     qemu_thread_get_self(cpu->thread);
+    qemu_thread_init_context();
     env->thread_id = qemu_get_thread_id();
 
     sigemptyset(&waitset);
@@ -813,6 +815,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
 
     qemu_tcg_init_cpu_signals();
     qemu_thread_get_self(cpu->thread);
+    qemu_thread_init_context();
 
     /* signal CPU creation */
     qemu_mutex_lock(&qemu_global_mutex);
diff --git a/exec.c b/exec.c
index 46da08c..ea672c6 100644
--- a/exec.c
+++ b/exec.c
@@ -3449,6 +3449,49 @@ static bool address_space_section_lookup_ref(AddressSpace *as,
     return safe_ref;
 }
 
+typedef struct ThreadContext {
+  DispatchType dispatch_type;
+  unsigned int mmio_req_pending;
+} ThreadContext;
+
+static __thread ThreadContext *thread_context;
+
+void qemu_thread_init_context(void)
+{
+    thread_context = g_new(ThreadContext, 1);
+    thread_context->dispatch_type = DISPATCH_INIT;
+    thread_context->mmio_req_pending = 0;
+}
+
+void qemu_thread_set_dispatch_type(DispatchType type)
+{
+    thread_context->dispatch_type = type;
+}
+
+void qemu_thread_reset_dispatch_type(void)
+{
+    thread_context->dispatch_type = DISPATCH_INIT;
+}
+
+static bool address_space_inc_req_pending(void)
+{
+    bool nested = false;
+
+    /* currently, only mmio out of big lock, and need this to avoid dead lock */
+    if (thread_context->dispatch_type == DISPATCH_MMIO) {
+        nested = ++thread_context->mmio_req_pending > 1 ? true : false;
+    }
+
+    return nested;
+}
+
+static void address_space_dec_req_pending(void)
+{
+    if (thread_context->dispatch_type == DISPATCH_MMIO) {
+        thread_context->mmio_req_pending--;
+    }
+}
+
 void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
                       int len, bool is_write)
 {
@@ -3459,6 +3502,7 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
     target_phys_addr_t page;
     bool safe_ref = false;
     MemoryRegionSection *section, obj_mrs;
+    bool nested_dma = false;
 
     while (len > 0) {
         page = addr & TARGET_PAGE_MASK;
@@ -3485,10 +3529,17 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
             memory_region_section_lookup_ref(d, page, &obj_mrs);
         }
         section = &obj_mrs;
+        nested_dma = address_space_inc_req_pending();
 
         if (is_write) {
             if (!memory_region_is_ram(section->mr)) {
                 target_phys_addr_t addr1;
+
+                /* To fix, will filter iommu case */
+                if (nested_dma) {
+                    fprintf(stderr, "can not support nested DMA");
+                    abort();
+                }
                 addr1 = memory_region_section_addr(section, addr);
                 /* XXX: could force cpu_single_env to NULL to avoid
                    potential bugs */
@@ -3522,6 +3573,12 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
             if (!(memory_region_is_ram(section->mr) ||
                   memory_region_is_romd(section->mr))) {
                 target_phys_addr_t addr1;
+
+                /* To fix, will filter iommu case */
+                if (nested_dma) {
+                    fprintf(stderr, "can not support nested DMA");
+                    abort();
+                }
                 /* I/O case */
                 addr1 = memory_region_section_addr(section, addr);
                 if (l >= 4 && ((addr1 & 3) == 0)) {
@@ -3549,6 +3606,7 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
                 qemu_put_ram_ptr(ptr);
             }
         }
+        address_space_dec_req_pending();
         memory_region_section_unref(&obj_mrs);
         len -= l;
         buf += l;
diff --git a/qemu-thread.h b/qemu-thread.h
index 05fdaaf..bb9535e 100644
--- a/qemu-thread.h
+++ b/qemu-thread.h
@@ -7,6 +7,11 @@
 typedef struct QemuMutex QemuMutex;
 typedef struct QemuCond QemuCond;
 typedef struct QemuThread QemuThread;
+typedef enum {
+  DISPATCH_INIT = 0,
+  DISPATCH_MMIO,
+  DISPATCH_IO,
+} DispatchType;
 
 #ifdef _WIN32
 #include "qemu-thread-win32.h"
@@ -46,4 +51,7 @@ void qemu_thread_get_self(QemuThread *thread);
 bool qemu_thread_is_self(QemuThread *thread);
 void qemu_thread_exit(void *retval);
 
+void qemu_thread_init_context(void);
+void qemu_thread_set_dispatch_type(DispatchType type);
+void qemu_thread_reset_dispatch_type(void);
 #endif
diff --git a/vl.c b/vl.c
index ee3c43a..be8d825 100644
--- a/vl.c
+++ b/vl.c
@@ -3439,6 +3439,7 @@ int main(int argc, char **argv, char **envp)
             add_device_config(DEV_VIRTCON, "vc:80Cx24C");
     }
 
+    qemu_thread_init_context();
     socket_init();
 
     if (qemu_opts_foreach(qemu_find_opts("chardev"), chardev_init_func, NULL, 1) != 0)
-- 
1.7.4.4

[Qemu-devel] [patch v5 8/8] vcpu: push mmio dispatcher out of big lock

[Liu Ping Fan]

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 kvm-all.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index c2c6909..e92e43c 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1573,10 +1573,14 @@ int kvm_cpu_exec(CPUArchState *env)
             break;
         case KVM_EXIT_MMIO:
             DPRINTF("handle_mmio\n");
+            qemu_mutex_unlock_iothread();
+            qemu_thread_set_dispatch_type(DISPATCH_MMIO);
             cpu_physical_memory_rw(run->mmio.phys_addr,
                                    run->mmio.data,
                                    run->mmio.len,
                                    run->mmio.is_write);
+            qemu_thread_reset_dispatch_type();
+            qemu_mutex_lock_iothread();
             ret = 0;
             break;
         case KVM_EXIT_IRQ_WINDOW_OPEN:
-- 
1.7.4.4

Re: [Qemu-devel] [patch v5 5/8] memory: introduce local lock for address space

[Peter Maydell]

On 28 October 2012 23:48, Liu Ping Fan <pingfank@linux.vnet.ibm.com> wrote:
> For those address spaces which want to be able out of big lock, they
> will be protected by their own local.

Are you sure this patch compiles? It seems to only be changing
the prototype and implementation of address_space_init() to take
an extra parameter, but not any of the callers...

-- PMM

Re: [Qemu-devel] [patch v5 5/8] memory: introduce local lock for address space

[liu ping fan]

On Mon, Oct 29, 2012 at 3:42 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 28 October 2012 23:48, Liu Ping Fan <pingfank@linux.vnet.ibm.com> wrote:
>> For those address spaces which want to be able out of big lock, they
>> will be protected by their own local.
>
> Are you sure this patch compiles? It seems to only be changing
> the prototype and implementation of address_space_init() to take
> an extra parameter, but not any of the callers...
>
The caller is in the next patch. Need to rearrange.

Thanks,
pingfan

> -- PMM
>

Re: [Qemu-devel] [patch v5 7/8] memory: introduce tls context to record nested dma

[Paolo Bonzini]

Il 29/10/2012 00:48, Liu Ping Fan ha scritto:
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  cpus.c        |    3 ++
>  exec.c        |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-thread.h |    8 +++++++
>  vl.c          |    1 +
>  4 files changed, 70 insertions(+), 0 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 191cbf5..e67d80f 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -733,6 +733,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
>  
>      qemu_mutex_lock(&qemu_global_mutex);
>      qemu_thread_get_self(cpu->thread);
> +    qemu_thread_init_context();
>      env->thread_id = qemu_get_thread_id();
>      cpu_single_env = env;
>  
> @@ -774,6 +775,7 @@ static void *qemu_dummy_cpu_thread_fn(void *arg)
>  
>      qemu_mutex_lock_iothread();
>      qemu_thread_get_self(cpu->thread);
> +    qemu_thread_init_context();
>      env->thread_id = qemu_get_thread_id();
>  
>      sigemptyset(&waitset);
> @@ -813,6 +815,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>  
>      qemu_tcg_init_cpu_signals();
>      qemu_thread_get_self(cpu->thread);
> +    qemu_thread_init_context();
>  
>      /* signal CPU creation */
>      qemu_mutex_lock(&qemu_global_mutex);
> diff --git a/exec.c b/exec.c
> index 46da08c..ea672c6 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3449,6 +3449,49 @@ static bool address_space_section_lookup_ref(AddressSpace *as,
>      return safe_ref;
>  }
>  
> +typedef struct ThreadContext {
> +  DispatchType dispatch_type;
> +  unsigned int mmio_req_pending;
> +} ThreadContext;
> +
> +static __thread ThreadContext *thread_context;
> +
> +void qemu_thread_init_context(void)
> +{
> +    thread_context = g_new(ThreadContext, 1);
> +    thread_context->dispatch_type = DISPATCH_INIT;
> +    thread_context->mmio_req_pending = 0;
> +}

No need for this:

static __thread ThreadContext thread_context = {
    .dispatch_type = DISPATCH_INIT,
    .mmio_req_pending = 0
};

Paolo

Re: [Qemu-devel] [patch v5 5/8] memory: introduce local lock for address space

[Avi Kivity]

On 10/29/2012 01:48 AM, Liu Ping Fan wrote:
> For those address spaces which want to be able out of big lock, they
> will be protected by their own local.
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  memory.c |   11 ++++++++++-
>  memory.h |    5 ++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index 2f68d67..ff34aed 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1532,9 +1532,15 @@ void memory_listener_unregister(MemoryListener *listener)
>      QTAILQ_REMOVE(&memory_listeners, listener, link);
>  }
>  
> -void address_space_init(AddressSpace *as, MemoryRegion *root)
> +void address_space_init(AddressSpace *as, MemoryRegion *root, bool lock)


Why not always use the lock?  Even if the big lock is taken, it doesn't
hurt.  And eventually all address spaces will be fine-grained.

>  {
>      memory_region_transaction_begin();
> +    if (lock) {
> +        as->lock = g_new(QemuMutex, 1);
> +        qemu_mutex_init(as->lock);
> +    } else {
> +        as->lock = NULL;
> +    }
>      as->root = root;
>      as->current_map = g_new(FlatView, 1);
>      flatview_init(as->current_map);
> @@ -1553,6 +1559,9 @@ void address_space_destroy(AddressSpace *as)
>      QTAILQ_REMOVE(&address_spaces, as, address_spaces_link);
>      address_space_destroy_dispatch(as);
>      flatview_destroy(as->current_map);
> +    if (as->lock) {
> +        g_free(as->lock);
> +    }
>      g_free(as->current_map);
>  }
>  
> diff --git a/memory.h b/memory.h
> index 79393f1..12d1c56 100644
> --- a/memory.h
> +++ b/memory.h
> @@ -22,6 +22,7 @@
>  #include "cpu-common.h"
>  #include "targphys.h"
>  #include "qemu-queue.h"
> +#include "qemu-thread.h"
>  #include "iorange.h"
>  #include "ioport.h"
>  #include "int128.h"
> @@ -164,6 +165,7 @@ typedef struct AddressSpace AddressSpace;
>   */
>  struct AddressSpace {
>      /* All fields are private. */
> +    QemuMutex *lock;
>      const char *name;
>      MemoryRegion *root;
>      struct FlatView *current_map;
> @@ -801,8 +803,9 @@ void mtree_info(fprintf_function mon_printf, void *f);
>   *
>   * @as: an uninitialized #AddressSpace
>   * @root: a #MemoryRegion that routes addesses for the address space
> + * @lock: if true, the physmap protected by local lock, otherwise big lock
>   */
> -void address_space_init(AddressSpace *as, MemoryRegion *root);
> +void address_space_init(AddressSpace *as, MemoryRegion *root, bool lock);
>  
>  
>  /**
> 


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [patch v5 6/8] memory: make mmio dispatch able to be out of biglock

[Avi Kivity]

On 10/29/2012 01:48 AM, Liu Ping Fan wrote:
> Without biglock, we try to protect the mr by increase refcnt.
> If we can inc refcnt, go backward and resort to biglock.
> 
> Another point is memory radix-tree can be flushed by another
> thread, so we should get the copy of terminal mr to survive
> from such issue.
> 

> +static bool memory_region_section_ref(MemoryRegionSection *mrs)
> +{
> +    MemoryRegion *mr;
> +    bool ret = false;
> +
> +    mr = mrs->mr;
> +    if (mr->ops && mr->ops->ref) {
> +        ret = mr->ops->ref(mr);

I still don't see why ->ref() needs to return something.

> +    }
> +    return ret;
> +}
> +
>  
>      while (len > 0) {
>          page = addr & TARGET_PAGE_MASK;
>          l = (page + TARGET_PAGE_SIZE) - addr;
>          if (l > len)
>              l = len;
> -        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
> +
> +        if (as->lock) {
> +            qemu_mutex_lock(as->lock);
> +            safe_ref = memory_region_section_lookup_ref(d, page, &obj_mrs);
> +            qemu_mutex_unlock(as->lock);
> +            if (!safe_ref) {
> +                qemu_mutex_lock_iothread();
> +                qemu_mutex_lock(as->lock);
> +                /* when 2nd try, mem map can change, need to judge it again */
> +                safe_ref = memory_region_section_lookup_ref(d, page, &obj_mrs);
> +                qemu_mutex_unlock(as->lock);
> +                if (safe_ref) {
> +                    qemu_mutex_unlock_iothread();
> +                }
> +            }
> +        } else {
> +            /* Caller hold the big lock */
> +            memory_region_section_lookup_ref(d, page, &obj_mrs);

It's not a property of the address space, it's a property of the caller.

> +        }
> +        section = &obj_mrs;
>  
>          if (is_write) {
>              if (!memory_region_is_ram(section->mr)) {


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [patch v5 5/8] memory: introduce local lock for address space

[liu ping fan]

On Mon, Oct 29, 2012 at 5:32 PM, Avi Kivity <avi@redhat.com> wrote:
> On 10/29/2012 01:48 AM, Liu Ping Fan wrote:
>> For those address spaces which want to be able out of big lock, they
>> will be protected by their own local.
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  memory.c |   11 ++++++++++-
>>  memory.h |    5 ++++-
>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/memory.c b/memory.c
>> index 2f68d67..ff34aed 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -1532,9 +1532,15 @@ void memory_listener_unregister(MemoryListener *listener)
>>      QTAILQ_REMOVE(&memory_listeners, listener, link);
>>  }
>>
>> -void address_space_init(AddressSpace *as, MemoryRegion *root)
>> +void address_space_init(AddressSpace *as, MemoryRegion *root, bool lock)
>
>
> Why not always use the lock?  Even if the big lock is taken, it doesn't
> hurt.  And eventually all address spaces will be fine-grained.
>
I had thought only mmio is out of big lock's protection. While others
address space will take extra expense. So leave them until they are
ready to be out of big lock.

>>  {
>>      memory_region_transaction_begin();
>> +    if (lock) {
>> +        as->lock = g_new(QemuMutex, 1);
>> +        qemu_mutex_init(as->lock);
>> +    } else {
>> +        as->lock = NULL;
>> +    }
>>      as->root = root;
>>      as->current_map = g_new(FlatView, 1);
>>      flatview_init(as->current_map);
>> @@ -1553,6 +1559,9 @@ void address_space_destroy(AddressSpace *as)
>>      QTAILQ_REMOVE(&address_spaces, as, address_spaces_link);
>>      address_space_destroy_dispatch(as);
>>      flatview_destroy(as->current_map);
>> +    if (as->lock) {
>> +        g_free(as->lock);
>> +    }
>>      g_free(as->current_map);
>>  }
>>
>> diff --git a/memory.h b/memory.h
>> index 79393f1..12d1c56 100644
>> --- a/memory.h
>> +++ b/memory.h
>> @@ -22,6 +22,7 @@
>>  #include "cpu-common.h"
>>  #include "targphys.h"
>>  #include "qemu-queue.h"
>> +#include "qemu-thread.h"
>>  #include "iorange.h"
>>  #include "ioport.h"
>>  #include "int128.h"
>> @@ -164,6 +165,7 @@ typedef struct AddressSpace AddressSpace;
>>   */
>>  struct AddressSpace {
>>      /* All fields are private. */
>> +    QemuMutex *lock;
>>      const char *name;
>>      MemoryRegion *root;
>>      struct FlatView *current_map;
>> @@ -801,8 +803,9 @@ void mtree_info(fprintf_function mon_printf, void *f);
>>   *
>>   * @as: an uninitialized #AddressSpace
>>   * @root: a #MemoryRegion that routes addesses for the address space
>> + * @lock: if true, the physmap protected by local lock, otherwise big lock
>>   */
>> -void address_space_init(AddressSpace *as, MemoryRegion *root);
>> +void address_space_init(AddressSpace *as, MemoryRegion *root, bool lock);
>>
>>
>>  /**
>>
>
>
> --
> error compiling committee.c: too many arguments to function
>

Re: [Qemu-devel] [patch v5 6/8] memory: make mmio dispatch able to be out of biglock

[liu ping fan]

On Mon, Oct 29, 2012 at 5:41 PM, Avi Kivity <avi@redhat.com> wrote:
> On 10/29/2012 01:48 AM, Liu Ping Fan wrote:
>> Without biglock, we try to protect the mr by increase refcnt.
>> If we can inc refcnt, go backward and resort to biglock.
>>
>> Another point is memory radix-tree can be flushed by another
>> thread, so we should get the copy of terminal mr to survive
>> from such issue.
>>
>
>> +static bool memory_region_section_ref(MemoryRegionSection *mrs)
>> +{
>> +    MemoryRegion *mr;
>> +    bool ret = false;
>> +
>> +    mr = mrs->mr;
>> +    if (mr->ops && mr->ops->ref) {
>> +        ret = mr->ops->ref(mr);
>
> I still don't see why ->ref() needs to return something.
>
My original design use it to trace refcnt on object, but now it
abandons.  Will drop it.
>> +    }
>> +    return ret;
>> +}
>> +
>>
>>      while (len > 0) {
>>          page = addr & TARGET_PAGE_MASK;
>>          l = (page + TARGET_PAGE_SIZE) - addr;
>>          if (l > len)
>>              l = len;
>> -        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
>> +
>> +        if (as->lock) {
>> +            qemu_mutex_lock(as->lock);
>> +            safe_ref = memory_region_section_lookup_ref(d, page, &obj_mrs);
>> +            qemu_mutex_unlock(as->lock);
>> +            if (!safe_ref) {
>> +                qemu_mutex_lock_iothread();
>> +                qemu_mutex_lock(as->lock);
>> +                /* when 2nd try, mem map can change, need to judge it again */
>> +                safe_ref = memory_region_section_lookup_ref(d, page, &obj_mrs);
>> +                qemu_mutex_unlock(as->lock);
>> +                if (safe_ref) {
>> +                    qemu_mutex_unlock_iothread();
>> +                }
>> +            }
>> +        } else {
>> +            /* Caller hold the big lock */
>> +            memory_region_section_lookup_ref(d, page, &obj_mrs);
>
> It's not a property of the address space, it's a property of the caller.
>
Sorry, what is your meaning?

>> +        }
>> +        section = &obj_mrs;
>>
>>          if (is_write) {
>>              if (!memory_region_is_ram(section->mr)) {
>
>
> --
> error compiling committee.c: too many arguments to function
>

Re: [Qemu-devel] [patch v5 6/8] memory: make mmio dispatch able to be out of biglock

[liu ping fan]

On Tue, Oct 30, 2012 at 3:06 PM, liu ping fan <qemulist@gmail.com> wrote:
> On Mon, Oct 29, 2012 at 5:41 PM, Avi Kivity <avi@redhat.com> wrote:
>> On 10/29/2012 01:48 AM, Liu Ping Fan wrote:
>>> Without biglock, we try to protect the mr by increase refcnt.
>>> If we can inc refcnt, go backward and resort to biglock.
>>>
>>> Another point is memory radix-tree can be flushed by another
>>> thread, so we should get the copy of terminal mr to survive
>>> from such issue.
>>>
>>
>>> +static bool memory_region_section_ref(MemoryRegionSection *mrs)
>>> +{
>>> +    MemoryRegion *mr;
>>> +    bool ret = false;
>>> +
>>> +    mr = mrs->mr;
>>> +    if (mr->ops && mr->ops->ref) {
>>> +        ret = mr->ops->ref(mr);
>>
>> I still don't see why ->ref() needs to return something.
>>
> My original design use it to trace refcnt on object, but now it
> abandons.  Will drop it.
>>> +    }
>>> +    return ret;
>>> +}
>>> +
>>>
>>>      while (len > 0) {
>>>          page = addr & TARGET_PAGE_MASK;
>>>          l = (page + TARGET_PAGE_SIZE) - addr;
>>>          if (l > len)
>>>              l = len;
>>> -        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
>>> +
>>> +        if (as->lock) {
>>> +            qemu_mutex_lock(as->lock);
>>> +            safe_ref = memory_region_section_lookup_ref(d, page, &obj_mrs);
>>> +            qemu_mutex_unlock(as->lock);
>>> +            if (!safe_ref) {
>>> +                qemu_mutex_lock_iothread();
>>> +                qemu_mutex_lock(as->lock);
>>> +                /* when 2nd try, mem map can change, need to judge it again */
>>> +                safe_ref = memory_region_section_lookup_ref(d, page, &obj_mrs);
>>> +                qemu_mutex_unlock(as->lock);
>>> +                if (safe_ref) {
>>> +                    qemu_mutex_unlock_iothread();
>>> +                }
>>> +            }
>>> +        } else {
>>> +            /* Caller hold the big lock */
>>> +            memory_region_section_lookup_ref(d, page, &obj_mrs);
>>
>> It's not a property of the address space, it's a property of the caller.
>>
> Sorry, what is your meaning?
>
ping?

>>> +        }
>>> +        section = &obj_mrs;
>>>
>>>          if (is_write) {
>>>              if (!memory_region_is_ram(section->mr)) {
>>
>>
>> --
>> error compiling committee.c: too many arguments to function
>>

Re: [Qemu-devel] [patch v5 5/8] memory: introduce local lock for address space

[Avi Kivity]

On 10/29/2012 11:46 AM, liu ping fan wrote:
> On Mon, Oct 29, 2012 at 5:32 PM, Avi Kivity <avi@redhat.com> wrote:
>> On 10/29/2012 01:48 AM, Liu Ping Fan wrote:
>>> For those address spaces which want to be able out of big lock, they
>>> will be protected by their own local.
>>>
>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>> ---
>>>  memory.c |   11 ++++++++++-
>>>  memory.h |    5 ++++-
>>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/memory.c b/memory.c
>>> index 2f68d67..ff34aed 100644
>>> --- a/memory.c
>>> +++ b/memory.c
>>> @@ -1532,9 +1532,15 @@ void memory_listener_unregister(MemoryListener *listener)
>>>      QTAILQ_REMOVE(&memory_listeners, listener, link);
>>>  }
>>>
>>> -void address_space_init(AddressSpace *as, MemoryRegion *root)
>>> +void address_space_init(AddressSpace *as, MemoryRegion *root, bool lock)
>>
>>
>> Why not always use the lock?  Even if the big lock is taken, it doesn't
>> hurt.  And eventually all address spaces will be fine-grained.
>>
> I had thought only mmio is out of big lock's protection. While others
> address space will take extra expense. So leave them until they are
> ready to be out of big lock.

The other address spaces are pio (which also needs fine-grained locking)
and the dma address spaces (which are like address_space_memory, except
they are accessed via DMA instead of from the vcpu).



-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [patch v5 6/8] memory: make mmio dispatch able to be out of biglock

[Avi Kivity]

On 10/30/2012 09:06 AM, liu ping fan wrote:
>>>      while (len > 0) {
>>>          page = addr & TARGET_PAGE_MASK;
>>>          l = (page + TARGET_PAGE_SIZE) - addr;
>>>          if (l > len)
>>>              l = len;
>>> -        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
>>> +
>>> +        if (as->lock) {
>>> +            qemu_mutex_lock(as->lock);
>>> +            safe_ref = memory_region_section_lookup_ref(d, page, &obj_mrs);
>>> +            qemu_mutex_unlock(as->lock);
>>> +            if (!safe_ref) {
>>> +                qemu_mutex_lock_iothread();
>>> +                qemu_mutex_lock(as->lock);
>>> +                /* when 2nd try, mem map can change, need to judge it again */
>>> +                safe_ref = memory_region_section_lookup_ref(d, page, &obj_mrs);
>>> +                qemu_mutex_unlock(as->lock);
>>> +                if (safe_ref) {
>>> +                    qemu_mutex_unlock_iothread();
>>> +                }
>>> +            }
>>> +        } else {
>>> +            /* Caller hold the big lock */
>>> +            memory_region_section_lookup_ref(d, page, &obj_mrs);
>>
>> It's not a property of the address space, it's a property of the caller.
>>
> Sorry, what is your meaning?

Whether the caller holds the big lock or not is not known by the address
space.  It is only known by the caller itself.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [patch v5 5/8] memory: introduce local lock for address space

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2059 bytes --]

On 2012-11-01 16:45, Avi Kivity wrote:
> On 10/29/2012 11:46 AM, liu ping fan wrote:
>> On Mon, Oct 29, 2012 at 5:32 PM, Avi Kivity <avi@redhat.com> wrote:
>>> On 10/29/2012 01:48 AM, Liu Ping Fan wrote:
>>>> For those address spaces which want to be able out of big lock, they
>>>> will be protected by their own local.
>>>>
>>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>> ---
>>>>  memory.c |   11 ++++++++++-
>>>>  memory.h |    5 ++++-
>>>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/memory.c b/memory.c
>>>> index 2f68d67..ff34aed 100644
>>>> --- a/memory.c
>>>> +++ b/memory.c
>>>> @@ -1532,9 +1532,15 @@ void memory_listener_unregister(MemoryListener *listener)
>>>>      QTAILQ_REMOVE(&memory_listeners, listener, link);
>>>>  }
>>>>
>>>> -void address_space_init(AddressSpace *as, MemoryRegion *root)
>>>> +void address_space_init(AddressSpace *as, MemoryRegion *root, bool lock)
>>>
>>>
>>> Why not always use the lock?  Even if the big lock is taken, it doesn't
>>> hurt.  And eventually all address spaces will be fine-grained.
>>>
>> I had thought only mmio is out of big lock's protection. While others
>> address space will take extra expense. So leave them until they are
>> ready to be out of big lock.
> 
> The other address spaces are pio (which also needs fine-grained locking)
> and the dma address spaces (which are like address_space_memory, except
> they are accessed via DMA instead of from the vcpu).

The problem is with memory regions that don't do fine-grained locking
yet, thus don't provide ref/unref. Then we fall back to taking BQL
across dispatch. If the dispatch caller already holds the BQL, we will
bail out.

As I understand the series, as->lock == NULL means that we will never
take any lock during dispatch as the caller is not yet ready for
fine-grained locking. This prevents the problem - for PIO at least. But
this series should break TCG as it calls into MMIO dispatch from the
VCPU while holding the BQL.

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] [patch v5 5/8] memory: introduce local lock for address space

[liu ping fan]

On Fri, Nov 2, 2012 at 2:44 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2012-11-01 16:45, Avi Kivity wrote:
>> On 10/29/2012 11:46 AM, liu ping fan wrote:
>>> On Mon, Oct 29, 2012 at 5:32 PM, Avi Kivity <avi@redhat.com> wrote:
>>>> On 10/29/2012 01:48 AM, Liu Ping Fan wrote:
>>>>> For those address spaces which want to be able out of big lock, they
>>>>> will be protected by their own local.
>>>>>
>>>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>>> ---
>>>>>  memory.c |   11 ++++++++++-
>>>>>  memory.h |    5 ++++-
>>>>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/memory.c b/memory.c
>>>>> index 2f68d67..ff34aed 100644
>>>>> --- a/memory.c
>>>>> +++ b/memory.c
>>>>> @@ -1532,9 +1532,15 @@ void memory_listener_unregister(MemoryListener *listener)
>>>>>      QTAILQ_REMOVE(&memory_listeners, listener, link);
>>>>>  }
>>>>>
>>>>> -void address_space_init(AddressSpace *as, MemoryRegion *root)
>>>>> +void address_space_init(AddressSpace *as, MemoryRegion *root, bool lock)
>>>>
>>>>
>>>> Why not always use the lock?  Even if the big lock is taken, it doesn't
>>>> hurt.  And eventually all address spaces will be fine-grained.
>>>>
>>> I had thought only mmio is out of big lock's protection. While others
>>> address space will take extra expense. So leave them until they are
>>> ready to be out of big lock.
>>
>> The other address spaces are pio (which also needs fine-grained locking)
>> and the dma address spaces (which are like address_space_memory, except
>> they are accessed via DMA instead of from the vcpu).
>
> The problem is with memory regions that don't do fine-grained locking
> yet, thus don't provide ref/unref. Then we fall back to taking BQL
> across dispatch. If the dispatch caller already holds the BQL, we will
> bail out.
>
Yes, these asymmetrice callers are bothering. Currently, I just make
exceptions for them, and would like to make the biglock recursive.
But this motivation may make bug not easy to find.

> As I understand the series, as->lock == NULL means that we will never
> take any lock during dispatch as the caller is not yet ready for
> fine-grained locking. This prevents the problem - for PIO at least. But
> this series should break TCG as it calls into MMIO dispatch from the
> VCPU while holding the BQL.
>
What about add another condition "dispatch_type == DISPATCH_MMIO" to
tell this situation.

Regards,
Pingfan
> Jan
>
>

Re: [Qemu-devel] [patch v5 5/8] memory: introduce local lock for address space

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2824 bytes --]

On 2012-11-02 01:52, liu ping fan wrote:
> On Fri, Nov 2, 2012 at 2:44 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2012-11-01 16:45, Avi Kivity wrote:
>>> On 10/29/2012 11:46 AM, liu ping fan wrote:
>>>> On Mon, Oct 29, 2012 at 5:32 PM, Avi Kivity <avi@redhat.com> wrote:
>>>>> On 10/29/2012 01:48 AM, Liu Ping Fan wrote:
>>>>>> For those address spaces which want to be able out of big lock, they
>>>>>> will be protected by their own local.
>>>>>>
>>>>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>  memory.c |   11 ++++++++++-
>>>>>>  memory.h |    5 ++++-
>>>>>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/memory.c b/memory.c
>>>>>> index 2f68d67..ff34aed 100644
>>>>>> --- a/memory.c
>>>>>> +++ b/memory.c
>>>>>> @@ -1532,9 +1532,15 @@ void memory_listener_unregister(MemoryListener *listener)
>>>>>>      QTAILQ_REMOVE(&memory_listeners, listener, link);
>>>>>>  }
>>>>>>
>>>>>> -void address_space_init(AddressSpace *as, MemoryRegion *root)
>>>>>> +void address_space_init(AddressSpace *as, MemoryRegion *root, bool lock)
>>>>>
>>>>>
>>>>> Why not always use the lock?  Even if the big lock is taken, it doesn't
>>>>> hurt.  And eventually all address spaces will be fine-grained.
>>>>>
>>>> I had thought only mmio is out of big lock's protection. While others
>>>> address space will take extra expense. So leave them until they are
>>>> ready to be out of big lock.
>>>
>>> The other address spaces are pio (which also needs fine-grained locking)
>>> and the dma address spaces (which are like address_space_memory, except
>>> they are accessed via DMA instead of from the vcpu).
>>
>> The problem is with memory regions that don't do fine-grained locking
>> yet, thus don't provide ref/unref. Then we fall back to taking BQL
>> across dispatch. If the dispatch caller already holds the BQL, we will
>> bail out.
>>
> Yes, these asymmetrice callers are bothering. Currently, I just make
> exceptions for them, and would like to make the biglock recursive.
> But this motivation may make bug not easy to find.
> 
>> As I understand the series, as->lock == NULL means that we will never
>> take any lock during dispatch as the caller is not yet ready for
>> fine-grained locking. This prevents the problem - for PIO at least. But
>> this series should break TCG as it calls into MMIO dispatch from the
>> VCPU while holding the BQL.
>>
> What about add another condition "dispatch_type == DISPATCH_MMIO" to
> tell this situation.

An alternative pattern that we will also use for core services is to
provide an additional entry point, one that indicates that the caller
doesn't hold the BQL. Then we will gradually move things over until the
existing entry point is obsolete.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] [patch v5 7/8] memory: introduce tls context to record nested dma

[Jan Kiszka]

On 2012-10-29 00:48, Liu Ping Fan wrote:
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  cpus.c        |    3 ++
>  exec.c        |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-thread.h |    8 +++++++
>  vl.c          |    1 +
>  4 files changed, 70 insertions(+), 0 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 191cbf5..e67d80f 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -733,6 +733,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
>  
>      qemu_mutex_lock(&qemu_global_mutex);
>      qemu_thread_get_self(cpu->thread);
> +    qemu_thread_init_context();
>      env->thread_id = qemu_get_thread_id();
>      cpu_single_env = env;
>  
> @@ -774,6 +775,7 @@ static void *qemu_dummy_cpu_thread_fn(void *arg)
>  
>      qemu_mutex_lock_iothread();
>      qemu_thread_get_self(cpu->thread);
> +    qemu_thread_init_context();
>      env->thread_id = qemu_get_thread_id();
>  
>      sigemptyset(&waitset);
> @@ -813,6 +815,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>  
>      qemu_tcg_init_cpu_signals();
>      qemu_thread_get_self(cpu->thread);
> +    qemu_thread_init_context();
>  
>      /* signal CPU creation */
>      qemu_mutex_lock(&qemu_global_mutex);
> diff --git a/exec.c b/exec.c
> index 46da08c..ea672c6 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3449,6 +3449,49 @@ static bool address_space_section_lookup_ref(AddressSpace *as,
>      return safe_ref;
>  }
>  
> +typedef struct ThreadContext {
> +  DispatchType dispatch_type;
> +  unsigned int mmio_req_pending;
> +} ThreadContext;
> +
> +static __thread ThreadContext *thread_context;
> +
> +void qemu_thread_init_context(void)
> +{
> +    thread_context = g_new(ThreadContext, 1);
> +    thread_context->dispatch_type = DISPATCH_INIT;
> +    thread_context->mmio_req_pending = 0;
> +}
> +
> +void qemu_thread_set_dispatch_type(DispatchType type)
> +{
> +    thread_context->dispatch_type = type;
> +}
> +
> +void qemu_thread_reset_dispatch_type(void)
> +{
> +    thread_context->dispatch_type = DISPATCH_INIT;
> +}
> +
> +static bool address_space_inc_req_pending(void)
> +{
> +    bool nested = false;
> +
> +    /* currently, only mmio out of big lock, and need this to avoid dead lock */
> +    if (thread_context->dispatch_type == DISPATCH_MMIO) {
> +        nested = ++thread_context->mmio_req_pending > 1 ? true : false;
> +    }
> +
> +    return nested;
> +}
> +
> +static void address_space_dec_req_pending(void)
> +{
> +    if (thread_context->dispatch_type == DISPATCH_MMIO) {
> +        thread_context->mmio_req_pending--;
> +    }
> +}
> +
>  void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
>                        int len, bool is_write)
>  {
> @@ -3459,6 +3502,7 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
>      target_phys_addr_t page;
>      bool safe_ref = false;
>      MemoryRegionSection *section, obj_mrs;
> +    bool nested_dma = false;
>  
>      while (len > 0) {
>          page = addr & TARGET_PAGE_MASK;
> @@ -3485,10 +3529,17 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
>              memory_region_section_lookup_ref(d, page, &obj_mrs);
>          }
>          section = &obj_mrs;
> +        nested_dma = address_space_inc_req_pending();
>  
>          if (is_write) {
>              if (!memory_region_is_ram(section->mr)) {
>                  target_phys_addr_t addr1;
> +
> +                /* To fix, will filter iommu case */
> +                if (nested_dma) {
> +                    fprintf(stderr, "can not support nested DMA");
> +                    abort();
> +                }
>                  addr1 = memory_region_section_addr(section, addr);
>                  /* XXX: could force cpu_single_env to NULL to avoid
>                     potential bugs */
> @@ -3522,6 +3573,12 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
>              if (!(memory_region_is_ram(section->mr) ||
>                    memory_region_is_romd(section->mr))) {
>                  target_phys_addr_t addr1;
> +
> +                /* To fix, will filter iommu case */
> +                if (nested_dma) {
> +                    fprintf(stderr, "can not support nested DMA");
> +                    abort();
> +                }
>                  /* I/O case */
>                  addr1 = memory_region_section_addr(section, addr);
>                  if (l >= 4 && ((addr1 & 3) == 0)) {
> @@ -3549,6 +3606,7 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
>                  qemu_put_ram_ptr(ptr);
>              }
>          }
> +        address_space_dec_req_pending();
>          memory_region_section_unref(&obj_mrs);
>          len -= l;
>          buf += l;
> diff --git a/qemu-thread.h b/qemu-thread.h
> index 05fdaaf..bb9535e 100644
> --- a/qemu-thread.h
> +++ b/qemu-thread.h
> @@ -7,6 +7,11 @@
>  typedef struct QemuMutex QemuMutex;
>  typedef struct QemuCond QemuCond;
>  typedef struct QemuThread QemuThread;
> +typedef enum {
> +  DISPATCH_INIT = 0,
> +  DISPATCH_MMIO,
> +  DISPATCH_IO,
> +} DispatchType;
>  
>  #ifdef _WIN32
>  #include "qemu-thread-win32.h"
> @@ -46,4 +51,7 @@ void qemu_thread_get_self(QemuThread *thread);
>  bool qemu_thread_is_self(QemuThread *thread);
>  void qemu_thread_exit(void *retval);
>  
> +void qemu_thread_init_context(void);
> +void qemu_thread_set_dispatch_type(DispatchType type);
> +void qemu_thread_reset_dispatch_type(void);
>  #endif
> diff --git a/vl.c b/vl.c
> index ee3c43a..be8d825 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3439,6 +3439,7 @@ int main(int argc, char **argv, char **envp)
>              add_device_config(DEV_VIRTCON, "vc:80Cx24C");
>      }
>  
> +    qemu_thread_init_context();
>      socket_init();
>  
>      if (qemu_opts_foreach(qemu_find_opts("chardev"), chardev_init_func, NULL, 1) != 0)
> 

In fact, this is not targeting nested DMA but nesting of memory region
callbacks, and there potential deadlock issue. Triggering a DMA with RAM
as target from within an MMIO (or later PIO) handler is perfectly fine
and not rejected here. You should clarify the scope and purpose of this
patch in its changelog.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [patch v5 7/8] memory: introduce tls context to record nested dma

[liu ping fan]

On Fri, Nov 2, 2012 at 6:39 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2012-10-29 00:48, Liu Ping Fan wrote:
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  cpus.c        |    3 ++
>>  exec.c        |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  qemu-thread.h |    8 +++++++
>>  vl.c          |    1 +
>>  4 files changed, 70 insertions(+), 0 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index 191cbf5..e67d80f 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -733,6 +733,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
>>
>>      qemu_mutex_lock(&qemu_global_mutex);
>>      qemu_thread_get_self(cpu->thread);
>> +    qemu_thread_init_context();
>>      env->thread_id = qemu_get_thread_id();
>>      cpu_single_env = env;
>>
>> @@ -774,6 +775,7 @@ static void *qemu_dummy_cpu_thread_fn(void *arg)
>>
>>      qemu_mutex_lock_iothread();
>>      qemu_thread_get_self(cpu->thread);
>> +    qemu_thread_init_context();
>>      env->thread_id = qemu_get_thread_id();
>>
>>      sigemptyset(&waitset);
>> @@ -813,6 +815,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>>
>>      qemu_tcg_init_cpu_signals();
>>      qemu_thread_get_self(cpu->thread);
>> +    qemu_thread_init_context();
>>
>>      /* signal CPU creation */
>>      qemu_mutex_lock(&qemu_global_mutex);
>> diff --git a/exec.c b/exec.c
>> index 46da08c..ea672c6 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -3449,6 +3449,49 @@ static bool address_space_section_lookup_ref(AddressSpace *as,
>>      return safe_ref;
>>  }
>>
>> +typedef struct ThreadContext {
>> +  DispatchType dispatch_type;
>> +  unsigned int mmio_req_pending;
>> +} ThreadContext;
>> +
>> +static __thread ThreadContext *thread_context;
>> +
>> +void qemu_thread_init_context(void)
>> +{
>> +    thread_context = g_new(ThreadContext, 1);
>> +    thread_context->dispatch_type = DISPATCH_INIT;
>> +    thread_context->mmio_req_pending = 0;
>> +}
>> +
>> +void qemu_thread_set_dispatch_type(DispatchType type)
>> +{
>> +    thread_context->dispatch_type = type;
>> +}
>> +
>> +void qemu_thread_reset_dispatch_type(void)
>> +{
>> +    thread_context->dispatch_type = DISPATCH_INIT;
>> +}
>> +
>> +static bool address_space_inc_req_pending(void)
>> +{
>> +    bool nested = false;
>> +
>> +    /* currently, only mmio out of big lock, and need this to avoid dead lock */
>> +    if (thread_context->dispatch_type == DISPATCH_MMIO) {
>> +        nested = ++thread_context->mmio_req_pending > 1 ? true : false;
>> +    }
>> +
>> +    return nested;
>> +}
>> +
>> +static void address_space_dec_req_pending(void)
>> +{
>> +    if (thread_context->dispatch_type == DISPATCH_MMIO) {
>> +        thread_context->mmio_req_pending--;
>> +    }
>> +}
>> +
>>  void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
>>                        int len, bool is_write)
>>  {
>> @@ -3459,6 +3502,7 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
>>      target_phys_addr_t page;
>>      bool safe_ref = false;
>>      MemoryRegionSection *section, obj_mrs;
>> +    bool nested_dma = false;
>>
>>      while (len > 0) {
>>          page = addr & TARGET_PAGE_MASK;
>> @@ -3485,10 +3529,17 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
>>              memory_region_section_lookup_ref(d, page, &obj_mrs);
>>          }
>>          section = &obj_mrs;
>> +        nested_dma = address_space_inc_req_pending();
>>
>>          if (is_write) {
>>              if (!memory_region_is_ram(section->mr)) {
>>                  target_phys_addr_t addr1;
>> +
>> +                /* To fix, will filter iommu case */
>> +                if (nested_dma) {
>> +                    fprintf(stderr, "can not support nested DMA");
>> +                    abort();
>> +                }
>>                  addr1 = memory_region_section_addr(section, addr);
>>                  /* XXX: could force cpu_single_env to NULL to avoid
>>                     potential bugs */
>> @@ -3522,6 +3573,12 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
>>              if (!(memory_region_is_ram(section->mr) ||
>>                    memory_region_is_romd(section->mr))) {
>>                  target_phys_addr_t addr1;
>> +
>> +                /* To fix, will filter iommu case */
>> +                if (nested_dma) {
>> +                    fprintf(stderr, "can not support nested DMA");
>> +                    abort();
>> +                }
>>                  /* I/O case */
>>                  addr1 = memory_region_section_addr(section, addr);
>>                  if (l >= 4 && ((addr1 & 3) == 0)) {
>> @@ -3549,6 +3606,7 @@ void address_space_rw(AddressSpace *as, target_phys_addr_t addr, uint8_t *buf,
>>                  qemu_put_ram_ptr(ptr);
>>              }
>>          }
>> +        address_space_dec_req_pending();
>>          memory_region_section_unref(&obj_mrs);
>>          len -= l;
>>          buf += l;
>> diff --git a/qemu-thread.h b/qemu-thread.h
>> index 05fdaaf..bb9535e 100644
>> --- a/qemu-thread.h
>> +++ b/qemu-thread.h
>> @@ -7,6 +7,11 @@
>>  typedef struct QemuMutex QemuMutex;
>>  typedef struct QemuCond QemuCond;
>>  typedef struct QemuThread QemuThread;
>> +typedef enum {
>> +  DISPATCH_INIT = 0,
>> +  DISPATCH_MMIO,
>> +  DISPATCH_IO,
>> +} DispatchType;
>>
>>  #ifdef _WIN32
>>  #include "qemu-thread-win32.h"
>> @@ -46,4 +51,7 @@ void qemu_thread_get_self(QemuThread *thread);
>>  bool qemu_thread_is_self(QemuThread *thread);
>>  void qemu_thread_exit(void *retval);
>>
>> +void qemu_thread_init_context(void);
>> +void qemu_thread_set_dispatch_type(DispatchType type);
>> +void qemu_thread_reset_dispatch_type(void);
>>  #endif
>> diff --git a/vl.c b/vl.c
>> index ee3c43a..be8d825 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -3439,6 +3439,7 @@ int main(int argc, char **argv, char **envp)
>>              add_device_config(DEV_VIRTCON, "vc:80Cx24C");
>>      }
>>
>> +    qemu_thread_init_context();
>>      socket_init();
>>
>>      if (qemu_opts_foreach(qemu_find_opts("chardev"), chardev_init_func, NULL, 1) != 0)
>>
>
> In fact, this is not targeting nested DMA but nesting of memory region
> callbacks, and there potential deadlock issue. Triggering a DMA with RAM
> as target from within an MMIO (or later PIO) handler is perfectly fine
> and not rejected here. You should clarify the scope and purpose of this
> patch in its changelog.
>
Applied, thanks.


> Jan
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux
>

Re: [Qemu-devel] [patch v5 7/8] memory: introduce tls context to record nested dma

[liu ping fan]

On Mon, Oct 29, 2012 at 4:51 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 29/10/2012 00:48, Liu Ping Fan ha scritto:
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  cpus.c        |    3 ++
>>  exec.c        |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  qemu-thread.h |    8 +++++++
>>  vl.c          |    1 +
>>  4 files changed, 70 insertions(+), 0 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index 191cbf5..e67d80f 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -733,6 +733,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
>>
>>      qemu_mutex_lock(&qemu_global_mutex);
>>      qemu_thread_get_self(cpu->thread);
>> +    qemu_thread_init_context();
>>      env->thread_id = qemu_get_thread_id();
>>      cpu_single_env = env;
>>
>> @@ -774,6 +775,7 @@ static void *qemu_dummy_cpu_thread_fn(void *arg)
>>
>>      qemu_mutex_lock_iothread();
>>      qemu_thread_get_self(cpu->thread);
>> +    qemu_thread_init_context();
>>      env->thread_id = qemu_get_thread_id();
>>
>>      sigemptyset(&waitset);
>> @@ -813,6 +815,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>>
>>      qemu_tcg_init_cpu_signals();
>>      qemu_thread_get_self(cpu->thread);
>> +    qemu_thread_init_context();
>>
>>      /* signal CPU creation */
>>      qemu_mutex_lock(&qemu_global_mutex);
>> diff --git a/exec.c b/exec.c
>> index 46da08c..ea672c6 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -3449,6 +3449,49 @@ static bool address_space_section_lookup_ref(AddressSpace *as,
>>      return safe_ref;
>>  }
>>
>> +typedef struct ThreadContext {
>> +  DispatchType dispatch_type;
>> +  unsigned int mmio_req_pending;
>> +} ThreadContext;
>> +
>> +static __thread ThreadContext *thread_context;
>> +
>> +void qemu_thread_init_context(void)
>> +{
>> +    thread_context = g_new(ThreadContext, 1);
>> +    thread_context->dispatch_type = DISPATCH_INIT;
>> +    thread_context->mmio_req_pending = 0;
>> +}
>
> No need for this:
>
> static __thread ThreadContext thread_context = {
>     .dispatch_type = DISPATCH_INIT,
>     .mmio_req_pending = 0
> };
>
applied, thanks.

> Paolo
>
>

Re: [Qemu-devel] [patch v5 5/8] memory: introduce local lock for address space

[Avi Kivity]

On 11/02/2012 10:00 AM, Jan Kiszka wrote:
> > 
> >> As I understand the series, as->lock == NULL means that we will never
> >> take any lock during dispatch as the caller is not yet ready for
> >> fine-grained locking. This prevents the problem - for PIO at least. But
> >> this series should break TCG as it calls into MMIO dispatch from the
> >> VCPU while holding the BQL.
> >>
> > What about add another condition "dispatch_type == DISPATCH_MMIO" to
> > tell this situation.
>
> An alternative pattern that we will also use for core services is to
> provide an additional entry point, one that indicates that the caller
> doesn't hold the BQL. Then we will gradually move things over until the
> existing entry point is obsolete.
>

I like it (or rather, least dislike it).  So we'd have
address_space_rw() and address_space_rw_unlocked() (or maybe,
address_space_rw() and address_space_rw_locked(), to indicate the
preferred way of doing things).

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.