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

[Laurent Vivier]

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