On 01.08.2012, at 19:36, Bhushan Bharat-R65777 wrote: > > >> -----Original Message----- >> From: Alexander Graf [mailto:agraf@xxxxxxx] >> Sent: Wednesday, August 01, 2012 7:57 AM >> To: Bhushan Bharat-R65777 >> Cc: qemu-ppc@xxxxxxxxxx List; kvm-ppc@xxxxxxxxxxxxxxx; Bhushan Bharat-R65777; >> qemu-devel qemu-devel; KVM list >> Subject: Re: [PATCH 4/4] Enable kvm emulated watchdog >> >> >> On 20.07.2012, at 07:23, Bharat Bhushan wrote: >> >>> Enable the KVM emulated watchdog if KVM supports (use the capability >>> enablement in watchdog handler). Also watchdog exit >>> (KVM_EXIT_WATCHDOG) handling is added. >>> Watchdog state machine is cleared whenever VM state changes to running. >>> This is to handle the cases like return from debug halt etc. >>> >>> Signed-off-by: Bharat Bhushan <bharat.bhushan@xxxxxxxxxxxxx> >>> --- >>> v4: Enbale watchdog support only when user specifies watchdog-action >>> Earlier this was [3/3] of the patch series. Now because i separated >>> linux-header updation in separate patch, so this become [4/4] >>> >>> v3: >>> - TSR clearing is removed in whatchdog exit handling as this is >>> no more needed. >>> >>> v2: >>> - Merged ([PATCH 3/4] Watchdog exit handling support) and >>> ([PATCH 4/4] Enable to use kvm emulated watchdog) >>> - Clear watchdog state machine when VM state changes to running. >>> >>> hw/ppc_booke.c | 5 ++++ >>> sysemu.h | 1 + >>> target-ppc/cpu.h | 1 + >>> target-ppc/kvm.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> vl.c | 2 + >>> 5 files changed, 78 insertions(+), 0 deletions(-) >>> >>> diff --git a/hw/ppc_booke.c b/hw/ppc_booke.c index 837a5b6..478dbcd >>> 100644 >>> --- a/hw/ppc_booke.c >>> +++ b/hw/ppc_booke.c >>> @@ -203,6 +203,11 @@ static void booke_wdt_cb(void *opaque) >>> booke_timer->wdt_timer); } >>> >>> +void ppc_booke_watchdog_clear_tsr(CPUPPCState *env, target_ulong tsr) >>> +{ >>> + env->spr[SPR_BOOKE_TSR] = tsr & ~(TSR_ENW | TSR_WIS | >>> +TSR_WRS_MASK); } >>> + >>> void store_booke_tsr(CPUPPCState *env, target_ulong val) { >>> env->spr[SPR_BOOKE_TSR] &= ~val; >>> diff --git a/sysemu.h b/sysemu.h >>> index bc2c788..fc388b7 100644 >>> --- a/sysemu.h >>> +++ b/sysemu.h >>> @@ -131,6 +131,7 @@ extern uint8_t *boot_splash_filedata; extern int >>> boot_splash_filedata_size; extern uint8_t qemu_extra_params_fw[2]; >>> extern QEMUClock *rtc_clock; >>> +extern int enable_watchdog_support; >>> >>> #define MAX_NODES 64 >>> extern int nb_numa_nodes; >>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h index >>> ca2fc21..163389a 100644 >>> --- a/target-ppc/cpu.h >>> +++ b/target-ppc/cpu.h >>> @@ -1191,6 +1191,7 @@ void store_40x_dbcr0 (CPUPPCState *env, uint32_t >>> val); void store_40x_sler (CPUPPCState *env, uint32_t val); void >>> store_booke_tcr (CPUPPCState *env, target_ulong val); void >>> store_booke_tsr (CPUPPCState *env, target_ulong val); >>> +void ppc_booke_watchdog_clear_tsr(CPUPPCState *env, target_ulong >>> +tsr); >>> void booke206_flush_tlb(CPUPPCState *env, int flags, const int >>> check_iprot); target_phys_addr_t booke206_tlb_to_page_size(CPUPPCState >>> *env, ppcmas_tlb_t *tlb); int ppcmas_tlb_check(CPUPPCState *env, >>> ppcmas_tlb_t *tlb, diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c >>> index b6ef72d..0226b5e 100644 >>> --- a/target-ppc/kvm.c >>> +++ b/target-ppc/kvm.c >>> @@ -32,6 +32,7 @@ >>> #include "device_tree.h" >>> #include "hw/sysbus.h" >>> #include "hw/spapr.h" >>> +#include "hw/watchdog.h" >>> >>> #include "hw/sysbus.h" >>> #include "hw/spapr.h" >>> @@ -60,6 +61,7 @@ static int cap_booke_sregs; static int cap_ppc_smt; >>> static int cap_ppc_rma; static int cap_spapr_tce; >>> +static int cap_ppc_watchdog; >>> >>> /* XXX We have a race condition where we actually have a level triggered >>> * interrupt, but the infrastructure can't expose that yet, so the guest >>> @@ -86,6 +88,7 @@ int kvm_arch_init(KVMState *s) >>> cap_ppc_smt = kvm_check_extension(s, KVM_CAP_PPC_SMT); >>> cap_ppc_rma = kvm_check_extension(s, KVM_CAP_PPC_RMA); >>> cap_spapr_tce = kvm_check_extension(s, KVM_CAP_SPAPR_TCE); >>> + cap_ppc_watchdog = kvm_check_extension(s, KVM_CAP_PPC_BOOKE_WATCHDOG); >>> >>> if (!cap_interrupt_level) { >>> fprintf(stderr, "KVM: Couldn't find level irq capability. Expect the " >>> @@ -168,6 +171,30 @@ static int kvm_booke206_tlb_init(CPUPPCState *env) >>> return 0; >>> } >>> >>> +static int kvm_watchdog_enable(CPUPPCState *env) >>> +{ >>> + int ret; >>> + struct kvm_enable_cap encap = {}; >>> + >>> + if (!kvm_enabled()) { >>> + return 0; >>> + } >>> + >>> + if (!cap_ppc_watchdog) { >>> + printf("warning: KVM does not support watchdog"); >>> + return 0; >>> + } >>> + >>> + encap.cap = KVM_CAP_PPC_BOOKE_WATCHDOG; >>> + ret = kvm_vcpu_ioctl(env, KVM_ENABLE_CAP, &encap); >>> + if (ret < 0) { >>> + fprintf(stderr, "%s: couldn't enable KVM_CAP_PPC_BOOKE_WATCHDOG: >> %s\n", >>> + __func__, strerror(-ret)); >>> + return ret; >>> + } >>> + >>> + return ret; >>> +} >>> >>> #if defined(TARGET_PPC64) >>> static void kvm_get_fallback_smmu_info(CPUPPCState *env, >>> @@ -371,6 +398,32 @@ static inline void kvm_fixup_page_sizes(CPUPPCState *env) >>> >>> #endif /* !defined (TARGET_PPC64) */ >>> >>> +static void cpu_state_change_handler(void *opaque, int running, RunState >> state) >>> +{ >>> + CPUPPCState *env = opaque; >>> + >>> + struct kvm_sregs sregs; >> >> = { }; >> >>> + >>> + if (!running) >>> + return; >> >> Did you run checkpatch.pl? You're also missing a comment here which case we want >> to act upon. >> >>> + >>> + /* >>> + * Clear watchdog interrupt condition by clearing TSR. >>> + * Similar logic needed to be implemented for watchdog >>> + * emulation in qemu. >>> + */ >>> + if (cap_booke_sregs && cap_ppc_watchdog) { >>> + kvm_vcpu_ioctl(env, KVM_GET_SREGS, &sregs); >>> + >>> + /* Clear TSR.ENW, TSR.WIS and TSR.WRS */ >>> + ppc_booke_watchdog_clear_tsr(env, sregs.u.e.tsr); >>> + sregs.u.e.tsr = env->spr[SPR_BOOKE_TSR]; >>> + sregs.u.e.update_special = KVM_SREGS_E_UPDATE_TSR; >>> + >>> + kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs); >> >> I'd prefer to see that logic implemented in the booke timer code with some >> special case around it for the kvm enabled case. But the event action is >> independent of kvm. > > Also we are calling ppc_booke_watchdog_clear_tsr(), which is defined in hw/ppc_booke.c. So there it can take any action apart from clearing watchdog state in TSR register. Sure, but tomorrow someone comes along and implements watchdog emulation for TCG, and suddenly he will find himself duplicating the same logic you have here. Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
