[libvirt] [PATCH 0/2] implement crashloaded event for pvpanic
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
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
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 --------------
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
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!
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
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) {
participants (3)
-
Markus Armbruster -
Paolo Bonzini -
zhenwei pi