Paolo Bonzini <pbonz...@redhat.com> writes:
> From: Jan Kiszka <jan.kis...@siemens.com>
>
> The MMIO case is further broken up in two cases: if the caller does not
> hold the 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. In this case, the caller should not hold _any_ lock
> (a friendly suggestion which is disregarded by virtio-scsi-dataplane).
>
> Signed-off-by: Jan Kiszka <jan.kis...@siemens.com>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> exec.c | 66
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 57 insertions(+), 9 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 094f87e..78c99f6 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"
>
> @@ -2318,11 +2319,27 @@ static int memory_access_size(MemoryRegion *mr,
> unsigned l, hwaddr addr)
> return l;
> }
>
> -static void prepare_mmio_access(MemoryRegion *mr)
> +static bool prepare_mmio_access(MemoryRegion *mr)
> {
> + bool unlocked = !qemu_mutex_iothread_locked();
> + bool 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();
> + }
> }
> +
> + return release_lock;
> }
This is where I get confused between the advantage of this over however
same pid recursive locking. If you use recursive locking you don't need
to add a bunch of state to remind you of when to release the lock. Then
you'd just need:
static void prepare_mmio_access(MemoryRegion *mr)
{
if (mr->global_locking) {
qemu_mutex_lock_iothread();
}
if (mr->flush_coalesced_mmio) {
qemu_mutex_lock_iothread();
qemu_flush_coalesced_mmio_buffer();
qemu_mutex_unlock_iothread();
}
}
and a bunch of:
if (mr->global_locking)
qemu_mutex_unlock_iothread();
in the access functions. Although I suspect you could push the
mr->global_locking up to the dispatch functions.
Am I missing something here?
>
> MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
> @@ -2334,6 +2351,7 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr
> addr, MemTxAttrs attrs,
> hwaddr addr1;
> MemoryRegion *mr;
> MemTxResult result = MEMTX_OK;
> + bool release_lock = false;
>
> rcu_read_lock();
> while (len > 0) {
> @@ -2342,7 +2360,7 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr
> addr, MemTxAttrs attrs,
>
> if (is_write) {
> if (!memory_access_is_direct(mr, is_write)) {
> - prepare_mmio_access(mr);
> + release_lock |= prepare_mmio_access(mr);
> l = memory_access_size(mr, l, addr1);
> /* XXX: could force current_cpu to NULL to avoid
> potential bugs */
> @@ -2384,7 +2402,7 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr
> addr, MemTxAttrs attrs,
> } else {
> if (!memory_access_is_direct(mr, is_write)) {
> /* I/O case */
> - prepare_mmio_access(mr);
> + release_lock |= prepare_mmio_access(mr);
> l = memory_access_size(mr, l, addr1);
> switch (l) {
> case 8:
> @@ -2420,6 +2438,12 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr
> addr, MemTxAttrs attrs,
> memcpy(buf, ptr, l);
> }
> }
> +
> + if (release_lock) {
> + qemu_mutex_unlock_iothread();
> + release_lock = false;
> + }
> +
> len -= l;
> buf += l;
> addr += l;
> @@ -2746,11 +2770,12 @@ static inline uint32_t
> address_space_ldl_internal(AddressSpace *as, hwaddr addr,
> hwaddr l = 4;
> hwaddr addr1;
> MemTxResult r;
> + bool release_lock = false;
>
> rcu_read_lock();
> mr = address_space_translate(as, addr, &addr1, &l, false);
> if (l < 4 || !memory_access_is_direct(mr, false)) {
> - prepare_mmio_access(mr);
> + release_lock |= prepare_mmio_access(mr);
>
> /* I/O case */
> r = memory_region_dispatch_read(mr, addr1, &val, 4, attrs);
> @@ -2784,6 +2809,9 @@ static inline uint32_t
> address_space_ldl_internal(AddressSpace *as, hwaddr addr,
> if (result) {
> *result = r;
> }
> + if (release_lock) {
> + qemu_mutex_unlock_iothread();
> + }
> rcu_read_unlock();
> return val;
> }
> @@ -2836,12 +2864,13 @@ static inline uint64_t
> address_space_ldq_internal(AddressSpace *as, hwaddr addr,
> hwaddr l = 8;
> hwaddr addr1;
> MemTxResult r;
> + bool release_lock = false;
>
> rcu_read_lock();
> mr = address_space_translate(as, addr, &addr1, &l,
> false);
> if (l < 8 || !memory_access_is_direct(mr, false)) {
> - prepare_mmio_access(mr);
> + release_lock |= prepare_mmio_access(mr);
>
> /* I/O case */
> r = memory_region_dispatch_read(mr, addr1, &val, 8, attrs);
> @@ -2875,6 +2904,9 @@ static inline uint64_t
> address_space_ldq_internal(AddressSpace *as, hwaddr addr,
> if (result) {
> *result = r;
> }
> + if (release_lock) {
> + qemu_mutex_unlock_iothread();
> + }
> rcu_read_unlock();
> return val;
> }
> @@ -2947,12 +2979,13 @@ static inline uint32_t
> address_space_lduw_internal(AddressSpace *as,
> hwaddr l = 2;
> hwaddr addr1;
> MemTxResult r;
> + bool release_lock = false;
>
> rcu_read_lock();
> mr = address_space_translate(as, addr, &addr1, &l,
> false);
> if (l < 2 || !memory_access_is_direct(mr, false)) {
> - prepare_mmio_access(mr);
> + release_lock |= prepare_mmio_access(mr);
>
> /* I/O case */
> r = memory_region_dispatch_read(mr, addr1, &val, 2, attrs);
> @@ -2986,6 +3019,9 @@ static inline uint32_t
> address_space_lduw_internal(AddressSpace *as,
> if (result) {
> *result = r;
> }
> + if (release_lock) {
> + qemu_mutex_unlock_iothread();
> + }
> rcu_read_unlock();
> return val;
> }
> @@ -3037,12 +3073,13 @@ void address_space_stl_notdirty(AddressSpace *as,
> hwaddr addr, uint32_t val,
> hwaddr l = 4;
> hwaddr addr1;
> MemTxResult r;
> + bool release_lock = false;
>
> rcu_read_lock();
> mr = address_space_translate(as, addr, &addr1, &l,
> true);
> if (l < 4 || !memory_access_is_direct(mr, true)) {
> - prepare_mmio_access(mr);
> + release_lock |= prepare_mmio_access(mr);
>
> r = memory_region_dispatch_write(mr, addr1, val, 4, attrs);
> } else {
> @@ -3063,6 +3100,9 @@ void address_space_stl_notdirty(AddressSpace *as,
> hwaddr addr, uint32_t val,
> if (result) {
> *result = r;
> }
> + if (release_lock) {
> + qemu_mutex_unlock_iothread();
> + }
> rcu_read_unlock();
> }
>
> @@ -3083,12 +3123,13 @@ static inline void
> address_space_stl_internal(AddressSpace *as,
> hwaddr l = 4;
> hwaddr addr1;
> MemTxResult r;
> + bool release_lock = false;
>
> rcu_read_lock();
> mr = address_space_translate(as, addr, &addr1, &l,
> true);
> if (l < 4 || !memory_access_is_direct(mr, true)) {
> - prepare_mmio_access(mr);
> + release_lock |= prepare_mmio_access(mr);
>
> #if defined(TARGET_WORDS_BIGENDIAN)
> if (endian == DEVICE_LITTLE_ENDIAN) {
> @@ -3121,6 +3162,9 @@ static inline void
> address_space_stl_internal(AddressSpace *as,
> if (result) {
> *result = r;
> }
> + if (release_lock) {
> + qemu_mutex_unlock_iothread();
> + }
> rcu_read_unlock();
> }
>
> @@ -3190,11 +3234,12 @@ static inline void
> address_space_stw_internal(AddressSpace *as,
> hwaddr l = 2;
> hwaddr addr1;
> MemTxResult r;
> + bool release_lock = false;
>
> rcu_read_lock();
> mr = address_space_translate(as, addr, &addr1, &l, true);
> if (l < 2 || !memory_access_is_direct(mr, true)) {
> - prepare_mmio_access(mr);
> + release_lock |= prepare_mmio_access(mr);
>
> #if defined(TARGET_WORDS_BIGENDIAN)
> if (endian == DEVICE_LITTLE_ENDIAN) {
> @@ -3227,6 +3272,9 @@ static inline void
> address_space_stw_internal(AddressSpace *as,
> if (result) {
> *result = r;
> }
> + if (release_lock) {
> + qemu_mutex_unlock_iothread();
> + }
> rcu_read_unlock();
> }
--
Alex Bennée
