[RFC] accel: add cpu_reset

Claudio Fontana posted 1 patch 3 years ago
Failed in applying to current master (apply log)
accel/accel-common.c        | 9 +++++++++
hw/core/cpu.c               | 3 ++-
include/hw/core/accel-cpu.h | 2 ++
include/qemu/accel.h        | 6 ++++++
target/i386/cpu.c           | 4 ----
target/i386/kvm/kvm-cpu.c   | 6 ++++++
6 files changed, 25 insertions(+), 5 deletions(-)
[RFC] accel: add cpu_reset
Posted by Claudio Fontana 3 years ago
XXX
---
 accel/accel-common.c        | 9 +++++++++
 hw/core/cpu.c               | 3 ++-
 include/hw/core/accel-cpu.h | 2 ++
 include/qemu/accel.h        | 6 ++++++
 target/i386/cpu.c           | 4 ----
 target/i386/kvm/kvm-cpu.c   | 6 ++++++
 6 files changed, 25 insertions(+), 5 deletions(-)


This surprisingly works without moving cpu_reset() to a
specific_ss module, even though

accel-common.c is specific_ss,
hw/core/cpu.c  is common_ss.

How come the call to accel_reset_cpu works?

Ciao,

Claudio


diff --git a/accel/accel-common.c b/accel/accel-common.c
index cf07f78421..3331a9dcfd 100644
--- a/accel/accel-common.c
+++ b/accel/accel-common.c
@@ -121,6 +121,15 @@ bool accel_cpu_realizefn(CPUState *cpu, Error **errp)
     return true;
 }
 
+void accel_cpu_reset(CPUState *cpu)
+{
+    CPUClass *cc = CPU_GET_CLASS(cpu);
+
+    if (cc->accel_cpu && cc->accel_cpu->cpu_reset) {
+        cc->accel_cpu->cpu_reset(cpu);
+    }
+}
+
 static const TypeInfo accel_cpu_type = {
     .name = TYPE_ACCEL_CPU,
     .parent = TYPE_OBJECT,
diff --git a/hw/core/cpu.c b/hw/core/cpu.c
index 00330ba07d..590a0d934f 100644
--- a/hw/core/cpu.c
+++ b/hw/core/cpu.c
@@ -35,6 +35,7 @@
 #include "trace/trace-root.h"
 #include "qemu/plugin.h"
 #include "sysemu/hw_accel.h"
+#include "qemu/accel.h"
 
 CPUState *cpu_by_arch_id(int64_t id)
 {
@@ -230,7 +231,7 @@ void cpu_dump_statistics(CPUState *cpu, int flags)
 void cpu_reset(CPUState *cpu)
 {
     device_cold_reset(DEVICE(cpu));
-
+    accel_cpu_reset(cpu);
     trace_guest_cpu_reset(cpu);
 }
 
diff --git a/include/hw/core/accel-cpu.h b/include/hw/core/accel-cpu.h
index 5dbfd79955..700a5bd266 100644
--- a/include/hw/core/accel-cpu.h
+++ b/include/hw/core/accel-cpu.h
@@ -33,6 +33,8 @@ typedef struct AccelCPUClass {
     void (*cpu_class_init)(CPUClass *cc);
     void (*cpu_instance_init)(CPUState *cpu);
     bool (*cpu_realizefn)(CPUState *cpu, Error **errp);
+    void (*cpu_reset)(CPUState *cpu);
+
 } AccelCPUClass;
 
 #endif /* ACCEL_CPU_H */
diff --git a/include/qemu/accel.h b/include/qemu/accel.h
index 4f4c283f6f..8d3a15b916 100644
--- a/include/qemu/accel.h
+++ b/include/qemu/accel.h
@@ -91,4 +91,10 @@ void accel_cpu_instance_init(CPUState *cpu);
  */
 bool accel_cpu_realizefn(CPUState *cpu, Error **errp);
 
+/**
+ * accel_cpu_reset:
+ * @cpu: The CPU that needs to call accel-specific reset.
+ */
+void accel_cpu_reset(CPUState *cpu);
+
 #endif /* QEMU_ACCEL_H */
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 48a08df438..ad233b823d 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5780,10 +5780,6 @@ static void x86_cpu_reset(DeviceState *dev)
     apic_designate_bsp(cpu->apic_state, s->cpu_index == 0);
 
     s->halted = !cpu_is_bsp(cpu);
-
-    if (kvm_enabled()) {
-        kvm_arch_reset_vcpu(cpu);
-    }
 #endif
 }
 
diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
index c660ad4293..ffdc9afddb 100644
--- a/target/i386/kvm/kvm-cpu.c
+++ b/target/i386/kvm/kvm-cpu.c
@@ -130,12 +130,18 @@ static void kvm_cpu_instance_init(CPUState *cs)
     }
 }
 
+static void kvm_cpu_reset(CPUState *cpu)
+{
+    kvm_arch_reset_vcpu(X86_CPU(cpu));
+}
+
 static void kvm_cpu_accel_class_init(ObjectClass *oc, void *data)
 {
     AccelCPUClass *acc = ACCEL_CPU_CLASS(oc);
 
     acc->cpu_realizefn = kvm_cpu_realizefn;
     acc->cpu_instance_init = kvm_cpu_instance_init;
+    acc->cpu_reset = kvm_cpu_reset;
 }
 static const TypeInfo kvm_cpu_accel_type_info = {
     .name = ACCEL_CPU_NAME("kvm"),
-- 
2.26.2


Re: [RFC] accel: add cpu_reset
Posted by Paolo Bonzini 3 years ago
On 22/03/21 14:27, Claudio Fontana wrote:
> This surprisingly works without moving cpu_reset() to a specific_ss 
> module, even though accel-common.c is specific_ss, hw/core/cpu.c is 
> common_ss. How come the call to accel_reset_cpu works?

I don't understand the question.  Why wouldn't it work? :)

Paolo


Re: [RFC] accel: add cpu_reset
Posted by Claudio Fontana 3 years ago
On 3/22/21 2:31 PM, Paolo Bonzini wrote:
> On 22/03/21 14:27, Claudio Fontana wrote:
>> This surprisingly works without moving cpu_reset() to a specific_ss 
>> module, even though accel-common.c is specific_ss, hw/core/cpu.c is 
>> common_ss. How come the call to accel_reset_cpu works?
> 
> I don't understand the question.  Why wouldn't it work? :)
> 
> Paolo
> 

Heh probably something I forgot or do not understand around the specific_ss / common_ss distinction.

I was under the (wrong?) impression that we build some tools or components that include common_ss objects, but not specific_ss.

And maybe I am just wrong, and things are simpler than I expected.

Ciao,

Claudio

Re: [RFC] accel: add cpu_reset
Posted by Paolo Bonzini 3 years ago
On 22/03/21 14:35, Claudio Fontana wrote:
> On 3/22/21 2:31 PM, Paolo Bonzini wrote:
>> On 22/03/21 14:27, Claudio Fontana wrote:
>>> This surprisingly works without moving cpu_reset() to a specific_ss
>>> module, even though accel-common.c is specific_ss, hw/core/cpu.c is
>>> common_ss. How come the call to accel_reset_cpu works?
>>
>> I don't understand the question.  Why wouldn't it work? :)
>>
>> Paolo
>>
> 
> Heh probably something I forgot or do not understand around the specific_ss / common_ss distinction.
> 
> I was under the (wrong?) impression that we build some tools or components that include common_ss objects, but not specific_ss.
> 
> And maybe I am just wrong, and things are simpler than I expected.

No, all emulators include:

- some parts of common_ss, compiled once per build.  These are files 
that do not use target-specific definitions.  Other sourcesets also 
define once-per-build files, and in fact they end up in common_ss via 
the add_all method of sourcesets; softmmu_ss, for example is added to 
common_ss under the CONFIG_SOFTMMU condition.

- some parts of specific_ss, compiled once per target because these 
files use target-specific definitions.

- the entirety of the respective hw/ and target/ sourcesets.

It is possible to include calls from one sourceset to another (including 
from common to specific) as long as the conditions ensure that the 
symbol is defined.

Paolo


Re: [RFC] accel: add cpu_reset
Posted by Claudio Fontana 3 years ago
On 3/22/21 2:45 PM, Paolo Bonzini wrote:
> On 22/03/21 14:35, Claudio Fontana wrote:
>> On 3/22/21 2:31 PM, Paolo Bonzini wrote:
>>> On 22/03/21 14:27, Claudio Fontana wrote:
>>>> This surprisingly works without moving cpu_reset() to a specific_ss
>>>> module, even though accel-common.c is specific_ss, hw/core/cpu.c is
>>>> common_ss. How come the call to accel_reset_cpu works?
>>>
>>> I don't understand the question.  Why wouldn't it work? :)
>>>
>>> Paolo
>>>
>>
>> Heh probably something I forgot or do not understand around the specific_ss / common_ss distinction.
>>
>> I was under the (wrong?) impression that we build some tools or components that include common_ss objects, but not specific_ss.
>>
>> And maybe I am just wrong, and things are simpler than I expected.
> 
> No, all emulators include:
> 
> - some parts of common_ss, compiled once per build.  These are files 
> that do not use target-specific definitions.  Other sourcesets also 
> define once-per-build files, and in fact they end up in common_ss via 
> the add_all method of sourcesets; softmmu_ss, for example is added to 
> common_ss under the CONFIG_SOFTMMU condition.
> 
> - some parts of specific_ss, compiled once per target because these 
> files use target-specific definitions.
> 
> - the entirety of the respective hw/ and target/ sourcesets.
> 
> It is possible to include calls from one sourceset to another (including 
> from common to specific) as long as the conditions ensure that the 
> symbol is defined.


I guess this last sentence is the more tricky for me to get: "as long as the conditions ensure that the symbol is defined".

> 
> Paolo
> 

Thanks for the explanation, I would assume that "make check" then would be able to catch such problems?

Which targets would I need to build to ensure that any problems with this are detected? Do we cover all of these cases with our gitlab CI?

Ciao,

Claudio




Re: [RFC] accel: add cpu_reset
Posted by Paolo Bonzini 3 years ago
On 22/03/21 14:51, Claudio Fontana wrote:
>>
>> It is possible to include calls from one sourceset to another (including
>> from common to specific) as long as the conditions ensure that the
>> symbol is defined.
> 
> I guess this last sentence is the more tricky for me to get: "as long as the conditions ensure that the symbol is defined".

It means that for example you need to cannot call block_ss code from 
common_ss without any condition, because for example usermode emulation 
targets will fail to link.  But you can freely call block_ss from files 
that are system-emulation only (such as hw/block or hw/scsi).

And on the contrary, as long as all specific_ss file implement the 
function, it is fine to call it from non-target-specific files without 
any condition.  This is what happens already with (for example) monitor 
commands that have a target-specific implementation but are present in 
all targets.

Paolo


Re: [RFC] accel: add cpu_reset
Posted by Philippe Mathieu-Daudé 3 years ago
On 3/22/21 2:27 PM, Claudio Fontana wrote:
> XXX
> ---
>  accel/accel-common.c        | 9 +++++++++
>  hw/core/cpu.c               | 3 ++-
>  include/hw/core/accel-cpu.h | 2 ++
>  include/qemu/accel.h        | 6 ++++++
>  target/i386/cpu.c           | 4 ----
>  target/i386/kvm/kvm-cpu.c   | 6 ++++++
>  6 files changed, 25 insertions(+), 5 deletions(-)
> 
> 
> This surprisingly works without moving cpu_reset() to a
> specific_ss module, even though
> 
> accel-common.c is specific_ss,
> hw/core/cpu.c  is common_ss.
> 
> How come the call to accel_reset_cpu works?

Each CPU optionally calls cpu_reset() manually?

$ git grep register_reset.*cpu
hw/arm/armv7m.c:334:    qemu_register_reset(armv7m_reset, cpu);
hw/arm/boot.c:1290:        qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
hw/cris/boot.c:101:    qemu_register_reset(main_cpu_reset, cpu);
hw/lm32/lm32_boards.c:162:    qemu_register_reset(main_cpu_reset,
reset_info);
hw/lm32/lm32_boards.c:289:    qemu_register_reset(main_cpu_reset,
reset_info);
hw/lm32/milkymist.c:238:    qemu_register_reset(main_cpu_reset, reset_info);
hw/m68k/q800.c:247:    qemu_register_reset(main_cpu_reset, cpu);
hw/m68k/virt.c:132:    qemu_register_reset(main_cpu_reset, cpu);
hw/microblaze/boot.c:134:    qemu_register_reset(main_cpu_reset, cpu);
hw/mips/cps.c:107:        qemu_register_reset(main_cpu_reset, cpu);
hw/mips/fuloong2e.c:269:    qemu_register_reset(main_cpu_reset, cpu);
hw/mips/jazz.c:195:    qemu_register_reset(main_cpu_reset, cpu);
hw/mips/loongson3_virt.c:545:        qemu_register_reset(main_cpu_reset,
cpu);
hw/mips/malta.c:1185:        qemu_register_reset(main_cpu_reset, cpu);
hw/mips/mipssim.c:170:    qemu_register_reset(main_cpu_reset, reset_info);
hw/moxie/moxiesim.c:120:    qemu_register_reset(main_cpu_reset, cpu);
hw/nios2/boot.c:138:    qemu_register_reset(main_cpu_reset, cpu);
hw/openrisc/openrisc_sim.c:160:
qemu_register_reset(main_cpu_reset, cpus[n]);
hw/ppc/e500.c:903:            qemu_register_reset(ppce500_cpu_reset, cpu);
hw/ppc/e500.c:907:            qemu_register_reset(ppce500_cpu_reset_sec,
cpu);
hw/ppc/mac_newworld.c:156:        qemu_register_reset(ppc_core99_reset,
cpu);
hw/ppc/mac_oldworld.c:118:
qemu_register_reset(ppc_heathrow_reset, cpu);
hw/ppc/ppc440_bamboo.c:192:    qemu_register_reset(main_cpu_reset, cpu);
hw/ppc/ppc4xx_devs.c:75:    qemu_register_reset(ppc4xx_reset, cpu);
hw/ppc/ppc_booke.c:369:
qemu_register_reset(ppc_booke_timer_reset_handle, cpu);
hw/ppc/prep.c:270:    qemu_register_reset(ppc_prep_reset, cpu);
hw/ppc/sam460ex.c:306:    qemu_register_reset(main_cpu_reset, cpu);
hw/ppc/spapr_cpu_core.c:245:
qemu_unregister_reset(spapr_cpu_core_reset_handler, sc);
hw/ppc/spapr_cpu_core.c:326:
qemu_register_reset(spapr_cpu_core_reset_handler, sc);
hw/ppc/virtex_ml507.c:233:    qemu_register_reset(main_cpu_reset, cpu);
hw/riscv/riscv_hart.c:51:    qemu_register_reset(riscv_harts_cpu_reset,
&s->harts[idx]);
hw/sh4/r2d.c:251:    qemu_register_reset(main_cpu_reset, reset_info);
hw/sparc/leon3.c:213:    qemu_register_reset(main_cpu_reset, reset_info);
hw/sparc/sun4m.c:828:    qemu_register_reset(sun4m_cpu_reset, cpu);
hw/sparc64/sparc64.c:357:    qemu_register_reset(main_cpu_reset,
reset_info);
hw/xtensa/sim.c:68:        qemu_register_reset(sim_reset, cpu);
hw/xtensa/xtfpga.c:270:        qemu_register_reset(xtfpga_reset, cpu);
target/i386/cpu.c:6859:    qemu_register_reset(x86_cpu_machine_reset_cb,
cpu);
target/i386/cpu.c:6942:
qemu_unregister_reset(x86_cpu_machine_reset_cb, dev);
target/i386/hax/hax-all.c:230:
qemu_register_reset(hax_reset_vcpu_state, (CPUArchState *) (cpu->env_ptr));
target/s390x/cpu.c:232:
qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
target/s390x/cpu.c:319:
qemu_unregister_reset(s390_cpu_machine_reset_cb, cpu);

Re: [RFC] accel: add cpu_reset
Posted by Claudio Fontana 3 years ago
On 3/22/21 2:42 PM, Philippe Mathieu-Daudé wrote:
> On 3/22/21 2:27 PM, Claudio Fontana wrote:
>> XXX
>> ---
>>  accel/accel-common.c        | 9 +++++++++
>>  hw/core/cpu.c               | 3 ++-
>>  include/hw/core/accel-cpu.h | 2 ++
>>  include/qemu/accel.h        | 6 ++++++
>>  target/i386/cpu.c           | 4 ----
>>  target/i386/kvm/kvm-cpu.c   | 6 ++++++
>>  6 files changed, 25 insertions(+), 5 deletions(-)
>>
>>
>> This surprisingly works without moving cpu_reset() to a
>> specific_ss module, even though
>>
>> accel-common.c is specific_ss,
>> hw/core/cpu.c  is common_ss.
>>
>> How come the call to accel_reset_cpu works?
> 
> Each CPU optionally calls cpu_reset() manually?

Hi Philippe, are you concerned about these calls?
Or what are you highlighting here?

They in turn call cpu_reset() so we should be good right?

Ciao,

Claudio

> 
> $ git grep register_reset.*cpu
> hw/arm/armv7m.c:334:    qemu_register_reset(armv7m_reset, cpu);
> hw/arm/boot.c:1290:        qemu_register_reset(do_cpu_reset, ARM_CPU(cs));
> hw/cris/boot.c:101:    qemu_register_reset(main_cpu_reset, cpu);
> hw/lm32/lm32_boards.c:162:    qemu_register_reset(main_cpu_reset,
> reset_info);
> hw/lm32/lm32_boards.c:289:    qemu_register_reset(main_cpu_reset,
> reset_info);
> hw/lm32/milkymist.c:238:    qemu_register_reset(main_cpu_reset, reset_info);
> hw/m68k/q800.c:247:    qemu_register_reset(main_cpu_reset, cpu);
> hw/m68k/virt.c:132:    qemu_register_reset(main_cpu_reset, cpu);
> hw/microblaze/boot.c:134:    qemu_register_reset(main_cpu_reset, cpu);
> hw/mips/cps.c:107:        qemu_register_reset(main_cpu_reset, cpu);
> hw/mips/fuloong2e.c:269:    qemu_register_reset(main_cpu_reset, cpu);
> hw/mips/jazz.c:195:    qemu_register_reset(main_cpu_reset, cpu);
> hw/mips/loongson3_virt.c:545:        qemu_register_reset(main_cpu_reset,
> cpu);
> hw/mips/malta.c:1185:        qemu_register_reset(main_cpu_reset, cpu);
> hw/mips/mipssim.c:170:    qemu_register_reset(main_cpu_reset, reset_info);
> hw/moxie/moxiesim.c:120:    qemu_register_reset(main_cpu_reset, cpu);
> hw/nios2/boot.c:138:    qemu_register_reset(main_cpu_reset, cpu);
> hw/openrisc/openrisc_sim.c:160:
> qemu_register_reset(main_cpu_reset, cpus[n]);
> hw/ppc/e500.c:903:            qemu_register_reset(ppce500_cpu_reset, cpu);
> hw/ppc/e500.c:907:            qemu_register_reset(ppce500_cpu_reset_sec,
> cpu);
> hw/ppc/mac_newworld.c:156:        qemu_register_reset(ppc_core99_reset,
> cpu);
> hw/ppc/mac_oldworld.c:118:
> qemu_register_reset(ppc_heathrow_reset, cpu);
> hw/ppc/ppc440_bamboo.c:192:    qemu_register_reset(main_cpu_reset, cpu);
> hw/ppc/ppc4xx_devs.c:75:    qemu_register_reset(ppc4xx_reset, cpu);
> hw/ppc/ppc_booke.c:369:
> qemu_register_reset(ppc_booke_timer_reset_handle, cpu);
> hw/ppc/prep.c:270:    qemu_register_reset(ppc_prep_reset, cpu);
> hw/ppc/sam460ex.c:306:    qemu_register_reset(main_cpu_reset, cpu);
> hw/ppc/spapr_cpu_core.c:245:
> qemu_unregister_reset(spapr_cpu_core_reset_handler, sc);
> hw/ppc/spapr_cpu_core.c:326:
> qemu_register_reset(spapr_cpu_core_reset_handler, sc);
> hw/ppc/virtex_ml507.c:233:    qemu_register_reset(main_cpu_reset, cpu);
> hw/riscv/riscv_hart.c:51:    qemu_register_reset(riscv_harts_cpu_reset,
> &s->harts[idx]);
> hw/sh4/r2d.c:251:    qemu_register_reset(main_cpu_reset, reset_info);
> hw/sparc/leon3.c:213:    qemu_register_reset(main_cpu_reset, reset_info);
> hw/sparc/sun4m.c:828:    qemu_register_reset(sun4m_cpu_reset, cpu);
> hw/sparc64/sparc64.c:357:    qemu_register_reset(main_cpu_reset,
> reset_info);
> hw/xtensa/sim.c:68:        qemu_register_reset(sim_reset, cpu);
> hw/xtensa/xtfpga.c:270:        qemu_register_reset(xtfpga_reset, cpu);
> target/i386/cpu.c:6859:    qemu_register_reset(x86_cpu_machine_reset_cb,
> cpu);
> target/i386/cpu.c:6942:
> qemu_unregister_reset(x86_cpu_machine_reset_cb, dev);
> target/i386/hax/hax-all.c:230:
> qemu_register_reset(hax_reset_vcpu_state, (CPUArchState *) (cpu->env_ptr));
> target/s390x/cpu.c:232:
> qemu_register_reset(s390_cpu_machine_reset_cb, cpu);
> target/s390x/cpu.c:319:
> qemu_unregister_reset(s390_cpu_machine_reset_cb, cpu);
> 


Re: [RFC] accel: add cpu_reset
Posted by Philippe Mathieu-Daudé 3 years ago
On 3/22/21 2:54 PM, Claudio Fontana wrote:
> On 3/22/21 2:42 PM, Philippe Mathieu-Daudé wrote:
>> On 3/22/21 2:27 PM, Claudio Fontana wrote:
>>> XXX
>>> ---
>>>  accel/accel-common.c        | 9 +++++++++
>>>  hw/core/cpu.c               | 3 ++-
>>>  include/hw/core/accel-cpu.h | 2 ++
>>>  include/qemu/accel.h        | 6 ++++++
>>>  target/i386/cpu.c           | 4 ----
>>>  target/i386/kvm/kvm-cpu.c   | 6 ++++++
>>>  6 files changed, 25 insertions(+), 5 deletions(-)
>>>
>>>
>>> This surprisingly works without moving cpu_reset() to a
>>> specific_ss module, even though
>>>
>>> accel-common.c is specific_ss,
>>> hw/core/cpu.c  is common_ss.
>>>
>>> How come the call to accel_reset_cpu works?
>>
>> Each CPU optionally calls cpu_reset() manually?
> 
> Hi Philippe, are you concerned about these calls?
> Or what are you highlighting here?
> 
> They in turn call cpu_reset() so we should be good right?

I guess I simply misunderstood your question :)