From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:50233) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gzfQu-0000ud-08 for qemu-devel@nongnu.org; Fri, 01 Mar 2019 05:30:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gzfQs-0000Fb-FT for qemu-devel@nongnu.org; Fri, 01 Mar 2019 05:30:23 -0500 References: <20190226045304.25618-1-david@gibson.dropbear.id.au> <20190226045304.25618-41-david@gibson.dropbear.id.au> <155140389645.9499.14782774845761701651@sif> From: David Hildenbrand Message-ID: <4038f585-feaa-2fca-cfb7-ba28416748bf@redhat.com> Date: Fri, 1 Mar 2019 11:30:18 +0100 MIME-Version: 1.0 In-Reply-To: <155140389645.9499.14782774845761701651@sif> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL 40/50] spapr_events: add support for phb hotplug events List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth , David Gibson , Thomas Huth Cc: lvivier@redhat.com, gkurz@kaod.org, qemu-devel@nongnu.org, Greg Kurz , qemu-ppc@nongnu.org, clg@kaod.org 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 >>> >>> Extend the existing EPOW event format we use for PCI >>> devices to emit PHB plug/unplug events. >>> >>> Signed-off-by: Michael Roth >>> Reviewed-by: David Gibson >>> Signed-off-by: Greg Kurz >>> Message-Id: <155059671405.1466090.535964535260503283.stgit@bahia.lab.toulouse-stg.fr.ibm.com> >>> Signed-off-by: David Gibson >>> --- >>> 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 > 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? -- Thanks, David / dhildenb