| Message ID | 20200309121103.20234-3-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cpu: Avoid latent bug calling cpu_reset() on uninitialized vCPU | expand |
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, assert a > vCPU is created before resetting it. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/core/cpu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/core/cpu.c b/hw/core/cpu.c > index fe65ca62ac..09e49f8d6a 100644 > --- a/hw/core/cpu.c > +++ b/hw/core/cpu.c > @@ -251,6 +251,7 @@ void cpu_reset(CPUState *cpu) > { > CPUClass *klass = CPU_GET_CLASS(cpu); > > + assert(cpu->created); > if (klass->reset != NULL) { > (*klass->reset)(cpu); > } This will conflict with the change to use DeviceClass::reset. Ideally we should do an equivalent assert in the DeviceClass (and flush out all the bugs where we forgot to realize the device before using it). thanks -- PMM
On 3/9/20 2:10 PM, Peter Maydell wrote: > 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, assert a >> vCPU is created before resetting it. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/core/cpu.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/core/cpu.c b/hw/core/cpu.c >> index fe65ca62ac..09e49f8d6a 100644 >> --- a/hw/core/cpu.c >> +++ b/hw/core/cpu.c >> @@ -251,6 +251,7 @@ void cpu_reset(CPUState *cpu) >> { >> CPUClass *klass = CPU_GET_CLASS(cpu); >> >> + assert(cpu->created); >> if (klass->reset != NULL) { >> (*klass->reset)(cpu); >> } > > This will conflict with the change to use DeviceClass::reset. > > Ideally we should do an equivalent assert in the DeviceClass > (and flush out all the bugs where we forgot to realize the > device before using it). OK (I should have sent as RFC probably). Anyway this fails the ppc64le/s390x linux-user tests on Travis-CI: qemu-ppc64le: hw/core/cpu.c:254: cpu_reset: Assertion `cpu->created' failed. qemu-s390x: hw/core/cpu.c:254: cpu_reset: Assertion `cpu->created' failed. > > thanks > -- PMM >
diff --git a/hw/core/cpu.c b/hw/core/cpu.c index fe65ca62ac..09e49f8d6a 100644 --- a/hw/core/cpu.c +++ b/hw/core/cpu.c @@ -251,6 +251,7 @@ void cpu_reset(CPUState *cpu) { CPUClass *klass = CPU_GET_CLASS(cpu); + assert(cpu->created); if (klass->reset != NULL) { (*klass->reset)(cpu); }
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, assert a vCPU is created before resetting it. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/core/cpu.c | 1 + 1 file changed, 1 insertion(+)