Re: [PATCH v4 4/4] hw/misc/pvpanic: add support for normal shutdowns

[Alejandro Jimenez <alejandro.j.jimenez@oracle.com>]

Hi Thomas,

On 1/7/24 09:05, Thomas Weißschuh wrote:
> Shutdown requests are normally hardware dependent.
> By extending pvpanic to also handle shutdown requests, guests can
> submit such requests with an easily implementable and cross-platform
> mechanism.
> 
> Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de>
> ---
>   docs/specs/pvpanic.rst    | 2 ++
>   hw/misc/pvpanic.c         | 5 +++++
>   include/hw/misc/pvpanic.h | 3 ++-
>   3 files changed, 9 insertions(+), 1 deletion(-)
> 
[snip]

>   -------------
> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> index a4982cc5928e..246f9ae4e992 100644
> --- a/hw/misc/pvpanic.c
> +++ b/hw/misc/pvpanic.c
> @@ -40,6 +40,11 @@ static void handle_event(int event)
>           qemu_system_guest_crashloaded(NULL);
>           return;
>       }
> +
> +    if (event & PVPANIC_SHUTDOWN) {
> +        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);

I would suggest that instead of directly requesting a system shutdown, we should follow the same convention/handling of the other pvpanic events and emit a QMP message signaling the specific event that took place, to help a management layer application that might be listening to determine the cause of the shutdown. It can also be a helpful signal to let us know if a guest is (ab)using the new functionality.

If you agree with my reasoning and you'd allow me to piggyback on your series, please add my complementary [PATCH 5/4] change that implements the suggestion:

--

 From da4355344771206b69fc97d40ae9cc6510239e14 Mon Sep 17 00:00:00 2001
From: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>
Date: Fri, 26 Jan 2024 17:54:16 +0000
Subject: [PATCH 5/4] pvpanic: Emit GUEST_PVSHUTDOWN QMP event on pvpanic
  shutdown signal

Emit a QMP event on receiving a PVPANIC_SHUTDOWN event. Even though a typical
SHUTDOWN event will be sent, it will be indistinguishable from a shutdown
originating from other cases (e.g. KVM exit due to KVM_SYSTEM_EVENT_SHUTDOWN)
that also issue the guest-shutdown cause.
A management layer application can detect the new GUEST_PVSHUTDOWN event to
determine if the guest is using the pvpanic interface to request shutdowns.

Signed-off-by: Alejandro Jimenez <alejandro.j.jimenez@oracle.com>

---
To be applied on top of the series:
hw/misc/pvpanic: add support for normal shutdowns
https://lore.kernel.org/all/20240107-pvpanic-shutdown-v4-0-81500a7e4081@t-8ch.de/#t
---
  hw/misc/pvpanic.c         |  2 +-
  include/sysemu/runstate.h |  1 +
  qapi/run-state.json       | 14 ++++++++++++++
  system/runstate.c         |  6 ++++++
  4 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 246f9ae4e9..24001b9437 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -42,7 +42,7 @@ static void handle_event(int event)
      }
  
      if (event & PVPANIC_SHUTDOWN) {
-        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+	qemu_system_guest_pvshutdown();
          return;
      }
  }
diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
index 0117d243c4..e210a37abf 100644
--- a/include/sysemu/runstate.h
+++ b/include/sysemu/runstate.h
@@ -104,6 +104,7 @@ void qemu_system_killed(int signal, pid_t pid);
  void qemu_system_reset(ShutdownCause reason);
  void qemu_system_guest_panicked(GuestPanicInformation *info);
  void qemu_system_guest_crashloaded(GuestPanicInformation *info);
+void qemu_system_guest_pvshutdown(void);
  bool qemu_system_dump_in_progress(void);
  
  #endif
diff --git a/qapi/run-state.json b/qapi/run-state.json
index 08bc99cb85..d5a63e14ba 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -460,6 +460,20 @@
  { 'event': 'GUEST_CRASHLOADED',
    'data': { 'action': 'GuestPanicAction', '*info': 'GuestPanicInformation' } }
  
+##
+# @GUEST_PVSHUTDOWN:
+#
+# Emitted when guest submits a shutdown request via pvpanic interface
+#
+# Since: 8.3
+#
+# Example:
+#
+# <- { "event": "GUEST_PVSHUTDOWN",
+#      "timestamp": { "seconds": 1648245259, "microseconds": 893771 } }
+##
+{ 'event': 'GUEST_PVSHUTDOWN' }
+
  ##
  # @GuestPanicAction:
  #
diff --git a/system/runstate.c b/system/runstate.c
index d6ab860eca..02b0a1f8b9 100644
--- a/system/runstate.c
+++ b/system/runstate.c
@@ -572,6 +572,12 @@ void qemu_system_guest_crashloaded(GuestPanicInformation *info)
      qapi_free_GuestPanicInformation(info);
  }
  
+void qemu_system_guest_pvshutdown(void)
+{
+    qapi_event_send_guest_pvshutdown();
+    qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+}
+
  void qemu_system_reset_request(ShutdownCause reason)
  {
      if (reboot_action == REBOOT_ACTION_SHUTDOWN &&
-- 
2.39.3


> +        return;
> +    }
>   }
>   
>   /* return supported events on read */
> diff --git a/include/hw/misc/pvpanic.h b/include/hw/misc/pvpanic.h
> index 48f2ec4c86a1..9e36a02d5a4f 100644
> --- a/include/hw/misc/pvpanic.h
> +++ b/include/hw/misc/pvpanic.h
> @@ -20,7 +20,8 @@
>   
>   #define PVPANIC_PANICKED	(1 << 0)
>   #define PVPANIC_CRASH_LOADED	(1 << 1)
> -#define PVPANIC_EVENTS (PVPANIC_PANICKED | PVPANIC_CRASH_LOADED)
> +#define PVPANIC_SHUTDOWN	(1 << 2)
> +#define PVPANIC_EVENTS (PVPANIC_PANICKED | PVPANIC_CRASH_LOADED | PVPANIC_SHUTDOWN)
>   
>   #define TYPE_PVPANIC_ISA_DEVICE "pvpanic"
>   #define TYPE_PVPANIC_PCI_DEVICE "pvpanic-pci"
>