| Message ID | 20210925145118.1361230-13-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | accel: Move has_work() from CPUClass to AccelOpsClass | expand |
+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
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 = {
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(+)