[PATCH for-6.2 v6 0/7] DEVICE_UNPLUG_ERROR QAPI event

[PATCH for-6.2 v6 0/7] DEVICE_UNPLUG_ERROR QAPI event

[Daniel Henrique Barboza]

Hi,

This version was rebased with QEMU master @ 7457b407edd6e8555e4b4 and
has a few simple changes based on Greg's review.

changes from v5:
- patch 1:
  * fixed function name in commit msg
-patch 6:
  * kept error message in a single line
- all patches:
  * added Greg's R-b
- v5 link: https://lists.gnu.org/archive/html/qemu-devel/2021-07/msg03239.html

changes from v4:
- patch 1 (new):
  * avoid emitting MEM_UNPLUG_ERROR when dev->id == NULL in
memory_hotplug.c
- patch 2 (new):
  * avoid emitting MEM_UNPLUG_ERROR when dev->id == NULL in
spapr.c
- patch 3 (new):
  * do not error_report() when drc->dev->id == NULL
- patch 4 (new):
  * update DEVICE_DELETED API docs
- patch 5 (former 1):
  * added 2 spaces after each sentence
  * appended "Some errors cannot be detected." in DEVICE_UNPLUG_ERROR
docs
  * changed release from 6.1 to 6.2 in docs
  * changed DEVICE_UNPLUG_ERROR API to behave more like device_deleted
- patches 6 and 7:
  * changed to use the new DEVICE_UNPLUG_ERROR API
- v4 link: https://lists.gnu.org/archive/html/qemu-devel/2021-07/msg01355.html

changes from v3:
- patch 1:
  * fixed format
- all patches:
  * rebased with master
  * added David's R-b
- v3 link: https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg05842.html

changes from v2:
- patch 1:
  * moved DEVICE_UNPLUG_ERROR declaration to qapi/qdev.json
  * updated 'device_del' description
  * added 'deprecated' notice on MEM_UNPLUG_ERROR
  * added MEM_UNPLUG_ERROR 'deprecated' info in docs/system/deprecated.rst
- patch 2:
  * send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
- patch 3 (new):
  * send DEVICE_UNPLUG_ERROR in acpi/memory_hotplug.c
- v2 link: https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg01304.html

changes from v1:
- former patches 1 and 2: dropped
- patch 1 (former 3): changed the version to '6.1'
- patch 2 (former 4): add a DEVICE_UNPLUG_ERROR event in the device
  unplug error path of CPUs and DIMMs
- v1 link: https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg04682.html


Daniel Henrique Barboza (7):
  hw/acpi/memory_hotplug.c: avoid sending MEM_UNPLUG_ERROR if dev->id is
    NULL
  spapr.c: avoid sending MEM_UNPLUG_ERROR if dev->id is NULL
  spapr_drc.c: do not error_report() when drc->dev->id == NULL
  qapi/qdev.json: fix DEVICE_DELETED parameters doc
  qapi/qdev.json: add DEVICE_UNPLUG_ERROR QAPI event
  spapr: use DEVICE_UNPLUG_ERROR to report unplug errors
  memory_hotplug.c: send DEVICE_UNPLUG_ERROR in
    acpi_memory_hotplug_write()

 docs/about/deprecated.rst | 10 ++++++++++
 hw/acpi/memory_hotplug.c  | 19 +++++++++++++++++--
 hw/ppc/spapr.c            | 17 +++++++++++++----
 hw/ppc/spapr_drc.c        | 21 +++++++++++++++------
 qapi/machine.json         |  6 +++++-
 qapi/qdev.json            | 34 +++++++++++++++++++++++++++++++---
 stubs/qdev.c              |  7 +++++++
 7 files changed, 98 insertions(+), 16 deletions(-)

-- 
2.31.1

[PATCH for-6.2 v6 1/7] hw/acpi/memory_hotplug.c: avoid sending MEM_UNPLUG_ERROR if dev->id is NULL

[Daniel Henrique Barboza]

qapi_event_send_mem_unplug_error() deals with @device being NULL by
replacing it with an empty string ("") when emitting the event. Aside
from the fact that this is a side effect that can be patched someday,
there's also the lack of utility that the event brings to listeners,
e.g.  "a memory unplug error happened somewhere".

We're better of not emitting the event if dev->id is NULL. Next patches
will introduce a new device unplug error event that is better suited to
deal with dev->id NULL scenarios. MEM_UNPLUG_ERROR will continue to be
emitted to avoid breaking existing APIs, but it'll be deprecated and
removed in the future.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/acpi/memory_hotplug.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index af37889423..e37acb0367 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -177,9 +177,14 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
             /* call pc-dimm unplug cb */
             hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
             if (local_err) {
+                const char *error_pretty = error_get_pretty(local_err);
+
                 trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
-                qapi_event_send_mem_unplug_error(dev->id,
-                                                 error_get_pretty(local_err));
+
+                if (dev->id) {
+                    qapi_event_send_mem_unplug_error(dev->id, error_pretty);
+                }
+
                 error_free(local_err);
                 break;
             }
-- 
2.31.1

[PATCH for-6.2 v6 2/7] spapr.c: avoid sending MEM_UNPLUG_ERROR if dev->id is NULL

[Daniel Henrique Barboza]

As done in hw/acpi/memory_hotplug.c, avoid sending
qapi_event_send_mem_unplug_error() if dev->id is NULL.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 81699d4f8b..1611d7ab05 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3688,9 +3688,11 @@ void spapr_memory_unplug_rollback(SpaprMachineState *spapr, DeviceState *dev)
      * Tell QAPI that something happened and the memory
      * hotunplug wasn't successful.
      */
-    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);
+    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);
+    }
 }
 
 /* Callback to be called during DRC release. */
-- 
2.31.1

[PATCH for-6.2 v6 3/7] spapr_drc.c: do not error_report() when drc->dev->id == NULL

[Daniel Henrique Barboza]

The error_report() call in drc_unisolate_logical() is not considering
that drc->dev->id can be NULL, and the underlying functions error_report()
calls to do its job (vprintf(), g_strdup_printf() ...) has undefined
behavior when trying to handle "%s" with NULL arguments.

Besides, there is no utility into reporting that an unknown device was
rejected by the guest.

Reviewed-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr_drc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index a2f2634601..a4d9496f76 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -167,8 +167,11 @@ static uint32_t drc_unisolate_logical(SpaprDrc *drc)
             }
 
             drc->unplug_requested = false;
-            error_report("Device hotunplug rejected by the guest "
-                         "for device %s", drc->dev->id);
+
+            if (drc->dev->id) {
+                error_report("Device hotunplug rejected by the guest "
+                             "for device %s", drc->dev->id);
+            }
 
             /*
              * TODO: send a QAPI DEVICE_UNPLUG_ERROR event when
-- 
2.31.1

[PATCH for-6.2 v6 4/7] qapi/qdev.json: fix DEVICE_DELETED parameters doc

[Daniel Henrique Barboza]

Clarify that @device is optional and that 'path' is the device
path from QOM.

This change follows Markus' suggestion verbatim, provided in full
context here:

https://lists.gnu.org/archive/html/qemu-devel/2021-07/msg01891.html

Suggested-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 qapi/qdev.json | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qapi/qdev.json b/qapi/qdev.json
index b83178220b..d1d3681a50 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -108,9 +108,9 @@
 # At this point, it's safe to reuse the specified device ID. Device removal can
 # be initiated by the guest or by HMP/QMP commands.
 #
-# @device: device name
+# @device: the device's ID if it has one
 #
-# @path: device path
+# @path: the device's path within the object model
 #
 # Since: 1.5
 #
-- 
2.31.1

[PATCH for-6.2 v6 5/7] qapi/qdev.json: add DEVICE_UNPLUG_ERROR QAPI event

[Daniel Henrique Barboza]

At this moment we only provide one event to report a hotunplug error,
MEM_UNPLUG_ERROR. As of Linux kernel 5.12 and QEMU 6.0.0, the pseries
machine is now able to report unplug errors for other device types, such
as CPUs.

Instead of creating a (device_type)_UNPLUG_ERROR for each new device,
create a generic DEVICE_UNPLUG_ERROR event that can be used by all
unplug errors in the future. This event has a similar API as the
existing DEVICE_DELETED event, with an extra optional 'msg' parameter
that can be used to explain the reason for the error.

With this new generic event, MEM_UNPLUG_ERROR is now marked as deprecated.

Reviewed-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 docs/about/deprecated.rst | 10 ++++++++++
 qapi/machine.json         |  6 +++++-
 qapi/qdev.json            | 30 +++++++++++++++++++++++++++++-
 stubs/qdev.c              |  7 +++++++
 4 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 6d438f1c8d..c0c3431ada 100644
--- a/docs/about/deprecated.rst
+++ b/docs/about/deprecated.rst
@@ -204,6 +204,16 @@ The ``I7200`` guest CPU relies on the nanoMIPS ISA, which is deprecated
 (the ISA has never been upstreamed to a compiler toolchain). Therefore
 this CPU is also deprecated.
 
+
+QEMU API (QAPI) events
+----------------------
+
+``MEM_UNPLUG_ERROR`` (since 6.2)
+''''''''''''''''''''''''''''''''''''''''''''''''''''''''
+
+Use the more generic event ``DEVICE_UNPLUG_ERROR`` instead.
+
+
 System emulator machines
 ------------------------
 
diff --git a/qapi/machine.json b/qapi/machine.json
index c3210ee1fb..a595c753d2 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1271,6 +1271,9 @@
 #
 # @msg: Informative message
 #
+# Features:
+# @deprecated: This event is deprecated. Use @DEVICE_UNPLUG_ERROR instead.
+#
 # Since: 2.4
 #
 # Example:
@@ -1283,7 +1286,8 @@
 #
 ##
 { 'event': 'MEM_UNPLUG_ERROR',
-  'data': { 'device': 'str', 'msg': 'str' } }
+  'data': { 'device': 'str', 'msg': 'str' },
+  'features': ['deprecated'] }
 
 ##
 # @SMPConfiguration:
diff --git a/qapi/qdev.json b/qapi/qdev.json
index d1d3681a50..52c36c7b9c 100644
--- a/qapi/qdev.json
+++ b/qapi/qdev.json
@@ -84,7 +84,9 @@
 #        This command merely requests that the guest begin the hot removal
 #        process.  Completion of the device removal process is signaled with a
 #        DEVICE_DELETED event. Guest reset will automatically complete removal
-#        for all devices.
+#        for all devices.  If an error in the hot removal process is detected,
+#        the device will not be removed and a DEVICE_UNPLUG_ERROR event is
+#        sent.  Some errors cannot be detected.
 #
 # Since: 0.14
 #
@@ -124,3 +126,29 @@
 ##
 { 'event': 'DEVICE_DELETED',
   'data': { '*device': 'str', 'path': 'str' } }
+
+##
+# @DEVICE_UNPLUG_ERROR:
+#
+# Emitted when a device hot unplug error occurs.
+#
+# @device: the device's ID if it has one
+#
+# @path: the device's path within the object model
+#
+# @msg: optional informative message
+#
+# Since: 6.2
+#
+# Example:
+#
+# <- { "event": "DEVICE_UNPLUG_ERROR"
+#      "data": { "device": "core1",
+#                "msg": "Device hotunplug rejected by the guest for device core1",
+#                "path": "/machine/peripheral/core1" },
+#      },
+#      "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
+#
+##
+{ 'event': 'DEVICE_UNPLUG_ERROR',
+  'data': { '*device': 'str', 'path': 'str' , '*msg': 'str' } }
diff --git a/stubs/qdev.c b/stubs/qdev.c
index 92e6143134..ffa8f7b59e 100644
--- a/stubs/qdev.c
+++ b/stubs/qdev.c
@@ -21,3 +21,10 @@ void qapi_event_send_device_deleted(bool has_device,
 {
     /* Nothing to do. */
 }
+
+void qapi_event_send_device_unplug_error(bool has_device, const char *device,
+                                         const char *path,
+                                         bool has_msg, const char *msg)
+{
+    /* Nothing to do. */
+}
-- 
2.31.1

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

[Daniel Henrique Barboza]

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);
 }
 
 /* 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[] = \
+"Device hotunplug rejected by the guest for device %s";
+
+            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);
             }
 
-            /*
-             * 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);
         }
 
         return RTAS_OUT_SUCCESS; /* Nothing to do */
-- 
2.31.1

[PATCH for-6.2 v6 7/7] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write()

[Daniel Henrique Barboza]

MEM_UNPLUG_ERROR is deprecated since the introduction of
DEVICE_UNPLUG_ERROR. Keep emitting both while the deprecation of
MEM_UNPLUG_ERROR is pending.

CC: Michael S. Tsirkin <mst@redhat.com>
CC: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/acpi/memory_hotplug.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index e37acb0367..a0772fe083 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -8,6 +8,7 @@
 #include "qapi/error.h"
 #include "qapi/qapi-events-acpi.h"
 #include "qapi/qapi-events-machine.h"
+#include "qapi/qapi-events-qdev.h"
 
 #define MEMORY_SLOTS_NUMBER          "MDNR"
 #define MEMORY_HOTPLUG_IO_REGION     "HPMR"
@@ -181,10 +182,19 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
 
                 trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
 
+                /*
+                 * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
+                 * while the deprecation of MEM_UNPLUG_ERROR is
+                 * pending.
+                 */
                 if (dev->id) {
                     qapi_event_send_mem_unplug_error(dev->id, error_pretty);
                 }
 
+                qapi_event_send_device_unplug_error(!!dev->id, dev->id,
+                                                    dev->canonical_path,
+                                                    true, error_pretty);
+
                 error_free(local_err);
                 break;
             }
-- 
2.31.1

Re: [PATCH for-6.2 v6 7/7] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write()

[Michael S. Tsirkin]

On Mon, Jul 19, 2021 at 05:08:27PM -0300, Daniel Henrique Barboza wrote:
> MEM_UNPLUG_ERROR is deprecated since the introduction of
> DEVICE_UNPLUG_ERROR. Keep emitting both while the deprecation of
> MEM_UNPLUG_ERROR is pending.
> 
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

Pls merge with rest of series.

> ---
>  hw/acpi/memory_hotplug.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index e37acb0367..a0772fe083 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -8,6 +8,7 @@
>  #include "qapi/error.h"
>  #include "qapi/qapi-events-acpi.h"
>  #include "qapi/qapi-events-machine.h"
> +#include "qapi/qapi-events-qdev.h"
>  
>  #define MEMORY_SLOTS_NUMBER          "MDNR"
>  #define MEMORY_HOTPLUG_IO_REGION     "HPMR"
> @@ -181,10 +182,19 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>  
>                  trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
>  
> +                /*
> +                 * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
> +                 * while the deprecation of MEM_UNPLUG_ERROR is
> +                 * pending.
> +                 */
>                  if (dev->id) {
>                      qapi_event_send_mem_unplug_error(dev->id, error_pretty);
>                  }
>  
> +                qapi_event_send_device_unplug_error(!!dev->id, dev->id,
> +                                                    dev->canonical_path,
> +                                                    true, error_pretty);
> +
>                  error_free(local_err);
>                  break;
>              }
> -- 
> 2.31.1

Re: [PATCH for-6.2 v6 7/7] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]

On Mon, Jul 19, 2021 at 05:13:44PM -0400, Michael S. Tsirkin wrote:
> On Mon, Jul 19, 2021 at 05:08:27PM -0300, Daniel Henrique Barboza wrote:
> > MEM_UNPLUG_ERROR is deprecated since the introduction of
> > DEVICE_UNPLUG_ERROR. Keep emitting both while the deprecation of
> > MEM_UNPLUG_ERROR is pending.
> > 
> > CC: Michael S. Tsirkin <mst@redhat.com>
> > CC: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>

Michael, can I assume I have your ack for 1/7 as well?
 
> Pls merge with rest of series.
> 
> > ---
> >  hw/acpi/memory_hotplug.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> > index e37acb0367..a0772fe083 100644
> > --- a/hw/acpi/memory_hotplug.c
> > +++ b/hw/acpi/memory_hotplug.c
> > @@ -8,6 +8,7 @@
> >  #include "qapi/error.h"
> >  #include "qapi/qapi-events-acpi.h"
> >  #include "qapi/qapi-events-machine.h"
> > +#include "qapi/qapi-events-qdev.h"
> >  
> >  #define MEMORY_SLOTS_NUMBER          "MDNR"
> >  #define MEMORY_HOTPLUG_IO_REGION     "HPMR"
> > @@ -181,10 +182,19 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
> >  
> >                  trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
> >  
> > +                /*
> > +                 * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
> > +                 * while the deprecation of MEM_UNPLUG_ERROR is
> > +                 * pending.
> > +                 */
> >                  if (dev->id) {
> >                      qapi_event_send_mem_unplug_error(dev->id, error_pretty);
> >                  }
> >  
> > +                qapi_event_send_device_unplug_error(!!dev->id, dev->id,
> > +                                                    dev->canonical_path,
> > +                                                    true, error_pretty);
> > +
> >                  error_free(local_err);
> >                  break;
> >              }
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH for-6.2 v6 0/7] DEVICE_UNPLUG_ERROR QAPI event

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3451 bytes --]

On Mon, Jul 19, 2021 at 05:08:20PM -0300, Daniel Henrique Barboza wrote:
> Hi,
> 
> This version was rebased with QEMU master @ 7457b407edd6e8555e4b4 and
> has a few simple changes based on Greg's review.

Tentatively applied to ppc-for-6.2.
> 
> changes from v5:
> - patch 1:
>   * fixed function name in commit msg
> -patch 6:
>   * kept error message in a single line
> - all patches:
>   * added Greg's R-b
> - v5 link: https://lists.gnu.org/archive/html/qemu-devel/2021-07/msg03239.html
> 
> changes from v4:
> - patch 1 (new):
>   * avoid emitting MEM_UNPLUG_ERROR when dev->id == NULL in
> memory_hotplug.c
> - patch 2 (new):
>   * avoid emitting MEM_UNPLUG_ERROR when dev->id == NULL in
> spapr.c
> - patch 3 (new):
>   * do not error_report() when drc->dev->id == NULL
> - patch 4 (new):
>   * update DEVICE_DELETED API docs
> - patch 5 (former 1):
>   * added 2 spaces after each sentence
>   * appended "Some errors cannot be detected." in DEVICE_UNPLUG_ERROR
> docs
>   * changed release from 6.1 to 6.2 in docs
>   * changed DEVICE_UNPLUG_ERROR API to behave more like device_deleted
> - patches 6 and 7:
>   * changed to use the new DEVICE_UNPLUG_ERROR API
> - v4 link: https://lists.gnu.org/archive/html/qemu-devel/2021-07/msg01355.html
> 
> changes from v3:
> - patch 1:
>   * fixed format
> - all patches:
>   * rebased with master
>   * added David's R-b
> - v3 link: https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg05842.html
> 
> changes from v2:
> - patch 1:
>   * moved DEVICE_UNPLUG_ERROR declaration to qapi/qdev.json
>   * updated 'device_del' description
>   * added 'deprecated' notice on MEM_UNPLUG_ERROR
>   * added MEM_UNPLUG_ERROR 'deprecated' info in docs/system/deprecated.rst
> - patch 2:
>   * send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
> - patch 3 (new):
>   * send DEVICE_UNPLUG_ERROR in acpi/memory_hotplug.c
> - v2 link: https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg01304.html
> 
> changes from v1:
> - former patches 1 and 2: dropped
> - patch 1 (former 3): changed the version to '6.1'
> - patch 2 (former 4): add a DEVICE_UNPLUG_ERROR event in the device
>   unplug error path of CPUs and DIMMs
> - v1 link: https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg04682.html
> 
> 
> Daniel Henrique Barboza (7):
>   hw/acpi/memory_hotplug.c: avoid sending MEM_UNPLUG_ERROR if dev->id is
>     NULL
>   spapr.c: avoid sending MEM_UNPLUG_ERROR if dev->id is NULL
>   spapr_drc.c: do not error_report() when drc->dev->id == NULL
>   qapi/qdev.json: fix DEVICE_DELETED parameters doc
>   qapi/qdev.json: add DEVICE_UNPLUG_ERROR QAPI event
>   spapr: use DEVICE_UNPLUG_ERROR to report unplug errors
>   memory_hotplug.c: send DEVICE_UNPLUG_ERROR in
>     acpi_memory_hotplug_write()
> 
>  docs/about/deprecated.rst | 10 ++++++++++
>  hw/acpi/memory_hotplug.c  | 19 +++++++++++++++++--
>  hw/ppc/spapr.c            | 17 +++++++++++++----
>  hw/ppc/spapr_drc.c        | 21 +++++++++++++++------
>  qapi/machine.json         |  6 +++++-
>  qapi/qdev.json            | 34 +++++++++++++++++++++++++++++++---
>  stubs/qdev.c              |  7 +++++++
>  7 files changed, 98 insertions(+), 16 deletions(-)
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH for-6.2 v6 1/7] hw/acpi/memory_hotplug.c: avoid sending MEM_UNPLUG_ERROR if dev->id is NULL

[Markus Armbruster]

I apologize for the not reviewing this promptly.

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

> qapi_event_send_mem_unplug_error() deals with @device being NULL by
> replacing it with an empty string ("") when emitting the event. Aside
> from the fact that this is a side effect that can be patched someday,

I guess by "side effect" you allude to the output visitor's misfeature
to map null pointer to "".

> there's also the lack of utility that the event brings to listeners,
> e.g.  "a memory unplug error happened somewhere".

True.

> We're better of not emitting the event if dev->id is NULL.

On a green field, yes.  But is it worth an incompatible change now?  I
doubt it.

>                                                            Next patches
> will introduce a new device unplug error event that is better suited to
> deal with dev->id NULL scenarios. MEM_UNPLUG_ERROR will continue to be
> emitted to avoid breaking existing APIs, but it'll be deprecated and
> removed in the future.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hw/acpi/memory_hotplug.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index af37889423..e37acb0367 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -177,9 +177,14 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>              /* call pc-dimm unplug cb */
>              hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
>              if (local_err) {
> +                const char *error_pretty = error_get_pretty(local_err);
> +
>                  trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
> -                qapi_event_send_mem_unplug_error(dev->id,
> -                                                 error_get_pretty(local_err));
> +
> +                if (dev->id) {
> +                    qapi_event_send_mem_unplug_error(dev->id, error_pretty);
> +                }
> +
>                  error_free(local_err);
>                  break;
>              }

Three options:

1. Make the incompatible change.  Keep this patch.  Needs a release
   note.

2. Continue to rely on the output visitor's misfeature.  Drop this
   patch.

   Relying on the misfeature is best avoided, at least in new code.

3. Make the mapping explicit here, and avoid relying on the misfeature.
   Something like the appended patch.

I don't like 1.  I think the choice between 2. and 3. depends on how it
fits with the remainder of this series.


diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index af37889423..89c411dd5c 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -178,7 +178,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
             hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
             if (local_err) {
                 trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
-                qapi_event_send_mem_unplug_error(dev->id,
+                qapi_event_send_mem_unplug_error(dev->id ?: "",
                                                  error_get_pretty(local_err));
                 error_free(local_err);
                 break;

Re: [PATCH for-6.2 v6 2/7] spapr.c: avoid sending MEM_UNPLUG_ERROR if dev->id is NULL

[Markus Armbruster]

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

> As done in hw/acpi/memory_hotplug.c, avoid sending
> qapi_event_send_mem_unplug_error() if dev->id is NULL.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hw/ppc/spapr.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 81699d4f8b..1611d7ab05 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3688,9 +3688,11 @@ void spapr_memory_unplug_rollback(SpaprMachineState *spapr, DeviceState *dev)
>       * Tell QAPI that something happened and the memory
>       * hotunplug wasn't successful.
>       */
> -    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);
> +    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);
> +    }
>  }
>  
>  /* Callback to be called during DRC release. */

My review of PATCH 1 applies.

Re: [PATCH for-6.2 v6 3/7] spapr_drc.c: do not error_report() when drc->dev->id == NULL

[Markus Armbruster]

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

> The error_report() call in drc_unisolate_logical() is not considering
> that drc->dev->id can be NULL, and the underlying functions error_report()
> calls to do its job (vprintf(), g_strdup_printf() ...) has undefined
> behavior when trying to handle "%s" with NULL arguments.
>
> Besides, there is no utility into reporting that an unknown device was
> rejected by the guest.
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hw/ppc/spapr_drc.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index a2f2634601..a4d9496f76 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -167,8 +167,11 @@ static uint32_t drc_unisolate_logical(SpaprDrc *drc)
>              }
>  
>              drc->unplug_requested = false;
> -            error_report("Device hotunplug rejected by the guest "
> -                         "for device %s", drc->dev->id);
> +
> +            if (drc->dev->id) {
> +                error_report("Device hotunplug rejected by the guest "
> +                             "for device %s", drc->dev->id);
> +            }
>  
>              /*
>               * TODO: send a QAPI DEVICE_UNPLUG_ERROR event when

This differs from PATCH 1 and 2 in that it actually fixes a crash bug.

The alternative is something like

               error_report("Device hotunplug rejected by the guest "
                            "for device %s", drc->dev->id ?: "");

If the maintainer is okay with dropping the message when the device has
no ID, then so am I:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH for-6.2 v6 4/7] qapi/qdev.json: fix DEVICE_DELETED parameters doc

[Markus Armbruster]

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

> Clarify that @device is optional and that 'path' is the device
> path from QOM.
>
> This change follows Markus' suggestion verbatim, provided in full
> context here:
>
> https://lists.gnu.org/archive/html/qemu-devel/2021-07/msg01891.html
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  qapi/qdev.json | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index b83178220b..d1d3681a50 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -108,9 +108,9 @@
>  # At this point, it's safe to reuse the specified device ID. Device removal can
>  # be initiated by the guest or by HMP/QMP commands.
>  #
> -# @device: device name
> +# @device: the device's ID if it has one
>  #
> -# @path: device path
> +# @path: the device's path within the object model

Recommend "the device's QOM path".

>  #
>  # Since: 1.5
>  #

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH for-6.2 v6 5/7] qapi/qdev.json: add DEVICE_UNPLUG_ERROR QAPI event

[Markus Armbruster]

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

> At this moment we only provide one event to report a hotunplug error,
> MEM_UNPLUG_ERROR. As of Linux kernel 5.12 and QEMU 6.0.0, the pseries
> machine is now able to report unplug errors for other device types, such
> as CPUs.
>
> Instead of creating a (device_type)_UNPLUG_ERROR for each new device,
> create a generic DEVICE_UNPLUG_ERROR event that can be used by all
> unplug errors in the future. This event has a similar API as the
> existing DEVICE_DELETED event, with an extra optional 'msg' parameter
> that can be used to explain the reason for the error.
>
> With this new generic event, MEM_UNPLUG_ERROR is now marked as deprecated.
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  docs/about/deprecated.rst | 10 ++++++++++
>  qapi/machine.json         |  6 +++++-
>  qapi/qdev.json            | 30 +++++++++++++++++++++++++++++-
>  stubs/qdev.c              |  7 +++++++
>  4 files changed, 51 insertions(+), 2 deletions(-)
>
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 6d438f1c8d..c0c3431ada 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -204,6 +204,16 @@ The ``I7200`` guest CPU relies on the nanoMIPS ISA, which is deprecated
>  (the ISA has never been upstreamed to a compiler toolchain). Therefore
>  this CPU is also deprecated.
>  
> +
> +QEMU API (QAPI) events
> +----------------------
> +
> +``MEM_UNPLUG_ERROR`` (since 6.2)
> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +Use the more generic event ``DEVICE_UNPLUG_ERROR`` instead.
> +
> +
>  System emulator machines
>  ------------------------
>  
> diff --git a/qapi/machine.json b/qapi/machine.json
> index c3210ee1fb..a595c753d2 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1271,6 +1271,9 @@
>  #
>  # @msg: Informative message
>  #
> +# Features:
> +# @deprecated: This event is deprecated. Use @DEVICE_UNPLUG_ERROR instead.
> +#
>  # Since: 2.4
>  #
>  # Example:
> @@ -1283,7 +1286,8 @@
>  #
>  ##
>  { 'event': 'MEM_UNPLUG_ERROR',
> -  'data': { 'device': 'str', 'msg': 'str' } }
> +  'data': { 'device': 'str', 'msg': 'str' },
> +  'features': ['deprecated'] }
>  
>  ##
>  # @SMPConfiguration:
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index d1d3681a50..52c36c7b9c 100644
> --- a/qapi/qdev.json
> +++ b/qapi/qdev.json
> @@ -84,7 +84,9 @@
>  #        This command merely requests that the guest begin the hot removal
>  #        process.  Completion of the device removal process is signaled with a
>  #        DEVICE_DELETED event. Guest reset will automatically complete removal
> -#        for all devices.
> +#        for all devices.  If an error in the hot removal process is detected,
> +#        the device will not be removed and a DEVICE_UNPLUG_ERROR event is
> +#        sent.  Some errors cannot be detected.
>  #
>  # Since: 0.14
>  #
> @@ -124,3 +126,29 @@
>  ##
>  { 'event': 'DEVICE_DELETED',
>    'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @DEVICE_UNPLUG_ERROR:
> +#
> +# Emitted when a device hot unplug error occurs.
> +#
> +# @device: the device's ID if it has one
> +#
> +# @path: the device's path within the object model

Recommend "the device's QOM path".

> +#
> +# @msg: optional informative message

Is this useful?  I guess the remaining patches will tell.

> +#
> +# Since: 6.2
> +#
> +# Example:
> +#
> +# <- { "event": "DEVICE_UNPLUG_ERROR"
> +#      "data": { "device": "core1",
> +#                "msg": "Device hotunplug rejected by the guest for device core1",
> +#                "path": "/machine/peripheral/core1" },
> +#      },
> +#      "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
> +#
> +##
> +{ 'event': 'DEVICE_UNPLUG_ERROR',
> +  'data': { '*device': 'str', 'path': 'str' , '*msg': 'str' } }
> diff --git a/stubs/qdev.c b/stubs/qdev.c
> index 92e6143134..ffa8f7b59e 100644
> --- a/stubs/qdev.c
> +++ b/stubs/qdev.c
> @@ -21,3 +21,10 @@ void qapi_event_send_device_deleted(bool has_device,
>  {
>      /* Nothing to do. */
>  }
> +
> +void qapi_event_send_device_unplug_error(bool has_device, const char *device,
> +                                         const char *path,
> +                                         bool has_msg, const char *msg)
> +{
> +    /* Nothing to do. */
> +}

I'm reserving judgement on member @msg.  Other than that:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

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 */

Re: [PATCH for-6.2 v6 7/7] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write()

[Markus Armbruster]

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

> MEM_UNPLUG_ERROR is deprecated since the introduction of
> DEVICE_UNPLUG_ERROR. Keep emitting both while the deprecation of
> MEM_UNPLUG_ERROR is pending.
>
> CC: Michael S. Tsirkin <mst@redhat.com>
> CC: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hw/acpi/memory_hotplug.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index e37acb0367..a0772fe083 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -8,6 +8,7 @@
>  #include "qapi/error.h"
>  #include "qapi/qapi-events-acpi.h"
>  #include "qapi/qapi-events-machine.h"
> +#include "qapi/qapi-events-qdev.h"
>  
>  #define MEMORY_SLOTS_NUMBER          "MDNR"
>  #define MEMORY_HOTPLUG_IO_REGION     "HPMR"
> @@ -181,10 +182,19 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>  
>                  trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
>  
> +                /*
> +                 * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
> +                 * while the deprecation of MEM_UNPLUG_ERROR is
> +                 * pending.
> +                 */
>                  if (dev->id) {
>                      qapi_event_send_mem_unplug_error(dev->id, error_pretty);
>                  }
>  
> +                qapi_event_send_device_unplug_error(!!dev->id, dev->id,
> +                                                    dev->canonical_path,
> +                                                    true, error_pretty);
> +
>                  error_free(local_err);
>                  break;
>              }

Unlike the previous patch, "msg" is present even when !dev->id.  Makes
me doubt the previous patch's conditional passing of "msg" some more.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH for-6.2 v6 3/7] spapr_drc.c: do not error_report() when drc->dev->id == NULL

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2277 bytes --]

On Sat, Aug 07, 2021 at 03:41:52PM +0200, Markus Armbruster wrote:
> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
> 
> > The error_report() call in drc_unisolate_logical() is not considering
> > that drc->dev->id can be NULL, and the underlying functions error_report()
> > calls to do its job (vprintf(), g_strdup_printf() ...) has undefined
> > behavior when trying to handle "%s" with NULL arguments.
> >
> > Besides, there is no utility into reporting that an unknown device was
> > rejected by the guest.
> >
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> >  hw/ppc/spapr_drc.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index a2f2634601..a4d9496f76 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -167,8 +167,11 @@ static uint32_t drc_unisolate_logical(SpaprDrc *drc)
> >              }
> >  
> >              drc->unplug_requested = false;
> > -            error_report("Device hotunplug rejected by the guest "
> > -                         "for device %s", drc->dev->id);
> > +
> > +            if (drc->dev->id) {
> > +                error_report("Device hotunplug rejected by the guest "
> > +                             "for device %s", drc->dev->id);
> > +            }
> >  
> >              /*
> >               * TODO: send a QAPI DEVICE_UNPLUG_ERROR event when
> 
> This differs from PATCH 1 and 2 in that it actually fixes a crash bug.
> 
> The alternative is something like
> 
>                error_report("Device hotunplug rejected by the guest "
>                             "for device %s", drc->dev->id ?: "");
> 
> If the maintainer is okay with dropping the message when the device has
> no ID, then so am I:

I am, given how briefly this message has even existed - and through
all that time it would have crashed if you tried.

> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Folded into my tree.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH for-6.2 v6 7/7] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2475 bytes --]

On Sat, Aug 07, 2021 at 04:09:40PM +0200, Markus Armbruster wrote:
> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
> 
> > MEM_UNPLUG_ERROR is deprecated since the introduction of
> > DEVICE_UNPLUG_ERROR. Keep emitting both while the deprecation of
> > MEM_UNPLUG_ERROR is pending.
> >
> > CC: Michael S. Tsirkin <mst@redhat.com>
> > CC: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> >  hw/acpi/memory_hotplug.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> > index e37acb0367..a0772fe083 100644
> > --- a/hw/acpi/memory_hotplug.c
> > +++ b/hw/acpi/memory_hotplug.c
> > @@ -8,6 +8,7 @@
> >  #include "qapi/error.h"
> >  #include "qapi/qapi-events-acpi.h"
> >  #include "qapi/qapi-events-machine.h"
> > +#include "qapi/qapi-events-qdev.h"
> >  
> >  #define MEMORY_SLOTS_NUMBER          "MDNR"
> >  #define MEMORY_HOTPLUG_IO_REGION     "HPMR"
> > @@ -181,10 +182,19 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
> >  
> >                  trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
> >  
> > +                /*
> > +                 * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_ERROR
> > +                 * while the deprecation of MEM_UNPLUG_ERROR is
> > +                 * pending.
> > +                 */
> >                  if (dev->id) {
> >                      qapi_event_send_mem_unplug_error(dev->id, error_pretty);
> >                  }
> >  
> > +                qapi_event_send_device_unplug_error(!!dev->id, dev->id,
> > +                                                    dev->canonical_path,
> > +                                                    true, error_pretty);
> > +
> >                  error_free(local_err);
> >                  break;
> >              }
> 
> Unlike the previous patch, "msg" is present even when !dev->id.  Makes
> me doubt the previous patch's conditional passing of "msg" some more.

Daniel, if you can address Markus' comments and send another spin,
I'll replace the draft I have in ppc-for-6.2 with the new version.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Daniel Henrique Barboza]

On 8/7/21 11:06 AM, Markus Armbruster wrote:
> 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"?


It won't have a different 'msg' for the current use of the event in both ppc64
and x86. It'll always be the same '<dev> hotunplug rejected by the guest'
message.

The idea is that a future caller might want to insert a more informative
message, such as "hotunplug failed: memory is being used by kernel space"
or any other more specific condition. But then I guess we can argue that,
if that time comes, one can just add this new optional 'msg' member in this
event, and for now we can live without it.

Would you oppose to renaming this new event to "DEVICE_UNPLUG_GUEST_ERROR"
and then remove the 'msg' member? I guess this rename would make it clearer
for management that we're reporting a guest side error, making any further
clarifications via 'msg' unneeded.


Thanks,


Daniel




> 
> 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 */
> 

Re: [PATCH for-6.2 v6 1/7] hw/acpi/memory_hotplug.c: avoid sending MEM_UNPLUG_ERROR if dev->id is NULL

[Daniel Henrique Barboza]

On 8/7/21 10:38 AM, Markus Armbruster wrote:
> I apologize for the not reviewing this promptly.
> 
> Daniel Henrique Barboza <danielhb413@gmail.com> writes:
> 
>> qapi_event_send_mem_unplug_error() deals with @device being NULL by
>> replacing it with an empty string ("") when emitting the event. Aside
>> from the fact that this is a side effect that can be patched someday,
> 
> I guess by "side effect" you allude to the output visitor's misfeature
> to map null pointer to "".
> 
>> there's also the lack of utility that the event brings to listeners,
>> e.g.  "a memory unplug error happened somewhere".
> 
> True.
> 
>> We're better of not emitting the event if dev->id is NULL.
> 
> On a green field, yes.  But is it worth an incompatible change now?  I
> doubt it.
> 
>>                                                             Next patches
>> will introduce a new device unplug error event that is better suited to
>> deal with dev->id NULL scenarios. MEM_UNPLUG_ERROR will continue to be
>> emitted to avoid breaking existing APIs, but it'll be deprecated and
>> removed in the future.
>>
>> Suggested-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Greg Kurz <groug@kaod.org>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>>   hw/acpi/memory_hotplug.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
>> index af37889423..e37acb0367 100644
>> --- a/hw/acpi/memory_hotplug.c
>> +++ b/hw/acpi/memory_hotplug.c
>> @@ -177,9 +177,14 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>>               /* call pc-dimm unplug cb */
>>               hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
>>               if (local_err) {
>> +                const char *error_pretty = error_get_pretty(local_err);
>> +
>>                   trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
>> -                qapi_event_send_mem_unplug_error(dev->id,
>> -                                                 error_get_pretty(local_err));
>> +
>> +                if (dev->id) {
>> +                    qapi_event_send_mem_unplug_error(dev->id, error_pretty);
>> +                }
>> +
>>                   error_free(local_err);
>>                   break;
>>               }
> 
> Three options:
> 
> 1. Make the incompatible change.  Keep this patch.  Needs a release
>     note.
> 
> 2. Continue to rely on the output visitor's misfeature.  Drop this
>     patch.
> 
>     Relying on the misfeature is best avoided, at least in new code.
> 
> 3. Make the mapping explicit here, and avoid relying on the misfeature.
>     Something like the appended patch.

I like (3) more. If no one opposes I'll respin patches 1 and 2 with this
approach.


Daniel

> 
> I don't like 1.  I think the choice between 2. and 3. depends on how it
> fits with the remainder of this series.
> 
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index af37889423..89c411dd5c 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -178,7 +178,7 @@ static void acpi_memory_hotplug_write(void *opaque, hwaddr addr, uint64_t data,
>               hotplug_handler_unplug(hotplug_ctrl, dev, &local_err);
>               if (local_err) {
>                   trace_mhp_acpi_pc_dimm_delete_failed(mem_st->selector);
> -                qapi_event_send_mem_unplug_error(dev->id,
> +                qapi_event_send_mem_unplug_error(dev->id ?: "",
>                                                    error_get_pretty(local_err));
>                   error_free(local_err);
>                   break;
> 

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7337 bytes --]

On Mon, Aug 09, 2021 at 03:47:14PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 8/7/21 11:06 AM, Markus Armbruster wrote:
> > 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"?
> 
> 
> It won't have a different 'msg' for the current use of the event in both ppc64
> and x86. It'll always be the same '<dev> hotunplug rejected by the guest'
> message.
> 
> The idea is that a future caller might want to insert a more informative
> message, such as "hotunplug failed: memory is being used by kernel space"
> or any other more specific condition. But then I guess we can argue that,
> if that time comes, one can just add this new optional 'msg' member in this
> event, and for now we can live without it.

Right.  We could also consider making the current message more
specific about why we chose to cancel the unplug: e.g. "guest
unisolated DRC after unplug request" for PAPR, and something
appropriate to the ACPI specifics for x86.  Not sure if that's useful
enough to justify it.

> Would you oppose to renaming this new event to "DEVICE_UNPLUG_GUEST_ERROR"
> and then remove the 'msg' member? I guess this rename would make it clearer
> for management that we're reporting a guest side error, making any further
> clarifications via 'msg' unneeded.
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> 
> 
> > 
> > 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 */
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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:

> On 8/7/21 11:06 AM, Markus Armbruster wrote:
>> 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"?
>
>
> It won't have a different 'msg' for the current use of the event in both ppc64
> and x86. It'll always be the same '<dev> hotunplug rejected by the guest'
> message.
>
> The idea is that a future caller might want to insert a more informative
> message, such as "hotunplug failed: memory is being used by kernel space"
> or any other more specific condition. But then I guess we can argue that,
> if that time comes, one can just add this new optional 'msg' member in this
> event, and for now we can live without it.
>
> Would you oppose to renaming this new event to "DEVICE_UNPLUG_GUEST_ERROR"
> and then remove the 'msg' member? I guess this rename would make it clearer
> for management that we're reporting a guest side error, making any further
> clarifications via 'msg' unneeded.

No objection.