Re: [PATCH v7 12/40] accel/qtest: Implement AccelOpsClass::has_work()

[Laurent Vivier <lvivier@redhat.com>]

On 25/09/2021 18:01, Philippe Mathieu-Daudé wrote:
> 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'.
> 

In fact, with the migration test we have CPU running (it's the purpose of the test), qtest 
allows to have several accelerators, "-accel qtest" adds the qtest control in QEMU and we 
have then a KVM or TCG accel to be able to run some qtest commands with a CPU running.

Thanks,
Laurent