| Message ID | 20200309121103.20234-2-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cpu: Avoid latent bug calling cpu_reset() on uninitialized vCPU | expand |
Le 09/03/2020 à 13:11, Philippe Mathieu-Daudé a écrit : > cpu_reset() might modify architecture-specific fields allocated > by qemu_init_vcpu(). To avoid bugs similar to the one fixed in > commit 00d0f7cb66 when introducing new architectures, move the > cpu_reset() calls after qemu_init_vcpu(). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > target/cris/cpu.c | 2 +- > target/lm32/cpu.c | 3 +-- > target/m68k/cpu.c | 2 +- > target/mips/cpu.c | 2 +- > target/sh4/cpu.c | 2 +- > target/tilegx/cpu.c | 2 +- > target/tricore/cpu.c | 2 +- > 7 files changed, 7 insertions(+), 8 deletions(-) > > diff --git a/target/cris/cpu.c b/target/cris/cpu.c > index 17c6712e29..9b8b99840d 100644 > --- a/target/cris/cpu.c > +++ b/target/cris/cpu.c > @@ -134,8 +134,8 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp) > return; > } > > - cpu_reset(cs); > qemu_init_vcpu(cs); > + cpu_reset(cs); > > ccc->parent_realize(dev, errp); > } > diff --git a/target/lm32/cpu.c b/target/lm32/cpu.c > index 687bf35e65..56f7b97c8f 100644 > --- a/target/lm32/cpu.c > +++ b/target/lm32/cpu.c > @@ -132,9 +132,8 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp) > return; > } > > - cpu_reset(cs); > - > qemu_init_vcpu(cs); > + cpu_reset(cs); > > lcc->parent_realize(dev, errp); > } > diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c > index f0653cda2f..51ca62694e 100644 > --- a/target/m68k/cpu.c > +++ b/target/m68k/cpu.c > @@ -247,8 +247,8 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp) > > m68k_cpu_init_gdb(cpu); > > - cpu_reset(cs); > qemu_init_vcpu(cs); > + cpu_reset(cs); > > mcc->parent_realize(dev, errp); > } > diff --git a/target/mips/cpu.c b/target/mips/cpu.c > index 6cd6b9650b..553945005f 100644 > --- a/target/mips/cpu.c > +++ b/target/mips/cpu.c > @@ -149,8 +149,8 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp) > > cpu_mips_realize_env(&cpu->env); > > - cpu_reset(cs); > qemu_init_vcpu(cs); > + cpu_reset(cs); > > mcc->parent_realize(dev, errp); > } > diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c > index 70c8d8170f..2564436719 100644 > --- a/target/sh4/cpu.c > +++ b/target/sh4/cpu.c > @@ -184,8 +184,8 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp) > return; > } > > - cpu_reset(cs); > qemu_init_vcpu(cs); > + cpu_reset(cs); > > scc->parent_realize(dev, errp); > } > diff --git a/target/tilegx/cpu.c b/target/tilegx/cpu.c > index cd422a0467..7e9982197f 100644 > --- a/target/tilegx/cpu.c > +++ b/target/tilegx/cpu.c > @@ -91,8 +91,8 @@ static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp) > return; > } > > - cpu_reset(cs); > qemu_init_vcpu(cs); > + cpu_reset(cs); > > tcc->parent_realize(dev, errp); > } > diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c > index 85bc9f03a1..c5a5d54569 100644 > --- a/target/tricore/cpu.c > +++ b/target/tricore/cpu.c > @@ -94,8 +94,8 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp) > if (tricore_feature(env, TRICORE_FEATURE_131)) { > set_feature(env, TRICORE_FEATURE_13); > } > - cpu_reset(cs); > qemu_init_vcpu(cs); > + cpu_reset(cs); > > tcc->parent_realize(dev, errp); > } > Reviewed-by: Laurent Vivier <laurent@vivier.eu>
On Mon, 9 Mar 2020 at 12:11, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > cpu_reset() might modify architecture-specific fields allocated > by qemu_init_vcpu(). To avoid bugs similar to the one fixed in > commit 00d0f7cb66 when introducing new architectures, move the > cpu_reset() calls after qemu_init_vcpu(). > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Why do we need to call cpu_reset() from realize anyway? Generally for devices this is incorrect as they should be being reset by some other mechanism. Obviously actually determining that dropping the cpu_reset() call is safe would require some tedious auditing. If we do do a cpu_reset() in realize, should it be after the call to the parent realize function ? thanks -- PMM
Le 09/03/2020 à 14:09, Peter Maydell a écrit : > On Mon, 9 Mar 2020 at 12:11, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> cpu_reset() might modify architecture-specific fields allocated >> by qemu_init_vcpu(). To avoid bugs similar to the one fixed in >> commit 00d0f7cb66 when introducing new architectures, move the >> cpu_reset() calls after qemu_init_vcpu(). >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > Why do we need to call cpu_reset() from realize anyway? > Generally for devices this is incorrect as they should be > being reset by some other mechanism. I have this same change in my branch for q800 to set the initial PC and stack pointer read from memory on cold boot. Do we have another way to do that? Thanks, Laurent
On Mon, 9 Mar 2020 at 13:13, Laurent Vivier <laurent@vivier.eu> wrote: > > Le 09/03/2020 à 14:09, Peter Maydell a écrit : > > On Mon, 9 Mar 2020 at 12:11, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >> > >> cpu_reset() might modify architecture-specific fields allocated > >> by qemu_init_vcpu(). To avoid bugs similar to the one fixed in > >> commit 00d0f7cb66 when introducing new architectures, move the > >> cpu_reset() calls after qemu_init_vcpu(). > >> > >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > > Why do we need to call cpu_reset() from realize anyway? > > Generally for devices this is incorrect as they should be > > being reset by some other mechanism. > > I have this same change in my branch for q800 to set the initial PC and > stack pointer read from memory on cold boot. > > Do we have another way to do that? The expectation at the moment is that the board code should register a reset function with qemu_register_reset() which calls cpu_reset(). Relying on doing a reset in realize won't work for the case where there's a QEMU system reset, because we don't re-init/realize everything, we just call all the reset hooks. If m68k reads pc/sp from memory on reset you'll probably run into the same reset-ordering vs hw/cpu/loader.c that Arm M-profile has; we currently work around that in the arm reset function. I had hoped that 3-phase reset would resolve this reset order issue, but it has not turned out to be so easy. thanks -- PMM
Le 09/03/2020 à 14:18, Peter Maydell a écrit : > On Mon, 9 Mar 2020 at 13:13, Laurent Vivier <laurent@vivier.eu> wrote: >> >> Le 09/03/2020 à 14:09, Peter Maydell a écrit : >>> On Mon, 9 Mar 2020 at 12:11, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >>>> >>>> cpu_reset() might modify architecture-specific fields allocated >>>> by qemu_init_vcpu(). To avoid bugs similar to the one fixed in >>>> commit 00d0f7cb66 when introducing new architectures, move the >>>> cpu_reset() calls after qemu_init_vcpu(). >>>> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> >>> Why do we need to call cpu_reset() from realize anyway? >>> Generally for devices this is incorrect as they should be >>> being reset by some other mechanism. >> >> I have this same change in my branch for q800 to set the initial PC and >> stack pointer read from memory on cold boot. >> >> Do we have another way to do that? > > The expectation at the moment is that the board code should > register a reset function with qemu_register_reset() which > calls cpu_reset(). Relying on doing a reset in realize won't > work for the case where there's a QEMU system reset, because > we don't re-init/realize everything, we just call all the > reset hooks. > > If m68k reads pc/sp from memory on reset you'll probably run > into the same reset-ordering vs hw/cpu/loader.c that Arm M-profile > has; we currently work around that in the arm reset function. > I had hoped that 3-phase reset would resolve this reset order > issue, but it has not turned out to be so easy. Thank you for the hint, I'll have a look. Laurent
diff --git a/target/cris/cpu.c b/target/cris/cpu.c index 17c6712e29..9b8b99840d 100644 --- a/target/cris/cpu.c +++ b/target/cris/cpu.c @@ -134,8 +134,8 @@ static void cris_cpu_realizefn(DeviceState *dev, Error **errp) return; } - cpu_reset(cs); qemu_init_vcpu(cs); + cpu_reset(cs); ccc->parent_realize(dev, errp); } diff --git a/target/lm32/cpu.c b/target/lm32/cpu.c index 687bf35e65..56f7b97c8f 100644 --- a/target/lm32/cpu.c +++ b/target/lm32/cpu.c @@ -132,9 +132,8 @@ static void lm32_cpu_realizefn(DeviceState *dev, Error **errp) return; } - cpu_reset(cs); - qemu_init_vcpu(cs); + cpu_reset(cs); lcc->parent_realize(dev, errp); } diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c index f0653cda2f..51ca62694e 100644 --- a/target/m68k/cpu.c +++ b/target/m68k/cpu.c @@ -247,8 +247,8 @@ static void m68k_cpu_realizefn(DeviceState *dev, Error **errp) m68k_cpu_init_gdb(cpu); - cpu_reset(cs); qemu_init_vcpu(cs); + cpu_reset(cs); mcc->parent_realize(dev, errp); } diff --git a/target/mips/cpu.c b/target/mips/cpu.c index 6cd6b9650b..553945005f 100644 --- a/target/mips/cpu.c +++ b/target/mips/cpu.c @@ -149,8 +149,8 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp) cpu_mips_realize_env(&cpu->env); - cpu_reset(cs); qemu_init_vcpu(cs); + cpu_reset(cs); mcc->parent_realize(dev, errp); } diff --git a/target/sh4/cpu.c b/target/sh4/cpu.c index 70c8d8170f..2564436719 100644 --- a/target/sh4/cpu.c +++ b/target/sh4/cpu.c @@ -184,8 +184,8 @@ static void superh_cpu_realizefn(DeviceState *dev, Error **errp) return; } - cpu_reset(cs); qemu_init_vcpu(cs); + cpu_reset(cs); scc->parent_realize(dev, errp); } diff --git a/target/tilegx/cpu.c b/target/tilegx/cpu.c index cd422a0467..7e9982197f 100644 --- a/target/tilegx/cpu.c +++ b/target/tilegx/cpu.c @@ -91,8 +91,8 @@ static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp) return; } - cpu_reset(cs); qemu_init_vcpu(cs); + cpu_reset(cs); tcc->parent_realize(dev, errp); } diff --git a/target/tricore/cpu.c b/target/tricore/cpu.c index 85bc9f03a1..c5a5d54569 100644 --- a/target/tricore/cpu.c +++ b/target/tricore/cpu.c @@ -94,8 +94,8 @@ static void tricore_cpu_realizefn(DeviceState *dev, Error **errp) if (tricore_feature(env, TRICORE_FEATURE_131)) { set_feature(env, TRICORE_FEATURE_13); } - cpu_reset(cs); qemu_init_vcpu(cs); + cpu_reset(cs); tcc->parent_realize(dev, errp); }
cpu_reset() might modify architecture-specific fields allocated by qemu_init_vcpu(). To avoid bugs similar to the one fixed in commit 00d0f7cb66 when introducing new architectures, move the cpu_reset() calls after qemu_init_vcpu(). Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- target/cris/cpu.c | 2 +- target/lm32/cpu.c | 3 +-- target/m68k/cpu.c | 2 +- target/mips/cpu.c | 2 +- target/sh4/cpu.c | 2 +- target/tilegx/cpu.c | 2 +- target/tricore/cpu.c | 2 +- 7 files changed, 7 insertions(+), 8 deletions(-)