Implement QTest has_work() handler in AccelOpsClass.
Since no CPU are available under QTest accelerator,
this function is not reachable.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 accel/qtest/qtest.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c
index 7e6b8110d52..903c39ec1bf 100644
--- a/accel/qtest/qtest.c
+++ b/accel/qtest/qtest.c
@@ -47,12 +47,18 @@ static const TypeInfo qtest_accel_type = {
 };
 module_obj(TYPE_QTEST_ACCEL);
 
+static bool qtest_cpu_has_work(CPUState *cpu)
+{
+    g_assert_not_reached();
+}
+
 static void qtest_accel_ops_class_init(ObjectClass *oc, void *data)
 {
     AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
 
     ops->create_vcpu_thread = dummy_start_vcpu_thread;
     ops->get_virtual_clock = qtest_get_virtual_clock;
+    ops->has_work = qtest_cpu_has_work;
 };
 
 static const TypeInfo qtest_accel_ops_type = {
-- 
2.31.1

+Igor

On 9/25/21 16:50, Philippe Mathieu-Daudé wrote:
> Implement QTest has_work() handler in AccelOpsClass.
> Since no CPU are available under QTest accelerator,
> this function is not reachable.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   accel/qtest/qtest.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/accel/qtest/qtest.c b/accel/qtest/qtest.c
> index 7e6b8110d52..903c39ec1bf 100644
> --- a/accel/qtest/qtest.c
> +++ b/accel/qtest/qtest.c
> @@ -47,12 +47,18 @@ static const TypeInfo qtest_accel_type = {
>   };
>   module_obj(TYPE_QTEST_ACCEL);
>   
> +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

I understand we want to test CPU hotplug, however I'm not sure what
part of the accelerator can be tested, QTest is almost empty stubs.

I suppose it is in my interest to 'return false' here and call it
a day...

>   static void qtest_accel_ops_class_init(ObjectClass *oc, void *data)
>   {
>       AccelOpsClass *ops = ACCEL_OPS_CLASS(oc);
>   
>       ops->create_vcpu_thread = dummy_start_vcpu_thread;
>       ops->get_virtual_clock = qtest_get_virtual_clock;
> +    ops->has_work = qtest_cpu_has_work;
>   };
>   
>   static const TypeInfo qtest_accel_ops_type = {
> 

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.


r~

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'.

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