* [Qemu-devel] [PATCH 1/8] memory: Add global-locking property to memory regions
2015-03-18 13:21 [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in KVM Paolo Bonzini
@ 2015-03-18 13:21 ` Paolo Bonzini
2015-03-18 13:21 ` [Qemu-devel] [PATCH 2/8] exec: move rcu_read_lock/unlock to address_space_translate callers Paolo Bonzini
` (7 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Paolo Bonzini @ 2015-03-18 13:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
From: Jan Kiszka <jan.kiszka@siemens.com>
This introduces the memory region property "global_locking". It is true
by default. By setting it to false, a device model can request BQL-free
dispatching of region accesses to its r/w handlers. The actual BQL
break-up will be provided in a separate patch.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
include/exec/memory.h | 26 ++++++++++++++++++++++++++
memory.c | 11 +++++++++++
2 files changed, 37 insertions(+)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 06ffa1d..0ee2079 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -158,6 +158,7 @@ struct MemoryRegion {
bool rom_device;
bool warning_printed; /* For reservations */
bool flush_coalesced_mmio;
+ bool global_locking;
MemoryRegion *alias;
hwaddr alias_offset;
int32_t priority;
@@ -778,6 +779,31 @@ void memory_region_set_flush_coalesced(MemoryRegion *mr);
void memory_region_clear_flush_coalesced(MemoryRegion *mr);
/**
+ * memory_region_set_global_locking: Declares the access processing requires
+ * QEMU's global lock.
+ *
+ * When this is invoked, access to this memory regions will be processed while
+ * holding the global lock of QEMU. This is the default behavior of memory
+ * regions.
+ *
+ * @mr: the memory region to be updated.
+ */
+void memory_region_set_global_locking(MemoryRegion *mr);
+
+/**
+ * memory_region_clear_global_locking: Declares that access processing does
+ * not depend on the QEMU global lock.
+ *
+ * By clearing this property, accesses to the memory region will be processed
+ * outside of QEMU's global lock (unless the lock is held on when issuing the
+ * access request). In this case, the device model implementing the access
+ * handlers is responsible for synchronization of concurrency.
+ *
+ * @mr: the memory region to be updated.
+ */
+void memory_region_clear_global_locking(MemoryRegion *mr);
+
+/**
* memory_region_add_eventfd: Request an eventfd to be triggered when a word
* is written to a location.
*
diff --git a/memory.c b/memory.c
index ee3f2a8..cc798e1 100644
--- a/memory.c
+++ b/memory.c
@@ -953,6 +953,7 @@ static void memory_region_initfn(Object *obj)
mr->ops = &unassigned_mem_ops;
mr->enabled = true;
mr->romd_mode = true;
+ mr->global_locking = true;
mr->destructor = memory_region_destructor_none;
QTAILQ_INIT(&mr->subregions);
QTAILQ_INIT(&mr->coalesced);
@@ -1549,6 +1550,16 @@ void memory_region_clear_flush_coalesced(MemoryRegion *mr)
}
}
+void memory_region_set_global_locking(MemoryRegion *mr)
+{
+ mr->global_locking = true;
+}
+
+void memory_region_clear_global_locking(MemoryRegion *mr)
+{
+ mr->global_locking = false;
+}
+
void memory_region_add_eventfd(MemoryRegion *mr,
hwaddr addr,
unsigned size,
--
2.3.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH 2/8] exec: move rcu_read_lock/unlock to address_space_translate callers
2015-03-18 13:21 [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in KVM Paolo Bonzini
2015-03-18 13:21 ` [Qemu-devel] [PATCH 1/8] memory: Add global-locking property to memory regions Paolo Bonzini
@ 2015-03-18 13:21 ` Paolo Bonzini
2015-03-19 13:27 ` Jan Kiszka
2015-03-18 13:21 ` [Qemu-devel] [PATCH 3/8] memory: Provide address_space_rw_unlocked Paolo Bonzini
` (6 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Paolo Bonzini @ 2015-03-18 13:21 UTC (permalink / raw)
To: qemu-devel
Once address_space_translate will be called outside the BQL, the returned
MemoryRegion might disappear as soon as the RCU read-side critical section
ends. Avoid this by moving the critical section to the callers.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
exec.c | 33 +++++++++++++++++++++++++++++----
hw/vfio/common.c | 7 +++++--
include/exec/memory.h | 3 ++-
translate-all.c | 3 +++
4 files changed, 39 insertions(+), 7 deletions(-)
diff --git a/exec.c b/exec.c
index 3552192..b264e76 100644
--- a/exec.c
+++ b/exec.c
@@ -373,6 +373,7 @@ static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
return false;
}
+/* Called from RCU critical section */
MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
hwaddr *xlat, hwaddr *plen,
bool is_write)
@@ -382,7 +383,6 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
MemoryRegion *mr;
hwaddr len = *plen;
- rcu_read_lock();
for (;;) {
AddressSpaceDispatch *d = atomic_rcu_read(&as->dispatch);
section = address_space_translate_internal(d, addr, &addr, plen, true);
@@ -411,7 +411,6 @@ MemoryRegion *address_space_translate(AddressSpace *as, hwaddr addr,
*plen = len;
*xlat = addr;
- rcu_read_unlock();
return mr;
}
@@ -2306,6 +2305,7 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
MemoryRegion *mr;
bool error = false;
+ rcu_read_lock();
while (len > 0) {
l = len;
mr = address_space_translate(as, addr, &addr1, &l, is_write);
@@ -2384,6 +2384,7 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
buf += l;
addr += l;
}
+ rcu_read_unlock();
return error;
}
@@ -2419,6 +2420,7 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
hwaddr addr1;
MemoryRegion *mr;
+ rcu_read_lock();
while (len > 0) {
l = len;
mr = address_space_translate(as, addr, &addr1, &l, true);
@@ -2444,6 +2446,7 @@ static inline void cpu_physical_memory_write_rom_internal(AddressSpace *as,
buf += l;
addr += l;
}
+ rcu_read_unlock();
}
/* used for ROM loading : can write in RAM and ROM */
@@ -2552,6 +2555,7 @@ bool address_space_access_valid(AddressSpace *as, hwaddr addr, int len, bool is_
MemoryRegion *mr;
hwaddr l, xlat;
+ rcu_read_lock();
while (len > 0) {
l = len;
mr = address_space_translate(as, addr, &xlat, &l, is_write);
@@ -2565,6 +2569,7 @@ bool address_space_access_valid(AddressSpace *as, hwaddr addr, int len, bool is_
len -= l;
addr += l;
}
+ rcu_read_unlock();
return true;
}
@@ -2591,9 +2596,12 @@ void *address_space_map(AddressSpace *as,
}
l = len;
+ rcu_read_lock();
mr = address_space_translate(as, addr, &xlat, &l, is_write);
+
if (!memory_access_is_direct(mr, is_write)) {
if (atomic_xchg(&bounce.in_use, true)) {
+ rcu_read_unlock();
return NULL;
}
/* Avoid unbounded allocations */
@@ -2608,6 +2616,7 @@ void *address_space_map(AddressSpace *as,
address_space_read(as, addr, bounce.buffer, l);
}
+ rcu_read_unlock();
*plen = l;
return bounce.buffer;
}
@@ -2631,6 +2640,7 @@ void *address_space_map(AddressSpace *as,
}
memory_region_ref(mr);
+ rcu_read_unlock();
*plen = done;
return qemu_ram_ptr_length(raddr + base, plen);
}
@@ -2690,6 +2700,7 @@ static inline uint32_t ldl_phys_internal(AddressSpace *as, hwaddr addr,
hwaddr l = 4;
hwaddr addr1;
+ rcu_read_lock();
mr = address_space_translate(as, addr, &addr1, &l, false);
if (l < 4 || !memory_access_is_direct(mr, false)) {
/* I/O case */
@@ -2720,6 +2731,7 @@ static inline uint32_t ldl_phys_internal(AddressSpace *as, hwaddr addr,
break;
}
}
+ rcu_read_unlock();
return val;
}
@@ -2748,6 +2760,7 @@ static inline uint64_t ldq_phys_internal(AddressSpace *as, hwaddr addr,
hwaddr l = 8;
hwaddr addr1;
+ rcu_read_lock();
mr = address_space_translate(as, addr, &addr1, &l,
false);
if (l < 8 || !memory_access_is_direct(mr, false)) {
@@ -2779,6 +2792,7 @@ static inline uint64_t ldq_phys_internal(AddressSpace *as, hwaddr addr,
break;
}
}
+ rcu_read_unlock();
return val;
}
@@ -2815,6 +2829,7 @@ static inline uint32_t lduw_phys_internal(AddressSpace *as, hwaddr addr,
hwaddr l = 2;
hwaddr addr1;
+ rcu_read_lock();
mr = address_space_translate(as, addr, &addr1, &l,
false);
if (l < 2 || !memory_access_is_direct(mr, false)) {
@@ -2846,6 +2861,7 @@ static inline uint32_t lduw_phys_internal(AddressSpace *as, hwaddr addr,
break;
}
}
+ rcu_read_unlock();
return val;
}
@@ -2874,6 +2890,7 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val)
hwaddr l = 4;
hwaddr addr1;
+ rcu_read_lock();
mr = address_space_translate(as, addr, &addr1, &l,
true);
if (l < 4 || !memory_access_is_direct(mr, true)) {
@@ -2892,6 +2909,7 @@ void stl_phys_notdirty(AddressSpace *as, hwaddr addr, uint32_t val)
}
}
}
+ rcu_read_unlock();
}
/* warning: addr must be aligned */
@@ -2904,6 +2922,7 @@ static inline void stl_phys_internal(AddressSpace *as,
hwaddr l = 4;
hwaddr addr1;
+ rcu_read_lock();
mr = address_space_translate(as, addr, &addr1, &l,
true);
if (l < 4 || !memory_access_is_direct(mr, true)) {
@@ -2934,6 +2953,7 @@ static inline void stl_phys_internal(AddressSpace *as,
}
invalidate_and_set_dirty(addr1, 4);
}
+ rcu_read_unlock();
}
void stl_phys(AddressSpace *as, hwaddr addr, uint32_t val)
@@ -2968,6 +2988,7 @@ static inline void stw_phys_internal(AddressSpace *as,
hwaddr l = 2;
hwaddr addr1;
+ rcu_read_lock();
mr = address_space_translate(as, addr, &addr1, &l, true);
if (l < 2 || !memory_access_is_direct(mr, true)) {
#if defined(TARGET_WORDS_BIGENDIAN)
@@ -2997,6 +3018,7 @@ static inline void stw_phys_internal(AddressSpace *as,
}
invalidate_and_set_dirty(addr1, 2);
}
+ rcu_read_unlock();
}
void stw_phys(AddressSpace *as, hwaddr addr, uint32_t val)
@@ -3083,12 +3105,15 @@ bool cpu_physical_memory_is_io(hwaddr phys_addr)
{
MemoryRegion*mr;
hwaddr l = 1;
+ bool res;
+ rcu_read_lock();
mr = address_space_translate(&address_space_memory,
phys_addr, &phys_addr, &l, false);
- return !(memory_region_is_ram(mr) ||
- memory_region_is_romd(mr));
+ res = !(memory_region_is_ram(mr) || memory_region_is_romd(mr));
+ rcu_read_unlock();
+ return res;
}
void qemu_ram_foreach_block(RAMBlockIterFunc func, void *opaque)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index b012620..b1045da 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -270,13 +270,14 @@ static void vfio_iommu_map_notify(Notifier *n, void *data)
* this IOMMU to its immediate target. We need to translate
* it the rest of the way through to memory.
*/
+ rcu_read_lock();
mr = address_space_translate(&address_space_memory,
iotlb->translated_addr,
&xlat, &len, iotlb->perm & IOMMU_WO);
if (!memory_region_is_ram(mr)) {
error_report("iommu map to non memory area %"HWADDR_PRIx"",
xlat);
- return;
+ goto out;
}
/*
* Translation truncates length to the IOMMU page size,
@@ -284,7 +285,7 @@ static void vfio_iommu_map_notify(Notifier *n, void *data)
*/
if (len & iotlb->addr_mask) {
error_report("iommu has granularity incompatible with target AS");
- return;
+ goto out;
}
if ((iotlb->perm & IOMMU_RW) != IOMMU_NONE) {
@@ -307,6 +308,8 @@ static void vfio_iommu_map_notify(Notifier *n, void *data)
iotlb->addr_mask + 1, ret);
}
}
+out:
+ rcu_read_unlock();
}
static void vfio_listener_region_add(MemoryListener *listener,
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 0ee2079..0644dc6 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1118,7 +1118,8 @@ bool address_space_write(AddressSpace *as, hwaddr addr,
bool address_space_read(AddressSpace *as, hwaddr addr, uint8_t *buf, int len);
/* address_space_translate: translate an address range into an address space
- * into a MemoryRegion and an address range into that section
+ * into a MemoryRegion and an address range into that section. Add a reference
+ * to that region.
*
* @as: #AddressSpace to be accessed
* @addr: address within that address space
diff --git a/translate-all.c b/translate-all.c
index 9f47ce7..9ee8aa6 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1458,14 +1458,17 @@ void tb_invalidate_phys_addr(AddressSpace *as, hwaddr addr)
MemoryRegion *mr;
hwaddr l = 1;
+ rcu_read_lock();
mr = address_space_translate(as, addr, &addr, &l, false);
if (!(memory_region_is_ram(mr)
|| memory_region_is_romd(mr))) {
+ rcu_read_unlock();
return;
}
ram_addr = (memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK)
+ addr;
tb_invalidate_phys_page_range(ram_addr, ram_addr + 1, 0);
+ rcu_read_unlock();
}
#endif /* !defined(CONFIG_USER_ONLY) */
--
2.3.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 2/8] exec: move rcu_read_lock/unlock to address_space_translate callers
2015-03-18 13:21 ` [Qemu-devel] [PATCH 2/8] exec: move rcu_read_lock/unlock to address_space_translate callers Paolo Bonzini
@ 2015-03-19 13:27 ` Jan Kiszka
2015-03-19 14:55 ` Jan Kiszka
0 siblings, 1 reply; 18+ messages in thread
From: Jan Kiszka @ 2015-03-19 13:27 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
On 2015-03-18 14:21, Paolo Bonzini wrote:
> @@ -2591,9 +2596,12 @@ void *address_space_map(AddressSpace *as,
> }
>
> l = len;
> + rcu_read_lock();
> mr = address_space_translate(as, addr, &xlat, &l, is_write);
> +
> if (!memory_access_is_direct(mr, is_write)) {
> if (atomic_xchg(&bounce.in_use, true)) {
Doesn't apply due to differences in the line above.
What is this patch based on? Do you have a public branch where I can
find the series and its dependencies?
> + rcu_read_unlock();
> return NULL;
> }
> /* Avoid unbounded allocations */
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 2/8] exec: move rcu_read_lock/unlock to address_space_translate callers
2015-03-19 13:27 ` Jan Kiszka
@ 2015-03-19 14:55 ` Jan Kiszka
2015-03-19 16:23 ` Paolo Bonzini
0 siblings, 1 reply; 18+ messages in thread
From: Jan Kiszka @ 2015-03-19 14:55 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
On 2015-03-19 14:27, Jan Kiszka wrote:
> On 2015-03-18 14:21, Paolo Bonzini wrote:
>> @@ -2591,9 +2596,12 @@ void *address_space_map(AddressSpace *as,
>> }
>>
>> l = len;
>> + rcu_read_lock();
>> mr = address_space_translate(as, addr, &xlat, &l, is_write);
>> +
>> if (!memory_access_is_direct(mr, is_write)) {
>> if (atomic_xchg(&bounce.in_use, true)) {
>
> Doesn't apply due to differences in the line above.
>
> What is this patch based on? Do you have a public branch where I can
> find the series and its dependencies?
Think I found the series:
http://thread.gmane.org/gmane.comp.emulators.qemu/326076. At least it
builds and runs.
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH 3/8] memory: Provide address_space_rw_unlocked
2015-03-18 13:21 [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in KVM Paolo Bonzini
2015-03-18 13:21 ` [Qemu-devel] [PATCH 1/8] memory: Add global-locking property to memory regions Paolo Bonzini
2015-03-18 13:21 ` [Qemu-devel] [PATCH 2/8] exec: move rcu_read_lock/unlock to address_space_translate callers Paolo Bonzini
@ 2015-03-18 13:21 ` Paolo Bonzini
2015-03-18 13:21 ` [Qemu-devel] [PATCH 4/8] kvm: First step to push iothread lock out of inner run loop Paolo Bonzini
` (5 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Paolo Bonzini @ 2015-03-18 13:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
From: Jan Kiszka <jan.kiszka@siemens.com>
Break up address_space_rw into two version: The standard one continues
to require the caller to hold BQL on invocation, the unlocked one takes
or avoids BQL depending on the locking strategy of the target memory
region and its coalesced MMIO handling.
As memory_region_read/write_accessor may now also run without BQL held,
we need to move coalesced MMIO flushing earlier in the dispatch process.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
exec.c | 41 +++++++++++++++++++++++++++++++++++++++--
include/exec/memory.h | 19 +++++++++++++++++++
memory.c | 6 ------
3 files changed, 58 insertions(+), 8 deletions(-)
diff --git a/exec.c b/exec.c
index b264e76..d60abfc 100644
--- a/exec.c
+++ b/exec.c
@@ -48,6 +48,7 @@
#endif
#include "exec/cpu-all.h"
#include "qemu/rcu_queue.h"
+#include "qemu/main-loop.h"
#include "exec/cputlb.h"
#include "translate-all.h"
@@ -2295,8 +2296,9 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
return l;
}
-bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
- int len, bool is_write)
+static bool address_space_rw_internal(AddressSpace *as, hwaddr addr,
+ uint8_t *buf, int len, bool is_write,
+ bool unlocked)
{
hwaddr l;
uint8_t *ptr;
@@ -2304,12 +2306,29 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
hwaddr addr1;
MemoryRegion *mr;
bool error = false;
+ bool release_lock;
rcu_read_lock();
while (len > 0) {
l = len;
mr = address_space_translate(as, addr, &addr1, &l, is_write);
+ release_lock = false;
+ if (unlocked && mr->global_locking) {
+ qemu_mutex_lock_iothread();
+ unlocked = false;
+ release_lock = true;
+ }
+ if (mr->flush_coalesced_mmio) {
+ if (unlocked) {
+ qemu_mutex_lock_iothread();
+ }
+ qemu_flush_coalesced_mmio_buffer();
+ if (unlocked) {
+ qemu_mutex_unlock_iothread();
+ }
+ }
+
if (is_write) {
if (!memory_access_is_direct(mr, is_write)) {
l = memory_access_size(mr, l, addr1);
@@ -2380,6 +2399,12 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
memcpy(buf, ptr, l);
}
}
+
+ if (release_lock) {
+ qemu_mutex_unlock_iothread();
+ unlocked = true;
+ }
+
len -= l;
buf += l;
addr += l;
@@ -2389,6 +2414,18 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
return error;
}
+bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
+ int len, bool is_write)
+{
+ return address_space_rw_internal(as, addr, buf, len, is_write, false);
+}
+
+bool address_space_rw_unlocked(AddressSpace *as, hwaddr addr, uint8_t *buf,
+ int len, bool is_write)
+{
+ return address_space_rw_internal(as, addr, buf, len, is_write, true);
+}
+
bool address_space_write(AddressSpace *as, hwaddr addr,
const uint8_t *buf, int len)
{
diff --git a/include/exec/memory.h b/include/exec/memory.h
index 0644dc6..cf39a40 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1084,6 +1084,8 @@ void address_space_destroy(AddressSpace *as);
* Return true if the operation hit any unassigned memory or encountered an
* IOMMU fault.
*
+ * NOTE: The iothread lock must be held when calling this function.
+ *
* @as: #AddressSpace to be accessed
* @addr: address within that address space
* @buf: buffer with the data transferred
@@ -1093,6 +1095,23 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
int len, bool is_write);
/**
+ * address_space_rw_unlocked: read from or write to an address space outside
+ * of the iothread lock.
+ *
+ * Return true if the operation hit any unassigned memory or encountered an
+ * IOMMU fault.
+ *
+ * NOTE: The iothread lock *must not* be held when calling this function.
+ *
+ * @as: #AddressSpace to be accessed
+ * @addr: address within that address space
+ * @buf: buffer with the data transferred
+ * @is_write: indicates the transfer direction
+ */
+bool address_space_rw_unlocked(AddressSpace *as, hwaddr addr, uint8_t *buf,
+ int len, bool is_write);
+
+/**
* address_space_write: write to address space.
*
* Return true if the operation hit any unassigned memory or encountered an
diff --git a/memory.c b/memory.c
index cc798e1..838e2f1 100644
--- a/memory.c
+++ b/memory.c
@@ -391,9 +391,6 @@ static void memory_region_read_accessor(MemoryRegion *mr,
{
uint64_t tmp;
- if (mr->flush_coalesced_mmio) {
- qemu_flush_coalesced_mmio_buffer();
- }
tmp = mr->ops->read(mr->opaque, addr, size);
trace_memory_region_ops_read(mr, addr, tmp, size);
*value |= (tmp & mask) << shift;
@@ -422,9 +419,6 @@ static void memory_region_write_accessor(MemoryRegion *mr,
{
uint64_t tmp;
- if (mr->flush_coalesced_mmio) {
- qemu_flush_coalesced_mmio_buffer();
- }
tmp = (*value >> shift) & mask;
trace_memory_region_ops_write(mr, addr, tmp, size);
mr->ops->write(mr->opaque, addr, tmp, size);
--
2.3.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH 4/8] kvm: First step to push iothread lock out of inner run loop
2015-03-18 13:21 [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in KVM Paolo Bonzini
` (2 preceding siblings ...)
2015-03-18 13:21 ` [Qemu-devel] [PATCH 3/8] memory: Provide address_space_rw_unlocked Paolo Bonzini
@ 2015-03-18 13:21 ` Paolo Bonzini
2015-03-18 13:21 ` [Qemu-devel] [PATCH 5/8] kvm: Switch to unlocked PIO Paolo Bonzini
` (4 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Paolo Bonzini @ 2015-03-18 13:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
From: Jan Kiszka <jan.kiszka@siemens.com>
This opens the path to get rid of the iothread lock on vmexits in KVM
mode. On x86, the in-kernel irqchips has to be used because we otherwise
need to synchronize APIC and other per-cpu state accesses that could be
changed concurrently.
s390x and ARM should be fine without specific locking as their
pre/post-run callbacks are empty. MIPS and POWER require locking for
the pre-run callback.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
kvm-all.c | 14 ++++++++++++--
target-i386/kvm.c | 18 ++++++++++++++++++
target-mips/kvm.c | 4 ++++
target-ppc/kvm.c | 4 ++++
4 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/kvm-all.c b/kvm-all.c
index 55025cc..8da1deb 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1765,6 +1765,8 @@ int kvm_cpu_exec(CPUState *cpu)
return EXCP_HLT;
}
+ qemu_mutex_unlock_iothread();
+
do {
if (cpu->kvm_vcpu_dirty) {
kvm_arch_put_registers(cpu, KVM_PUT_RUNTIME_STATE);
@@ -1781,11 +1783,9 @@ int kvm_cpu_exec(CPUState *cpu)
*/
qemu_cpu_kick_self();
}
- qemu_mutex_unlock_iothread();
run_ret = kvm_vcpu_ioctl(cpu, KVM_RUN, 0);
- qemu_mutex_lock_iothread();
kvm_arch_post_run(cpu, run);
if (run_ret < 0) {
@@ -1804,19 +1804,23 @@ int kvm_cpu_exec(CPUState *cpu)
switch (run->exit_reason) {
case KVM_EXIT_IO:
DPRINTF("handle_io\n");
+ qemu_mutex_lock_iothread();
kvm_handle_io(run->io.port,
(uint8_t *)run + run->io.data_offset,
run->io.direction,
run->io.size,
run->io.count);
+ qemu_mutex_unlock_iothread();
ret = 0;
break;
case KVM_EXIT_MMIO:
DPRINTF("handle_mmio\n");
+ qemu_mutex_lock_iothread();
cpu_physical_memory_rw(run->mmio.phys_addr,
run->mmio.data,
run->mmio.len,
run->mmio.is_write);
+ qemu_mutex_unlock_iothread();
ret = 0;
break;
case KVM_EXIT_IRQ_WINDOW_OPEN:
@@ -1825,7 +1829,9 @@ int kvm_cpu_exec(CPUState *cpu)
break;
case KVM_EXIT_SHUTDOWN:
DPRINTF("shutdown\n");
+ qemu_mutex_lock_iothread();
qemu_system_reset_request();
+ qemu_mutex_unlock_iothread();
ret = EXCP_INTERRUPT;
break;
case KVM_EXIT_UNKNOWN:
@@ -1854,11 +1860,15 @@ int kvm_cpu_exec(CPUState *cpu)
break;
default:
DPRINTF("kvm_arch_handle_exit\n");
+ qemu_mutex_lock_iothread();
ret = kvm_arch_handle_exit(cpu, run);
+ qemu_mutex_unlock_iothread();
break;
}
} while (ret == 0);
+ qemu_mutex_lock_iothread();
+
if (ret < 0) {
cpu_dump_state(cpu, stderr, fprintf, CPU_DUMP_CODE);
vm_stop(RUN_STATE_INTERNAL_ERROR);
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 41d09e5..94cff8c 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -2191,7 +2191,10 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
/* Inject NMI */
if (cpu->interrupt_request & CPU_INTERRUPT_NMI) {
+ qemu_mutex_lock_iothread();
cpu->interrupt_request &= ~CPU_INTERRUPT_NMI;
+ qemu_mutex_unlock_iothread();
+
DPRINTF("injected NMI\n");
ret = kvm_vcpu_ioctl(cpu, KVM_NMI);
if (ret < 0) {
@@ -2200,6 +2203,10 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
}
}
+ if (!kvm_irqchip_in_kernel()) {
+ qemu_mutex_lock_iothread();
+ }
+
/* Force the VCPU out of its inner loop to process any INIT requests
* or (for userspace APIC, but it is cheap to combine the checks here)
* pending TPR access reports.
@@ -2243,6 +2250,8 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
DPRINTF("setting tpr\n");
run->cr8 = cpu_get_apic_tpr(x86_cpu->apic_state);
+
+ qemu_mutex_unlock_iothread();
}
}
@@ -2256,8 +2265,17 @@ void kvm_arch_post_run(CPUState *cpu, struct kvm_run *run)
} else {
env->eflags &= ~IF_MASK;
}
+
+ /* We need to protect the apic state against concurrent accesses from
+ * different threads in case the userspace irqchip is used. */
+ if (!kvm_irqchip_in_kernel()) {
+ qemu_mutex_lock_iothread();
+ }
cpu_set_apic_tpr(x86_cpu->apic_state, run->cr8);
cpu_set_apic_base(x86_cpu->apic_state, run->apic_base);
+ if (!kvm_irqchip_in_kernel()) {
+ qemu_mutex_unlock_iothread();
+ }
}
int kvm_arch_process_async_events(CPUState *cs)
diff --git a/target-mips/kvm.c b/target-mips/kvm.c
index 4d1f7ea..7f02be7 100644
--- a/target-mips/kvm.c
+++ b/target-mips/kvm.c
@@ -98,6 +98,8 @@ void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run)
int r;
struct kvm_mips_interrupt intr;
+ qemu_mutex_lock_iothread();
+
if ((cs->interrupt_request & CPU_INTERRUPT_HARD) &&
cpu_mips_io_interrupts_pending(cpu)) {
intr.cpu = -1;
@@ -108,6 +110,8 @@ void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run)
__func__, cs->cpu_index, intr.irq);
}
}
+
+ qemu_mutex_unlock_iothread();
}
void kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 12328a4..ce2498a 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1241,6 +1241,8 @@ void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run)
int r;
unsigned irq;
+ qemu_mutex_lock_iothread();
+
/* PowerPC QEMU tracks the various core input pins (interrupt, critical
* interrupt, reset, etc) in PPC-specific env->irq_input_state. */
if (!cap_interrupt_level &&
@@ -1268,6 +1270,8 @@ void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run)
/* We don't know if there are more interrupts pending after this. However,
* the guest will return to userspace in the course of handling this one
* anyways, so we will get a chance to deliver the rest. */
+
+ qemu_mutex_unlock_iothread();
}
void kvm_arch_post_run(CPUState *cpu, struct kvm_run *run)
--
2.3.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH 5/8] kvm: Switch to unlocked PIO
2015-03-18 13:21 [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in KVM Paolo Bonzini
` (3 preceding siblings ...)
2015-03-18 13:21 ` [Qemu-devel] [PATCH 4/8] kvm: First step to push iothread lock out of inner run loop Paolo Bonzini
@ 2015-03-18 13:21 ` Paolo Bonzini
2015-03-18 13:21 ` [Qemu-devel] [PATCH 6/8] exec: mark unassigned_io_ops as unlocked Paolo Bonzini
` (3 subsequent siblings)
8 siblings, 0 replies; 18+ messages in thread
From: Paolo Bonzini @ 2015-03-18 13:21 UTC (permalink / raw)
To: qemu-devel; +Cc: Jan Kiszka
From: Jan Kiszka <jan.kiszka@siemens.com>
Do not take the BQL before dispatching PIO requests of KVM VCPUs.
Instead, call the unlocked version of address_space_rw. This enables
completely BQL-free PIO handling in KVM mode for upcoming devices with
fine-grained locking.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
kvm-all.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/kvm-all.c b/kvm-all.c
index 8da1deb..2848e5b 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1647,8 +1647,8 @@ static void kvm_handle_io(uint16_t port, void *data, int direction, int size,
uint8_t *ptr = data;
for (i = 0; i < count; i++) {
- address_space_rw(&address_space_io, port, ptr, size,
- direction == KVM_EXIT_IO_OUT);
+ address_space_rw_unlocked(&address_space_io, port, ptr, size,
+ direction == KVM_EXIT_IO_OUT);
ptr += size;
}
}
@@ -1804,13 +1804,11 @@ int kvm_cpu_exec(CPUState *cpu)
switch (run->exit_reason) {
case KVM_EXIT_IO:
DPRINTF("handle_io\n");
- qemu_mutex_lock_iothread();
kvm_handle_io(run->io.port,
(uint8_t *)run + run->io.data_offset,
run->io.direction,
run->io.size,
run->io.count);
- qemu_mutex_unlock_iothread();
ret = 0;
break;
case KVM_EXIT_MMIO:
--
2.3.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH 6/8] exec: mark unassigned_io_ops as unlocked
2015-03-18 13:21 [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in KVM Paolo Bonzini
` (4 preceding siblings ...)
2015-03-18 13:21 ` [Qemu-devel] [PATCH 5/8] kvm: Switch to unlocked PIO Paolo Bonzini
@ 2015-03-18 13:21 ` Paolo Bonzini
2015-03-18 14:33 ` Jan Kiszka
2015-03-18 13:21 ` [Qemu-devel] [PATCH 7/8] acpi: mark PMTIMER " Paolo Bonzini
` (2 subsequent siblings)
8 siblings, 1 reply; 18+ messages in thread
From: Paolo Bonzini @ 2015-03-18 13:21 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
exec.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/exec.c b/exec.c
index d60abfc..c1a2e9e 100644
--- a/exec.c
+++ b/exec.c
@@ -2197,6 +2197,7 @@ static void memory_map_init(void)
system_io = g_malloc(sizeof(*system_io));
memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
65536);
+ memory_region_clear_global_locking(system_io);
address_space_init(&address_space_io, system_io, "I/O");
memory_listener_register(&core_memory_listener, &address_space_memory);
--
2.3.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH 6/8] exec: mark unassigned_io_ops as unlocked
2015-03-18 13:21 ` [Qemu-devel] [PATCH 6/8] exec: mark unassigned_io_ops as unlocked Paolo Bonzini
@ 2015-03-18 14:33 ` Jan Kiszka
2015-03-18 14:53 ` Paolo Bonzini
0 siblings, 1 reply; 18+ messages in thread
From: Jan Kiszka @ 2015-03-18 14:33 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
On 2015-03-18 14:21, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
A commit log should briefly state why we can do this safely (no state
dependency, no need for locking). Yes, it's trivial in this case but
already a little bit less trivial in case of the PM timer.
> ---
> exec.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/exec.c b/exec.c
> index d60abfc..c1a2e9e 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2197,6 +2197,7 @@ static void memory_map_init(void)
> system_io = g_malloc(sizeof(*system_io));
> memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
> 65536);
> + memory_region_clear_global_locking(system_io);
> address_space_init(&address_space_io, system_io, "I/O");
>
> memory_listener_register(&core_memory_listener, &address_space_memory);
>
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH 6/8] exec: mark unassigned_io_ops as unlocked
2015-03-18 14:33 ` Jan Kiszka
@ 2015-03-18 14:53 ` Paolo Bonzini
0 siblings, 0 replies; 18+ messages in thread
From: Paolo Bonzini @ 2015-03-18 14:53 UTC (permalink / raw)
To: Jan Kiszka, qemu-devel
On 18/03/2015 15:33, Jan Kiszka wrote:
> On 2015-03-18 14:21, Paolo Bonzini wrote:
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> A commit log should briefly state why we can do this safely (no state
> dependency, no need for locking). Yes, it's trivial in this case but
> already a little bit less trivial in case of the PM timer.
Guilty as charged...
Paolo
>> ---
>> exec.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/exec.c b/exec.c
>> index d60abfc..c1a2e9e 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -2197,6 +2197,7 @@ static void memory_map_init(void)
>> system_io = g_malloc(sizeof(*system_io));
>> memory_region_init_io(system_io, NULL, &unassigned_io_ops, NULL, "io",
>> 65536);
>> + memory_region_clear_global_locking(system_io);
>> address_space_init(&address_space_io, system_io, "I/O");
>>
>> memory_listener_register(&core_memory_listener, &address_space_memory);
>>
>
> Jan
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [Qemu-devel] [PATCH 7/8] acpi: mark PMTIMER as unlocked
2015-03-18 13:21 [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in KVM Paolo Bonzini
` (5 preceding siblings ...)
2015-03-18 13:21 ` [Qemu-devel] [PATCH 6/8] exec: mark unassigned_io_ops as unlocked Paolo Bonzini
@ 2015-03-18 13:21 ` Paolo Bonzini
2015-03-18 13:21 ` [Qemu-devel] [PATCH 8/8] kvm: Switch to unlocked MMIO Paolo Bonzini
2015-03-18 14:33 ` [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in KVM Jan Kiszka
8 siblings, 0 replies; 18+ messages in thread
From: Paolo Bonzini @ 2015-03-18 13:21 UTC (permalink / raw)
To: qemu-devel
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
hw/acpi/core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 51913d6..954a96b 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -527,6 +527,7 @@ void acpi_pm_tmr_init(ACPIREGS *ar, acpi_update_sci_fn update_sci,
ar->tmr.timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, acpi_pm_tmr_timer, ar);
memory_region_init_io(&ar->tmr.io, memory_region_owner(parent),
&acpi_pm_tmr_ops, ar, "acpi-tmr", 4);
+ memory_region_clear_global_locking(&ar->tmr.io);
memory_region_add_subregion(parent, 8, &ar->tmr.io);
}
--
2.3.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* [Qemu-devel] [PATCH 8/8] kvm: Switch to unlocked MMIO
2015-03-18 13:21 [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in KVM Paolo Bonzini
` (6 preceding siblings ...)
2015-03-18 13:21 ` [Qemu-devel] [PATCH 7/8] acpi: mark PMTIMER " Paolo Bonzini
@ 2015-03-18 13:21 ` Paolo Bonzini
2015-03-18 14:33 ` [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in KVM Jan Kiszka
8 siblings, 0 replies; 18+ messages in thread
From: Paolo Bonzini @ 2015-03-18 13:21 UTC (permalink / raw)
To: qemu-devel
Do not take the BQL before dispatching MMIO requests of KVM VCPUs.
Instead, call the unlocked version of address_space_rw. This enables
completely BQL-free MMIO handling in KVM mode for upcoming devices with
fine-grained locking.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
kvm-all.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/kvm-all.c b/kvm-all.c
index 2848e5b..fa0cfed 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1813,12 +1813,11 @@ int kvm_cpu_exec(CPUState *cpu)
break;
case KVM_EXIT_MMIO:
DPRINTF("handle_mmio\n");
- qemu_mutex_lock_iothread();
- cpu_physical_memory_rw(run->mmio.phys_addr,
- run->mmio.data,
- run->mmio.len,
- run->mmio.is_write);
- qemu_mutex_unlock_iothread();
+ address_space_rw_unlocked(&address_space_memory,
+ run->mmio.phys_addr,
+ run->mmio.data,
+ run->mmio.len,
+ run->mmio.is_write);
ret = 0;
break;
case KVM_EXIT_IRQ_WINDOW_OPEN:
--
2.3.0
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in KVM
2015-03-18 13:21 [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in KVM Paolo Bonzini
` (7 preceding siblings ...)
2015-03-18 13:21 ` [Qemu-devel] [PATCH 8/8] kvm: Switch to unlocked MMIO Paolo Bonzini
@ 2015-03-18 14:33 ` Jan Kiszka
2015-03-18 14:52 ` Paolo Bonzini
8 siblings, 1 reply; 18+ messages in thread
From: Jan Kiszka @ 2015-03-18 14:33 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
On 2015-03-18 14:21, Paolo Bonzini wrote:
> And here we are... These are the changes required to make the BQL
> optional for memory access, and use that support in KVM. For now,
> only one device model is changed to do unlocked accesses.
>
> Please review!
>
> Jan Kiszka (4):
> memory: Add global-locking property to memory regions
> memory: Provide address_space_rw_unlocked
> kvm: First step to push iothread lock out of inner run loop
> kvm: Switch to unlocked PIO
>
> Paolo Bonzini (4):
> exec: move rcu_read_lock/unlock to address_space_translate callers
> exec: mark unassigned_io_ops as unlocked
> acpi: mark PMTIMER as unlocked
> kvm: Switch to unlocked MMIO
>
> exec.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++-----
> hw/acpi/core.c | 1 +
> hw/vfio/common.c | 7 +++--
> include/exec/memory.h | 48 ++++++++++++++++++++++++++++++++-
> kvm-all.c | 23 ++++++++++------
> memory.c | 17 +++++++-----
> target-i386/kvm.c | 18 +++++++++++++
> target-mips/kvm.c | 4 +++
> target-ppc/kvm.c | 4 +++
> translate-all.c | 3 +++
> 10 files changed, 177 insertions(+), 23 deletions(-)
>
Just in time: I'm planning to rebase our queue soon, specifically to
benefit from RCU support. Will let you know if it works on top of this
series.
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in KVM
2015-03-18 14:33 ` [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in KVM Jan Kiszka
@ 2015-03-18 14:52 ` Paolo Bonzini
2015-03-18 15:04 ` Jan Kiszka
0 siblings, 1 reply; 18+ messages in thread
From: Paolo Bonzini @ 2015-03-18 14:52 UTC (permalink / raw)
To: Jan Kiszka, qemu-devel
On 18/03/2015 15:33, Jan Kiszka wrote:
> Just in time: I'm planning to rebase our queue soon, specifically to
> benefit from RCU support. Will let you know if it works on top of this
> series.
Great. FWIW, this is the most similar version to the one we played with
in 2013.
I have an alternative one that keeps the single address_space_rw API,
and checks if you have the iothread mutex using thread-local storage.
The reason for that is that I would have to introduce
address_space_map/unmap_unlocked too, which I didn't really like. Also,
in the not-so-long term we want the same code (e.g. SCSI layer) to run
in both locked and unlocked mode.
What do you think?
Paolo
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in KVM
2015-03-18 14:52 ` Paolo Bonzini
@ 2015-03-18 15:04 ` Jan Kiszka
2015-03-19 8:52 ` Paolo Bonzini
0 siblings, 1 reply; 18+ messages in thread
From: Jan Kiszka @ 2015-03-18 15:04 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel
On 2015-03-18 15:52, Paolo Bonzini wrote:
>
>
> On 18/03/2015 15:33, Jan Kiszka wrote:
>> Just in time: I'm planning to rebase our queue soon, specifically to
>> benefit from RCU support. Will let you know if it works on top of this
>> series.
>
> Great. FWIW, this is the most similar version to the one we played with
> in 2013.
>
> I have an alternative one that keeps the single address_space_rw API,
> and checks if you have the iothread mutex using thread-local storage.
qemu_mutex_iothread_is_locked() with a qemu_global_mutex-specific flag
or qemu_mutex_is_locked(&qemu_global_mutex)?
> The reason for that is that I would have to introduce
> address_space_map/unmap_unlocked too, which I didn't really like. Also,
> in the not-so-long term we want the same code (e.g. SCSI layer) to run
> in both locked and unlocked mode.
>
> What do you think?
I cannot envision the code differences yet as we didn't have the need to
play with lockless MMIO or even DMA so far (will probably happen soon,
though - for networking). For me it boils down to the code impact as
well, how much we can reuse by hiding locked vs. unlocked mode, and how
much we may make more fragile and harder to design correctly by doing so.
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.4 0/8] memory: enable unlocked PIO/MMIO in KVM
2015-03-18 15:04 ` Jan Kiszka
@ 2015-03-19 8:52 ` Paolo Bonzini
0 siblings, 0 replies; 18+ messages in thread
From: Paolo Bonzini @ 2015-03-19 8:52 UTC (permalink / raw)
To: Jan Kiszka, qemu-devel
On 18/03/2015 16:04, Jan Kiszka wrote:
> On 2015-03-18 15:52, Paolo Bonzini wrote:
>>
>>
>> On 18/03/2015 15:33, Jan Kiszka wrote:
>>> Just in time: I'm planning to rebase our queue soon, specifically to
>>> benefit from RCU support. Will let you know if it works on top of this
>>> series.
>>
>> Great. FWIW, this is the most similar version to the one we played with
>> in 2013.
>>
>> I have an alternative one that keeps the single address_space_rw API,
>> and checks if you have the iothread mutex using thread-local storage.
>
> qemu_mutex_iothread_is_locked() with a qemu_global_mutex-specific flag
> or qemu_mutex_is_locked(&qemu_global_mutex)?
qemu_mutex_iothread_is_locked().
>> The reason for that is that I would have to introduce
>> address_space_map/unmap_unlocked too, which I didn't really like. Also,
>> in the not-so-long term we want the same code (e.g. SCSI layer) to run
>> in both locked and unlocked mode.
>>
>> What do you think?
>
> I cannot envision the code differences yet as we didn't have the need to
> play with lockless MMIO or even DMA so far (will probably happen soon,
> though - for networking). For me it boils down to the code impact as
> well, how much we can reuse by hiding locked vs. unlocked mode, and how
> much we may make more fragile and harder to design correctly by doing so.
The main reason, for me, is that I would have to either cut-and-paste
code between address_space_map and address_space_map_unlocked, or I
would have to use address_space_map_internal. And so on through the
whole stack, and at some point the _internal calls are not so _internal
anymore.
So at least _internal could be renamed, but I am not sure it's the best
choice.
The locking policy is easy: either call it with the BQL held (and any
other locks you care about), or call it with no locks held.
The code impact is small. Actually it's simplest to just paste the
diff here:
diff --git a/cpus.c b/cpus.c
index 1ce90a1..58bcc4f 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1116,6 +1116,13 @@ bool qemu_in_vcpu_thread(void)
return current_cpu && qemu_cpu_is_self(current_cpu);
}
+static __thread bool iothread_locked = false;
+
+bool qemu_mutex_iothread_locked(void)
+{
+ return iothread_locked;
+}
+
void qemu_mutex_lock_iothread(void)
{
atomic_inc(&iothread_requesting_mutex);
@@ -1130,10 +1137,12 @@ void qemu_mutex_lock_iothread(void)
atomic_dec(&iothread_requesting_mutex);
qemu_cond_broadcast(&qemu_io_proceeded_cond);
}
+ iothread_locked = true;
}
void qemu_mutex_unlock_iothread(void)
{
+ iothread_locked = false;
qemu_mutex_unlock(&qemu_global_mutex);
}
diff --git a/exec.c b/exec.c
index c1a2e9e..6692f45 100644
--- a/exec.c
+++ b/exec.c
@@ -2297,9 +2297,9 @@ static int memory_access_size(MemoryRegion *mr, unsigned l, hwaddr addr)
return l;
}
-static bool address_space_rw_internal(AddressSpace *as, hwaddr addr,
- uint8_t *buf, int len, bool is_write,
- bool unlocked)
+static bool address_space_rw(AddressSpace *as, hwaddr addr,
+ uint8_t *buf, int len, bool is_write,
+ bool unlocked)
{
hwaddr l;
uint8_t *ptr;
@@ -2307,6 +2307,7 @@ static bool address_space_rw_internal(AddressSpace *as, hwaddr addr,
hwaddr addr1;
MemoryRegion *mr;
bool error = false;
+ bool unlocked = !qemu_mutex_iothread_locked();
bool release_lock;
rcu_read_lock();
@@ -2415,18 +2416,6 @@ static bool address_space_rw_internal(AddressSpace *as, hwaddr addr,
return error;
}
-bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
- int len, bool is_write)
-{
- return address_space_rw_internal(as, addr, buf, len, is_write, false);
-}
-
-bool address_space_rw_unlocked(AddressSpace *as, hwaddr addr, uint8_t *buf,
- int len, bool is_write)
-{
- return address_space_rw_internal(as, addr, buf, len, is_write, true);
-}
-
bool address_space_write(AddressSpace *as, hwaddr addr,
const uint8_t *buf, int len)
{
diff --git a/include/exec/memory.h b/include/exec/memory.h
index cf39a40..0644dc6 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1084,7 +1084,9 @@ void address_space_destroy(AddressSpace *as);
* Return true if the operation hit any unassigned memory or encountered an
* IOMMU fault.
*
- * NOTE: The iothread lock must be held when calling this function.
+ * NOTE: If the iothread lock is not held when calling this function,
+ * _no_ other lock must be held. Otherwise you risk lock inversion
+ * deadlocks.
*
* @as: #AddressSpace to be accessed
* @addr: address within that address space
@@ -1095,23 +1093,6 @@ bool address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
int len, bool is_write);
/**
- * address_space_rw_unlocked: read from or write to an address space outside
- * of the iothread lock.
- *
- * Return true if the operation hit any unassigned memory or encountered an
- * IOMMU fault.
- *
- * NOTE: The iothread lock *must not* be held when calling this function.
- *
- * @as: #AddressSpace to be accessed
- * @addr: address within that address space
- * @buf: buffer with the data transferred
- * @is_write: indicates the transfer direction
- */
-bool address_space_rw_unlocked(AddressSpace *as, hwaddr addr, uint8_t *buf,
- int len, bool is_write);
-
-/**
* address_space_write: write to address space.
*
* Return true if the operation hit any unassigned memory or encountered an
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index 62c68c0..6b74eb9 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -270,6 +270,16 @@ int qemu_add_child_watch(pid_t pid);
#endif
/**
+ * qemu_mutex_iothread_locked: Return lock status of the main loop mutex.
+ *
+ * The main loop mutex is the coarsest lock in QEMU, and as such it
+ * must always be taken outside other locks. This function helps
+ * functions take different paths depending on whether the current
+ * thread is running within the main loop mutex.
+ */
+bool qemu_mutex_iothread_locked(void);
+
+/**
* qemu_mutex_lock_iothread: Lock the main loop mutex.
*
* This function locks the main loop mutex. The mutex is taken by
diff --git a/kvm-all.c b/kvm-all.c
index fa0cfed..5cac7e7 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1647,8 +1647,8 @@ static void kvm_handle_io(uint16_t port, void *data, int direction, int size,
uint8_t *ptr = data;
for (i = 0; i < count; i++) {
- address_space_rw_unlocked(&address_space_io, port, ptr, size,
- direction == KVM_EXIT_IO_OUT);
+ address_space_rw(&address_space_io, port, ptr, size,
+ direction == KVM_EXIT_IO_OUT);
ptr += size;
}
}
@@ -1813,11 +1813,11 @@ int kvm_cpu_exec(CPUState *cpu)
break;
case KVM_EXIT_MMIO:
DPRINTF("handle_mmio\n");
- address_space_rw_unlocked(&address_space_memory,
- run->mmio.phys_addr,
- run->mmio.data,
- run->mmio.len,
- run->mmio.is_write);
+ address_space_rw(&address_space_memory,
+ run->mmio.phys_addr,
+ run->mmio.data,
+ run->mmio.len,
+ run->mmio.is_write);
ret = 0;
break;
case KVM_EXIT_IRQ_WINDOW_OPEN:
diff --git a/stubs/iothread-lock.c b/stubs/iothread-lock.c
index 5d8aca1..dda6f6b 100644
--- a/stubs/iothread-lock.c
+++ b/stubs/iothread-lock.c
@@ -1,6 +1,11 @@
#include "qemu-common.h"
#include "qemu/main-loop.h"
+bool qemu_mutex_iothread_locked(void)
+{
+ return true;
+}
+
void qemu_mutex_lock_iothread(void)
{
}
Paolo
^ permalink raw reply related [flat|nested] 18+ messages in thread