From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932085Ab3BHBxE (ORCPT ); Thu, 7 Feb 2013 20:53:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38071 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757010Ab3BHBxA (ORCPT ); Thu, 7 Feb 2013 20:53:00 -0500 Date: Thu, 7 Feb 2013 23:39:47 -0200 From: Marcelo Tosatti To: Hu Tao Cc: kvm list , qemu-devel , "linux-kernel@vger.kernel.org" , "Daniel P. Berrange" , KAMEZAWA Hiroyuki , Jan Kiszka , Gleb Natapov , Blue Swirl , Eric Blake , Andrew Jones , Sasha Levin , Luiz Capitulino , Wen Congyang Subject: Re: [PATCH v12 rebased] kvm: notify host when the guest is panicked Message-ID: <20130208013947.GA3364@amt.cnet> References: <1358925575-4505-1-git-send-email-hutao@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1358925575-4505-1-git-send-email-hutao@cn.fujitsu.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Wed, Jan 23, 2013 at 03:19:21PM +0800, Hu Tao wrote: > We can know the guest is panicked when the guest runs on xen. > But we do not have such feature on kvm. > > Another purpose of this feature is: management app(for example: > libvirt) can do auto dump when the guest is panicked. If management > app does not do auto dump, the guest's user can do dump by hand if > he sees the guest is panicked. > > We have three solutions to implement this feature: > 1. use vmcall > 2. use I/O port > 3. use virtio-serial. > > We have decided to avoid touching hypervisor. The reason why I choose > choose the I/O port is: > 1. it is easier to implememt > 2. it does not depend any virtual device > 3. it can work when starting the kernel > > Signed-off-by: Wen Congyang > Signed-off-by: Hu Tao > --- > arch/ia64/kvm/irq.h | 19 +++++++++++++ > arch/powerpc/include/asm/kvm_para.h | 18 ++++++++++++ > arch/s390/include/asm/kvm_para.h | 19 +++++++++++++ > arch/x86/include/asm/kvm_para.h | 20 ++++++++++++++ > arch/x86/include/uapi/asm/kvm_para.h | 2 ++ > arch/x86/kernel/kvm.c | 53 ++++++++++++++++++++++++++++++++++++ > include/linux/kvm_para.h | 18 ++++++++++++ > include/uapi/linux/kvm_para.h | 6 ++++ > kernel/panic.c | 4 +++ > 9 files changed, 159 insertions(+) > > diff --git a/arch/ia64/kvm/irq.h b/arch/ia64/kvm/irq.h > index c0785a7..b3870f8 100644 > --- a/arch/ia64/kvm/irq.h > +++ b/arch/ia64/kvm/irq.h > @@ -30,4 +30,23 @@ static inline int irqchip_in_kernel(struct kvm *kvm) > return 1; > } > > +static inline int kvm_arch_pv_event_init(void) > +{ > + return 0; > +} > + > +static inline unsigned int kvm_arch_pv_features(void) > +{ > + return 0; > +} > + > +static inline void kvm_arch_pv_eject_event(unsigned int event) > +{ > +} > + > +static inline bool kvm_arch_pv_event_enabled(void) > +{ > + return false; > +} > + The interface is x86 only, no need to touch other architectures. > #endif > diff --git a/arch/powerpc/include/asm/kvm_para.h b/arch/powerpc/include/asm/kvm_para.h > index 2b11965..17dd013 100644 > --- a/arch/powerpc/include/asm/kvm_para.h > +++ b/arch/powerpc/include/asm/kvm_para.h > @@ -144,4 +144,22 @@ static inline bool kvm_check_and_clear_guest_paused(void) > return false; > } > > +static inline int kvm_arch_pv_event_init(void) > +{ > + return 0; > +} > + > +static inline unsigned int kvm_arch_pv_features(void) > +{ > + return 0; > +} > + > +static inline void kvm_arch_pv_eject_event(unsigned int event) > +{ > +} > + > +static inline bool kvm_arch_pv_event_enabled(void) > +{ > + return false; > +} > #endif /* __POWERPC_KVM_PARA_H__ */ > diff --git a/arch/s390/include/asm/kvm_para.h b/arch/s390/include/asm/kvm_para.h > index e0f8423..81d87ec 100644 > --- a/arch/s390/include/asm/kvm_para.h > +++ b/arch/s390/include/asm/kvm_para.h > @@ -154,4 +154,23 @@ static inline bool kvm_check_and_clear_guest_paused(void) > return false; > } > > +static inline int kvm_arch_pv_event_init(void) > +{ > + return 0; > +} > + > +static inline unsigned int kvm_arch_pv_features(void) > +{ > + return 0; > +} > + > +static inline void kvm_arch_pv_eject_event(unsigned int event) > +{ > +} > + > +static inline bool kvm_arch_pv_event_enabled(void) > +{ > + return false; > +} > + > #endif /* __S390_KVM_PARA_H */ > --- a/arch/x86/include/asm/kvm_para.h > +++ b/arch/x86/include/asm/kvm_para.h > @@ -133,4 +133,24 @@ static inline void kvm_disable_steal_time(void) > } > #endif > > +static inline int kvm_arch_pv_event_init(void) > +{ > + if (!request_region(KVM_PV_EVENT_PORT, 4, "KVM_PV_EVENT")) > + return -1; > + > + return 0; > +} This should be in a driver in arch/x86/kernel/kvm-panic.c, or so. > + > +static inline unsigned int kvm_arch_pv_features(void) > +{ > + return inl(KVM_PV_EVENT_PORT); > +} > + > +static inline void kvm_arch_pv_eject_event(unsigned int event) > +{ > + outl(event, KVM_PV_EVENT_PORT); > +} > + > +bool kvm_arch_pv_event_enabled(void); > + > #endif /* _ASM_X86_KVM_PARA_H */ > diff --git a/arch/x86/include/uapi/asm/kvm_para.h b/arch/x86/include/uapi/asm/kvm_para.h > index 06fdbd9..c15ef33 100644 > --- a/arch/x86/include/uapi/asm/kvm_para.h > +++ b/arch/x86/include/uapi/asm/kvm_para.h > @@ -96,5 +96,7 @@ struct kvm_vcpu_pv_apf_data { > #define KVM_PV_EOI_ENABLED KVM_PV_EOI_MASK > #define KVM_PV_EOI_DISABLED 0x0 > > +#define KVM_PV_EVENT_PORT (0x505UL) > + No need for the ioport to be hard coded. What are the options to communicate an address to the guest? An MSR, via ACPI? > > #endif /* _UAPI_ASM_X86_KVM_PARA_H */ > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c > index 9c2bd8b..0aa7b3e 100644 > --- a/arch/x86/kernel/kvm.c > +++ b/arch/x86/kernel/kvm.c > @@ -73,6 +73,20 @@ static int parse_no_kvmclock_vsyscall(char *arg) > > early_param("no-kvmclock-vsyscall", parse_no_kvmclock_vsyscall); > > +static int pv_event = 1; > +static int parse_no_pv_event(char *arg) > +{ > + pv_event = 0; > + return 0; > +} > + > +bool kvm_arch_pv_event_enabled(void) > +{ > + return !!pv_event; > +} > + > +early_param("no-pv-event", parse_no_pv_event); > + "pv-event" is a bad name for an interface which is specific to notify panic events. Please use pv-panic everywhere. > static DEFINE_PER_CPU(struct kvm_vcpu_pv_apf_data, apf_reason) __aligned(64); > static DEFINE_PER_CPU(struct kvm_steal_time, steal_time) __aligned(64); > static int has_steal_clock = 0; > @@ -385,6 +399,17 @@ static struct notifier_block kvm_pv_reboot_nb = { > .notifier_call = kvm_pv_reboot_notify, > }; > > +static int > +kvm_pv_panic_notify(struct notifier_block *nb, unsigned long code, void *unused) > +{ > + kvm_pv_eject_event(KVM_PV_EVENT_PANICKED); > + return NOTIFY_DONE; > +} Why 'eject' ? > + > +static struct notifier_block kvm_pv_panic_nb = { > + .notifier_call = kvm_pv_panic_notify, > +}; > + > static u64 kvm_steal_clock(int cpu) > { > u64 steal; > @@ -462,6 +487,34 @@ static void __init kvm_apf_trap_init(void) > set_intr_gate(14, &async_page_fault); > } > > +static void __init kvm_pv_panicked_event_init(void) > +{ > + if (!kvm_para_available()) > + return; > + > + if (kvm_pv_has_feature(KVM_PV_FEATURE_PANICKED)) > + atomic_notifier_chain_register(&panic_notifier_list, > + &kvm_pv_panic_nb); > +} > + > +static inline int kvm_pv_event_init(void) > +{ > + return kvm_arch_pv_event_init(); > +} > + > +static int __init enable_pv_event(void) > +{ > + if (pv_event) { > + if (kvm_pv_event_init()) > + return 0; > + > + kvm_pv_panicked_event_init(); > + } > + > + return 0; > +} > +arch_initcall(enable_pv_event); Call the initialization code from kvm_guest_init, only one function is necessary. > + > void __init kvm_guest_init(void) > { > int i; > diff --git a/include/linux/kvm_para.h b/include/linux/kvm_para.h > index 00a97bb..6fb6198 100644 > --- a/include/linux/kvm_para.h > +++ b/include/linux/kvm_para.h > @@ -10,4 +10,22 @@ static inline int kvm_para_has_feature(unsigned int feature) > return 1; > return 0; > } > + > +static inline int kvm_pv_has_feature(unsigned int feature) > +{ > + if (kvm_arch_pv_features() & (1UL << feature)) > + return 1; > + return 0; > +} > + > +static inline void kvm_pv_eject_event(unsigned int event) > +{ > + kvm_arch_pv_eject_event(event); > +} > + > +static inline bool kvm_pv_event_enabled(void) > +{ > + return kvm_arch_pv_event_enabled(); > +} No need for this helpers, as noted. > #endif /* __LINUX_KVM_PARA_H */ > diff --git a/include/uapi/linux/kvm_para.h b/include/uapi/linux/kvm_para.h > index cea2c5c..c41ddce 100644 > --- a/include/uapi/linux/kvm_para.h > +++ b/include/uapi/linux/kvm_para.h > @@ -20,6 +20,12 @@ > #define KVM_HC_FEATURES 3 > #define KVM_HC_PPC_MAP_MAGIC_PAGE 4 > > +/* The bit of supported pv event */ > +#define KVM_PV_FEATURE_PANICKED 0 > + > +/* The pv event value */ > +#define KVM_PV_EVENT_PANICKED 1 > + This is a hypercall header. You want arch/x86/include/asm/kvm_para.h > /* > * hypercalls use architecture specific > */ > diff --git a/kernel/panic.c b/kernel/panic.c > index e1b2822..a764d2e 100644 > --- a/kernel/panic.c > +++ b/kernel/panic.c > @@ -23,6 +23,7 @@ > #include > #include > #include > +#include > > #define PANIC_TIMER_STEP 100 > #define PANIC_BLINK_SPD 18 > @@ -132,6 +133,9 @@ void panic(const char *fmt, ...) > if (!panic_blink) > panic_blink = no_blink; > > + if (kvm_pv_event_enabled()) > + panic_timeout = 0; > + What is the rationale behind this?