Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog

Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog

[Alexander Graf]

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@freescale.com>
> ---
> 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.

> +    }
> +}
> +
> 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?

Also, we probably want this called from the booke watchdog initialization code.

> +        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?

> 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

Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Wednesday, August 01, 2012 7:57 AM
> To: Bhushan Bharat-R65777
> Cc: qemu-ppc@nongnu.org List; kvm-ppc@vger.kernel.org; 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@freescale.com>
> > ---
> > 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
> 

Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog

[Scott Wood]

On 08/01/2012 12:27 PM, Bhushan Bharat-R65777 wrote:
> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Wednesday, August 01, 2012 7:57 AM
>> To: Bhushan Bharat-R65777
>> Cc: qemu-ppc@nongnu.org List; kvm-ppc@vger.kernel.org; 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:
>>> @@ -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?

On real hardware, if software sets WRC to a non-zero value, the watchdog
action is a system reset.  The user doesn't have to do anything special.

-Scott

Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Wednesday, August 01, 2012 11:31 PM
> To: Bhushan Bharat-R65777
> Cc: Alexander Graf; qemu-ppc@nongnu.org List; kvm-ppc@vger.kernel.org; qemu-
> devel qemu-devel; KVM list
> Subject: Re: [PATCH 4/4] Enable kvm emulated watchdog
> 
> On 08/01/2012 12:27 PM, Bhushan Bharat-R65777 wrote:
> >
> >
> >> -----Original Message-----
> >> From: Alexander Graf [mailto:agraf@suse.de]
> >> Sent: Wednesday, August 01, 2012 7:57 AM
> >> To: Bhushan Bharat-R65777
> >> Cc: qemu-ppc@nongnu.org List; kvm-ppc@vger.kernel.org; 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:
> >>> @@ -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?
> 
> On real hardware, if software sets WRC to a non-zero value, the watchdog action
> is a system reset.  The user doesn't have to do anything special.

Scott, maybe I misunderstood you comment, did not you commented to enable kvm watchdog if there is watchdog action provided by use. 

Thanks
-Bharat

Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog

[Scott Wood]

On 08/02/2012 10:56 AM, Bhushan Bharat-R65777 wrote:
> 
> 
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Wednesday, August 01, 2012 11:31 PM
>> To: Bhushan Bharat-R65777
>> Cc: Alexander Graf; qemu-ppc@nongnu.org List; kvm-ppc@vger.kernel.org; qemu-
>> devel qemu-devel; KVM list
>> Subject: Re: [PATCH 4/4] Enable kvm emulated watchdog
>>
>> On 08/01/2012 12:27 PM, Bhushan Bharat-R65777 wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Alexander Graf [mailto:agraf@suse.de]
>>>> Sent: Wednesday, August 01, 2012 7:57 AM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: qemu-ppc@nongnu.org List; kvm-ppc@vger.kernel.org; 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:
>>>>> @@ -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?
>>
>> On real hardware, if software sets WRC to a non-zero value, the watchdog action
>> is a system reset.  The user doesn't have to do anything special.
> 
> Scott, maybe I misunderstood you comment, did not you commented to
> enable kvm watchdog if there is watchdog action provided by use.

I changed my mind. :-)

The main difference between the user specifically asking for an action
of system reset, and the user not asking for anything, is that only in
the former case should we error out if watchdog functionality isn't
available -- but if it's a pain to distinguish, don't error out in
either case.

-Scott

Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Friday, August 03, 2012 2:02 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; Alexander Graf; qemu-ppc@nongnu.org List; kvm-
> ppc@vger.kernel.org; qemu-devel qemu-devel; KVM list
> Subject: Re: [PATCH 4/4] Enable kvm emulated watchdog
> 
> On 08/02/2012 10:56 AM, Bhushan Bharat-R65777 wrote:
> >
> >
> >> -----Original Message-----
> >> From: Wood Scott-B07421
> >> Sent: Wednesday, August 01, 2012 11:31 PM
> >> To: Bhushan Bharat-R65777
> >> Cc: Alexander Graf; qemu-ppc@nongnu.org List;
> >> kvm-ppc@vger.kernel.org; qemu- devel qemu-devel; KVM list
> >> Subject: Re: [PATCH 4/4] Enable kvm emulated watchdog
> >>
> >> On 08/01/2012 12:27 PM, Bhushan Bharat-R65777 wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Alexander Graf [mailto:agraf@suse.de]
> >>>> Sent: Wednesday, August 01, 2012 7:57 AM
> >>>> To: Bhushan Bharat-R65777
> >>>> Cc: qemu-ppc@nongnu.org List; kvm-ppc@vger.kernel.org; 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:
> >>>>> @@ -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?
> >>
> >> On real hardware, if software sets WRC to a non-zero value, the
> >> watchdog action is a system reset.  The user doesn't have to do anything
> special.
> >
> > Scott, maybe I misunderstood you comment, did not you commented to
> > enable kvm watchdog if there is watchdog action provided by use.
> 
> I changed my mind. :-)
> 
> The main difference between the user specifically asking for an action of system
> reset, and the user not asking for anything, is that only in the former case
> should we error out if watchdog functionality isn't available -- but if it's a
> pain to distinguish, don't error out in either case.

:-) ..  ok 

-Bharat

> 
> -Scott

Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog

[Alexander Graf]

On 01.08.2012, at 20:00, Scott Wood wrote:

> On 08/01/2012 12:27 PM, Bhushan Bharat-R65777 wrote:
>> 
>> 
>>> -----Original Message-----
>>> From: Alexander Graf [mailto:agraf@suse.de]
>>> Sent: Wednesday, August 01, 2012 7:57 AM
>>> To: Bhushan Bharat-R65777
>>> Cc: qemu-ppc@nongnu.org List; kvm-ppc@vger.kernel.org; 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:
>>>> @@ -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?
> 
> On real hardware, if software sets WRC to a non-zero value, the watchdog
> action is a system reset.  The user doesn't have to do anything special.

So we eventually want a machine default that says "default watchdog action is system reset" that a user can then override. But we can do that in a later step.


Alex

Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog

[Alexander Graf]

On 01.08.2012, at 19:27, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Wednesday, August 01, 2012 7:57 AM
>> To: Bhushan Bharat-R65777
>> Cc: qemu-ppc@nongnu.org List; kvm-ppc@vger.kernel.org; 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@freescale.com>
>>> ---
>>> 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

Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog

[Bhushan Bharat-R65777]

> -----Original Message-----
> From: Alexander Graf [mailto:agraf@suse.de]
> Sent: Wednesday, August 01, 2012 7:57 AM
> To: Bhushan Bharat-R65777
> Cc: qemu-ppc@nongnu.org List; kvm-ppc@vger.kernel.org; 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@freescale.com>
> > ---
> > 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.

Thanks
-Bharat

Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog

[Alexander Graf]

On 01.08.2012, at 19:36, Bhushan Bharat-R65777 wrote:

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:agraf@suse.de]
>> Sent: Wednesday, August 01, 2012 7:57 AM
>> To: Bhushan Bharat-R65777
>> Cc: qemu-ppc@nongnu.org List; kvm-ppc@vger.kernel.org; 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@freescale.com>
>>> ---
>>> 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