Patchwork [3/3,v2] Enable kvm emulated watchdog

login
register
mail settings
Submitter Bharat Bhushan
Date Dec. 28, 2012, 5:16 a.m.
Message ID <1356671812-7634-4-git-send-email-bharat.bhushan@freescale.com>
Download mbox | patch
Permalink /patch/208412/
State New
Headers show

Comments

Bharat Bhushan - Dec. 28, 2012, 5:16 a.m.
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@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(-)
Alexander Graf - Jan. 10, 2013, 3:36 p.m.
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.bhushan@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

> 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;
> +}
> #ifndef CONFIG_USER_ONLY
> static inline off_t kvmppc_alloc_rma(const char *name, MemoryRegion *sysmem)
> {
> -- 
> 1.7.0.4
> 
>
Bharat Bhushan - Jan. 11, 2013, 6:42 a.m.
> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Thursday, January 10, 2013 9:07 PM
> To: Bhushan Bharat-R65777
> Cc: qemu-devel@nongnu.org; qemu-ppc@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.bhushan@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
> >
> >
>
Alexander Graf - Jan. 11, 2013, 11:01 a.m.
Am 11.01.2013 um 07:42 schrieb Bhushan Bharat-R65777 <R65777@freescale.com>:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Thursday, January 10, 2013 9:07 PM
>> To: Bhushan Bharat-R65777
>> Cc: qemu-devel@nongnu.org; qemu-ppc@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.bhushan@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

Hrm. Somehow I didn't see those lines during review :). Yes, it's fine - sorry.

Alex

> 
> Thanks
> -Bharat
> 
>>> #ifndef CONFIG_USER_ONLY
>>> static inline off_t kvmppc_alloc_rma(const char *name, MemoryRegion *sysmem)
>>> {
>>> --
>>> 1.7.0.4
>>> 
>>> 
>> 
> 
>

Patch

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)
+{
+    CPUPPCState *env = opaque;
+
+    if (!running) {
+        return;
+    }
+
+    /*
+     * Clear watchdog interrupt condition by clearing TSR.
+     */
+    ppc_booke_watchdog_clear_tsr(env);
+
+    kvmppc_synch_sregs_tsr(env);
+}
+
 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);
+    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
 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;
+}
 #ifndef CONFIG_USER_ONLY
 static inline off_t kvmppc_alloc_rma(const char *name, MemoryRegion *sysmem)
 {