Paolo Bonzini <pbonz...@redhat.com> writes:
> 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.
I'll fix that for the next iteration. The main else leg still needs to
check though as it doesn't know if the mutex was grabbed before a
siglongjmp.
>
>>> }
>>> 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.
Any longjmp should:
tb_lock_reset();
if (qemu_mutex_iothread_locked()) {
qemu_mutex_unlock_iothread();
}
To reset the locks.
>
> Thanks,
>
> Paolo
--
Alex Bennée
