> -----Original Message-----
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Thursday, January 10, 2013 9:07 PM
> To: Bhushan Bharat-R65777
> Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org; Bhushan Bharat-R65777
> Subject: Re: [PATCH 3/3 v2] Enable kvm emulated watchdog
>
>
> On 28.12.2012, at 06:16, 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.bhus...@freescale.com>
> > ---
> > v2:
> > - access cap_* from target_ppc/kvm.c only.
> > - Added wrapper functions in target_ppc/kvm.c for
> > enable_watchdog and tsr_sregs synchronization.
> > - Incorporated other Review comments
> >
> > hw/ppc.h | 1 +
> > hw/ppc_booke.c | 36 +++++++++++++++++++++++++++++++-
> > target-ppc/kvm.c | 56
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > target-ppc/kvm_ppc.h | 11 +++++++++
> > 4 files changed, 103 insertions(+), 1 deletions(-)
> >
> > diff --git a/hw/ppc.h b/hw/ppc.h
> > index 2f3ea27..6ad9e1f 100644
> > --- a/hw/ppc.h
> > +++ b/hw/ppc.h
> > @@ -90,3 +90,4 @@ enum {
> >
> > /* ppc_booke.c */
> > void ppc_booke_timers_init(CPUPPCState *env, uint32_t freq, uint32_t
> > flags);
> > +void ppc_booke_watchdog_clear_tsr(CPUPPCState *env);
> > diff --git a/hw/ppc_booke.c b/hw/ppc_booke.c index 837a5b6..7273259
> > 100644
> > --- a/hw/ppc_booke.c
> > +++ b/hw/ppc_booke.c
> > @@ -28,7 +28,7 @@
> > #include "nvram.h"
> > #include "qemu-log.h"
> > #include "loader.h"
> > -
> > +#include "kvm_ppc.h"
> >
> > /* Timer Control Register */
> >
> > @@ -203,6 +203,11 @@ static void booke_wdt_cb(void *opaque)
> > booke_timer->wdt_timer); }
> >
> > +void ppc_booke_watchdog_clear_tsr(CPUPPCState *env) {
> > + env->spr[SPR_BOOKE_TSR] &= ~(TSR_ENW | TSR_WIS | TSR_WRS_MASK); }
> > +
> > void store_booke_tsr(CPUPPCState *env, target_ulong val) {
> > env->spr[SPR_BOOKE_TSR] &= ~val;
> > @@ -241,10 +246,27 @@ static void ppc_booke_timer_reset_handle(void *opaque)
> > booke_update_irq(env);
> > }
> >
> > +static void cpu_state_change_handler(void *opaque, int running,
> > +RunState state) {
>
> Needs a comment when this happens
>
> > + CPUPPCState *env = opaque;
> > +
> > + if (!running) {
> > + return;
> > + }
> > +
> > + /*
> > + * Clear watchdog interrupt condition by clearing TSR.
> > + */
> > + ppc_booke_watchdog_clear_tsr(env);
> > +
> > + kvmppc_synch_sregs_tsr(env);
>
> kvmppc_sync_tsr. Also please add the sync to store_booke_tsr(). Then here, you
> can just do
>
> store_booke_tsr(TSR_ENW | TSR_WIS | TSR_WRS_MASK);
>
> > +}
> > +
> > void ppc_booke_timers_init(CPUPPCState *env, uint32_t freq, uint32_t
> > flags) {
> > ppc_tb_t *tb_env;
> > booke_timer_t *booke_timer;
> > + int ret = 0;
> >
> > tb_env = g_malloc0(sizeof(ppc_tb_t));
> > booke_timer = g_malloc0(sizeof(booke_timer_t)); @@ -262,5 +284,17
> > @@ void ppc_booke_timers_init(CPUPPCState *env, uint32_t freq, uint32_t
> > flags)
> > booke_timer->wdt_timer =
> > qemu_new_timer_ns(vm_clock, &booke_wdt_cb, env);
> >
> > + ret = kvmppc_booke_watchdog_enable(env);
> > +
> > + if (ret) {
> > + /* TODO: Start the QEMU emulated watchdog if not running on KVM.
> > + * Also start the QEMU emulated watchdog if KVM does not support
> > + * emulated watchdog or somehow it is not enabled (supported but
> > + * not enabled is though some bug and requires debugging :)).
> > + */
> > + }
> > +
> > + qemu_add_vm_change_state_handler(cpu_state_change_handler, env);
> > +
> > qemu_register_reset(ppc_booke_timer_reset_handle, env); } diff
> > --git a/target-ppc/kvm.c b/target-ppc/kvm.c index 3f5df57..6828afa
> > 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -32,10 +32,12 @@
> > #include "device_tree.h"
> > #include "hw/sysbus.h"
> > #include "hw/spapr.h"
> > +#include "hw/watchdog.h"
> >
> > #include "hw/sysbus.h"
> > #include "hw/spapr.h"
> > #include "hw/spapr_vio.h"
> > +#include "hw/ppc.h"
> >
> > //#define DEBUG_KVM
> >
> > @@ -61,6 +63,7 @@ static int cap_ppc_smt; static int cap_ppc_rma;
> > static int cap_spapr_tce; static int cap_hior;
> > +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
> > @@ -90,6 +93,7 @@ int kvm_arch_init(KVMState *s)
> > cap_ppc_rma = kvm_check_extension(s, KVM_CAP_PPC_RMA);
> > cap_spapr_tce = kvm_check_extension(s, KVM_CAP_SPAPR_TCE);
> > cap_hior = kvm_check_extension(s, KVM_CAP_PPC_HIOR);
> > + 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 "
> > @@ -823,6 +827,12 @@ int kvm_arch_handle_exit(CPUPPCState *env, struct
> > kvm_run
> *run)
> > ret = 0;
> > break;
> > #endif
> > + case KVM_EXIT_WATCHDOG:
> > + dprintf("handle watchdog expiry\n");
> > + watchdog_perform_action();
> > + ret = 0;
> > + break;
> > +
> > default:
> > fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
> > ret = -1;
> > @@ -1239,3 +1249,49 @@ int kvm_arch_on_sigbus(int code, void *addr)
> > {
> > return 1;
> > }
> > +
> > +int kvmppc_synch_sregs_tsr(CPUPPCState *env)
> > +{
> > + struct kvm_sregs sregs;
> > + int ret;
> > +
> > + if (!kvm_enabled() || !cap_booke_sregs || !cap_ppc_watchdog) {
> > + return -1;
> > + }
> > +
> > + ret = kvm_vcpu_ioctl(env, KVM_GET_SREGS, &sregs);
> > +
> > + env->spr[SPR_BOOKE_TSR] = sregs.u.e.tsr;
> > + ppc_booke_watchdog_clear_tsr(env);
>
> You're getting _and_ setting it in the same function? This is nonsense. Please
> split it into a get and a set function if you need it.
>
> > + sregs.u.e.tsr = env->spr[SPR_BOOKE_TSR];
> > + sregs.u.e.update_special = KVM_SREGS_E_UPDATE_TSR;
> > +
> > + ret = kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs);
> > +
> > + return ret;
> > +}
> > +
> > +int kvmppc_booke_watchdog_enable(CPUPPCState *env)
> > +{
> > + int ret;
> > + struct kvm_enable_cap encap = {};
> > +
> > + if (!kvm_enabled()) {
> > + return -1;
> > + }
> > +
> > + if (!cap_ppc_watchdog) {
> > + printf("warning: KVM does not support watchdog");
> > + return -1;
> > + }
> > +
> > + 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;
> > +}
> > diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> > index baad6eb..4ccc9fa 100644
> > --- a/target-ppc/kvm_ppc.h
> > +++ b/target-ppc/kvm_ppc.h
> > @@ -23,6 +23,8 @@ int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf,
> > int
> buf_len);
> > int kvmppc_set_interrupt(CPUPPCState *env, int irq, int level);
> > void kvmppc_set_papr(CPUPPCState *env);
> > int kvmppc_smt_threads(void);
> > +int kvmppc_synch_sregs_tsr(CPUPPCState *cenv);
> > +int kvmppc_booke_watchdog_enable(CPUPPCState *env);
> > #ifndef CONFIG_USER_ONLY
>
> Missing code for !CONFIG_KVM. Please always check compile your code on x86.
Alex is not the below code covers !CONFIG_KVM ...
>
>
> Alex
>
> > off_t kvmppc_alloc_rma(const char *name, MemoryRegion *sysmem);
> > void *kvmppc_create_spapr_tce(uint32_t liobn, uint32_t window_size, int
> > *pfd);
> > @@ -79,6 +81,15 @@ static inline int kvmppc_smt_threads(void)
> > return 1;
> > }
> >
> > +static inline int kvmppc_synch_sregs_tsr(CPUPPCState *env)
> > +{
> > + return -1;
> > +}
> > +
> > +static inline int kvmppc_booke_watchdog_enable(CPUPPCState *env)
> > +{
> > + return -1;
> > +}
I mean this one
Thanks
-Bharat
> > #ifndef CONFIG_USER_ONLY
> > static inline off_t kvmppc_alloc_rma(const char *name, MemoryRegion *sysmem)
> > {
> > --
> > 1.7.0.4
> >
> >
>
