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

diff --git a/docs/specs/pvpanic.rst b/docs/specs/pvpanic.rst
index f894bc19555f..796cc0348a38 100644
--- a/docs/specs/pvpanic.rst
+++ b/docs/specs/pvpanic.rst
@@ -29,6 +29,8 @@ bit 1
   a guest panic has happened and will be handled by the guest;
   the host should record it or report it, but should not affect
   the execution of the guest.
+bit 2
+  a guest shutdown has happened and should be processed by the host
 
 PCI Interface
 -------------
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);
+        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"

-- 
2.43.0

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

Hi Alejandro,

On 2024-01-26 13:47:33-0500, Alejandro Jimenez wrote:
> 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.

This sounds reasonable, thanks for the suggestion and patch.

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

I picked up the patch and will test and resend the series in a few days.

[snip]

If one of the maintainers reads this:

Maybe patch 1, 2 and 3 could already be picked up as they seem not to be
controversial.
Then I can also continue to remove the UAPI header on the kernel side.

Thomas

On Sun, Jan 07 2024, Thomas Weißschuh <thomas@t-8ch.de> 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(-)

Not deeply involved with this, but looks reasonable to me.

Acked-by: Cornelia Huck <cohuck@redhat.com>