On 24/05/2016 23:28, Sergey Fedorov wrote:
> On 05/04/16 18:32, Alex Bennée wrote:
>> diff --git a/cpu-exec.c b/cpu-exec.c
>> index bd50fef..f558508 100644
>> --- a/cpu-exec.c
>> +++ b/cpu-exec.c
>> @@ -28,6 +28,7 @@
>> #include "qemu/rcu.h"
>> #include "exec/tb-hash.h"
>> #include "exec/log.h"
>> +#include "qemu/main-loop.h"
>> #if defined(TARGET_I386) && !defined(CONFIG_USER_ONLY)
>> #include "hw/i386/apic.h"
>> #endif
>> @@ -452,6 +453,8 @@ int cpu_exec(CPUState *cpu)
>> for(;;) {
>> interrupt_request = cpu->interrupt_request;
>> if (unlikely(interrupt_request)) {
>> + qemu_mutex_lock_iothread();
>> +
>> if (unlikely(cpu->singlestep_enabled & SSTEP_NOIRQ)) {
>> /* Mask out external interrupts for this step. */
>> interrupt_request &= ~CPU_INTERRUPT_SSTEP_MASK;
>> @@ -504,6 +507,11 @@ int cpu_exec(CPUState *cpu)
>> the program flow was changed */
>> next_tb = 0;
>> }
>> +
>> + if (qemu_mutex_iothread_locked()) {
>> + qemu_mutex_unlock_iothread();
>> + }
>> +
>
> Why do we need to check for "qemu_mutex_iothread_locked()" here?
> iothread lock is always held here, isn't it?
Correct.
>> }
>> if (unlikely(cpu->exit_request
>> || replay_has_interrupt())) {
> (snip)
>> diff --git a/cputlb.c b/cputlb.c
>> index 466663b..1412049 100644
>> --- a/cputlb.c
>> +++ b/cputlb.c
>> @@ -18,6 +18,7 @@
>> */
>>
>> #include "qemu/osdep.h"
>> +#include "qemu/main-loop.h"
>
> No need in this include.
>
>> #include "cpu.h"
>> #include "exec/exec-all.h"
>> #include "exec/memory.h"
>> diff --git a/exec.c b/exec.c
>> index c46c123..9e83673 100644
>> --- a/exec.c
>> +++ b/exec.c
> (snip)
>> @@ -2347,8 +2353,14 @@ static void io_mem_init(void)
>> memory_region_init_io(&io_mem_rom, NULL, &unassigned_mem_ops, NULL,
>> NULL, UINT64_MAX);
>> memory_region_init_io(&io_mem_unassigned, NULL, &unassigned_mem_ops,
>> NULL,
>> NULL, UINT64_MAX);
>> +
>> + /* io_mem_notdirty calls tb_invalidate_phys_page_fast,
>> + * which must be called without the iothread mutex.
>> + */
>
> notdirty_mem_write() operates on page dirty flags. Is it safe to do so
> out of iothread lock?
Yes, see commit 5f2cb94 ("memory: make
cpu_physical_memory_sync_dirty_bitmap() fully atomic", 2014-12-02) and
those before.
>> memory_region_init_io(&io_mem_notdirty, NULL, ¬dirty_mem_ops, NULL,
>> NULL, UINT64_MAX);
>> + memory_region_clear_global_locking(&io_mem_notdirty);
>> +
>> memory_region_init_io(&io_mem_watch, NULL, &watch_mem_ops, NULL,
>> NULL, UINT64_MAX);
>> }
> (snip)
>> diff --git a/translate-all.c b/translate-all.c
>> index 935d24c..0c377bb 100644
>> --- a/translate-all.c
>> +++ b/translate-all.c
>> @@ -1318,6 +1318,7 @@ void tb_invalidate_phys_range(tb_page_addr_t start,
>> tb_page_addr_t end)
>> * this TB.
>> *
>> * Called with mmap_lock held for user-mode emulation
>> + * If called from generated code, iothread mutex must not be held.
>
> What does that mean? If iothread mutex is not required by the function
> then why mention it here at all?
If this function can take the iothread mutex itself, this would cause a
deadlock. I'm not sure if it could though.
Another possibility is that this function can longjmp out into cpu_exec,
and then the iothread mutex would not be released. I think this is more
likely.
Thanks,
Paolo
