All of lore.kernel.org
 help / color / mirror / Atom feed
From: Markus Armbruster <armbru@redhat.com>
To: Daniel Henrique Barboza <danielhb413@gmail.com>
Cc: eblake@redhat.com, groug@kaod.org, qemu-ppc@nongnu.org,
	qemu-devel@nongnu.org, david@gibson.dropbear.id.au
Subject: Re: [PATCH v4 1/3] qapi/qdev.json: add DEVICE_UNPLUG_ERROR QAPI event
Date: Thu, 08 Jul 2021 15:01:20 +0200	[thread overview]
Message-ID: <87r1g96jtr.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20210707003314.37110-2-danielhb413@gmail.com> (Daniel Henrique Barboza's message of "Tue, 6 Jul 2021 21:33:12 -0300")

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

> At this moment we only provide one event to report a hotunplug error,
> MEM_UNPLUG_ERROR. As of Linux kernel 5.12 and QEMU 6.0.0, the pseries
> machine is now able to report unplug errors for other device types, such
> as CPUs.
>
> Instead of creating a (device_type)_UNPLUG_ERROR for each new device,
> create a generic DEVICE_UNPLUG_ERROR event that can be used by all
> unplug errors in the future.
>
> With this new generic event, MEM_UNPLUG_ERROR is now marked as deprecated.
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  docs/system/deprecated.rst | 10 ++++++++++
>  qapi/machine.json          |  6 +++++-
>  qapi/qdev.json             | 27 ++++++++++++++++++++++++++-
>  3 files changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
> index 70e08baff6..ca6c7f9d43 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.1)
> +''''''''''''''''''''''''''''''''''''''''''''''''''''''''
> +
> +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 b83178220b..349d7439fa 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.

"If an error ... is detected" kind of implies that some errors may go
undetected.  Let's spell this out more clearly.  Perhaps append "Some
errors cannot be detected."

DEVICE_UNPLUG_ERROR's unrelability is awkward.  Best we can do in the
general case.  Can we do better in special cases, and would it be
worthwhile?  If yes, it should probably be done on top.

Two spaces between sentences for consistency with the existing text, please.

>  #
>  # Since: 0.14
>  #
> @@ -124,3 +126,26 @@
>  ##
>  { 'event': 'DEVICE_DELETED',
>    'data': { '*device': 'str', 'path': 'str' } }
> +
> +##
> +# @DEVICE_UNPLUG_ERROR:
> +#
> +# Emitted when a device hot unplug error occurs.
> +#
> +# @device: device name
> +#
> +# @msg: Informative message
> +#
> +# Since: 6.1
> +#
> +# Example:
> +#
> +# <- { "event": "DEVICE_UNPLUG_ERROR"
> +#      "data": { "device": "dimm1",
> +#                "msg": "Memory hotunplug rejected by the guest for device dimm1"
> +#      },
> +#      "timestamp": { "seconds": 1615570772, "microseconds": 202844 } }
> +#
> +##
> +{ 'event': 'DEVICE_UNPLUG_ERROR',
> +  'data': { 'device': 'str', 'msg': 'str' } }

Hmm.

DEVICE_DELETED provides the device ID if the device has one, and the QOM
path.  Documentation is less than clear for both (not your patch's
fault).

DEVICE_UNPLUG_ERROR provides the device ID unconditionally, and doesn't
provide the QOM path.  What if the device doesn't have a device ID?

I suspect DEVICE_UNPLUG_ERROR should match DEVICE_DELETED exactly.

Bonus (for me, not for you): improving the unclear documentation becomes
your patch's problem.  Here's my attempt:

   # @device: the device's ID if it has one
   #
   # @path: the device's path within the object model



  parent reply	other threads:[~2021-07-08 13:08 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-07  0:33 [PATCH v4 0/3] DEVICE_UNPLUG_ERROR QAPI event Daniel Henrique Barboza
2021-07-07  0:33 ` [PATCH v4 1/3] qapi/qdev.json: add " Daniel Henrique Barboza
2021-07-07  9:26   ` Greg Kurz
2021-07-08 13:01   ` Markus Armbruster [this message]
2021-07-08 14:20     ` Daniel Henrique Barboza
2021-07-12  2:26     ` David Gibson
2021-07-07  0:33 ` [PATCH v4 2/3] spapr: use DEVICE_UNPLUG_ERROR to report unplug errors Daniel Henrique Barboza
2021-07-07  9:28   ` Greg Kurz
2021-07-08 13:08   ` Markus Armbruster
2021-07-07  0:33 ` [PATCH v4 3/3] memory_hotplug.c: send DEVICE_UNPLUG_ERROR in acpi_memory_hotplug_write() Daniel Henrique Barboza
2021-07-07  9:28   ` Greg Kurz
2021-07-08 13:08   ` Markus Armbruster
2021-07-09  8:39     ` Igor Mammedov
2021-07-09 11:25       ` Markus Armbruster
2021-07-09 13:38         ` Igor Mammedov
2021-07-10  6:57           ` Markus Armbruster
2021-07-11  8:49             ` Daniel Henrique Barboza
2021-07-08  0:49 ` [PATCH v4 0/3] DEVICE_UNPLUG_ERROR QAPI event David Gibson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87r1g96jtr.fsf@dusky.pond.sub.org \
    --to=armbru@redhat.com \
    --cc=danielhb413@gmail.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=eblake@redhat.com \
    --cc=groug@kaod.org \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.