| Message ID | 20210204163931.7358-22-cfontana@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i386 cleanup PART 2 | expand |
On 2/4/21 6:39 AM, Claudio Fontana wrote: > @@ -6725,10 +6722,8 @@ static void x86_cpu_initfn(Object *obj) > x86_cpu_load_model(cpu, xcc->model); > } > > - /* if required, do the accelerator-specific cpu initialization */ > - if (cc->accel_cpu) { > - cc->accel_cpu->cpu_instance_init(CPU(obj)); > - } > + /* if required, do accelerator-specific cpu initializations */ > + accel_cpu_instance_init(CPU(obj)); > } Why is this only done for x86? r~
On 2/5/21 9:14 PM, Richard Henderson wrote: > On 2/4/21 6:39 AM, Claudio Fontana wrote: >> @@ -6725,10 +6722,8 @@ static void x86_cpu_initfn(Object *obj) >> x86_cpu_load_model(cpu, xcc->model); >> } >> >> - /* if required, do the accelerator-specific cpu initialization */ >> - if (cc->accel_cpu) { >> - cc->accel_cpu->cpu_instance_init(CPU(obj)); >> - } >> + /* if required, do accelerator-specific cpu initializations */ >> + accel_cpu_instance_init(CPU(obj)); >> } > > Why is this only done for x86? > > > r~ > It makes sense to include the other architectures. As the next step I would like to apply this to ARM, but to me it makes sense to first complete Philippe's series, which reshuffles things so that TCG-only / KVM-only builds are both possible and error-free: https://www.mail-archive.com/qemu-devel@nongnu.org/msg777627.html Thanks, Claudio
On 2/8/21 1:50 PM, Claudio Fontana wrote: > On 2/5/21 9:14 PM, Richard Henderson wrote: >> On 2/4/21 6:39 AM, Claudio Fontana wrote: >>> @@ -6725,10 +6722,8 @@ static void x86_cpu_initfn(Object *obj) >>> x86_cpu_load_model(cpu, xcc->model); >>> } >>> >>> - /* if required, do the accelerator-specific cpu initialization */ >>> - if (cc->accel_cpu) { >>> - cc->accel_cpu->cpu_instance_init(CPU(obj)); >>> - } >>> + /* if required, do accelerator-specific cpu initializations */ >>> + accel_cpu_instance_init(CPU(obj)); >>> } >> >> Why is this only done for x86? >> >> >> r~ >> > > It makes sense to include the other architectures. > > As the next step I would like to apply this to ARM, but to me it makes sense to first complete Philippe's series, > which reshuffles things so that TCG-only / KVM-only builds are both possible and error-free: > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg777627.html TBH this series is very unlikely to be merged before yours, so go ahead... (eventually you can cherry-pick what you need from it).
On 2/8/21 1:54 PM, Philippe Mathieu-Daudé wrote: > On 2/8/21 1:50 PM, Claudio Fontana wrote: >> On 2/5/21 9:14 PM, Richard Henderson wrote: >>> On 2/4/21 6:39 AM, Claudio Fontana wrote: >>>> @@ -6725,10 +6722,8 @@ static void x86_cpu_initfn(Object *obj) >>>> x86_cpu_load_model(cpu, xcc->model); >>>> } >>>> >>>> - /* if required, do the accelerator-specific cpu initialization */ >>>> - if (cc->accel_cpu) { >>>> - cc->accel_cpu->cpu_instance_init(CPU(obj)); >>>> - } >>>> + /* if required, do accelerator-specific cpu initializations */ >>>> + accel_cpu_instance_init(CPU(obj)); >>>> } >>> >>> Why is this only done for x86? >>> >>> >>> r~ >>> >> >> It makes sense to include the other architectures. >> >> As the next step I would like to apply this to ARM, but to me it makes sense to first complete Philippe's series, >> which reshuffles things so that TCG-only / KVM-only builds are both possible and error-free: >> >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg777627.html > > TBH this series is very unlikely to be merged before yours, > so go ahead... (eventually you can cherry-pick what you need > from it). > > After taking a short look at the ARM side, I think I need first a detour to split more CONFIG_USER_ONLY code from the rest. I'll experiment with using meson to add a target-only user_ss variable, and apply it to i386 and arm. Ciao, Claudio
On 2/8/21 1:54 PM, Philippe Mathieu-Daudé wrote: > On 2/8/21 1:50 PM, Claudio Fontana wrote: >> On 2/5/21 9:14 PM, Richard Henderson wrote: >>> On 2/4/21 6:39 AM, Claudio Fontana wrote: >>>> @@ -6725,10 +6722,8 @@ static void x86_cpu_initfn(Object *obj) >>>> x86_cpu_load_model(cpu, xcc->model); >>>> } >>>> >>>> - /* if required, do the accelerator-specific cpu initialization */ >>>> - if (cc->accel_cpu) { >>>> - cc->accel_cpu->cpu_instance_init(CPU(obj)); >>>> - } >>>> + /* if required, do accelerator-specific cpu initializations */ >>>> + accel_cpu_instance_init(CPU(obj)); >>>> } >>> >>> Why is this only done for x86? >>> >>> >>> r~ >>> >> >> It makes sense to include the other architectures. >> >> As the next step I would like to apply this to ARM, but to me it makes sense to first complete Philippe's series, >> which reshuffles things so that TCG-only / KVM-only builds are both possible and error-free: >> >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg777627.html > > TBH this series is very unlikely to be merged before yours, > so go ahead... (eventually you can cherry-pick what you need > from it). > Hi Philippe, Peter, I am working on ARM right now, splitting things between user/system, tcg/kvm. One difficulty I found in particular is with the ARM PMU code. Currently the PMU code is mixed in with the TCG helpers, in target/arm/helper.c. Now, with KVM we should be using the KVM PMU, but still the KVM code currently calls pmu_init and registers the timer: In target/arm/cpu.c initialization we see: " if (arm_feature(env, ARM_FEATURE_PMU)) { pmu_init(cpu); if (!kvm_enabled()) { arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0); arm_register_el_change_hook(cpu, &pmu_post_el_change, 0); } #ifndef CONFIG_USER_ONLY cpu->pmu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, arm_pmu_timer_cb, cpu); #endif " Now, even for KVM, pmu_init is called and the cpu->pmu_timer is registered, but I don't see arm_pmu_timer_cb triggering. Is the pmu_timer really necessary also for KVM builds, or should it actually be TCG-only? Could you share your hints on which parts of the PMU code in helper.c should actually be shared if any? Thanks a lot, Claudio
diff --git a/include/qemu/accel.h b/include/qemu/accel.h index b9d6d69eb8..da0c8ab523 100644 --- a/include/qemu/accel.h +++ b/include/qemu/accel.h @@ -78,4 +78,17 @@ int accel_init_machine(AccelState *accel, MachineState *ms); void accel_setup_post(MachineState *ms); #endif /* !CONFIG_USER_ONLY */ +/** + * accel_cpu_instance_init: + * @cpu: The CPU that needs to do accel-specific object initializations. + */ +void accel_cpu_instance_init(CPUState *cpu); + +/** + * accel_cpu_realizefn: + * @cpu: The CPU that needs to call accel-specific cpu realization. + * @errp: currently unused. + */ +void accel_cpu_realizefn(CPUState *cpu, Error **errp); + #endif /* QEMU_ACCEL_H */ diff --git a/accel/accel-common.c b/accel/accel-common.c index 9901b0531c..0f6fb4fb66 100644 --- a/accel/accel-common.c +++ b/accel/accel-common.c @@ -89,6 +89,25 @@ void accel_init_interfaces(AccelClass *ac) accel_init_cpu_interfaces(ac); } +void accel_cpu_instance_init(CPUState *cpu) +{ + CPUClass *cc = CPU_GET_CLASS(cpu); + + if (cc->accel_cpu && cc->accel_cpu->cpu_instance_init) { + cc->accel_cpu->cpu_instance_init(cpu); + } +} + +void accel_cpu_realizefn(CPUState *cpu, Error **errp) +{ + CPUClass *cc = CPU_GET_CLASS(cpu); + + if (cc->accel_cpu && cc->accel_cpu->cpu_realizefn) { + /* NB: errp parameter is unused currently */ + cc->accel_cpu->cpu_realizefn(cpu, errp); + } +} + static const TypeInfo accel_cpu_type = { .name = TYPE_ACCEL_CPU, .parent = TYPE_OBJECT, diff --git a/cpu.c b/cpu.c index ba5d272c1e..25e6fbfa2c 100644 --- a/cpu.c +++ b/cpu.c @@ -130,11 +130,7 @@ void cpu_exec_realizefn(CPUState *cpu, Error **errp) CPUClass *cc = CPU_GET_CLASS(cpu); cpu_list_add(cpu); - - if (cc->accel_cpu) { - /* NB: errp parameter is unused currently */ - cc->accel_cpu->cpu_realizefn(cpu, errp); - } + accel_cpu_realizefn(cpu, errp); #ifdef CONFIG_TCG /* NB: errp parameter is unused currently */ diff --git a/target/i386/cpu.c b/target/i386/cpu.c index d7735f0ed3..be068fcc5e 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -28,7 +28,6 @@ #include "sysemu/kvm.h" #include "sysemu/reset.h" #include "sysemu/hvf.h" -#include "hw/core/accel-cpu.h" #include "sysemu/xen.h" #include "sysemu/whpx.h" #include "kvm/kvm_i386.h" @@ -6675,8 +6674,6 @@ static void x86_cpu_initfn(Object *obj) { X86CPU *cpu = X86_CPU(obj); X86CPUClass *xcc = X86_CPU_GET_CLASS(obj); - CPUClass *cc = CPU_CLASS(xcc); - CPUX86State *env = &cpu->env; env->nr_dies = 1; @@ -6725,10 +6722,8 @@ static void x86_cpu_initfn(Object *obj) x86_cpu_load_model(cpu, xcc->model); } - /* if required, do the accelerator-specific cpu initialization */ - if (cc->accel_cpu) { - cc->accel_cpu->cpu_instance_init(CPU(obj)); - } + /* if required, do accelerator-specific cpu initializations */ + accel_cpu_instance_init(CPU(obj)); } static int64_t x86_cpu_get_arch_id(CPUState *cs)
avoid open coding the accesses to cpu->accel_cpu interfaces, and instead introduce: accel_cpu_instance_init, accel_cpu_realizefn to be used by the targets/ initfn code, and by cpu_exec_realizefn respectively. Signed-off-by: Claudio Fontana <cfontana@suse.de> --- include/qemu/accel.h | 13 +++++++++++++ accel/accel-common.c | 19 +++++++++++++++++++ cpu.c | 6 +----- target/i386/cpu.c | 9 ++------- 4 files changed, 35 insertions(+), 12 deletions(-)