> -----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 ? > > > + } > > +} > > + > > 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? > > Also, we probably want this called from the booke watchdog initialization code. You mean hw/ppc_booke.c, right? > > > + 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. Thanks -Bharat > > > QEMUOptionRom option_rom[MAX_OPTION_ROMS]; > > int nb_option_roms; > > int semihosting_enabled = 0; > > @@ -2888,6 +2889,7 @@ int main(int argc, char **argv, char **envp) > > fprintf(stderr, "Unknown -watchdog-action parameter\n"); > > exit(1); > > } > > + enable_watchdog_support = 1; > > break; > > case QEMU_OPTION_virtiocon: > > add_device_config(DEV_VIRTCON, optarg); > > PS: Please CC qemu-devel and kvm@vger for the next round and follow-up mails. > > > 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
