| Message ID | 20210322132800.7470-2-cfontana@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i386 cleanup PART 2 | expand |
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
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
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);
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
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
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); >
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
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 :)
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"),