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

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

[Thomas Weißschuh]

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.

The background is the usage of minimal Linux kernels with different
architectures for testing purposes.
Poweroff support varies highly per architecture and requires a bunch of
code to be compiled to work.
pvpanic on the other hand is very small and uniform.

I sent an RFC[0] for this before to qemu-devel and lkml which didn't
generate feedback, so let's discuss the concrete proposal.

Patch 1 and 2 are general cleanups, that seems useful even without this
proposal being implemented.

A corresponding patch has been submitted for Linux [1].
This is also where the request was voiced to drop move away from a
pvpanic uapi header in Linux.

[0] https://lore.kernel.org/all/984794aa-4af0-4c68-a74e-7420ec3151a5@t-8ch.de/
[1] https://lore.kernel.org/lkml/20231104-pvpanic-shutdown-v1-1-5ee7c9b3e301@weissschuh.net/

Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de>
---
Changes in v4:
- Rebase on 8.2 master
- Resend after tree reopened and holidays
- Link to v3: https://lore.kernel.org/r/20231129-pvpanic-shutdown-v3-0-c9a2892fc523@t-8ch.de

Changes in v3:
- Drop from Linux imported pvpanic header as discussed with Cornelia and
  requested by Greg
- Link to v2: https://lore.kernel.org/r/20231128-pvpanic-shutdown-v2-0-830393b45cb6@t-8ch.de

Changes in v2:
- Remove RFC status
- Add Ack from Thomas to 2nd patch
- Fix typo in title of 2nd patch
- Link to v1: https://lore.kernel.org/r/20231104-pvpanic-shutdown-v1-0-02353157891b@t-8ch.de

---
Thomas Weißschuh (4):
      linux-headers: drop pvpanic.h
      hw/misc/pvpanic: centralize definition of supported events
      tests/qtest/pvpanic: use centralized definition of supported events
      hw/misc/pvpanic: add support for normal shutdowns

 docs/specs/pvpanic.rst                   | 2 ++
 hw/misc/pvpanic-isa.c                    | 3 +--
 hw/misc/pvpanic-pci.c                    | 3 +--
 hw/misc/pvpanic.c                        | 8 ++++++--
 include/hw/misc/pvpanic.h                | 5 +++++
 include/standard-headers/linux/pvpanic.h | 9 ---------
 scripts/update-linux-headers.sh          | 3 +--
 tests/qtest/pvpanic-pci-test.c           | 5 +++--
 tests/qtest/pvpanic-test.c               | 5 +++--
 9 files changed, 22 insertions(+), 21 deletions(-)
---
base-commit: 0c1eccd368af8805ec0fb11e6cf25d0684d37328
change-id: 20231104-pvpanic-shutdown-02e4b4cb4949

Best regards,
-- 
Thomas Weißschuh <thomas@t-8ch.de>

[PATCH v4 1/4] linux-headers: drop pvpanic.h

[Thomas Weißschuh]

misc/pvpanic.h from the Linux UAPI does not define a Linux UAPI but a
qemu device API.

This leads to a weird process when updates to the interface are needed:
1) Change to the specification in the qemu tree
2) Change to the header in the Linux tree
3) Re-import of the header into Qemu.

The kernel prefers to drop the header anyways.

Prepare for the removal from the Linux UAPI headers by moving the
contents to the existing pvpanic.h header.

Link: https://lore.kernel.org/lkml/2023110431-pacemaker-pruning-0e4c@gregkh/
Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de>
---
 hw/misc/pvpanic-isa.c                    | 1 -
 hw/misc/pvpanic-pci.c                    | 1 -
 hw/misc/pvpanic.c                        | 1 -
 include/hw/misc/pvpanic.h                | 3 +++
 include/standard-headers/linux/pvpanic.h | 9 ---------
 scripts/update-linux-headers.sh          | 3 +--
 6 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/hw/misc/pvpanic-isa.c b/hw/misc/pvpanic-isa.c
index ccec50f61bbd..ef438a31fbe9 100644
--- a/hw/misc/pvpanic-isa.c
+++ b/hw/misc/pvpanic-isa.c
@@ -21,7 +21,6 @@
 #include "hw/misc/pvpanic.h"
 #include "qom/object.h"
 #include "hw/isa/isa.h"
-#include "standard-headers/linux/pvpanic.h"
 #include "hw/acpi/acpi_aml_interface.h"
 
 OBJECT_DECLARE_SIMPLE_TYPE(PVPanicISAState, PVPANIC_ISA_DEVICE)
diff --git a/hw/misc/pvpanic-pci.c b/hw/misc/pvpanic-pci.c
index c01e4ce8646a..01e269b55284 100644
--- a/hw/misc/pvpanic-pci.c
+++ b/hw/misc/pvpanic-pci.c
@@ -21,7 +21,6 @@
 #include "hw/misc/pvpanic.h"
 #include "qom/object.h"
 #include "hw/pci/pci_device.h"
-#include "standard-headers/linux/pvpanic.h"
 
 OBJECT_DECLARE_SIMPLE_TYPE(PVPanicPCIState, PVPANIC_PCI_DEVICE)
 
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 1540e9091a45..4915ef256e74 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -21,7 +21,6 @@
 #include "hw/qdev-properties.h"
 #include "hw/misc/pvpanic.h"
 #include "qom/object.h"
-#include "standard-headers/linux/pvpanic.h"
 
 static void handle_event(int event)
 {
diff --git a/include/hw/misc/pvpanic.h b/include/hw/misc/pvpanic.h
index fab94165d03d..dffca827f77a 100644
--- a/include/hw/misc/pvpanic.h
+++ b/include/hw/misc/pvpanic.h
@@ -18,6 +18,9 @@
 #include "exec/memory.h"
 #include "qom/object.h"
 
+#define PVPANIC_PANICKED	(1 << 0)
+#define PVPANIC_CRASH_LOADED	(1 << 1)
+
 #define TYPE_PVPANIC_ISA_DEVICE "pvpanic"
 #define TYPE_PVPANIC_PCI_DEVICE "pvpanic-pci"
 
diff --git a/include/standard-headers/linux/pvpanic.h b/include/standard-headers/linux/pvpanic.h
deleted file mode 100644
index 54b7485390d3..000000000000
--- a/include/standard-headers/linux/pvpanic.h
+++ /dev/null
@@ -1,9 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
-
-#ifndef __PVPANIC_H__
-#define __PVPANIC_H__
-
-#define PVPANIC_PANICKED	(1 << 0)
-#define PVPANIC_CRASH_LOADED	(1 << 1)
-
-#endif /* __PVPANIC_H__ */
diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
index 34295c0fe55b..555bdc8af2eb 100755
--- a/scripts/update-linux-headers.sh
+++ b/scripts/update-linux-headers.sh
@@ -215,8 +215,7 @@ for i in "$tmpdir"/include/linux/*virtio*.h \
          "$tmpdir/include/linux/const.h" \
          "$tmpdir/include/linux/kernel.h" \
          "$tmpdir/include/linux/vhost_types.h" \
-         "$tmpdir/include/linux/sysinfo.h" \
-         "$tmpdir/include/misc/pvpanic.h"; do
+         "$tmpdir/include/linux/sysinfo.h"; do
     cp_portable "$i" "$output/include/standard-headers/linux"
 done
 mkdir -p "$output/include/standard-headers/drm"

-- 
2.43.0

Re: [PATCH v4 1/4] linux-headers: drop pvpanic.h

[Thomas Huth]

On 07/01/2024 15.05, Thomas Weißschuh wrote:
> misc/pvpanic.h from the Linux UAPI does not define a Linux UAPI but a
> qemu device API.
> 
> This leads to a weird process when updates to the interface are needed:
> 1) Change to the specification in the qemu tree
> 2) Change to the header in the Linux tree
> 3) Re-import of the header into Qemu.
> 
> The kernel prefers to drop the header anyways.
> 
> Prepare for the removal from the Linux UAPI headers by moving the
> contents to the existing pvpanic.h header.
> 
> Link: https://lore.kernel.org/lkml/2023110431-pacemaker-pruning-0e4c@gregkh/
> Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de>
> ---
>   hw/misc/pvpanic-isa.c                    | 1 -
>   hw/misc/pvpanic-pci.c                    | 1 -
>   hw/misc/pvpanic.c                        | 1 -
>   include/hw/misc/pvpanic.h                | 3 +++
>   include/standard-headers/linux/pvpanic.h | 9 ---------
>   scripts/update-linux-headers.sh          | 3 +--
>   6 files changed, 4 insertions(+), 14 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v4 1/4] linux-headers: drop pvpanic.h

[Cornelia Huck]

On Sun, Jan 07 2024, Thomas Weißschuh <thomas@t-8ch.de> wrote:

> misc/pvpanic.h from the Linux UAPI does not define a Linux UAPI but a
> qemu device API.
>
> This leads to a weird process when updates to the interface are needed:
> 1) Change to the specification in the qemu tree
> 2) Change to the header in the Linux tree
> 3) Re-import of the header into Qemu.
>
> The kernel prefers to drop the header anyways.
>
> Prepare for the removal from the Linux UAPI headers by moving the
> contents to the existing pvpanic.h header.
>
> Link: https://lore.kernel.org/lkml/2023110431-pacemaker-pruning-0e4c@gregkh/
> Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de>
> ---
>  hw/misc/pvpanic-isa.c                    | 1 -
>  hw/misc/pvpanic-pci.c                    | 1 -
>  hw/misc/pvpanic.c                        | 1 -
>  include/hw/misc/pvpanic.h                | 3 +++
>  include/standard-headers/linux/pvpanic.h | 9 ---------
>  scripts/update-linux-headers.sh          | 3 +--
>  6 files changed, 4 insertions(+), 14 deletions(-)

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

[PATCH v4 2/4] hw/misc/pvpanic: centralize definition of supported events

[Thomas Weißschuh]

The different components of pvpanic duplicate the list of supported
events. Move it to the shared header file to minimize changes when new
events are added.

Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de>
---
 hw/misc/pvpanic-isa.c     | 2 +-
 hw/misc/pvpanic-pci.c     | 2 +-
 hw/misc/pvpanic.c         | 2 +-
 include/hw/misc/pvpanic.h | 1 +
 4 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/misc/pvpanic-isa.c b/hw/misc/pvpanic-isa.c
index ef438a31fbe9..9a923b786907 100644
--- a/hw/misc/pvpanic-isa.c
+++ b/hw/misc/pvpanic-isa.c
@@ -101,7 +101,7 @@ static void build_pvpanic_isa_aml(AcpiDevAmlIf *adev, Aml *scope)
 static Property pvpanic_isa_properties[] = {
     DEFINE_PROP_UINT16(PVPANIC_IOPORT_PROP, PVPanicISAState, ioport, 0x505),
     DEFINE_PROP_UINT8("events", PVPanicISAState, pvpanic.events,
-                      PVPANIC_PANICKED | PVPANIC_CRASH_LOADED),
+                      PVPANIC_EVENTS),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/misc/pvpanic-pci.c b/hw/misc/pvpanic-pci.c
index 01e269b55284..be4063121e1d 100644
--- a/hw/misc/pvpanic-pci.c
+++ b/hw/misc/pvpanic-pci.c
@@ -54,7 +54,7 @@ static void pvpanic_pci_realizefn(PCIDevice *dev, Error **errp)
 
 static Property pvpanic_pci_properties[] = {
     DEFINE_PROP_UINT8("events", PVPanicPCIState, pvpanic.events,
-                      PVPANIC_PANICKED | PVPANIC_CRASH_LOADED),
+                      PVPANIC_EVENTS),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 4915ef256e74..a4982cc5928e 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -26,7 +26,7 @@ static void handle_event(int event)
 {
     static bool logged;
 
-    if (event & ~(PVPANIC_PANICKED | PVPANIC_CRASH_LOADED) && !logged) {
+    if (event & ~PVPANIC_EVENTS && !logged) {
         qemu_log_mask(LOG_GUEST_ERROR, "pvpanic: unknown event %#x.\n", event);
         logged = true;
     }
diff --git a/include/hw/misc/pvpanic.h b/include/hw/misc/pvpanic.h
index dffca827f77a..48f2ec4c86a1 100644
--- a/include/hw/misc/pvpanic.h
+++ b/include/hw/misc/pvpanic.h
@@ -20,6 +20,7 @@
 
 #define PVPANIC_PANICKED	(1 << 0)
 #define PVPANIC_CRASH_LOADED	(1 << 1)
+#define PVPANIC_EVENTS (PVPANIC_PANICKED | PVPANIC_CRASH_LOADED)
 
 #define TYPE_PVPANIC_ISA_DEVICE "pvpanic"
 #define TYPE_PVPANIC_PCI_DEVICE "pvpanic-pci"

-- 
2.43.0

Re: [PATCH v4 2/4] hw/misc/pvpanic: centralize definition of supported events

[Thomas Huth]

On 07/01/2024 15.05, Thomas Weißschuh wrote:
> The different components of pvpanic duplicate the list of supported
> events. Move it to the shared header file to minimize changes when new
> events are added.
> 
> Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de>
> ---
>   hw/misc/pvpanic-isa.c     | 2 +-
>   hw/misc/pvpanic-pci.c     | 2 +-
>   hw/misc/pvpanic.c         | 2 +-
>   include/hw/misc/pvpanic.h | 1 +
>   4 files changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH v4 2/4] hw/misc/pvpanic: centralize definition of supported events

[Cornelia Huck]

On Sun, Jan 07 2024, Thomas Weißschuh <thomas@t-8ch.de> wrote:

> The different components of pvpanic duplicate the list of supported
> events. Move it to the shared header file to minimize changes when new
> events are added.
>
> Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de>
> ---
>  hw/misc/pvpanic-isa.c     | 2 +-
>  hw/misc/pvpanic-pci.c     | 2 +-
>  hw/misc/pvpanic.c         | 2 +-
>  include/hw/misc/pvpanic.h | 1 +
>  4 files changed, 4 insertions(+), 3 deletions(-)

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

[PATCH v4 3/4] tests/qtest/pvpanic: use centralized definition of supported events

[Thomas Weißschuh]

Avoid the necessity to update all tests when new events are added
to the device.

Acked-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de>
---
 tests/qtest/pvpanic-pci-test.c | 5 +++--
 tests/qtest/pvpanic-test.c     | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/pvpanic-pci-test.c b/tests/qtest/pvpanic-pci-test.c
index 2c05b376ba72..b372caf41dc0 100644
--- a/tests/qtest/pvpanic-pci-test.c
+++ b/tests/qtest/pvpanic-pci-test.c
@@ -16,6 +16,7 @@
 #include "qapi/qmp/qdict.h"
 #include "libqos/pci.h"
 #include "libqos/pci-pc.h"
+#include "hw/misc/pvpanic.h"
 #include "hw/pci/pci_regs.h"
 
 static void test_panic_nopause(void)
@@ -34,7 +35,7 @@ static void test_panic_nopause(void)
     bar = qpci_iomap(dev, 0, NULL);
 
     qpci_memread(dev, bar, 0, &val, sizeof(val));
-    g_assert_cmpuint(val, ==, 3);
+    g_assert_cmpuint(val, ==, PVPANIC_EVENTS);
 
     val = 1;
     qpci_memwrite(dev, bar, 0, &val, sizeof(val));
@@ -67,7 +68,7 @@ static void test_panic(void)
     bar = qpci_iomap(dev, 0, NULL);
 
     qpci_memread(dev, bar, 0, &val, sizeof(val));
-    g_assert_cmpuint(val, ==, 3);
+    g_assert_cmpuint(val, ==, PVPANIC_EVENTS);
 
     val = 1;
     qpci_memwrite(dev, bar, 0, &val, sizeof(val));
diff --git a/tests/qtest/pvpanic-test.c b/tests/qtest/pvpanic-test.c
index 78f1cf8186b0..ccc603472f5d 100644
--- a/tests/qtest/pvpanic-test.c
+++ b/tests/qtest/pvpanic-test.c
@@ -10,6 +10,7 @@
 #include "qemu/osdep.h"
 #include "libqtest.h"
 #include "qapi/qmp/qdict.h"
+#include "hw/misc/pvpanic.h"
 
 static void test_panic_nopause(void)
 {
@@ -20,7 +21,7 @@ static void test_panic_nopause(void)
     qts = qtest_init("-device pvpanic -action panic=none");
 
     val = qtest_inb(qts, 0x505);
-    g_assert_cmpuint(val, ==, 3);
+    g_assert_cmpuint(val, ==, PVPANIC_EVENTS);
 
     qtest_outb(qts, 0x505, 0x1);
 
@@ -43,7 +44,7 @@ static void test_panic(void)
     qts = qtest_init("-device pvpanic -action panic=pause");
 
     val = qtest_inb(qts, 0x505);
-    g_assert_cmpuint(val, ==, 3);
+    g_assert_cmpuint(val, ==, PVPANIC_EVENTS);
 
     qtest_outb(qts, 0x505, 0x1);
 

-- 
2.43.0

Re: [PATCH v4 3/4] tests/qtest/pvpanic: use centralized definition of supported events

[Cornelia Huck]

On Sun, Jan 07 2024, Thomas Weißschuh <thomas@t-8ch.de> wrote:

> Avoid the necessity to update all tests when new events are added
> to the device.
>
> Acked-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de>
> ---
>  tests/qtest/pvpanic-pci-test.c | 5 +++--
>  tests/qtest/pvpanic-test.c     | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)

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

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

[Thomas Weißschuh]

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

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

[Cornelia Huck]

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>

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

[Alejandro Jimenez]

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

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

[Thomas Weißschuh]

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