On 01/03/2019 11.48, Greg Kurz wrote: > 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. >
Could this work: diff --git a/hw/ppc/spapr_ovec.c b/hw/ppc/spapr_ovec.c --- a/hw/ppc/spapr_ovec.c +++ b/hw/ppc/spapr_ovec.c @@ -12,6 +12,7 @@ */ #include "qemu/osdep.h" +#include "hw/ppc/spapr.h" #include "hw/ppc/spapr_ovec.h" #include "qemu/bitmap.h" #include "exec/address-spaces.h" @@ -134,7 +135,9 @@ bool spapr_ovec_test(sPAPROptionVector *ov, long bitnr) /* support memory unplug for qtest */ if (qtest_enabled() && bitnr == OV5_HP_EVT) { - return true; + sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); + + return spapr->use_hotplug_event_source; } return test_bit(bitnr, ov->bitmap) ? true : false; ? Thomas