[PATCH v2 0/3] hw/misc/pvpanic: add support for normal shutdowns

[PATCH v2 0/3] 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.

I'll send the corresponding Linux patch to LKML.

[0] https://lore.kernel.org/all/984794aa-4af0-4c68-a74e-7420ec3151a5@t-8ch.de/

Signed-off-by: Thomas Weißschuh <thomas@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 (3):
      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                | 2 ++
 include/standard-headers/linux/pvpanic.h | 1 +
 tests/qtest/pvpanic-pci-test.c           | 5 +++--
 tests/qtest/pvpanic-test.c               | 5 +++--
 8 files changed, 19 insertions(+), 10 deletions(-)
---
base-commit: 9155a938cf8fdcb29b760acb8a742bb48be9000f
change-id: 20231104-pvpanic-shutdown-02e4b4cb4949

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

[PATCH v2 1/3] 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     | 3 +--
 hw/misc/pvpanic-pci.c     | 3 +--
 hw/misc/pvpanic.c         | 3 +--
 include/hw/misc/pvpanic.h | 2 ++
 4 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/hw/misc/pvpanic-isa.c b/hw/misc/pvpanic-isa.c
index ccec50f61bbd..9a923b786907 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)
@@ -102,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 fbcaa50731b3..8898d280d2ef 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)
 
@@ -55,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 1540e9091a45..a4982cc5928e 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -21,13 +21,12 @@
 #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)
 {
     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 fab94165d03d..198047dc86ff 100644
--- a/include/hw/misc/pvpanic.h
+++ b/include/hw/misc/pvpanic.h
@@ -17,11 +17,13 @@
 
 #include "exec/memory.h"
 #include "qom/object.h"
+#include "standard-headers/linux/pvpanic.h"
 
 #define TYPE_PVPANIC_ISA_DEVICE "pvpanic"
 #define TYPE_PVPANIC_PCI_DEVICE "pvpanic-pci"
 
 #define PVPANIC_IOPORT_PROP "ioport"
+#define PVPANIC_EVENTS (PVPANIC_PANICKED | PVPANIC_CRASH_LOADED)
 
 /*
  * PVPanicState for any device type

-- 
2.43.0

[PATCH v2 2/3] 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

[PATCH v2 3/3] 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                | 2 +-
 include/standard-headers/linux/pvpanic.h | 1 +
 4 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 198047dc86ff..fa76ad93d998 100644
--- a/include/hw/misc/pvpanic.h
+++ b/include/hw/misc/pvpanic.h
@@ -23,7 +23,7 @@
 #define TYPE_PVPANIC_PCI_DEVICE "pvpanic-pci"
 
 #define PVPANIC_IOPORT_PROP "ioport"
-#define PVPANIC_EVENTS (PVPANIC_PANICKED | PVPANIC_CRASH_LOADED)
+#define PVPANIC_EVENTS (PVPANIC_PANICKED | PVPANIC_CRASH_LOADED | PVPANIC_SHUTDOWN)
 
 /*
  * PVPanicState for any device type
diff --git a/include/standard-headers/linux/pvpanic.h b/include/standard-headers/linux/pvpanic.h
index 54b7485390d3..38e53ad45929 100644
--- a/include/standard-headers/linux/pvpanic.h
+++ b/include/standard-headers/linux/pvpanic.h
@@ -5,5 +5,6 @@
 
 #define PVPANIC_PANICKED	(1 << 0)
 #define PVPANIC_CRASH_LOADED	(1 << 1)
+#define PVPANIC_SHUTDOWN       	(1 << 2)
 
 #endif /* __PVPANIC_H__ */

-- 
2.43.0

Re: [PATCH v2 3/3] hw/misc/pvpanic: add support for normal shutdowns

[Cornelia Huck]

On Tue, Nov 28 2023, 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                | 2 +-
>  include/standard-headers/linux/pvpanic.h | 1 +
>  4 files changed, 9 insertions(+), 1 deletion(-)

(...)

> diff --git a/include/standard-headers/linux/pvpanic.h b/include/standard-headers/linux/pvpanic.h
> index 54b7485390d3..38e53ad45929 100644
> --- a/include/standard-headers/linux/pvpanic.h
> +++ b/include/standard-headers/linux/pvpanic.h
> @@ -5,5 +5,6 @@
>  
>  #define PVPANIC_PANICKED	(1 << 0)
>  #define PVPANIC_CRASH_LOADED	(1 << 1)
> +#define PVPANIC_SHUTDOWN       	(1 << 2)
>  
>  #endif /* __PVPANIC_H__ */
>

This hunk needs to come in via a separate headers update, or has to be
split out into a placeholder patch if it is not included in the Linux
kernel yet.

Re: [PATCH v2 3/3] hw/misc/pvpanic: add support for normal shutdowns

[Thomas Weißschuh]

On 2023-11-29 09:23:46+0100, Cornelia Huck wrote:
> On Tue, Nov 28 2023, 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                | 2 +-
> >  include/standard-headers/linux/pvpanic.h | 1 +
> >  4 files changed, 9 insertions(+), 1 deletion(-)
> 
> (...)
> 
> > diff --git a/include/standard-headers/linux/pvpanic.h b/include/standard-headers/linux/pvpanic.h
> > index 54b7485390d3..38e53ad45929 100644
> > --- a/include/standard-headers/linux/pvpanic.h
> > +++ b/include/standard-headers/linux/pvpanic.h
> > @@ -5,5 +5,6 @@
> >  
> >  #define PVPANIC_PANICKED	(1 << 0)
> >  #define PVPANIC_CRASH_LOADED	(1 << 1)
> > +#define PVPANIC_SHUTDOWN       	(1 << 2)
> >  
> >  #endif /* __PVPANIC_H__ */
> >
> 
> This hunk needs to come in via a separate headers update, or has to be
> split out into a placeholder patch if it is not included in the Linux
> kernel yet.

Greg KH actually want this header removed from the Linux UAPI headers,
as it is not in fact a Linux UAPI [0].
It's also a weird workflow to have the specification in qemu but the
header as part of Linux that is re-imported in qemu.

What do you think about maintaining the header as a private part of qemu
and dropping it from Linux UAPI?

Contrary to my response to Greg this wouldn't break old versions of
qemu, as qemu is using a private copy that would still exist there.

[0] https://lore.kernel.org/lkml/2023110431-pacemaker-pruning-0e4c@gregkh/

Re: [PATCH v2 3/3] hw/misc/pvpanic: add support for normal shutdowns

[Cornelia Huck]

On Wed, Nov 29 2023, Thomas Weißschuh <thomas@t-8ch.de> wrote:
> On 2023-11-29 09:23:46+0100, Cornelia Huck wrote:
>> On Tue, Nov 28 2023, Thomas Weißschuh <thomas@t-8ch.de> wrote:
>> > diff --git a/include/standard-headers/linux/pvpanic.h b/include/standard-headers/linux/pvpanic.h
>> > index 54b7485390d3..38e53ad45929 100644
>> > --- a/include/standard-headers/linux/pvpanic.h
>> > +++ b/include/standard-headers/linux/pvpanic.h
>> > @@ -5,5 +5,6 @@
>> >  
>> >  #define PVPANIC_PANICKED	(1 << 0)
>> >  #define PVPANIC_CRASH_LOADED	(1 << 1)
>> > +#define PVPANIC_SHUTDOWN       	(1 << 2)
>> >  
>> >  #endif /* __PVPANIC_H__ */
>> >
>> 
>> This hunk needs to come in via a separate headers update, or has to be
>> split out into a placeholder patch if it is not included in the Linux
>> kernel yet.
>
> Greg KH actually want this header removed from the Linux UAPI headers,
> as it is not in fact a Linux UAPI [0].
> It's also a weird workflow to have the specification in qemu but the
> header as part of Linux that is re-imported in qemu.
>
> What do you think about maintaining the header as a private part of qemu
> and dropping it from Linux UAPI?
>
> Contrary to my response to Greg this wouldn't break old versions of
> qemu, as qemu is using a private copy that would still exist there.
>
> [0] https://lore.kernel.org/lkml/2023110431-pacemaker-pruning-0e4c@gregkh/

Hm... we have a bunch of examples where we use things exported via the
Linux uapi header files that are not a kernel<->userspace interface, but
rather a host<->guest interface (sometimes defining the interface,
sometimes more as a convenience mechanism). I agree that this is not
quite what the Linux uapi is supposed to be (and yes, it's weird), but
we've being doing that for many years now and changing it would be a
non-zero effort (and we'd have to figure out another way to make sure
the kernel and QEMU do not diverge if there's no authorative third party
around.)

In the case of the pvpanic device, this seems manageable, though; if we
decide to go that way, we should

1. copy the header on the QEMU side somewhere else under include/ and
   remove it from the header update script
2. wait until this hits QEMU mainline (so nobody will try to run the old
   update script)
3. move the uapi file on the Linux side

(We've had changes in the kernel break the update script before, but if
we can do it more smoothly, I'd prefer that way -- the kernel merge
window won't open before the new year anyway, and by that time, we'll
have the QEMU tree open again.)

Main downside is that you'd have extra hassle for something that looks
like a straightforward feature, which is not ideal. (Also, are we sure
that nobody else consumes that header file?)

I'm not sure if dealing with the other host<->guest interfaces that get
copied over is worth the effort, though...

Opinions?

Re: [PATCH v2 3/3] hw/misc/pvpanic: add support for normal shutdowns

[Thomas Weißschuh]

On 2023-11-29 14:15:14+0100, Cornelia Huck wrote:
> On Wed, Nov 29 2023, Thomas Weißschuh <thomas@t-8ch.de> wrote:
> > On 2023-11-29 09:23:46+0100, Cornelia Huck wrote:
> >> On Tue, Nov 28 2023, Thomas Weißschuh <thomas@t-8ch.de> wrote:
> >> > diff --git a/include/standard-headers/linux/pvpanic.h b/include/standard-headers/linux/pvpanic.h
> >> > index 54b7485390d3..38e53ad45929 100644
> >> > --- a/include/standard-headers/linux/pvpanic.h
> >> > +++ b/include/standard-headers/linux/pvpanic.h
> >> > @@ -5,5 +5,6 @@
> >> >  
> >> >  #define PVPANIC_PANICKED	(1 << 0)
> >> >  #define PVPANIC_CRASH_LOADED	(1 << 1)
> >> > +#define PVPANIC_SHUTDOWN       	(1 << 2)
> >> >  
> >> >  #endif /* __PVPANIC_H__ */
> >> >
> >> 
> >> This hunk needs to come in via a separate headers update, or has to be
> >> split out into a placeholder patch if it is not included in the Linux
> >> kernel yet.
> >
> > Greg KH actually want this header removed from the Linux UAPI headers,
> > as it is not in fact a Linux UAPI [0].
> > It's also a weird workflow to have the specification in qemu but the
> > header as part of Linux that is re-imported in qemu.
> >
> > What do you think about maintaining the header as a private part of qemu
> > and dropping it from Linux UAPI?
> >
> > Contrary to my response to Greg this wouldn't break old versions of
> > qemu, as qemu is using a private copy that would still exist there.
> >
> > [0] https://lore.kernel.org/lkml/2023110431-pacemaker-pruning-0e4c@gregkh/
> 
> Hm... we have a bunch of examples where we use things exported via the
> Linux uapi header files that are not a kernel<->userspace interface, but
> rather a host<->guest interface (sometimes defining the interface,
> sometimes more as a convenience mechanism). I agree that this is not
> quite what the Linux uapi is supposed to be (and yes, it's weird), but
> we've being doing that for many years now and changing it would be a
> non-zero effort (and we'd have to figure out another way to make sure
> the kernel and QEMU do not diverge if there's no authorative third party
> around.)
> 
> In the case of the pvpanic device, this seems manageable, though; if we
> decide to go that way, we should
> 
> 1. copy the header on the QEMU side somewhere else under include/ and
>    remove it from the header update script

There is already include/hw/misc/pvpanic.h which seems to be the best
place.

> 2. wait until this hits QEMU mainline (so nobody will try to run the old
>    update script)
> 3. move the uapi file on the Linux side
> 
> (We've had changes in the kernel break the update script before, but if
> we can do it more smoothly, I'd prefer that way -- the kernel merge
> window won't open before the new year anyway, and by that time, we'll
> have the QEMU tree open again.)

The kernel side isn't urgent anyways.

> Main downside is that you'd have extra hassle for something that looks
> like a straightforward feature, which is not ideal. (Also, are we sure
> that nobody else consumes that header file?)

Otherwise I have the hassle on the kernel side :-)

Debian codesearch did not find other users.

> I'm not sure if dealing with the other host<->guest interfaces that get
> copied over is worth the effort, though...
> 
> Opinions?

I'll resend a new revision that drops the import this evening, if
nothing new comes up.