Re: [Qemu-ppc] [Qemu-devel] [PULL 40/50] spapr_events: add support for phb hotplug events

[Michael Roth]

Quoting Thomas Huth (2019-02-28 12:40:52)
> On 26/02/2019 05.52, David Gibson wrote:
> > From: Michael Roth <address@hidden>
> > 
> > Extend the existing EPOW event format we use for PCI
> > devices to emit PHB plug/unplug events.
> > 
> > Signed-off-by: Michael Roth <address@hidden>
> > Reviewed-by: David Gibson <address@hidden>
> > Signed-off-by: Greg Kurz <address@hidden>
> > Message-Id: <address@hidden>
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  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 <address@hidden>
  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:

  address@hidden:~/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
  address@hidden:~/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.

> 
>  Thomas
>