[PATCH v5 0/7] DEVICE_UNPLUG_ERROR QAPI event

[PATCH v5 0/7] DEVICE_UNPLUG_ERROR QAPI event

[Daniel Henrique Barboza]

Hi,

In this version the most notable changes are additional cleanups/fixes
patches that precedes the introduction of the new DEVICE_UNPLUG_ERROR
API, and the change of DEVICE_UNPLUG_ERROR to behave more in line
with DEVICE_DELETED, as suggested by Markus.

This is how the new event looks like:

{"execute": "device_del", "arguments": {"id": "core1"} }
{"return": {}}
{"timestamp": {"seconds": 1626117386, "microseconds": 182215},
 "event": "DEVICE_UNPLUG_ERROR",
 "data": {"device": "core1",
          "msg": "Device hotunplug rejected by the guest for device core1",
          "path": "/machine/peripheral/core1"}}  

I kept the 'msg' element since both MEM_UNPLUG_ERROR callers provides a
reason for the failed unplug. This element works like 'device' and it's
provided if available.


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/system/deprecated.rst | 10 ++++++++++
 hw/acpi/memory_hotplug.c   | 19 +++++++++++++++++--
 hw/ppc/spapr.c             | 17 +++++++++++++----
 hw/ppc/spapr_drc.c         | 20 ++++++++++++++------
 qapi/machine.json          |  6 +++++-
 qapi/qdev.json             | 34 +++++++++++++++++++++++++++++++---
 stubs/qdev.c               |  7 +++++++
 7 files changed, 97 insertions(+), 16 deletions(-)

-- 
2.31.1

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

[Daniel Henrique Barboza]

qapi_event_send_device_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>
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 v5 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>
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 v5 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.

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 v5 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>
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 v5 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.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 docs/system/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/system/deprecated.rst b/docs/system/deprecated.rst
index 6d438f1c8d..c0c3431ada 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/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 v5 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.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr.c     |  9 ++++++++-
 hw/ppc/spapr_drc.c | 17 +++++++++++------
 2 files changed, 19 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..a3a15fd2ed 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,10 @@ 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 +175,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 v5 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>
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 v5 1/7] hw/acpi/memory_hotplug.c: avoid sending MEM_UNPLUG_ERROR if dev->id is NULL

[Greg Kurz]

On Mon, 12 Jul 2021 16:43:33 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> qapi_event_send_device_unplug_error() deals with @device being NULL

s/qapi_event_send_device_unplug_error/qapi_event_send_mem_unplug_error ?

since I only see qapi_event_send_mem_unplug_error() in the diff.

> 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>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---

Appart from the nit in the changelog,

Reviewed-by: Greg Kurz <groug@kaod.org>

>  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;
>              }

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

[Greg Kurz]

On Mon, 12 Jul 2021 16:43:34 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> 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>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

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

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

[Greg Kurz]

On Mon, 12 Jul 2021 16:43:35 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> 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.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  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

Re: [PATCH v5 4/7] qapi/qdev.json: fix DEVICE_DELETED parameters doc

[Greg Kurz]

On Mon, 12 Jul 2021 16:43:36 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> 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>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  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
>  #

Re: [PATCH v5 5/7] qapi/qdev.json: add DEVICE_UNPLUG_ERROR QAPI event

[Greg Kurz]

On Mon, 12 Jul 2021 16:43:37 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> 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.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  docs/system/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/system/deprecated.rst b/docs/system/deprecated.rst
> index 6d438f1c8d..c0c3431ada 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/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
> +#

Good, we're in 6.1 soft freeze indeed :)

Reviewed-by: Greg Kurz <groug@kaod.org>

> +# 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. */
> +}

Re: [PATCH v5 6/7] spapr: use DEVICE_UNPLUG_ERROR to report unplug errors

[Greg Kurz]

On Mon, 12 Jul 2021 16:43:38 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> 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.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  hw/ppc/spapr.c     |  9 ++++++++-
>  hw/ppc/spapr_drc.c | 17 +++++++++++------
>  2 files changed, 19 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..a3a15fd2ed 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,10 @@ 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";

It is usually preferable to keep error messages on one line to ease
grepping. No big deal.

Reviewed-by: Greg Kurz <groug@kaod.org>

> +            g_autofree char *qapi_error = NULL;
> +
>              if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
>                  spapr = SPAPR_MACHINE(qdev_get_machine());
>  
> @@ -169,14 +175,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 */

Re: [PATCH v5 7/7] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write()

[Greg Kurz]

On Mon, 12 Jul 2021 16:43:39 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> 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>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  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;
>              }

Re: [PATCH v5 5/7] qapi/qdev.json: add DEVICE_UNPLUG_ERROR QAPI event

[David Gibson]

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

On Mon, Jul 12, 2021 at 04:43:37PM -0300, Daniel Henrique Barboza wrote:
> 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.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

This no longer applies clean for me - looks like deprecated.rst has
been moved.

At this point I'm willing to queue this tentatively for 6.2, but I'll
need a rebased respin (and fold in Greg's acks, please).

> ---
>  docs/system/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/system/deprecated.rst b/docs/system/deprecated.rst
> index 6d438f1c8d..c0c3431ada 100644
> --- a/docs/system/deprecated.rst
> +++ b/docs/system/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. */
> +}

-- 
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 --]