[libvirt] [PATCH 2/2] pvpanic: implement crashloaded event handling

zhenwei pi posted 2 patches 6 weeks ago

[libvirt] [PATCH 2/2] pvpanic: implement crashloaded event handling

Posted by zhenwei pi 6 weeks ago
Handle bit 1 write, then post event to monitor.

Suggested by Paolo, declear a new event, using GUEST_PANICKED could
cause upper layers to react by shutting down or rebooting the guest.

In advance for extention, add GuestPanicInformation in event message.

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 hw/misc/pvpanic.c         | 11 +++++++++--
 include/sysemu/runstate.h |  1 +
 qapi/run-state.json       | 22 +++++++++++++++++++++-
 vl.c                      | 12 ++++++++++++
 4 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index d65ac86478..4ebda7872a 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -21,11 +21,13 @@
 #include "hw/qdev-properties.h"
 #include "hw/misc/pvpanic.h"
 
-/* The bit of supported pv event */
+/* The bit of supported pv event, TODO: include uapi header and remove this */
 #define PVPANIC_F_PANICKED      0
+#define PVPANIC_F_CRASHLOADED   1
 
 /* The pv event value */
 #define PVPANIC_PANICKED        (1 << PVPANIC_F_PANICKED)
+#define PVPANIC_CRASHLOADED     (1 << PVPANIC_F_CRASHLOADED)
 
 #define ISA_PVPANIC_DEVICE(obj)    \
     OBJECT_CHECK(PVPanicState, (obj), TYPE_PVPANIC)
@@ -34,7 +36,7 @@ static void handle_event(int event)
 {
     static bool logged;
 
-    if (event & ~PVPANIC_PANICKED && !logged) {
+    if (event & ~(PVPANIC_PANICKED | PVPANIC_CRASHLOADED) && !logged) {
         qemu_log_mask(LOG_GUEST_ERROR, "pvpanic: unknown event %#x.\n", event);
         logged = true;
     }
@@ -43,6 +45,11 @@ static void handle_event(int event)
         qemu_system_guest_panicked(NULL);
         return;
     }
+
+    if (event & PVPANIC_CRASHLOADED) {
+        qemu_system_guest_crashloaded(NULL);
+        return;
+    }
 }
 
 #include "hw/isa/isa.h"
diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
index 0b41555609..f760094858 100644
--- a/include/sysemu/runstate.h
+++ b/include/sysemu/runstate.h
@@ -63,6 +63,7 @@ ShutdownCause qemu_reset_requested_get(void);
 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);
 
 #endif
 
diff --git a/qapi/run-state.json b/qapi/run-state.json
index d7477cd715..b7a91f3125 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -357,6 +357,26 @@
   'data': { 'action': 'GuestPanicAction', '*info': 'GuestPanicInformation' } }
 
 ##
+# @GUEST_CRASHLOADED:
+#
+# Emitted when guest OS crash loaded is detected
+#
+# @action: action that has been taken, currently always "run"
+#
+# @info: information about a panic (since 2.9)
+#
+# Since: 5.0
+#
+# Example:
+#
+# <- { "event": "GUEST_CRASHLOADED",
+#      "data": { "action": "run" } }
+#
+##
+{ 'event': 'GUEST_CRASHLOADED',
+  'data': { 'action': 'GuestPanicAction', '*info': 'GuestPanicInformation' } }
+
+##
 # @GuestPanicAction:
 #
 # An enumeration of the actions taken when guest OS panic is detected
@@ -366,7 +386,7 @@
 # Since: 2.1 (poweroff since 2.8)
 ##
 { 'enum': 'GuestPanicAction',
-  'data': [ 'pause', 'poweroff' ] }
+  'data': [ 'pause', 'poweroff', 'run' ] }
 
 ##
 # @GuestPanicInformationType:
diff --git a/vl.c b/vl.c
index 86474a55c9..5b1b2ef095 100644
--- a/vl.c
+++ b/vl.c
@@ -1468,6 +1468,18 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
     }
 }
 
+void qemu_system_guest_crashloaded(GuestPanicInformation *info)
+{
+    qemu_log_mask(LOG_GUEST_ERROR, "Guest crash loaded");
+
+    qapi_event_send_guest_crashloaded(GUEST_PANIC_ACTION_RUN,
+                                   !!info, info);
+
+    if (info) {
+        qapi_free_GuestPanicInformation(info);
+    }
+}
+
 void qemu_system_reset_request(ShutdownCause reason)
 {
     if (no_reboot && reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) {
-- 
2.11.0


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [PATCH 2/2] pvpanic: implement crashloaded event handling

Posted by Markus Armbruster 4 weeks ago
zhenwei pi <pizhenwei@bytedance.com> writes:

> Handle bit 1 write, then post event to monitor.
>
> Suggested by Paolo, declear a new event, using GUEST_PANICKED could
> cause upper layers to react by shutting down or rebooting the guest.
>
> In advance for extention, add GuestPanicInformation in event message.
>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  hw/misc/pvpanic.c         | 11 +++++++++--
>  include/sysemu/runstate.h |  1 +
>  qapi/run-state.json       | 22 +++++++++++++++++++++-
>  vl.c                      | 12 ++++++++++++
>  4 files changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
> index d65ac86478..4ebda7872a 100644
> --- a/hw/misc/pvpanic.c
> +++ b/hw/misc/pvpanic.c
> @@ -21,11 +21,13 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/misc/pvpanic.h"
>  
> -/* The bit of supported pv event */
> +/* The bit of supported pv event, TODO: include uapi header and remove this */
>  #define PVPANIC_F_PANICKED      0
> +#define PVPANIC_F_CRASHLOADED   1
>  
>  /* The pv event value */
>  #define PVPANIC_PANICKED        (1 << PVPANIC_F_PANICKED)
> +#define PVPANIC_CRASHLOADED     (1 << PVPANIC_F_CRASHLOADED)
>  
>  #define ISA_PVPANIC_DEVICE(obj)    \
>      OBJECT_CHECK(PVPanicState, (obj), TYPE_PVPANIC)
> @@ -34,7 +36,7 @@ static void handle_event(int event)
>  {
>      static bool logged;
>  
> -    if (event & ~PVPANIC_PANICKED && !logged) {
> +    if (event & ~(PVPANIC_PANICKED | PVPANIC_CRASHLOADED) && !logged) {
>          qemu_log_mask(LOG_GUEST_ERROR, "pvpanic: unknown event %#x.\n", event);
>          logged = true;
>      }
> @@ -43,6 +45,11 @@ static void handle_event(int event)
>          qemu_system_guest_panicked(NULL);
>          return;
>      }
> +
> +    if (event & PVPANIC_CRASHLOADED) {
> +        qemu_system_guest_crashloaded(NULL);
> +        return;
> +    }
>  }

Okay.  Possible simplification:

   static void handle_event(int event)
   {
       static bool logged;

       if (event & PVPANIC_PANICKED) {
           qemu_system_guest_panicked(NULL);
           return;
       }

       if (event & PVPANIC_CRASHLOADED) {
           qemu_system_guest_crashloaded(NULL);
           return;
       }

       if (!logged) {
           qemu_log_mask(LOG_GUEST_ERROR, "pvpanic: unknown event %#x.\n", event);
           logged = true;
       }
   }

>  
>  #include "hw/isa/isa.h"
> diff --git a/include/sysemu/runstate.h b/include/sysemu/runstate.h
> index 0b41555609..f760094858 100644
> --- a/include/sysemu/runstate.h
> +++ b/include/sysemu/runstate.h
> @@ -63,6 +63,7 @@ ShutdownCause qemu_reset_requested_get(void);
>  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);
>  
>  #endif
>  
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index d7477cd715..b7a91f3125 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -357,6 +357,26 @@
   ##
   # @GUEST_PANICKED:
   #
   # Emitted when guest OS panic is detected
   #
   # @action: action that has been taken, currently always "pause"

Not this patch's problem, but here goes anyway: 'currently always
"pause"' is wrong since 864111f422 "vl: exit qemu on guest panic if
-no-shutdown is not set".  See [*] below.

   #
   # @info: information about a panic (since 2.9)
   #
   # Since: 1.5
   #
   # Example:
   #
   # <- { "event": "GUEST_PANICKED",
   #      "data": { "action": "pause" } }
   #
   ##
   { 'event': 'GUEST_PANICKED',
>    'data': { 'action': 'GuestPanicAction', '*info': 'GuestPanicInformation' } }
>  
>  ##
> +# @GUEST_CRASHLOADED:
> +#
> +# Emitted when guest OS crash loaded is detected
> +#
> +# @action: action that has been taken, currently always "run"
> +#
> +# @info: information about a panic (since 2.9)
> +#
> +# Since: 5.0
> +#
> +# Example:
> +#
> +# <- { "event": "GUEST_CRASHLOADED",
> +#      "data": { "action": "run" } }
> +#
> +##
> +{ 'event': 'GUEST_CRASHLOADED',
> +  'data': { 'action': 'GuestPanicAction', '*info': 'GuestPanicInformation' } }
> +
> +##
>  # @GuestPanicAction:
>  #
>  # An enumeration of the actions taken when guest OS panic is detected
> @@ -366,7 +386,7 @@
>  # Since: 2.1 (poweroff since 2.8)
>  ##
>  { 'enum': 'GuestPanicAction',
> -  'data': [ 'pause', 'poweroff' ] }
> +  'data': [ 'pause', 'poweroff', 'run' ] }

We now have

    event               action
    GUEST_PANICKED      pause
    GUEST_PANICKED      poweroff
    GUEST_CRASHLOADED   run

Why have two events?

If there's a reason for two, why have their actions mixed up in a single
enum?

>  
>  ##
>  # @GuestPanicInformationType:
> diff --git a/vl.c b/vl.c
> index 86474a55c9..5b1b2ef095 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1468,6 +1468,18 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
   void qemu_system_guest_panicked(GuestPanicInformation *info)
   {
       qemu_log_mask(LOG_GUEST_ERROR, "Guest crashed");

       if (current_cpu) {
           current_cpu->crash_occurred = true;
       }
       qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_PAUSE,
                                      !!info, info);
       vm_stop(RUN_STATE_GUEST_PANICKED);
       if (!no_shutdown) {

[*] Here, we send event GUEST_PANICKED with action "poweroff":

           qapi_event_send_guest_panicked(GUEST_PANIC_ACTION_POWEROFF,
                                          !!info, info);
           qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_PANIC);
       }

Note that we send *two* GUEST_PANICKED events for the same guest panic,
one with action "pause", and a second one with action "poweroff".
Doesn't feel right to me.

Not this patch's problem, of course.

       if (info) {
           if (info->type == GUEST_PANIC_INFORMATION_TYPE_HYPER_V) {
               qemu_log_mask(LOG_GUEST_ERROR, "\nHV crash parameters: (%#"PRIx64
                             " %#"PRIx64" %#"PRIx64" %#"PRIx64" %#"PRIx64")\n",
                             info->u.hyper_v.arg1,
                             info->u.hyper_v.arg2,
                             info->u.hyper_v.arg3,
                             info->u.hyper_v.arg4,
                             info->u.hyper_v.arg5);
           } else if (info->type == GUEST_PANIC_INFORMATION_TYPE_S390) {
               qemu_log_mask(LOG_GUEST_ERROR, " on cpu %d: %s\n"
                             "PSW: 0x%016" PRIx64 " 0x%016" PRIx64"\n",
                             info->u.s390.core,
                             S390CrashReason_str(info->u.s390.reason),
                             info->u.s390.psw_mask,
                             info->u.s390.psw_addr);
           }
           qapi_free_GuestPanicInformation(info);
>      }
>  }
>  
> +void qemu_system_guest_crashloaded(GuestPanicInformation *info)
> +{
> +    qemu_log_mask(LOG_GUEST_ERROR, "Guest crash loaded");
> +
> +    qapi_event_send_guest_crashloaded(GUEST_PANIC_ACTION_RUN,
> +                                   !!info, info);
> +
> +    if (info) {
> +        qapi_free_GuestPanicInformation(info);
> +    }
> +}
> +
>  void qemu_system_reset_request(ShutdownCause reason)
>  {
>      if (no_reboot && reason != SHUTDOWN_CAUSE_SUBSYSTEM_RESET) {