| Message ID | 20240107-pvpanic-shutdown-v4-4-81500a7e4081@t-8ch.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/misc/pvpanic: add support for normal shutdowns | expand |
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>
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 &&
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
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"
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(-)