| Message ID | 20210926222716.1732932-14-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | accel: Move has_work() from CPUClass to AccelOpsClass | expand |
On 9/26/21 6:26 PM, Philippe Mathieu-Daudé wrote: > All accelerators but TCG implement their AccelOpsClass::has_work() > handler, meaning all the remaining CPUClass::has_work() ones are > only reachable from TCG accelerator; and these has_work() handlers > belong to TCGCPUOps. > > We will gradually move each target CPUClass::has_work() to > TCGCPUOps in the following commits. > For now, move the CPUClass::has_work() call to tcg_cpu_has_work(), > the TCG AccelOpsClass::has_work() implementation. > > Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org> > --- > include/hw/core/cpu.h | 2 +- > accel/tcg/tcg-accel-ops.c | 11 +++++++++++ > softmmu/cpus.c | 5 ----- > 3 files changed, 12 insertions(+), 6 deletions(-) Are we really really really sure this works? Device emulation raises e.g. CPU_INTERRUPT_HARD. We certainly test that bit in target/i386/kvm/kvm.c. But we don't check that bit in your kvm_cpu_has_work. We're currently checking that via cc->has_work(), in x86_cpu_pending_interrupt, but after this change we won't test it at all for kvm. r~
On 9/27/21 02:12, Richard Henderson wrote: > On 9/26/21 6:26 PM, Philippe Mathieu-Daudé wrote: >> All accelerators but TCG implement their AccelOpsClass::has_work() >> handler, meaning all the remaining CPUClass::has_work() ones are >> only reachable from TCG accelerator; and these has_work() handlers >> belong to TCGCPUOps. >> >> We will gradually move each target CPUClass::has_work() to >> TCGCPUOps in the following commits. >> For now, move the CPUClass::has_work() call to tcg_cpu_has_work(), >> the TCG AccelOpsClass::has_work() implementation. >> >> Signed-off-by: Philippe Mathieu-Daudé<f4bug@amsat.org> >> --- >> include/hw/core/cpu.h | 2 +- >> accel/tcg/tcg-accel-ops.c | 11 +++++++++++ >> softmmu/cpus.c | 5 ----- >> 3 files changed, 12 insertions(+), 6 deletions(-) > > Are we really really really sure this works? As sure as a green CI, so I wonder if KVM is really tested there... > Device emulation raises e.g. CPU_INTERRUPT_HARD. We certainly test that > bit in target/i386/kvm/kvm.c. But we don't check that bit in your > kvm_cpu_has_work. We're currently checking that via cc->has_work(), in > x86_cpu_pending_interrupt, but after this change we won't test it at all > for kvm. Indeed. I guess I misunderstood your v6 comment. I'll revisit. Sorry.
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index e2dd171a13f..114eb3b9b2c 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -89,7 +89,7 @@ struct SysemuCPUOps; * instantiatable CPU type. * @parse_features: Callback to parse command line arguments. * @reset_dump_flags: #CPUDumpFlags to use for reset logging. - * @has_work: Callback for checking if there is work to do. + * @has_work: Callback for checking if there is work to do. Only used by TCG. * @memory_rw_debug: Callback for GDB memory access. * @dump_state: Callback for dumping state. * @get_arch_id: Callback for getting architecture-dependent CPU ID. diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/tcg-accel-ops.c index 1a8e8390bd6..ebaacff1842 100644 --- a/accel/tcg/tcg-accel-ops.c +++ b/accel/tcg/tcg-accel-ops.c @@ -73,6 +73,16 @@ int tcg_cpus_exec(CPUState *cpu) return ret; } +static bool tcg_cpu_has_work(CPUState *cpu) +{ + CPUClass *cc = CPU_GET_CLASS(cpu); + + if (cc->has_work) { + return cc->has_work(cpu); + } + return false; +} + /* mask must never be zero, except for A20 change call */ void tcg_handle_interrupt(CPUState *cpu, int mask) { @@ -108,6 +118,7 @@ static void tcg_accel_ops_init(AccelOpsClass *ops) ops->kick_vcpu_thread = rr_kick_vcpu_thread; ops->handle_interrupt = tcg_handle_interrupt; } + ops->has_work = tcg_cpu_has_work; } static void tcg_accel_ops_class_init(ObjectClass *oc, void *data) diff --git a/softmmu/cpus.c b/softmmu/cpus.c index 5ffa02f9cef..bb16a25bcef 100644 --- a/softmmu/cpus.c +++ b/softmmu/cpus.c @@ -251,11 +251,6 @@ void cpu_interrupt(CPUState *cpu, int mask) bool cpu_has_work(CPUState *cpu) { - CPUClass *cc = CPU_GET_CLASS(cpu); - - if (cc->has_work && cc->has_work(cpu)) { - return true; - } if (cpus_accel->has_work && cpus_accel->has_work(cpu)) { return true; }
All accelerators but TCG implement their AccelOpsClass::has_work() handler, meaning all the remaining CPUClass::has_work() ones are only reachable from TCG accelerator; and these has_work() handlers belong to TCGCPUOps. We will gradually move each target CPUClass::has_work() to TCGCPUOps in the following commits. For now, move the CPUClass::has_work() call to tcg_cpu_has_work(), the TCG AccelOpsClass::has_work() implementation. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- include/hw/core/cpu.h | 2 +- accel/tcg/tcg-accel-ops.c | 11 +++++++++++ softmmu/cpus.c | 5 ----- 3 files changed, 12 insertions(+), 6 deletions(-)