| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <878sm1i4s1.fsf@dusky.pond.sub.org>
Date: Tue, 21 Jan 2020 09:37:34 +0100
From: Markus Armbruster <armbru@...hat.com>
To: zhenwei pi <pizhenwei@...edance.com>
Cc: pbonzini@...hat.com, yelu@...edance.com, libvir-list@...hat.com,
gregkh@...uxfoundation.org, qemu-devel@...gnu.org,
linux-kernel@...r.kernel.org, mprivozn@...hat.com
Subject: Re: [PATCH 2/2] pvpanic: implement crashloaded event handling
zhenwei pi <pizhenwei@...edance.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@...edance.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) {
Powered by blists - more mailing lists