Re: [PATCH v7 12/40] accel/qtest: Implement AccelOpsClass::has_work() - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: Richard Henderson <richard.henderson@linaro.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Thomas Huth <thuth@redhat.com>
Cc: Laurent Vivier <lvivier@redhat.com>,
	Igor Mammedov <imammedo@redhat.com>,
	qemu-devel@nongnu.org
Subject: Re: [PATCH v7 12/40] accel/qtest: Implement AccelOpsClass::has_work()
Date: Sat, 25 Sep 2021 18:01:54 +0200	[thread overview]
Message-ID: <b5f7eccb-7b19-02c2-bef7-c8cf9e544440@amsat.org> (raw)
In-Reply-To: <e1c7cb21-459e-15a3-7d99-da30c174ea76@linaro.org>

On 9/25/21 17:32, Richard Henderson wrote:
> On 9/25/21 11:27 AM, Philippe Mathieu-Daudé wrote:
>>> +static bool qtest_cpu_has_work(CPUState *cpu)
>>> +{
>>> +    g_assert_not_reached();
>>> +}
>>
>> Sigh, this triggers:
>>
>> Running test qtest-i386/cpu-plug-test
>> **
>> ERROR:../accel/qtest/qtest.c:52:qtest_cpu_has_work: code should not be 
>> reached
>> ERROR qtest-i386/cpu-plug-test - Bail out! 
>> ERROR:../accel/qtest/qtest.c:52:qtest_cpu_has_work: code should not be 
>> reached
>> Broken pipe
> 
> Ha ha, yes.  You beat me to the reply within minutes.
> 
>> I suppose it is in my interest to 'return false' here and call it
>> a day...
> 
> I *think* that's the right thing, but I could see maybe "true" also 
> makes sense.  I'll try and have a closer look.

So first I tested using "-machine pc,accel=qtest" -> no crash.

Looking closely at how check-qtest calls QEMU, it does:
"-machine pc -accel qtest". Isn't the sugar property supposed
to work that way?

Then the backtrace is:

Thread 5 "qemu-system-i38" hit Breakpoint 1, qtest_cpu_has_work 
(cpu=0x555556a08400) at accel/qtest/qtest.c:52
52          g_assert_not_reached();
(gdb) bt
#0  qtest_cpu_has_work (cpu=0x555556a08400) at accel/qtest/qtest.c:52
#1  0x0000555555c330ba in cpu_has_work (cpu=0x555556a08400) at 
softmmu/cpus.c:254
#2  0x0000555555c32ac8 in cpu_thread_is_idle (cpu=0x555556a08400) at 
softmmu/cpus.c:91
#3  0x0000555555c33584 in qemu_wait_io_event (cpu=0x555556a08400) at 
softmmu/cpus.c:417
#4  0x0000555555d8a7f4 in dummy_cpu_thread_fn (arg=0x555556a08400) at 
accel/dummy-cpus.c:53
#5  0x0000555555f469f6 in qemu_thread_start (args=0x5555574edae0) at 
util/qemu-thread-posix.c:557
#6  0x00007ffff4ff3299 in start_thread () at /lib64/libpthread.so.0
#7  0x00007ffff4f1b353 in clone () at /lib64/libc.so.6

dummy_cpu_thread_fn() content didn't change since its introduction
in commit c7f0f3b1c82 ("qtest: add test framework"):

    "The idea behind qtest is pretty simple.  Instead of executing
     a CPU via TCG or KVM, rely on an external process to send events
     to the device model that the CPU would normally generate."

Based on that description, qtest should provide a command to notify
whether the CPU has work to do or not.

Meanwhile, no qtest command = no work = 'return false'.