Message ID | 1502527090-11473-1-git-send-email-thuth@redhat.com |
---|---|
State | New |
Headers | show |
On Sat, 12 Aug 2017 10:38:10 +0200 Thomas Huth <thuth@redhat.com> wrote: > QEMU currently crashes when the user tries to add a spapr-cpu-core > on a non-pseries machine: > > $ qemu-system-ppc64 -S -machine ppce500,accel=tcg \ > -device POWER5+_v2.1-spapr-cpu-core > hw/ppc/spapr_cpu_core.c:178:spapr_cpu_core_realize_child: > Object 0x55cee1f55160 is not an instance of type spapr-machine > Aborted (core dumped) > > So let's add a proper check for the correct machine time with > a more friendly error message here. > > Reported-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/ppc/spapr_cpu_core.c | 9 ++++++++- > scripts/device-crash-test | 1 + > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > index ea278ce..0f3d653 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -175,11 +175,18 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp) > static void spapr_cpu_core_realize_child(Object *child, Error **errp) > { > Error *local_err = NULL; > - sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > + sPAPRMachineState *spapr; > CPUState *cs = CPU(child); > PowerPCCPU *cpu = POWERPC_CPU(cs); > Object *obj; > > + spapr = (sPAPRMachineState *)object_dynamic_cast(qdev_get_machine(), > + TYPE_SPAPR_MACHINE); > + if (!spapr) { > + error_setg(errp, "spapr-cpu-core needs a pseries machine"); > + return; > + } > + This is the realize function for individual threads. Maybe this sanity check should be performed earlier at the core level in spapr_cpu_core_realize() ? Also, spapr_cpu_core_realize_child() seem to only have spapr to pass it down to spapr_cpu_init()... ie, not even sure spapr_cpu_core_realize_child() actually needs a spapr variable. > object_property_set_bool(child, true, "realized", &local_err); > if (local_err) { > goto error; > diff --git a/scripts/device-crash-test b/scripts/device-crash-test > index e77b693..8eb2d02 100755 > --- a/scripts/device-crash-test > +++ b/scripts/device-crash-test > @@ -200,6 +200,7 @@ ERROR_WHITELIST = [ > {'log':r"Multiple VT220 operator consoles are not supported"}, > {'log':r"core 0 already populated"}, > {'log':r"could not find stage1 bootloader"}, > + {'log':r"spapr-cpu-core needs a pseries machine"}, > > # other exitcode=1 failures not listed above will just generate INFO messages: > {'exitcode':1, 'loglevel':logging.INFO},
On Sat, Aug 12, 2017 at 10:38:10AM +0200, Thomas Huth wrote: > QEMU currently crashes when the user tries to add a spapr-cpu-core > on a non-pseries machine: > > $ qemu-system-ppc64 -S -machine ppce500,accel=tcg \ > -device POWER5+_v2.1-spapr-cpu-core > hw/ppc/spapr_cpu_core.c:178:spapr_cpu_core_realize_child: > Object 0x55cee1f55160 is not an instance of type spapr-machine > Aborted (core dumped) > > So let's add a proper check for the correct machine time with > a more friendly error message here. > > Reported-by: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > hw/ppc/spapr_cpu_core.c | 9 ++++++++- > scripts/device-crash-test | 1 + > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > index ea278ce..0f3d653 100644 > --- a/hw/ppc/spapr_cpu_core.c > +++ b/hw/ppc/spapr_cpu_core.c > @@ -175,11 +175,18 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp) > static void spapr_cpu_core_realize_child(Object *child, Error **errp) > { > Error *local_err = NULL; > - sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > + sPAPRMachineState *spapr; > CPUState *cs = CPU(child); > PowerPCCPU *cpu = POWERPC_CPU(cs); > Object *obj; > > + spapr = (sPAPRMachineState *)object_dynamic_cast(qdev_get_machine(), > + TYPE_SPAPR_MACHINE); > + if (!spapr) { > + error_setg(errp, "spapr-cpu-core needs a pseries machine"); > + return; > + } > + > object_property_set_bool(child, true, "realized", &local_err); > if (local_err) { > goto error; > diff --git a/scripts/device-crash-test b/scripts/device-crash-test > index e77b693..8eb2d02 100755 > --- a/scripts/device-crash-test > +++ b/scripts/device-crash-test > @@ -200,6 +200,7 @@ ERROR_WHITELIST = [ > {'log':r"Multiple VT220 operator consoles are not supported"}, > {'log':r"core 0 already populated"}, > {'log':r"could not find stage1 bootloader"}, > + {'log':r"spapr-cpu-core needs a pseries machine"}, device/machine whitelist entries are preferred, if possible. This way, we can set expected=True and device-crash-test will avoid testing a device/machine combination known to be incompatible (when running in quick mode), or print a warning if it doesn't fail as expected (when running in full mode). I suggest the following (untested): # "spapr-cpu-core needs a pseries machine" {'machine':'(?!pseries.*)', 'device':'.*-spapr-cpu-core', 'expected':True},
On Sat, 12 Aug 2017 12:16:51 +0200 Greg Kurz <groug@kaod.org> wrote: > On Sat, 12 Aug 2017 10:38:10 +0200 > Thomas Huth <thuth@redhat.com> wrote: > > > QEMU currently crashes when the user tries to add a spapr-cpu-core > > on a non-pseries machine: > > > > $ qemu-system-ppc64 -S -machine ppce500,accel=tcg \ > > -device POWER5+_v2.1-spapr-cpu-core > > hw/ppc/spapr_cpu_core.c:178:spapr_cpu_core_realize_child: > > Object 0x55cee1f55160 is not an instance of type spapr-machine > > Aborted (core dumped) > > > > So let's add a proper check for the correct machine time with > > a more friendly error message here. > > > > Reported-by: Eduardo Habkost <ehabkost@redhat.com> > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > --- > > hw/ppc/spapr_cpu_core.c | 9 ++++++++- > > scripts/device-crash-test | 1 + > > 2 files changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > index ea278ce..0f3d653 100644 > > --- a/hw/ppc/spapr_cpu_core.c > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -175,11 +175,18 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp) > > static void spapr_cpu_core_realize_child(Object *child, Error **errp) > > { > > Error *local_err = NULL; > > - sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > > + sPAPRMachineState *spapr; > > CPUState *cs = CPU(child); > > PowerPCCPU *cpu = POWERPC_CPU(cs); > > Object *obj; > > > > + spapr = (sPAPRMachineState *)object_dynamic_cast(qdev_get_machine(), > > + TYPE_SPAPR_MACHINE); > > + if (!spapr) { > > + error_setg(errp, "spapr-cpu-core needs a pseries machine"); > > + return; > > + } > > + > > This is the realize function for individual threads. Maybe this sanity check > should be performed earlier at the core level in spapr_cpu_core_realize() ? > > Also, spapr_cpu_core_realize_child() seem to only have spapr to pass it down > to spapr_cpu_init()... ie, not even sure spapr_cpu_core_realize_child() > actually needs a spapr variable. > And spapr_cpu_init() doesn't even need the spapr machine object itself: it only needs the TYPE_PPC_VIRTUAL_HYPERVISOR object. I guess the right fix would be for spapr cpu core to have a "ppc-virtual-hypervisor property set in spapr_cpu_init() and tested in spapr_cpu_core_realize(). If the change is considered to be too invasive during hard freeze, I guess a sanity check in spapr_cpu_core_realize() is enough for now as a temporary bug fix. > > object_property_set_bool(child, true, "realized", &local_err); > > if (local_err) { > > goto error; > > diff --git a/scripts/device-crash-test b/scripts/device-crash-test > > index e77b693..8eb2d02 100755 > > --- a/scripts/device-crash-test > > +++ b/scripts/device-crash-test > > @@ -200,6 +200,7 @@ ERROR_WHITELIST = [ > > {'log':r"Multiple VT220 operator consoles are not supported"}, > > {'log':r"core 0 already populated"}, > > {'log':r"could not find stage1 bootloader"}, > > + {'log':r"spapr-cpu-core needs a pseries machine"}, > > > > # other exitcode=1 failures not listed above will just generate INFO messages: > > {'exitcode':1, 'loglevel':logging.INFO}, >
On Sat, Aug 12, 2017 at 12:16:51PM +0200, Greg Kurz wrote: > On Sat, 12 Aug 2017 10:38:10 +0200 > Thomas Huth <thuth@redhat.com> wrote: > > > QEMU currently crashes when the user tries to add a spapr-cpu-core > > on a non-pseries machine: > > > > $ qemu-system-ppc64 -S -machine ppce500,accel=tcg \ > > -device POWER5+_v2.1-spapr-cpu-core > > hw/ppc/spapr_cpu_core.c:178:spapr_cpu_core_realize_child: > > Object 0x55cee1f55160 is not an instance of type spapr-machine > > Aborted (core dumped) > > > > So let's add a proper check for the correct machine time with > > a more friendly error message here. > > > > Reported-by: Eduardo Habkost <ehabkost@redhat.com> > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > --- > > hw/ppc/spapr_cpu_core.c | 9 ++++++++- > > scripts/device-crash-test | 1 + > > 2 files changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > index ea278ce..0f3d653 100644 > > --- a/hw/ppc/spapr_cpu_core.c > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -175,11 +175,18 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp) > > static void spapr_cpu_core_realize_child(Object *child, Error **errp) > > { > > Error *local_err = NULL; > > - sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > > + sPAPRMachineState *spapr; > > CPUState *cs = CPU(child); > > PowerPCCPU *cpu = POWERPC_CPU(cs); > > Object *obj; > > > > + spapr = (sPAPRMachineState *)object_dynamic_cast(qdev_get_machine(), > > + TYPE_SPAPR_MACHINE); > > + if (!spapr) { > > + error_setg(errp, "spapr-cpu-core needs a pseries machine"); > > + return; > > + } > > + > > This is the realize function for individual threads. Maybe this sanity check > should be performed earlier at the core level in spapr_cpu_core_realize() ? Ah, yes, that would be a better way of doing it. I'm also not clear if you're proposing this for 2.10 or 2.11. > Also, spapr_cpu_core_realize_child() seem to only have spapr to pass it down > to spapr_cpu_init()... ie, not even sure spapr_cpu_core_realize_child() > actually needs a spapr variable. > > > object_property_set_bool(child, true, "realized", &local_err); > > if (local_err) { > > goto error; > > diff --git a/scripts/device-crash-test b/scripts/device-crash-test > > index e77b693..8eb2d02 100755 > > --- a/scripts/device-crash-test > > +++ b/scripts/device-crash-test > > @@ -200,6 +200,7 @@ ERROR_WHITELIST = [ > > {'log':r"Multiple VT220 operator consoles are not supported"}, > > {'log':r"core 0 already populated"}, > > {'log':r"could not find stage1 bootloader"}, > > + {'log':r"spapr-cpu-core needs a pseries machine"}, > > > > # other exitcode=1 failures not listed above will just generate INFO messages: > > {'exitcode':1, 'loglevel':logging.INFO}, >
On 14.08.2017 05:24, David Gibson wrote: > On Sat, Aug 12, 2017 at 12:16:51PM +0200, Greg Kurz wrote: >> On Sat, 12 Aug 2017 10:38:10 +0200 >> Thomas Huth <thuth@redhat.com> wrote: >> >>> QEMU currently crashes when the user tries to add a spapr-cpu-core >>> on a non-pseries machine: >>> >>> $ qemu-system-ppc64 -S -machine ppce500,accel=tcg \ >>> -device POWER5+_v2.1-spapr-cpu-core >>> hw/ppc/spapr_cpu_core.c:178:spapr_cpu_core_realize_child: >>> Object 0x55cee1f55160 is not an instance of type spapr-machine >>> Aborted (core dumped) >>> >>> So let's add a proper check for the correct machine time with >>> a more friendly error message here. >>> >>> Reported-by: Eduardo Habkost <ehabkost@redhat.com> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> hw/ppc/spapr_cpu_core.c | 9 ++++++++- >>> scripts/device-crash-test | 1 + >>> 2 files changed, 9 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c >>> index ea278ce..0f3d653 100644 >>> --- a/hw/ppc/spapr_cpu_core.c >>> +++ b/hw/ppc/spapr_cpu_core.c >>> @@ -175,11 +175,18 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp) >>> static void spapr_cpu_core_realize_child(Object *child, Error **errp) >>> { >>> Error *local_err = NULL; >>> - sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); >>> + sPAPRMachineState *spapr; >>> CPUState *cs = CPU(child); >>> PowerPCCPU *cpu = POWERPC_CPU(cs); >>> Object *obj; >>> >>> + spapr = (sPAPRMachineState *)object_dynamic_cast(qdev_get_machine(), >>> + TYPE_SPAPR_MACHINE); >>> + if (!spapr) { >>> + error_setg(errp, "spapr-cpu-core needs a pseries machine"); >>> + return; >>> + } >>> + >> >> This is the realize function for individual threads. Maybe this sanity check >> should be performed earlier at the core level in spapr_cpu_core_realize() ? > > Ah, yes, that would be a better way of doing it. > > I'm also not clear if you're proposing this for 2.10 or 2.11. I was not sure, either ;-) Anyway, it's not a bug that blocks normal usage of QEMU, and apparently there's a better but more extensive way to fix this, so let's postpone this to 2.11. Thomas
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c index ea278ce..0f3d653 100644 --- a/hw/ppc/spapr_cpu_core.c +++ b/hw/ppc/spapr_cpu_core.c @@ -175,11 +175,18 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp) static void spapr_cpu_core_realize_child(Object *child, Error **errp) { Error *local_err = NULL; - sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); + sPAPRMachineState *spapr; CPUState *cs = CPU(child); PowerPCCPU *cpu = POWERPC_CPU(cs); Object *obj; + spapr = (sPAPRMachineState *)object_dynamic_cast(qdev_get_machine(), + TYPE_SPAPR_MACHINE); + if (!spapr) { + error_setg(errp, "spapr-cpu-core needs a pseries machine"); + return; + } + object_property_set_bool(child, true, "realized", &local_err); if (local_err) { goto error; diff --git a/scripts/device-crash-test b/scripts/device-crash-test index e77b693..8eb2d02 100755 --- a/scripts/device-crash-test +++ b/scripts/device-crash-test @@ -200,6 +200,7 @@ ERROR_WHITELIST = [ {'log':r"Multiple VT220 operator consoles are not supported"}, {'log':r"core 0 already populated"}, {'log':r"could not find stage1 bootloader"}, + {'log':r"spapr-cpu-core needs a pseries machine"}, # other exitcode=1 failures not listed above will just generate INFO messages: {'exitcode':1, 'loglevel':logging.INFO},
QEMU currently crashes when the user tries to add a spapr-cpu-core on a non-pseries machine: $ qemu-system-ppc64 -S -machine ppce500,accel=tcg \ -device POWER5+_v2.1-spapr-cpu-core hw/ppc/spapr_cpu_core.c:178:spapr_cpu_core_realize_child: Object 0x55cee1f55160 is not an instance of type spapr-machine Aborted (core dumped) So let's add a proper check for the correct machine time with a more friendly error message here. Reported-by: Eduardo Habkost <ehabkost@redhat.com> Signed-off-by: Thomas Huth <thuth@redhat.com> --- hw/ppc/spapr_cpu_core.c | 9 ++++++++- scripts/device-crash-test | 1 + 2 files changed, 9 insertions(+), 1 deletion(-)