[PATCH v7 0/7] DEVICE_UNPLUG_GUEST_ERROR QAPI event

[PATCH v7 0/7] DEVICE_UNPLUG_GUEST_ERROR QAPI event

[Daniel Henrique Barboza]

Hi,

In this version the event was renamed and the optional 'msg'
attribute was removed. It also contains smaller changes based
on Markus' comments in v6.

changes from v6:
- patches 1 and 2:
  * handle dev->id = NULL explicitly with empty string
- patch 3:
  * added Markus' reviewed-by
- patch 4:
  * reworded 'path' attribute desc as 'the device's QOM path'
  * added Markus's reviewed-by
- patch 5:
  * event was renamed to 'DEVICE_UNPLUG_GUEST_ERROR'
  * reworded 'path' attribute desc as 'the device's QOM path'
  * removed the optional member 'msg' of the event
- patches 6 and 7:
  * changes due to event rename and lack of 'msg' attribute
- v6 link: https://lists.gnu.org/archive/html/qemu-devel/2021-07/msg04923.html


Daniel Henrique Barboza (7):
  memory_hotplug.c: handle dev->id = NULL in acpi_memory_hotplug_write()
  spapr.c: handle dev->id in spapr_memory_unplug_rollback()
  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_GUEST_ERROR QAPI event
  spapr: use DEVICE_UNPLUG_GUEST_ERROR to report unplug errors
  memory_hotplug.c: send DEVICE_UNPLUG_GUEST_ERROR in
    acpi_memory_hotplug_write()

 docs/about/deprecated.rst | 10 ++++++++++
 hw/acpi/memory_hotplug.c  | 11 ++++++++++-
 hw/ppc/spapr.c            | 12 ++++++++++--
 hw/ppc/spapr_drc.c        | 16 ++++++++++------
 qapi/machine.json         |  7 ++++++-
 qapi/qdev.json            | 32 +++++++++++++++++++++++++++++---
 stubs/qdev.c              |  7 +++++++
 7 files changed, 82 insertions(+), 13 deletions(-)

-- 
2.31.1

[PATCH v7 1/7] memory_hotplug.c: handle dev->id = NULL in acpi_memory_hotplug_write()

[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 behavior (qapi visitor mapping NULL pointer to
"") can be patched/changed someday, there's also the lack of utility
that the event brings to listeners, e.g. "a memory unplug error happened
somewhere".

In theory we should just avoit emitting this event at all if dev->id is
NULL, but this would be an incompatible change to existing guests.
Instead, let's make the forementioned behavior explicit: if dev->id is
NULL, pass an empty string to qapi_event_send_mem_unplug_error().

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/acpi/memory_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index af37889423..6a71de408b 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;
-- 
2.31.1

Re: [PATCH v7 1/7] memory_hotplug.c: handle dev->id = NULL in acpi_memory_hotplug_write()

[David Gibson]

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

On Tue, Aug 24, 2021 at 09:48:29PM -0300, Daniel Henrique Barboza wrote:
> 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 behavior (qapi visitor mapping NULL pointer to
> "") can be patched/changed someday, there's also the lack of utility
> that the event brings to listeners, e.g. "a memory unplug error happened
> somewhere".
> 
> In theory we should just avoit emitting this event at all if dev->id is
> NULL, but this would be an incompatible change to existing guests.
> Instead, let's make the forementioned behavior explicit: if dev->id is
> NULL, pass an empty string to qapi_event_send_mem_unplug_error().
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/acpi/memory_hotplug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index af37889423..6a71de408b 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;

-- 
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 v7 1/7] memory_hotplug.c: handle dev->id = NULL in acpi_memory_hotplug_write()

[Igor Mammedov]

On Tue, 24 Aug 2021 21:48:29 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> 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 behavior (qapi visitor mapping NULL pointer to
> "") can be patched/changed someday, there's also the lack of utility
> that the event brings to listeners, e.g. "a memory unplug error happened
> somewhere".
> 
> In theory we should just avoit emitting this event at all if dev->id is
> NULL, but this would be an incompatible change to existing guests.
> Instead, let's make the forementioned behavior explicit: if dev->id is
> NULL, pass an empty string to qapi_event_send_mem_unplug_error().
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/memory_hotplug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index af37889423..6a71de408b 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 v7 1/7] memory_hotplug.c: handle dev->id = NULL in acpi_memory_hotplug_write()

[Greg Kurz]

On Tue, 24 Aug 2021 21:48:29 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> 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 behavior (qapi visitor mapping NULL pointer to
> "") can be patched/changed someday, there's also the lack of utility
> that the event brings to listeners, e.g. "a memory unplug error happened
> somewhere".
> 
> In theory we should just avoit emitting this event at all if dev->id is
> NULL, but this would be an incompatible change to existing guests.
> Instead, let's make the forementioned behavior explicit: if dev->id is
> NULL, pass an empty string to qapi_event_send_mem_unplug_error().
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---

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

>  hw/acpi/memory_hotplug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index af37889423..6a71de408b 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;

[PATCH v7 2/7] spapr.c: handle dev->id in spapr_memory_unplug_rollback()

[Daniel Henrique Barboza]

As done in hw/acpi/memory_hotplug.c, pass an empty string if dev->id
is NULL to qapi_event_send_mem_unplug_error() to avoid relying on
a behavior that can be changed in the future.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/spapr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 81699d4f8b..4f1ee90e9e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3690,7 +3690,7 @@ void spapr_memory_unplug_rollback(SpaprMachineState *spapr, DeviceState *dev)
      */
     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_mem_unplug_error(dev->id ? : "", qapi_error);
 }
 
 /* Callback to be called during DRC release. */
-- 
2.31.1

Re: [PATCH v7 2/7] spapr.c: handle dev->id in spapr_memory_unplug_rollback()

[David Gibson]

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

On Tue, Aug 24, 2021 at 09:48:30PM -0300, Daniel Henrique Barboza wrote:
> As done in hw/acpi/memory_hotplug.c, pass an empty string if dev->id
> is NULL to qapi_event_send_mem_unplug_error() to avoid relying on
> a behavior that can be changed in the future.
> 
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 81699d4f8b..4f1ee90e9e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3690,7 +3690,7 @@ void spapr_memory_unplug_rollback(SpaprMachineState *spapr, DeviceState *dev)
>       */
>      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_mem_unplug_error(dev->id ? : "", qapi_error);
>  }
>  
>  /* Callback to be called during DRC release. */

-- 
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 v7 2/7] spapr.c: handle dev->id in spapr_memory_unplug_rollback()

[Greg Kurz]

On Tue, 24 Aug 2021 21:48:30 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> As done in hw/acpi/memory_hotplug.c, pass an empty string if dev->id
> is NULL to qapi_event_send_mem_unplug_error() to avoid relying on
> a behavior that can be changed in the future.
> 
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 81699d4f8b..4f1ee90e9e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3690,7 +3690,7 @@ void spapr_memory_unplug_rollback(SpaprMachineState *spapr, DeviceState *dev)
>       */
>      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_mem_unplug_error(dev->id ? : "", qapi_error);
>  }
>  
>  /* Callback to be called during DRC release. */

[PATCH v7 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>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
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

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

[David Gibson]

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

On Tue, Aug 24, 2021 at 09:48:31PM -0300, Daniel Henrique Barboza 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.
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

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

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

[PATCH v7 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>
Reviewed-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..0e9cb2ae88 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 QOM path
 #
 # Since: 1.5
 #
-- 
2.31.1

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

[David Gibson]

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

On Tue, Aug 24, 2021 at 09:48:32PM -0300, Daniel Henrique Barboza 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>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  qapi/qdev.json | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index b83178220b..0e9cb2ae88 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 QOM path
>  #
>  # Since: 1.5
>  #

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

[PATCH v7 5/7] qapi/qdev.json: add DEVICE_UNPLUG_GUEST_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_GUEST_ERROR event that can be used by all
guest side unplug errors in the future. This event has a similar API as
the existing DEVICE_DELETED event, always providing the QOM path of the
device and dev->id if there's any.

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

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 docs/about/deprecated.rst | 10 ++++++++++
 qapi/machine.json         |  7 ++++++-
 qapi/qdev.json            | 28 +++++++++++++++++++++++++++-
 stubs/qdev.c              |  7 +++++++
 4 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
index 6d438f1c8d..1a8ffc9381 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_GUEST_ERROR`` instead.
+
+
 System emulator machines
 ------------------------
 
diff --git a/qapi/machine.json b/qapi/machine.json
index 157712f006..cd397f1ee4 100644
--- a/qapi/machine.json
+++ b/qapi/machine.json
@@ -1271,6 +1271,10 @@
 #
 # @msg: Informative message
 #
+# Features:
+# @deprecated: This event is deprecated. Use @DEVICE_UNPLUG_GUEST_ERROR
+#              instead.
+#
 # Since: 2.4
 #
 # Example:
@@ -1283,7 +1287,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 0e9cb2ae88..8b1a1dd43b 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 a guest-side error in the hot removal process is
+#        detected, the device will not be removed and a DEVICE_UNPLUG_GUEST_ERROR
+#        event is sent.  Some errors cannot be detected.
 #
 # Since: 0.14
 #
@@ -124,3 +126,27 @@
 ##
 { 'event': 'DEVICE_DELETED',
   'data': { '*device': 'str', 'path': 'str' } }
+
+##
+# @DEVICE_UNPLUG_GUEST_ERROR:
+#
+# Emitted when a device hot unplug fails due to an internal guest
+# error.
+#
+# @device: the device's ID if it has one
+#
+# @path: the device's QOM path
+#
+# Since: 6.2
+#
+# Example:
+#
+# <- { "event": "DEVICE_UNPLUG_GUEST_ERROR"
+#      "data": { "device": "core1",
+#                "path": "/machine/peripheral/core1" },
+#      },
+#      "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
+#
+##
+{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
+  'data': { '*device': 'str', 'path': 'str' } }
diff --git a/stubs/qdev.c b/stubs/qdev.c
index 92e6143134..28d6d531e6 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_guest_error(bool has_device,
+                                               const char *device,
+                                               const char *path
+{
+    /* Nothing to do. */
+}
-- 
2.31.1

Re: [PATCH v7 5/7] qapi/qdev.json: add DEVICE_UNPLUG_GUEST_ERROR QAPI event

[David Gibson]

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

On Tue, Aug 24, 2021 at 09:48:33PM -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_GUEST_ERROR event that can be used by all
> guest side unplug errors in the future. This event has a similar API as
> the existing DEVICE_DELETED event, always providing the QOM path of the
> device and dev->id if there's any.
> 
> With this new generic event, MEM_UNPLUG_ERROR is now marked as deprecated.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  docs/about/deprecated.rst | 10 ++++++++++
>  qapi/machine.json         |  7 ++++++-
>  qapi/qdev.json            | 28 +++++++++++++++++++++++++++-
>  stubs/qdev.c              |  7 +++++++
>  4 files changed, 50 insertions(+), 2 deletions(-)

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> 
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 6d438f1c8d..1a8ffc9381 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_GUEST_ERROR`` instead.
> +
> +
>  System emulator machines
>  ------------------------
>  
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 157712f006..cd397f1ee4 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1271,6 +1271,10 @@
>  #
>  # @msg: Informative message
>  #
> +# Features:
> +# @deprecated: This event is deprecated. Use @DEVICE_UNPLUG_GUEST_ERROR
> +#              instead.
> +#
>  # Since: 2.4
>  #
>  # Example:
> @@ -1283,7 +1287,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 0e9cb2ae88..8b1a1dd43b 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 a guest-side error in the hot removal process is
> +#        detected, the device will not be removed and a DEVICE_UNPLUG_GUEST_ERROR
> +#        event is sent.  Some errors cannot be detected.
>  #
>  # Since: 0.14
>  #
> @@ -124,3 +126,27 @@
>  ##
>  { 'event': 'DEVICE_DELETED',
>    'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @DEVICE_UNPLUG_GUEST_ERROR:
> +#
> +# Emitted when a device hot unplug fails due to an internal guest
> +# error.
> +#
> +# @device: the device's ID if it has one
> +#
> +# @path: the device's QOM path
> +#
> +# Since: 6.2
> +#
> +# Example:
> +#
> +# <- { "event": "DEVICE_UNPLUG_GUEST_ERROR"
> +#      "data": { "device": "core1",
> +#                "path": "/machine/peripheral/core1" },
> +#      },
> +#      "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
> +#
> +##
> +{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
> +  'data': { '*device': 'str', 'path': 'str' } }
> diff --git a/stubs/qdev.c b/stubs/qdev.c
> index 92e6143134..28d6d531e6 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_guest_error(bool has_device,
> +                                               const char *device,
> +                                               const char *path
> +{
> +    /* 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 v7 5/7] qapi/qdev.json: add DEVICE_UNPLUG_GUEST_ERROR QAPI event

[Greg Kurz]

On Tue, 24 Aug 2021 21:48:33 -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_GUEST_ERROR event that can be used by all
> guest side unplug errors in the future. This event has a similar API as
> the existing DEVICE_DELETED event, always providing the QOM path of the
> device and dev->id if there's any.
> 
> With this new generic event, MEM_UNPLUG_ERROR is now marked as deprecated.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---

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

>  docs/about/deprecated.rst | 10 ++++++++++
>  qapi/machine.json         |  7 ++++++-
>  qapi/qdev.json            | 28 +++++++++++++++++++++++++++-
>  stubs/qdev.c              |  7 +++++++
>  4 files changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 6d438f1c8d..1a8ffc9381 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_GUEST_ERROR`` instead.
> +
> +
>  System emulator machines
>  ------------------------
>  
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 157712f006..cd397f1ee4 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1271,6 +1271,10 @@
>  #
>  # @msg: Informative message
>  #
> +# Features:
> +# @deprecated: This event is deprecated. Use @DEVICE_UNPLUG_GUEST_ERROR
> +#              instead.
> +#
>  # Since: 2.4
>  #
>  # Example:
> @@ -1283,7 +1287,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 0e9cb2ae88..8b1a1dd43b 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 a guest-side error in the hot removal process is
> +#        detected, the device will not be removed and a DEVICE_UNPLUG_GUEST_ERROR
> +#        event is sent.  Some errors cannot be detected.
>  #
>  # Since: 0.14
>  #
> @@ -124,3 +126,27 @@
>  ##
>  { 'event': 'DEVICE_DELETED',
>    'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @DEVICE_UNPLUG_GUEST_ERROR:
> +#
> +# Emitted when a device hot unplug fails due to an internal guest
> +# error.
> +#
> +# @device: the device's ID if it has one
> +#
> +# @path: the device's QOM path
> +#
> +# Since: 6.2
> +#
> +# Example:
> +#
> +# <- { "event": "DEVICE_UNPLUG_GUEST_ERROR"
> +#      "data": { "device": "core1",
> +#                "path": "/machine/peripheral/core1" },
> +#      },
> +#      "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
> +#
> +##
> +{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
> +  'data': { '*device': 'str', 'path': 'str' } }
> diff --git a/stubs/qdev.c b/stubs/qdev.c
> index 92e6143134..28d6d531e6 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_guest_error(bool has_device,
> +                                               const char *device,
> +                                               const char *path
> +{
> +    /* Nothing to do. */
> +}

Re: [PATCH v7 5/7] qapi/qdev.json: add DEVICE_UNPLUG_GUEST_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_GUEST_ERROR event that can be used by all
> guest side unplug errors in the future. This event has a similar API as
> the existing DEVICE_DELETED event, always providing the QOM path of the
> device and dev->id if there's any.
>
> With this new generic event, MEM_UNPLUG_ERROR is now marked as deprecated.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---

[...]

> diff --git a/qapi/qdev.json b/qapi/qdev.json
> index 0e9cb2ae88..8b1a1dd43b 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 a guest-side error in the hot removal process is
> +#        detected, the device will not be removed and a DEVICE_UNPLUG_GUEST_ERROR
> +#        event is sent.  Some errors cannot be detected.
>  #
>  # Since: 0.14
>  #
> @@ -124,3 +126,27 @@
>  ##
>  { 'event': 'DEVICE_DELETED',
>    'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @DEVICE_UNPLUG_GUEST_ERROR:
> +#
> +# Emitted when a device hot unplug fails due to an internal guest
> +# error.

Suggest to scratch "internal".

> +#
> +# @device: the device's ID if it has one
> +#
> +# @path: the device's QOM path
> +#
> +# Since: 6.2
> +#
> +# Example:
> +#
> +# <- { "event": "DEVICE_UNPLUG_GUEST_ERROR"
> +#      "data": { "device": "core1",
> +#                "path": "/machine/peripheral/core1" },
> +#      },
> +#      "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
> +#
> +##
> +{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
> +  'data': { '*device': 'str', 'path': 'str' } }

[...]

Re: [PATCH v7 5/7] qapi/qdev.json: add DEVICE_UNPLUG_GUEST_ERROR QAPI event

[David Gibson]

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

On Wed, Sep 01, 2021 at 03:19:26PM +0200, Markus Armbruster wrote:
> 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_GUEST_ERROR event that can be used by all
> > guest side unplug errors in the future. This event has a similar API as
> > the existing DEVICE_DELETED event, always providing the QOM path of the
> > device and dev->id if there's any.
> >
> > With this new generic event, MEM_UNPLUG_ERROR is now marked as deprecated.
> >
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> 
> [...]
> 
> > diff --git a/qapi/qdev.json b/qapi/qdev.json
> > index 0e9cb2ae88..8b1a1dd43b 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 a guest-side error in the hot removal process is
> > +#        detected, the device will not be removed and a DEVICE_UNPLUG_GUEST_ERROR
> > +#        event is sent.  Some errors cannot be detected.
> >  #
> >  # Since: 0.14
> >  #
> > @@ -124,3 +126,27 @@
> >  ##
> >  { 'event': 'DEVICE_DELETED',
> >    'data': { '*device': 'str', 'path': 'str' } }
> > +
> > +##
> > +# @DEVICE_UNPLUG_GUEST_ERROR:
> > +#
> > +# Emitted when a device hot unplug fails due to an internal guest
> > +# error.
> 
> Suggest to scratch "internal".

I'd suggest s/internal guest/guest reported/.  "guest error" is a bit
vague, this doesn't neccessarily indicate a bug in the guest.  The key
point is that we're reporting this event because the guest performed
some explicit action to tell us something went wrong with the plug
attempt.

> 
> > +#
> > +# @device: the device's ID if it has one
> > +#
> > +# @path: the device's QOM path
> > +#
> > +# Since: 6.2
> > +#
> > +# Example:
> > +#
> > +# <- { "event": "DEVICE_UNPLUG_GUEST_ERROR"
> > +#      "data": { "device": "core1",
> > +#                "path": "/machine/peripheral/core1" },
> > +#      },
> > +#      "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
> > +#
> > +##
> > +{ 'event': 'DEVICE_UNPLUG_GUEST_ERROR',
> > +  'data': { '*device': 'str', 'path': 'str' } }
> 
> [...]
> 

-- 
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 v7 5/7] qapi/qdev.json: add DEVICE_UNPLUG_GUEST_ERROR QAPI event

[Markus Armbruster]

David Gibson <david@gibson.dropbear.id.au> writes:

> On Wed, Sep 01, 2021 at 03:19:26PM +0200, Markus Armbruster wrote:
>> 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_GUEST_ERROR event that can be used by all
>> > guest side unplug errors in the future. This event has a similar API as
>> > the existing DEVICE_DELETED event, always providing the QOM path of the
>> > device and dev->id if there's any.
>> >
>> > With this new generic event, MEM_UNPLUG_ERROR is now marked as deprecated.
>> >
>> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> > ---
>> 
>> [...]
>> 
>> > diff --git a/qapi/qdev.json b/qapi/qdev.json
>> > index 0e9cb2ae88..8b1a1dd43b 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 a guest-side error in the hot removal process is
>> > +#        detected, the device will not be removed and a DEVICE_UNPLUG_GUEST_ERROR
>> > +#        event is sent.  Some errors cannot be detected.
>> >  #
>> >  # Since: 0.14
>> >  #
>> > @@ -124,3 +126,27 @@
>> >  ##
>> >  { 'event': 'DEVICE_DELETED',
>> >    'data': { '*device': 'str', 'path': 'str' } }
>> > +
>> > +##
>> > +# @DEVICE_UNPLUG_GUEST_ERROR:
>> > +#
>> > +# Emitted when a device hot unplug fails due to an internal guest
>> > +# error.
>> 
>> Suggest to scratch "internal".
>
> I'd suggest s/internal guest/guest reported/.  "guest error" is a bit
> vague, this doesn't neccessarily indicate a bug in the guest.  The key
> point is that we're reporting this event because the guest performed
> some explicit action to tell us something went wrong with the plug
> attempt.

Yes, that's better.

[...]

Re: [PATCH v7 5/7] qapi/qdev.json: add DEVICE_UNPLUG_GUEST_ERROR QAPI event

[Daniel Henrique Barboza]

On 9/4/21 8:49 AM, Markus Armbruster wrote:
> David Gibson <david@gibson.dropbear.id.au> writes:
> 
>> On Wed, Sep 01, 2021 at 03:19:26PM +0200, Markus Armbruster wrote:
>>> 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_GUEST_ERROR event that can be used by all
>>>> guest side unplug errors in the future. This event has a similar API as
>>>> the existing DEVICE_DELETED event, always providing the QOM path of the
>>>> device and dev->id if there's any.
>>>>
>>>> With this new generic event, MEM_UNPLUG_ERROR is now marked as deprecated.
>>>>
>>>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>>>> ---
>>>
>>> [...]
>>>
>>>> diff --git a/qapi/qdev.json b/qapi/qdev.json
>>>> index 0e9cb2ae88..8b1a1dd43b 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 a guest-side error in the hot removal process is
>>>> +#        detected, the device will not be removed and a DEVICE_UNPLUG_GUEST_ERROR
>>>> +#        event is sent.  Some errors cannot be detected.
>>>>   #
>>>>   # Since: 0.14
>>>>   #
>>>> @@ -124,3 +126,27 @@
>>>>   ##
>>>>   { 'event': 'DEVICE_DELETED',
>>>>     'data': { '*device': 'str', 'path': 'str' } }
>>>> +
>>>> +##
>>>> +# @DEVICE_UNPLUG_GUEST_ERROR:
>>>> +#
>>>> +# Emitted when a device hot unplug fails due to an internal guest
>>>> +# error.
>>>
>>> Suggest to scratch "internal".
>>
>> I'd suggest s/internal guest/guest reported/.  "guest error" is a bit
>> vague, this doesn't neccessarily indicate a bug in the guest.  The key
>> point is that we're reporting this event because the guest performed
>> some explicit action to tell us something went wrong with the plug
>> attempt.
> 
> Yes, that's better.


I agree.  David, let me know if you need another spin with this change.



Thanks,


Daniel

> 
> [...]
> 

Re: [PATCH v7 5/7] qapi/qdev.json: add DEVICE_UNPLUG_GUEST_ERROR QAPI event

[David Gibson]

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

On Mon, Sep 06, 2021 at 09:40:47AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 9/4/21 8:49 AM, Markus Armbruster wrote:
> > David Gibson <david@gibson.dropbear.id.au> writes:
> > 
> > > On Wed, Sep 01, 2021 at 03:19:26PM +0200, Markus Armbruster wrote:
> > > > 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_GUEST_ERROR event that can be used by all
> > > > > guest side unplug errors in the future. This event has a similar API as
> > > > > the existing DEVICE_DELETED event, always providing the QOM path of the
> > > > > device and dev->id if there's any.
> > > > > 
> > > > > With this new generic event, MEM_UNPLUG_ERROR is now marked as deprecated.
> > > > > 
> > > > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > > > ---
> > > > 
> > > > [...]
> > > > 
> > > > > diff --git a/qapi/qdev.json b/qapi/qdev.json
> > > > > index 0e9cb2ae88..8b1a1dd43b 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 a guest-side error in the hot removal process is
> > > > > +#        detected, the device will not be removed and a DEVICE_UNPLUG_GUEST_ERROR
> > > > > +#        event is sent.  Some errors cannot be detected.
> > > > >   #
> > > > >   # Since: 0.14
> > > > >   #
> > > > > @@ -124,3 +126,27 @@
> > > > >   ##
> > > > >   { 'event': 'DEVICE_DELETED',
> > > > >     'data': { '*device': 'str', 'path': 'str' } }
> > > > > +
> > > > > +##
> > > > > +# @DEVICE_UNPLUG_GUEST_ERROR:
> > > > > +#
> > > > > +# Emitted when a device hot unplug fails due to an internal guest
> > > > > +# error.
> > > > 
> > > > Suggest to scratch "internal".
> > > 
> > > I'd suggest s/internal guest/guest reported/.  "guest error" is a bit
> > > vague, this doesn't neccessarily indicate a bug in the guest.  The key
> > > point is that we're reporting this event because the guest performed
> > > some explicit action to tell us something went wrong with the plug
> > > attempt.
> > 
> > Yes, that's better.
> 
> 
> I agree.  David, let me know if you need another spin with this
> change.

Yes please.  I'm afraid I kind of lost track of the last posting.

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

[PATCH v7 6/7] spapr: use DEVICE_UNPLUG_GUEST_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_GUEST_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_GUEST_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     | 10 +++++++++-
 hw/ppc/spapr_drc.c |  9 +++++----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 4f1ee90e9e..206c536d3a 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,11 +3687,18 @@ 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_GUEST_ERROR until the deprecation of
+     * MEM_UNPLUG_ERROR is due.
      */
     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_guest_error(!!dev->id, dev->id,
+                                              dev->canonical_path);
 }
 
 /* Callback to be called during DRC release. */
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index a4d9496f76..f8ac0a10df 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 */
@@ -173,10 +175,9 @@ static uint32_t drc_unisolate_logical(SpaprDrc *drc)
                              "for device %s", drc->dev->id);
             }
 
-            /*
-             * TODO: send a QAPI DEVICE_UNPLUG_ERROR event when
-             * it is implemented.
-             */
+            qapi_event_send_device_unplug_guest_error(!!drc->dev->id,
+                                                      drc->dev->id,
+                                                      drc->dev->canonical_path);
         }
 
         return RTAS_OUT_SUCCESS; /* Nothing to do */
-- 
2.31.1

Re: [PATCH v7 6/7] spapr: use DEVICE_UNPLUG_GUEST_ERROR to report unplug errors

[David Gibson]

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

On Tue, Aug 24, 2021 at 09:48:34PM -0300, Daniel Henrique Barboza 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_GUEST_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_GUEST_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>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/ppc/spapr.c     | 10 +++++++++-
>  hw/ppc/spapr_drc.c |  9 +++++----
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 4f1ee90e9e..206c536d3a 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,11 +3687,18 @@ 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_GUEST_ERROR until the deprecation of
> +     * MEM_UNPLUG_ERROR is due.
>       */
>      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_guest_error(!!dev->id, dev->id,
> +                                              dev->canonical_path);
>  }
>  
>  /* Callback to be called during DRC release. */
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index a4d9496f76..f8ac0a10df 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 */
> @@ -173,10 +175,9 @@ static uint32_t drc_unisolate_logical(SpaprDrc *drc)
>                               "for device %s", drc->dev->id);
>              }
>  
> -            /*
> -             * TODO: send a QAPI DEVICE_UNPLUG_ERROR event when
> -             * it is implemented.
> -             */
> +            qapi_event_send_device_unplug_guest_error(!!drc->dev->id,
> +                                                      drc->dev->id,
> +                                                      drc->dev->canonical_path);
>          }
>  
>          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 --]

[PATCH v7 7/7] memory_hotplug.c: send DEVICE_UNPLUG_GUEST_ERROR in acpi_memory_hotplug_write()

[Daniel Henrique Barboza]

MEM_UNPLUG_ERROR is deprecated since the introduction of
DEVICE_UNPLUG_GUEST_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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 6a71de408b..d0fffcf787 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"
@@ -178,8 +179,16 @@ 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);
+
+                /*
+                 * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_GUEST_ERROR
+                 * while the deprecation of MEM_UNPLUG_ERROR is
+                 * pending.
+                 */
                 qapi_event_send_mem_unplug_error(dev->id ? : "",
                                                  error_get_pretty(local_err));
+                qapi_event_send_device_unplug_guest_error(!!dev->id, dev->id,
+                                                          dev->canonical_path);
                 error_free(local_err);
                 break;
             }
-- 
2.31.1

Re: [PATCH v7 7/7] memory_hotplug.c: send DEVICE_UNPLUG_GUEST_ERROR in acpi_memory_hotplug_write()

[David Gibson]

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

On Tue, Aug 24, 2021 at 09:48:35PM -0300, Daniel Henrique Barboza wrote:
> MEM_UNPLUG_ERROR is deprecated since the introduction of
> DEVICE_UNPLUG_GUEST_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>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/acpi/memory_hotplug.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 6a71de408b..d0fffcf787 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"
> @@ -178,8 +179,16 @@ 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);
> +
> +                /*
> +                 * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_GUEST_ERROR
> +                 * while the deprecation of MEM_UNPLUG_ERROR is
> +                 * pending.
> +                 */
>                  qapi_event_send_mem_unplug_error(dev->id ? : "",
>                                                   error_get_pretty(local_err));
> +                qapi_event_send_device_unplug_guest_error(!!dev->id, dev->id,
> +                                                          dev->canonical_path);
>                  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 v7 7/7] memory_hotplug.c: send DEVICE_UNPLUG_GUEST_ERROR in acpi_memory_hotplug_write()

[Igor Mammedov]

On Tue, 24 Aug 2021 21:48:35 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> MEM_UNPLUG_ERROR is deprecated since the introduction of
> DEVICE_UNPLUG_GUEST_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>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/memory_hotplug.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 6a71de408b..d0fffcf787 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"
> @@ -178,8 +179,16 @@ 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);
> +
> +                /*
> +                 * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_GUEST_ERROR
> +                 * while the deprecation of MEM_UNPLUG_ERROR is
> +                 * pending.
> +                 */
>                  qapi_event_send_mem_unplug_error(dev->id ? : "",
>                                                   error_get_pretty(local_err));
> +                qapi_event_send_device_unplug_guest_error(!!dev->id, dev->id,
> +                                                          dev->canonical_path);
>                  error_free(local_err);
>                  break;
>              }

Re: [PATCH v7 7/7] memory_hotplug.c: send DEVICE_UNPLUG_GUEST_ERROR in acpi_memory_hotplug_write()

[Michael S. Tsirkin]

On Tue, Aug 24, 2021 at 09:48:35PM -0300, Daniel Henrique Barboza wrote:
> MEM_UNPLUG_ERROR is deprecated since the introduction of
> DEVICE_UNPLUG_GUEST_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>

feel free to include with the rest of the patchset

> ---
>  hw/acpi/memory_hotplug.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 6a71de408b..d0fffcf787 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"
> @@ -178,8 +179,16 @@ 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);
> +
> +                /*
> +                 * Send both MEM_UNPLUG_ERROR and DEVICE_UNPLUG_GUEST_ERROR
> +                 * while the deprecation of MEM_UNPLUG_ERROR is
> +                 * pending.
> +                 */
>                  qapi_event_send_mem_unplug_error(dev->id ? : "",
>                                                   error_get_pretty(local_err));
> +                qapi_event_send_device_unplug_guest_error(!!dev->id, dev->id,
> +                                                          dev->canonical_path);
>                  error_free(local_err);
>                  break;
>              }
> -- 
> 2.31.1

Re: [PATCH v7 0/7] DEVICE_UNPLUG_GUEST_ERROR QAPI event

[Markus Armbruster]

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

> Hi,
>
> In this version the event was renamed and the optional 'msg'
> attribute was removed. It also contains smaller changes based
> on Markus' comments in v6.

Looks neat now, thanks!

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

Re: [PATCH v7 0/7] DEVICE_UNPLUG_GUEST_ERROR QAPI event

[Markus Armbruster]

Not yet merged.  David, would you like to do the pull request?

Re: [PATCH v7 0/7] DEVICE_UNPLUG_GUEST_ERROR QAPI event

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Not yet merged.  David, would you like to do the pull request?

Oops, there's v8.  Take that one of course.

Re: [PATCH v7 0/7] DEVICE_UNPLUG_GUEST_ERROR QAPI event

[David Gibson]

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

On Wed, Sep 22, 2021 at 02:03:33PM +0200, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Not yet merged.  David, would you like to do the pull request?
> 
> Oops, there's v8.  Take that one of course.

I have it in ppc-for-6.2, probably looking at a PR next week.

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