[PATCH 0/2] implement crashloaded event for pvpanic

[PATCH 0/2] implement crashloaded event for pvpanic

[zhenwei pi]

Guest may handle panic by itself, then just reboot without pvpanic
notification. Then We can't separate the abnormal reboot from
normal operation.

Declear bit 1 for pvpanic as crashloaded event. It should work with
guest kernel side. Link: https://lkml.org/lkml/2019/12/14/265
Before running kexec, guest could wirte this bit to notify host side.
Host side handles crashloaded event, posts event to upper layer.
Then guest side continues to run kexec.

Test with libvirt, libvirt could recieve the new event. The patch of
libvirt will be sent soon.

Zhenwei Pi (2):
  pvpanic: introduce crashloaded for pvpanic
  pvpanic: implement crashloaded event handling

 docs/specs/pvpanic.txt    |  8 ++++++--
 hw/misc/pvpanic.c         | 11 +++++++++--
 include/sysemu/runstate.h |  1 +
 qapi/run-state.json       | 22 +++++++++++++++++++++-
 vl.c                      | 12 ++++++++++++
 5 files changed, 49 insertions(+), 5 deletions(-)

-- 
2.11.0

[PATCH 1/2] pvpanic: introduce crashloaded for pvpanic

[zhenwei pi]

Add bit 1 for pvpanic. This bit means that guest hits a panic, but
guest wants to handle error by itself. Typical case: Linux guest runs
kdump in panic. It will help us to separate the abnormal reboot from
normal operation.

Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
---
 docs/specs/pvpanic.txt | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/docs/specs/pvpanic.txt b/docs/specs/pvpanic.txt
index c7bbacc778..bdea68a430 100644
--- a/docs/specs/pvpanic.txt
+++ b/docs/specs/pvpanic.txt
@@ -16,8 +16,12 @@ pvpanic exposes a single I/O port, by default 0x505. On read, the bits
 recognized by the device are set. Software should ignore bits it doesn't
 recognize. On write, the bits not recognized by the device are ignored.
 Software should set only bits both itself and the device recognize.
-Currently, only bit 0 is recognized, setting it indicates a guest panic
-has happened.
+
+Bit Definition
+--------------
+bit 0: setting it indicates a guest panic has happened.
+bit 1: named crashloaded. setting it indicates a guest panic and run
+       kexec to handle error by guest itself.
 
 ACPI Interface
 --------------
-- 
2.11.0

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

[zhenwei pi]

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

Re: [PATCH 1/2] pvpanic: introduce crashloaded for pvpanic

[Markus Armbruster]

zhenwei pi <pizhenwei@bytedance.com> writes:

> Add bit 1 for pvpanic. This bit means that guest hits a panic, but
> guest wants to handle error by itself. Typical case: Linux guest runs
> kdump in panic. It will help us to separate the abnormal reboot from
> normal operation.
>
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  docs/specs/pvpanic.txt | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/docs/specs/pvpanic.txt b/docs/specs/pvpanic.txt
> index c7bbacc778..bdea68a430 100644
> --- a/docs/specs/pvpanic.txt
> +++ b/docs/specs/pvpanic.txt
> @@ -16,8 +16,12 @@ pvpanic exposes a single I/O port, by default 0x505. On read, the bits
>  recognized by the device are set. Software should ignore bits it doesn't
>  recognize. On write, the bits not recognized by the device are ignored.
>  Software should set only bits both itself and the device recognize.

Guest software, I presume.

> -Currently, only bit 0 is recognized, setting it indicates a guest panic
> -has happened.
> +
> +Bit Definition
> +--------------
> +bit 0: setting it indicates a guest panic has happened.
> +bit 1: named crashloaded. setting it indicates a guest panic and run
> +       kexec to handle error by guest itself.

Suggest to scratch "named crashloaded."

The whole file is rather terse.  I figure that's okay as along as
there's just "guest panicked", because "kernel panic" is obvious enough.
The addition of "panicked, handling with kexec" makes it less obvious.
The commit message provides a bit more guidance.  Could that be worked
into this file?

>  
>  ACPI Interface
>  --------------

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

[Markus Armbruster]

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

Re: [PATCH 1/2] pvpanic: introduce crashloaded for pvpanic

[Paolo Bonzini]

On 21/01/20 09:22, Markus Armbruster wrote:
> zhenwei pi <pizhenwei@bytedance.com> writes:
> 
>> Add bit 1 for pvpanic. This bit means that guest hits a panic, but
>> guest wants to handle error by itself. Typical case: Linux guest runs
>> kdump in panic. It will help us to separate the abnormal reboot from
>> normal operation.
>>
>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
>> ---
>>  docs/specs/pvpanic.txt | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/specs/pvpanic.txt b/docs/specs/pvpanic.txt
>> index c7bbacc778..bdea68a430 100644
>> --- a/docs/specs/pvpanic.txt
>> +++ b/docs/specs/pvpanic.txt
>> @@ -16,8 +16,12 @@ pvpanic exposes a single I/O port, by default 0x505. On read, the bits
>>  recognized by the device are set. Software should ignore bits it doesn't
>>  recognize. On write, the bits not recognized by the device are ignored.
>>  Software should set only bits both itself and the device recognize.
> 
> Guest software, I presume.
> 
>> -Currently, only bit 0 is recognized, setting it indicates a guest panic
>> -has happened.
>> +
>> +Bit Definition
>> +--------------
>> +bit 0: setting it indicates a guest panic has happened.
>> +bit 1: named crashloaded. setting it indicates a guest panic and run
>> +       kexec to handle error by guest itself.
> 
> Suggest to scratch "named crashloaded."

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.

?

Paolo

Re: [PATCH 1/2] pvpanic: introduce crashloaded for pvpanic

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 21/01/20 09:22, Markus Armbruster wrote:
>> zhenwei pi <pizhenwei@bytedance.com> writes:
>> 
>>> Add bit 1 for pvpanic. This bit means that guest hits a panic, but
>>> guest wants to handle error by itself. Typical case: Linux guest runs
>>> kdump in panic. It will help us to separate the abnormal reboot from
>>> normal operation.
>>>
>>> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
>>> ---
>>>  docs/specs/pvpanic.txt | 8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/docs/specs/pvpanic.txt b/docs/specs/pvpanic.txt
>>> index c7bbacc778..bdea68a430 100644
>>> --- a/docs/specs/pvpanic.txt
>>> +++ b/docs/specs/pvpanic.txt
>>> @@ -16,8 +16,12 @@ pvpanic exposes a single I/O port, by default 0x505. On read, the bits
>>>  recognized by the device are set. Software should ignore bits it doesn't
>>>  recognize. On write, the bits not recognized by the device are ignored.
>>>  Software should set only bits both itself and the device recognize.
>> 
>> Guest software, I presume.
>> 
>>> -Currently, only bit 0 is recognized, setting it indicates a guest panic
>>> -has happened.
>>> +
>>> +Bit Definition
>>> +--------------
>>> +bit 0: setting it indicates a guest panic has happened.
>>> +bit 1: named crashloaded. setting it indicates a guest panic and run
>>> +       kexec to handle error by guest itself.
>> 
>> Suggest to scratch "named crashloaded."
>
> 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.
>
> ?
>
> Paolo

Works for me, thanks!