Re: [PATCH for-6.2 v6 6/7] spapr: use DEVICE_UNPLUG_ERROR to report unplug errors

[Markus Armbruster]

Daniel Henrique Barboza <danielhb413@gmail.com> writes:

> Linux Kernel 5.12 is now unisolating CPU DRCs in the device_removal
> error path, signalling that the hotunplug process wasn't successful.
> This allow us to send a DEVICE_UNPLUG_ERROR in drc_unisolate_logical()
> to signal this error to the management layer.
>
> We also have another error path in spapr_memory_unplug_rollback() for
> configured LMB DRCs. Kernels older than 5.13 will not unisolate the LMBs
> in the hotunplug error path, but it will reconfigure them. Let's send
> the DEVICE_UNPLUG_ERROR event in that code path as well to cover the
> case of older kernels.
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hw/ppc/spapr.c     |  9 ++++++++-
>  hw/ppc/spapr_drc.c | 18 ++++++++++++------
>  2 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 1611d7ab05..5459f9a7e9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -29,6 +29,7 @@
>  #include "qemu/datadir.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-events-machine.h"
> +#include "qapi/qapi-events-qdev.h"
>  #include "qapi/visitor.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/hostmem.h"
> @@ -3686,13 +3687,19 @@ void spapr_memory_unplug_rollback(SpaprMachineState 
> *spapr, DeviceState *dev)
>  
>      /*
>       * Tell QAPI that something happened and the memory
> -     * hotunplug wasn't successful.
> +     * hotunplug wasn't successful. Keep sending
> +     * MEM_UNPLUG_ERROR even while sending DEVICE_UNPLUG_ERROR
> +     * until the deprecation MEM_UNPLUG_ERROR is due.
>       */
>      if (dev->id) {
>          qapi_error = g_strdup_printf("Memory hotunplug rejected by the guest 
> "
>                                       "for device %s", dev->id);
>          qapi_event_send_mem_unplug_error(dev->id, qapi_error);
>      }
> +
> +    qapi_event_send_device_unplug_error(!!dev->id, dev->id,
> +                                        dev->canonical_path,
> +                                        qapi_error != NULL, qapi_error);
>  }
>  

When dev->id is null, we send something like

    {"event": "DEVICE_UNPLUG_ERROR",
     "data": {"path": "/machine/..."},
     "timestamp": ...}

Unless I'm missing something, this is all the information the management
application really needs.

When dev->id is non-null, we add to "data":

              "device": "dev123",
              "msg": "Memory hotunplug rejected by the guest for device dev123",

I'm fine with emitting the device ID when we have it.

What's the intended use of "msg"?

Could DEVICE_UNPLUG_ERROR ever be emitted for this device with a
different "msg"?

If "msg" is useful when dev->id is non-null, then it's likely useful
when dev->id is null.  Why not

              "msg": "Memory hotunplug rejected by the guest",

always?

If we do that here, we'll likely do it everywhere, and then member @msg
isn't actually optional.

>  /* Callback to be called during DRC release. */
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index a4d9496f76..8f0479631f 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -17,6 +17,8 @@
>  #include "hw/ppc/spapr_drc.h"
>  #include "qom/object.h"
>  #include "migration/vmstate.h"
> +#include "qapi/error.h"
> +#include "qapi/qapi-events-qdev.h"
>  #include "qapi/visitor.h"
>  #include "qemu/error-report.h"
>  #include "hw/ppc/spapr.h" /* for RTAS return codes */
> @@ -160,6 +162,11 @@ static uint32_t drc_unisolate_logical(SpaprDrc *drc)
>           * means that the kernel is refusing the removal.
>           */
>          if (drc->unplug_requested && drc->dev) {
> +            const char qapi_error_fmt[] = \

Drop the superfluous \

> +"Device hotunplug rejected by the guest for device %s";

Unusual indentation.

> +
> +            g_autofree char *qapi_error = NULL;
> +
>              if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
>                  spapr = SPAPR_MACHINE(qdev_get_machine());
>  
> @@ -169,14 +176,13 @@ static uint32_t drc_unisolate_logical(SpaprDrc *drc)
>              drc->unplug_requested = false;
>  
>              if (drc->dev->id) {
> -                error_report("Device hotunplug rejected by the guest "
> -                             "for device %s", drc->dev->id);
> +                qapi_error = g_strdup_printf(qapi_error_fmt, drc->dev->id);
> +                error_report(qapi_error_fmt, drc->dev->id);

Simpler:

                   qapi_error = ...
                   error_report("%s", qapi_error);

Matter of taste.  Maintainer decides.

>              }
>  
> -            /*
> -             * TODO: send a QAPI DEVICE_UNPLUG_ERROR event when
> -             * it is implemented.
> -             */
> +            qapi_event_send_device_unplug_error(!!drc->dev->id, drc->dev->id,
> +                                                drc->dev->canonical_path,
> +                                                qapi_error != NULL, 
> qapi_error);

My questions on "msg" apply.

>          }
>  
>          return RTAS_OUT_SUCCESS; /* Nothing to do */