| Message ID | 20230918160257.30127-5-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | exec/cpu: Call cpu_exec_realizefn() once in cpu_common_realize() | expand |
On 9/18/23 09:02, Philippe Mathieu-Daudé wrote: > QDev instance is expected to be in an unknown state until full > object realization. Thus we shouldn't call DeviceReset() on an > unrealized instance. Move the cpu_reset() call from*before* > the parent realize() handler (effectively cpu_common_realizefn) > to*after* it. > > Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org> > --- > RFC: > I haven't audited all the call sites, but plan to do it, > amending the result to this patch description. This used > to be a problem on some targets, but as of today's master > this series pass make check-{qtest,avocado}. > --- > target/arm/cpu.c | 2 +- > target/avr/cpu.c | 2 +- > target/cris/cpu.c | 2 +- > target/hexagon/cpu.c | 3 +-- > target/i386/cpu.c | 2 +- > target/loongarch/cpu.c | 2 +- > target/m68k/cpu.c | 2 +- > target/mips/cpu.c | 2 +- > target/nios2/cpu.c | 2 +- > target/openrisc/cpu.c | 2 +- > target/riscv/cpu.c | 2 +- > target/rx/cpu.c | 2 +- > target/s390x/cpu.c | 2 +- > target/sh4/cpu.c | 2 +- > target/tricore/cpu.c | 2 +- > 15 files changed, 15 insertions(+), 16 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Mon, 18 Sep 2023 18:02:37 +0200 Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > QDev instance is expected to be in an unknown state until full > object realization. Thus we shouldn't call DeviceReset() on an > unrealized instance. Move the cpu_reset() call from *before* > the parent realize() handler (effectively cpu_common_realizefn) > to *after* it. looking at: --cpu_reset() <- brings cpu to known (after reset|poweron) state cpu_common_realizefn() if (dev->hotplugged) { cpu_synchronize_post_init(cpu); cpu_resume(cpu); } ++cpu_reset() <-- looks to be too late, we already told hypervisor to run undefined CPU, didn't we? > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > RFC: > I haven't audited all the call sites, but plan to do it, > amending the result to this patch description. This used > to be a problem on some targets, but as of today's master > this series pass make check-{qtest,avocado}. > --- > target/arm/cpu.c | 2 +- > target/avr/cpu.c | 2 +- > target/cris/cpu.c | 2 +- > target/hexagon/cpu.c | 3 +-- > target/i386/cpu.c | 2 +- > target/loongarch/cpu.c | 2 +- > target/m68k/cpu.c | 2 +- > target/mips/cpu.c | 2 +- > target/nios2/cpu.c | 2 +- > target/openrisc/cpu.c | 2 +- > target/riscv/cpu.c | 2 +- > target/rx/cpu.c | 2 +- > target/s390x/cpu.c | 2 +- > target/sh4/cpu.c | 2 +- > target/tricore/cpu.c | 2 +- > 15 files changed, 15 insertions(+), 16 deletions(-) > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index b9e09a702d..6aca036b85 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -2278,9 +2278,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > } > > qemu_init_vcpu(cs); > - cpu_reset(cs); > > acc->parent_realize(dev, errp); > + cpu_reset(cs); > } > > static ObjectClass *arm_cpu_class_by_name(const char *cpu_model) > diff --git a/target/avr/cpu.c b/target/avr/cpu.c > index 8f741f258c..84d353f30e 100644 > --- a/target/avr/cpu.c > +++ b/target/avr/cpu.c > @@ -120,9 +120,9 @@ static void avr_cpu_realizefn(DeviceState *dev, Error **errp) > return; > } > qemu_init_vcpu(cs); > - cpu_reset(cs); > > mcc->parent_realize(dev, errp); > + cpu_reset(cs); > } > > static void avr_cpu_set_int(void *opaque, int irq, int level) > diff --git a/target/cris/cpu.c b/target/cris/cpu.c > index a6a93c2359..079872a5cc 100644 > --- a/target/cris/cpu.c > +++ b/target/cris/cpu.c > @@ -152,10 +152,10 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp) > return; > } > > - cpu_reset(cs); > qemu_init_vcpu(cs); > > ccc->parent_realize(dev, errp); > + cpu_reset(cs); > } > > #ifndef CONFIG_USER_ONLY > diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c > index f155936289..7edc32701f 100644 > --- a/target/hexagon/cpu.c > +++ b/target/hexagon/cpu.c > @@ -346,9 +346,8 @@ static void hexagon_cpu_realize(DeviceState *dev, Error **errp) > "hexagon-hvx.xml", 0); > > qemu_init_vcpu(cs); > - cpu_reset(cs); > - > mcc->parent_realize(dev, errp); > + cpu_reset(cs); > } > > static void hexagon_cpu_init(Object *obj) > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index a23d4795e0..7faaa6915f 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -7455,9 +7455,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > } > } > #endif /* !CONFIG_USER_ONLY */ > - cpu_reset(cs); > > xcc->parent_realize(dev, &local_err); > + cpu_reset(cs); > > out: > if (local_err != NULL) { > diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c > index 65f9320e34..8029e70e76 100644 > --- a/target/loongarch/cpu.c > +++ b/target/loongarch/cpu.c > @@ -565,10 +565,10 @@ static void loongarch_cpu_realizefn(DeviceState *dev, Error **errp) > > loongarch_cpu_register_gdb_regs_for_features(cs); > > - cpu_reset(cs); > qemu_init_vcpu(cs); > > lacc->parent_realize(dev, errp); > + cpu_reset(cs); > } > > #ifndef CONFIG_USER_ONLY > diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c > index 70d58471dc..2bc0a62f0e 100644 > --- a/target/m68k/cpu.c > +++ b/target/m68k/cpu.c > @@ -321,10 +321,10 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp) > > m68k_cpu_init_gdb(cpu); > > - cpu_reset(cs); > qemu_init_vcpu(cs); > > mcc->parent_realize(dev, errp); > + cpu_reset(cs); > } > > static void m68k_cpu_initfn(Object *obj) > diff --git a/target/mips/cpu.c b/target/mips/cpu.c > index 63da1948fd..8d6f633f72 100644 > --- a/target/mips/cpu.c > +++ b/target/mips/cpu.c > @@ -492,10 +492,10 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp) > fpu_init(env, env->cpu_model); > mvp_init(env); > > - cpu_reset(cs); > qemu_init_vcpu(cs); > > mcc->parent_realize(dev, errp); > + cpu_reset(cs); > } > > static void mips_cpu_initfn(Object *obj) > diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c > index bc5cbf81c2..876a6dcad2 100644 > --- a/target/nios2/cpu.c > +++ b/target/nios2/cpu.c > @@ -217,12 +217,12 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp) > > realize_cr_status(cs); > qemu_init_vcpu(cs); > - cpu_reset(cs); > > /* We have reserved storage for cpuid; might as well use it. */ > cpu->env.ctrl[CR_CPUID] = cs->cpu_index; > > ncc->parent_realize(dev, errp); > + cpu_reset(cs); > } > > #ifndef CONFIG_USER_ONLY > diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c > index 61d748cfdc..cd25f1e9d5 100644 > --- a/target/openrisc/cpu.c > +++ b/target/openrisc/cpu.c > @@ -142,9 +142,9 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp) > } > > qemu_init_vcpu(cs); > - cpu_reset(cs); > > occ->parent_realize(dev, errp); > + cpu_reset(cs); > } > > static void openrisc_cpu_initfn(Object *obj) > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index f227c7664e..7566757346 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -1532,9 +1532,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > #endif > > qemu_init_vcpu(cs); > - cpu_reset(cs); > > mcc->parent_realize(dev, errp); > + cpu_reset(cs); > } > > #ifndef CONFIG_USER_ONLY > diff --git a/target/rx/cpu.c b/target/rx/cpu.c > index 157e57da0f..c9c8443cbd 100644 > --- a/target/rx/cpu.c > +++ b/target/rx/cpu.c > @@ -138,9 +138,9 @@ static void rx_cpu_realize(DeviceState *dev, Error **errp) > } > > qemu_init_vcpu(cs); > - cpu_reset(cs); > > rcc->parent_realize(dev, errp); > + cpu_reset(cs); > } > > static void rx_cpu_set_irq(void *opaque, int no, int request) > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c > index df167493c3..0f0b11fd73 100644 > --- a/target/s390x/cpu.c > +++ b/target/s390x/cpu.c > @@ -254,6 +254,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) > s390_cpu_gdb_init(cs); > qemu_init_vcpu(cs); > > + scc->parent_realize(dev, &err); > /* > * KVM requires the initial CPU reset ioctl to be executed on the target > * CPU thread. CPU hotplug under single-threaded TCG will not work with > @@ -266,7 +267,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) > cpu_reset(cs); > } > > - scc->parent_realize(dev, &err); > out: > error_propagate(errp, err); > } > diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c > index 61769ffdfa..656d71f74a 100644 > --- a/target/sh4/cpu.c > +++ b/target/sh4/cpu.c > @@ -228,10 +228,10 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp) > return; > } > > - cpu_reset(cs); > qemu_init_vcpu(cs); > > scc->parent_realize(dev, errp); > + cpu_reset(cs); > } > > static void superh_cpu_initfn(Object *obj) > diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c > index 133a9ac70e..a3610aecca 100644 > --- a/target/tricore/cpu.c > +++ b/target/tricore/cpu.c > @@ -118,10 +118,10 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp) > if (tricore_has_feature(env, TRICORE_FEATURE_131)) { > set_feature(env, TRICORE_FEATURE_13); > } > - cpu_reset(cs); > qemu_init_vcpu(cs); > > tcc->parent_realize(dev, errp); > + cpu_reset(cs); > } > >
diff --git a/target/arm/cpu.c b/target/arm/cpu.c index b9e09a702d..6aca036b85 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -2278,9 +2278,9 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) } qemu_init_vcpu(cs); - cpu_reset(cs); acc->parent_realize(dev, errp); + cpu_reset(cs); } static ObjectClass *arm_cpu_class_by_name(const char *cpu_model) diff --git a/target/avr/cpu.c b/target/avr/cpu.c index 8f741f258c..84d353f30e 100644 --- a/target/avr/cpu.c +++ b/target/avr/cpu.c @@ -120,9 +120,9 @@ static void avr_cpu_realizefn(DeviceState *dev, Error **errp) return; } qemu_init_vcpu(cs); - cpu_reset(cs); mcc->parent_realize(dev, errp); + cpu_reset(cs); } static void avr_cpu_set_int(void *opaque, int irq, int level) diff --git a/target/cris/cpu.c b/target/cris/cpu.c index a6a93c2359..079872a5cc 100644 --- a/target/cris/cpu.c +++ b/target/cris/cpu.c @@ -152,10 +152,10 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp) return; } - cpu_reset(cs); qemu_init_vcpu(cs); ccc->parent_realize(dev, errp); + cpu_reset(cs); } #ifndef CONFIG_USER_ONLY diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c index f155936289..7edc32701f 100644 --- a/target/hexagon/cpu.c +++ b/target/hexagon/cpu.c @@ -346,9 +346,8 @@ static void hexagon_cpu_realize(DeviceState *dev, Error **errp) "hexagon-hvx.xml", 0); qemu_init_vcpu(cs); - cpu_reset(cs); - mcc->parent_realize(dev, errp); + cpu_reset(cs); } static void hexagon_cpu_init(Object *obj) diff --git a/target/i386/cpu.c b/target/i386/cpu.c index a23d4795e0..7faaa6915f 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -7455,9 +7455,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) } } #endif /* !CONFIG_USER_ONLY */ - cpu_reset(cs); xcc->parent_realize(dev, &local_err); + cpu_reset(cs); out: if (local_err != NULL) { diff --git a/target/loongarch/cpu.c b/target/loongarch/cpu.c index 65f9320e34..8029e70e76 100644 --- a/target/loongarch/cpu.c +++ b/target/loongarch/cpu.c @@ -565,10 +565,10 @@ static void loongarch_cpu_realizefn(DeviceState *dev, Error **errp) loongarch_cpu_register_gdb_regs_for_features(cs); - cpu_reset(cs); qemu_init_vcpu(cs); lacc->parent_realize(dev, errp); + cpu_reset(cs); } #ifndef CONFIG_USER_ONLY diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c index 70d58471dc..2bc0a62f0e 100644 --- a/target/m68k/cpu.c +++ b/target/m68k/cpu.c @@ -321,10 +321,10 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp) m68k_cpu_init_gdb(cpu); - cpu_reset(cs); qemu_init_vcpu(cs); mcc->parent_realize(dev, errp); + cpu_reset(cs); } static void m68k_cpu_initfn(Object *obj) diff --git a/target/mips/cpu.c b/target/mips/cpu.c index 63da1948fd..8d6f633f72 100644 --- a/target/mips/cpu.c +++ b/target/mips/cpu.c @@ -492,10 +492,10 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp) fpu_init(env, env->cpu_model); mvp_init(env); - cpu_reset(cs); qemu_init_vcpu(cs); mcc->parent_realize(dev, errp); + cpu_reset(cs); } static void mips_cpu_initfn(Object *obj) diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c index bc5cbf81c2..876a6dcad2 100644 --- a/target/nios2/cpu.c +++ b/target/nios2/cpu.c @@ -217,12 +217,12 @@ static void nios2_cpu_realizefn(DeviceState *dev, Error **errp) realize_cr_status(cs); qemu_init_vcpu(cs); - cpu_reset(cs); /* We have reserved storage for cpuid; might as well use it. */ cpu->env.ctrl[CR_CPUID] = cs->cpu_index; ncc->parent_realize(dev, errp); + cpu_reset(cs); } #ifndef CONFIG_USER_ONLY diff --git a/target/openrisc/cpu.c b/target/openrisc/cpu.c index 61d748cfdc..cd25f1e9d5 100644 --- a/target/openrisc/cpu.c +++ b/target/openrisc/cpu.c @@ -142,9 +142,9 @@ static void openrisc_cpu_realizefn(DeviceState *dev, Error **errp) } qemu_init_vcpu(cs); - cpu_reset(cs); occ->parent_realize(dev, errp); + cpu_reset(cs); } static void openrisc_cpu_initfn(Object *obj) diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index f227c7664e..7566757346 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -1532,9 +1532,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) #endif qemu_init_vcpu(cs); - cpu_reset(cs); mcc->parent_realize(dev, errp); + cpu_reset(cs); } #ifndef CONFIG_USER_ONLY diff --git a/target/rx/cpu.c b/target/rx/cpu.c index 157e57da0f..c9c8443cbd 100644 --- a/target/rx/cpu.c +++ b/target/rx/cpu.c @@ -138,9 +138,9 @@ static void rx_cpu_realize(DeviceState *dev, Error **errp) } qemu_init_vcpu(cs); - cpu_reset(cs); rcc->parent_realize(dev, errp); + cpu_reset(cs); } static void rx_cpu_set_irq(void *opaque, int no, int request) diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c index df167493c3..0f0b11fd73 100644 --- a/target/s390x/cpu.c +++ b/target/s390x/cpu.c @@ -254,6 +254,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) s390_cpu_gdb_init(cs); qemu_init_vcpu(cs); + scc->parent_realize(dev, &err); /* * KVM requires the initial CPU reset ioctl to be executed on the target * CPU thread. CPU hotplug under single-threaded TCG will not work with @@ -266,7 +267,6 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) cpu_reset(cs); } - scc->parent_realize(dev, &err); out: error_propagate(errp, err); } diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c index 61769ffdfa..656d71f74a 100644 --- a/target/sh4/cpu.c +++ b/target/sh4/cpu.c @@ -228,10 +228,10 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp) return; } - cpu_reset(cs); qemu_init_vcpu(cs); scc->parent_realize(dev, errp); + cpu_reset(cs); } static void superh_cpu_initfn(Object *obj) diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c index 133a9ac70e..a3610aecca 100644 --- a/target/tricore/cpu.c +++ b/target/tricore/cpu.c @@ -118,10 +118,10 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp) if (tricore_has_feature(env, TRICORE_FEATURE_131)) { set_feature(env, TRICORE_FEATURE_13); } - cpu_reset(cs); qemu_init_vcpu(cs); tcc->parent_realize(dev, errp); + cpu_reset(cs); }
QDev instance is expected to be in an unknown state until full object realization. Thus we shouldn't call DeviceReset() on an unrealized instance. Move the cpu_reset() call from *before* the parent realize() handler (effectively cpu_common_realizefn) to *after* it. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- RFC: I haven't audited all the call sites, but plan to do it, amending the result to this patch description. This used to be a problem on some targets, but as of today's master this series pass make check-{qtest,avocado}. --- target/arm/cpu.c | 2 +- target/avr/cpu.c | 2 +- target/cris/cpu.c | 2 +- target/hexagon/cpu.c | 3 +-- target/i386/cpu.c | 2 +- target/loongarch/cpu.c | 2 +- target/m68k/cpu.c | 2 +- target/mips/cpu.c | 2 +- target/nios2/cpu.c | 2 +- target/openrisc/cpu.c | 2 +- target/riscv/cpu.c | 2 +- target/rx/cpu.c | 2 +- target/s390x/cpu.c | 2 +- target/sh4/cpu.c | 2 +- target/tricore/cpu.c | 2 +- 15 files changed, 15 insertions(+), 16 deletions(-)