On 01.08.2012, at 19:27, 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. > > You want the logic inside the if (cap_booke_sregs && cap_ppc_watchdog) { } or complete function moved to hw/ppc_booke.c ? I want the logic to always treat TCG and KVM code as generic, and when we have to branch off to a KVM specific code path. In this case, this translates to dealing with state changes in generic code, so yes, the complete function. > >> >>> + } >>> +} >>> + >>> int kvm_arch_init_vcpu(CPUPPCState *cenv) >>> { >>> int ret; >>> @@ -384,6 +437,15 @@ int kvm_arch_init_vcpu(CPUPPCState *cenv) >>> return ret; >>> } >>> >>> + if (enable_watchdog_support) { >>> + ret = kvm_watchdog_enable(cenv); >> >> Do you think this is a good idea? Why would real hardware not implement a >> watchdog just because the user didn't select an action? > > If there is no watchdog action then why we want to run watchdog timer? Because software might rely on other aspects of the watchdog than its final expiry. > >> >> Also, we probably want this called from the booke watchdog initialization code. > > You mean hw/ppc_booke.c, right? Yup :) > >> >>> + if (ret) { >>> + return ret; >>> + } >>> + } >>> + >>> + qemu_add_vm_change_state_handler(cpu_state_change_handler, cenv); >>> + >>> idle_timer = qemu_new_timer_ns(vm_clock, kvm_kick_env, cenv); >>> >>> /* Some targets support access to KVM's guest TLB. */ >>> @@ -769,6 +831,13 @@ int kvm_arch_handle_exit(CPUPPCState *env, struct kvm_run >> *run) >>> ret = 1; >>> break; >>> #endif >>> +#ifdef KVM_EXIT_WATCHDOG >>> + case KVM_EXIT_WATCHDOG: >>> + dprintf("handle watchdog expiry\n"); >>> + watchdog_perform_action(); >>> + ret = 0; >>> + break; >>> +#endif >>> default: >>> fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason); >>> ret = -1; >>> diff --git a/vl.c b/vl.c >>> index 1329c30..f5427a8 100644 >>> --- a/vl.c >>> +++ b/vl.c >>> @@ -213,6 +213,7 @@ int no_shutdown = 0; >>> int cursor_hide = 1; >>> int graphic_rotate = 0; >>> const char *watchdog; >>> +int enable_watchdog_support = 0; >> >> This is just plain ugly. >> >> How do the other watchdog implementations today know if they're supposed to be >> used? And do we even need this bit? > > If we call watchdog enable from hw/ppc_booke.c then we do not this. Good :) 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
