On Fri, 1 Mar 2019 11:30:18 +0100
David Hildenbrand <da...@redhat.com> wrote:
> On 01.03.19 02:31, Michael Roth wrote:
> > Quoting Thomas Huth (2019-02-28 12:40:52)
> >> On 26/02/2019 05.52, David Gibson wrote:
> >>> From: Michael Roth <mdr...@linux.vnet.ibm.com>
> >>>
> >>> Extend the existing EPOW event format we use for PCI
> >>> devices to emit PHB plug/unplug events.
> >>>
> >>> Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com>
> >>> Reviewed-by: David Gibson <da...@gibson.dropbear.id.au>
> >>> Signed-off-by: Greg Kurz <gr...@kaod.org>
> >>> Message-Id:
> >>> <155059671405.1466090.535964535260503283.st...@bahia.lab.toulouse-stg.fr.ibm.com>
> >>> Signed-off-by: David Gibson <da...@gibson.dropbear.id.au>
> >>> ---
> >>> hw/ppc/spapr_events.c | 3 +++
> >>> 1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> >>> index b9c7ecb9e9..ab9a1f0063 100644
> >>> --- a/hw/ppc/spapr_events.c
> >>> +++ b/hw/ppc/spapr_events.c
> >>> @@ -526,6 +526,9 @@ static void spapr_hotplug_req_event(uint8_t hp_id,
> >>> uint8_t hp_action,
> >>> case SPAPR_DR_CONNECTOR_TYPE_CPU:
> >>> hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_CPU;
> >>> break;
> >>> + case SPAPR_DR_CONNECTOR_TYPE_PHB:
> >>> + hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_PHB;
> >>> + break;
> >>> default:
> >>> /* we shouldn't be signaling hotplug events for resources
> >>> * that don't support them
> >>
> >> I think this patch (or something else in this PULL request) broke CPU
> >> hot-plugging with older machine types:
> >>
> >> $ QTEST_QEMU_BINARY=ppc64-softmmu/qemu-system-ppc64 ./tests/cpu-plug-test
> >> -m=slow
> >> /ppc64/cpu-plug/pseries-3.1/device-add/2x3x1&maxcpus=6: OK
> >> /ppc64/cpu-plug/pseries-2.12-sxxm/device-add/2x3x1&maxcpus=6: OK
> >> /ppc64/cpu-plug/pseries-3.0/device-add/2x3x1&maxcpus=6: OK
> >> /ppc64/cpu-plug/pseries-2.10/device-add/2x3x1&maxcpus=6: OK
> >> /ppc64/cpu-plug/pseries-2.11/device-add/2x3x1&maxcpus=6: OK
> >> /ppc64/cpu-plug/pseries-2.12/device-add/2x3x1&maxcpus=6: OK
> >> /ppc64/cpu-plug/pseries-2.9/device-add/2x3x1&maxcpus=6: OK
> >> /ppc64/cpu-plug/pseries-2.7/device-add/2x3x1&maxcpus=6: **
> >> ERROR:/home/thuth/devel/qemu/hw/ppc/spapr_events.c:313:rtas_event_log_to_source:
> >> assertion failed: (source->enabled)
> >> Broken pipe
> >> /home/thuth/devel/qemu/tests/libqtest.c:143: kill_qemu() detected QEMU
> >> death from signal 6 (Aborted) (core dumped)
> >> Aborted (core dumped)
> >>
> >> Could you please have a look?
> >
> > Bisected to:
> >
> > commit b8165118f52ce5ee88565d3cec83d30374efdc96
> > Author: David Hildenbrand <da...@redhat.com>
> > Date: Mon Feb 18 10:21:58 2019 +0100
> >
> > spapr: support memory unplug for qtest
> >
> > Fake availability of OV5_HP_EVT, so we can test memory unplug in
> > qtest.
> >
> > Which makes sense since OV5_HP_EVT assumes that
> > spapr->spapr->use_hotplug_event_source == true, which isn't the default for
> > 2.7 and below.
> >
> > If I revert that I think I hit the bug it was meant to fix:
> >
> > mdroth@sif:~/w/qemu-build3$ make V=1 check-qtest-ppc64
> > ...
> > PASS 1 device-plug-test /ppc64/device-plug/pci-unplug-request
> > PASS 2 device-plug-test /ppc64/device-plug/spapr-cpu-unplug-request
> > **
> >
> > ERROR:/home/mdroth/w/qemu3.git/tests/device-plug-test.c:28:device_del_finish:
> > assertion failed: (qdict_haskey(resp, "return"))
> > ERROR - Bail out!
> > ERROR:/home/mdroth/w/qemu3.git/tests/device-plug-test.c:28:device_del_finish:
> > assertion failed: (qdict_haskey(resp, "return"))
> > Aborted (core dumped)
> > /home/mdroth/w/qemu3.git/tests/Makefile.include:875: recipe for target
> > 'check-qtest-ppc64' failed
> > make: *** [check-qtest-ppc64] Error 1
> > mdroth@sif:~/w/qemu-build3$
> >
> > Which is probably due to this check in
> > spapr_machine_device_unplug_request():
> >
> > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > if (spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT)) {
> > spapr_memory_unplug_request(hotplug_dev, dev, errp);
> > } else {
> > /* NOTE: this means there is a window after guest reset, prior
> > to
> > * CAS negotiation, where unplug requests will fail due to the
> > * capability not being detected yet. This is a bit different
> > than
> > * the case with PCI unplug, where the events will be queued and
> > * eventually handled by the guest after boot
> > */
> > error_setg(errp, "Memory hot unplug not supported for this
> > guest");
> > }
> >
> >
> >
> > This spapr-cpu-unplug-request test is failing because
> > spapr_ovec_test(sms->ov5_cas, OV5_HP_EVT) relies on the CAS-negotiated OV5
> > bit,
> > which wouldn't have happened with qtest. If we want to make these tests run
> > in
> > this scenario we probably need a different approach than the original
> > patch.
>
> We could rip out the patch along with the spapr memory unplug test.
> However it feels like a step back to not have any memory unplug tests
> for QEMU at all.
>
> Any spapr experts here if we can work around this?
>
Not sure about the expertise :) but I'm currently looking into it. As
you say, it would be unfortunate to drop a test because of that.
