* [PATCH 0/2] implement crashloaded event for pvpanic @ 2020-01-10 10:06 zhenwei pi 2020-01-10 10:06 ` [PATCH 1/2] pvpanic: introduce crashloaded " zhenwei pi 2020-01-10 10:06 ` [PATCH 2/2] pvpanic: implement crashloaded event handling zhenwei pi 0 siblings, 2 replies; 7+ messages in thread From: zhenwei pi @ 2020-01-10 10:06 UTC (permalink / raw) To: pbonzini Cc: qemu-devel, linux-kernel, gregkh, libvir-list, mprivozn, yelu, 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] pvpanic: introduce crashloaded for pvpanic 2020-01-10 10:06 [PATCH 0/2] implement crashloaded event for pvpanic zhenwei pi @ 2020-01-10 10:06 ` zhenwei pi 2020-01-21 8:22 ` Markus Armbruster 2020-01-10 10:06 ` [PATCH 2/2] pvpanic: implement crashloaded event handling zhenwei pi 1 sibling, 1 reply; 7+ messages in thread From: zhenwei pi @ 2020-01-10 10:06 UTC (permalink / raw) To: pbonzini Cc: qemu-devel, linux-kernel, gregkh, libvir-list, mprivozn, yelu, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] pvpanic: introduce crashloaded for pvpanic 2020-01-10 10:06 ` [PATCH 1/2] pvpanic: introduce crashloaded " zhenwei pi @ 2020-01-21 8:22 ` Markus Armbruster 2020-01-21 10:50 ` Paolo Bonzini 0 siblings, 1 reply; 7+ messages in thread From: Markus Armbruster @ 2020-01-21 8:22 UTC (permalink / raw) To: zhenwei pi Cc: pbonzini, yelu, libvir-list, gregkh, qemu-devel, linux-kernel, mprivozn 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 > -------------- ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] pvpanic: introduce crashloaded for pvpanic 2020-01-21 8:22 ` Markus Armbruster @ 2020-01-21 10:50 ` Paolo Bonzini 2020-01-21 13:38 ` Markus Armbruster 0 siblings, 1 reply; 7+ messages in thread From: Paolo Bonzini @ 2020-01-21 10:50 UTC (permalink / raw) To: Markus Armbruster, zhenwei pi Cc: yelu, libvir-list, gregkh, qemu-devel, linux-kernel, mprivozn 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] pvpanic: introduce crashloaded for pvpanic 2020-01-21 10:50 ` Paolo Bonzini @ 2020-01-21 13:38 ` Markus Armbruster 0 siblings, 0 replies; 7+ messages in thread From: Markus Armbruster @ 2020-01-21 13:38 UTC (permalink / raw) To: Paolo Bonzini Cc: zhenwei pi, yelu, libvir-list, gregkh, qemu-devel, linux-kernel, mprivozn 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! ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] pvpanic: implement crashloaded event handling 2020-01-10 10:06 [PATCH 0/2] implement crashloaded event for pvpanic zhenwei pi 2020-01-10 10:06 ` [PATCH 1/2] pvpanic: introduce crashloaded " zhenwei pi @ 2020-01-10 10:06 ` zhenwei pi 2020-01-21 8:37 ` Markus Armbruster 1 sibling, 1 reply; 7+ messages in thread From: zhenwei pi @ 2020-01-10 10:06 UTC (permalink / raw) To: pbonzini Cc: qemu-devel, linux-kernel, gregkh, libvir-list, mprivozn, yelu, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] pvpanic: implement crashloaded event handling 2020-01-10 10:06 ` [PATCH 2/2] pvpanic: implement crashloaded event handling zhenwei pi @ 2020-01-21 8:37 ` Markus Armbruster 0 siblings, 0 replies; 7+ messages in thread From: Markus Armbruster @ 2020-01-21 8:37 UTC (permalink / raw) To: zhenwei pi Cc: pbonzini, yelu, libvir-list, gregkh, qemu-devel, linux-kernel, mprivozn 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) { ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-01-21 13:39 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-10 10:06 [PATCH 0/2] implement crashloaded event for pvpanic zhenwei pi 2020-01-10 10:06 ` [PATCH 1/2] pvpanic: introduce crashloaded " zhenwei pi 2020-01-21 8:22 ` Markus Armbruster 2020-01-21 10:50 ` Paolo Bonzini 2020-01-21 13:38 ` Markus Armbruster 2020-01-10 10:06 ` [PATCH 2/2] pvpanic: implement crashloaded event handling zhenwei pi 2020-01-21 8:37 ` Markus Armbruster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).