* Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-01 2:27 ` Alexander Graf
0 siblings, 0 replies; 32+ messages in thread
From: Alexander Graf @ 2012-08-01 2:27 UTC (permalink / raw)
To: Bharat Bhushan
Cc: Bharat Bhushan, qemu-ppc@nongnu.org List, qemu-devel qemu-devel,
kvm-ppc, KVM list
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-01 2:27 ` Alexander Graf
0 siblings, 0 replies; 32+ messages in thread
From: Alexander Graf @ 2012-08-01 2:27 UTC (permalink / raw)
To: Bharat Bhushan
Cc: Bharat Bhushan, qemu-ppc@nongnu.org List, qemu-devel qemu-devel,
kvm-ppc, KVM list
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
^ permalink raw reply [flat|nested] 32+ messages in thread* RE: [PATCH 4/4] Enable kvm emulated watchdog
2012-08-01 2:27 ` Alexander Graf
(?)
@ 2012-08-01 17:27 ` Bhushan Bharat-R65777
-1 siblings, 0 replies; 32+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-08-01 17:27 UTC (permalink / raw)
To: Alexander Graf
Cc: qemu-ppc@nongnu.org List, kvm-ppc@vger.kernel.org,
qemu-devel qemu-devel, KVM list
> -----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
>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-01 17:27 ` Bhushan Bharat-R65777
0 siblings, 0 replies; 32+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-08-01 17:27 UTC (permalink / raw)
To: Alexander Graf
Cc: qemu-ppc@nongnu.org List, qemu-devel qemu-devel,
kvm-ppc@vger.kernel.org, KVM list
> -----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
>
^ permalink raw reply [flat|nested] 32+ messages in thread* RE: [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-01 17:27 ` Bhushan Bharat-R65777
0 siblings, 0 replies; 32+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-08-01 17:27 UTC (permalink / raw)
To: Alexander Graf
Cc: qemu-ppc@nongnu.org List, kvm-ppc@vger.kernel.org,
qemu-devel qemu-devel, KVM list
> -----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
>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 4/4] Enable kvm emulated watchdog
2012-08-01 17:27 ` Bhushan Bharat-R65777
(?)
@ 2012-08-01 18:00 ` Scott Wood
-1 siblings, 0 replies; 32+ messages in thread
From: Scott Wood @ 2012-08-01 18:00 UTC (permalink / raw)
To: Bhushan Bharat-R65777
Cc: Alexander Graf, qemu-ppc@nongnu.org List, kvm-ppc@vger.kernel.org,
qemu-devel qemu-devel, KVM list
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-01 18:00 ` Scott Wood
0 siblings, 0 replies; 32+ messages in thread
From: Scott Wood @ 2012-08-01 18:00 UTC (permalink / raw)
To: Bhushan Bharat-R65777
Cc: KVM list, qemu-ppc@nongnu.org List, Alexander Graf,
kvm-ppc@vger.kernel.org, qemu-devel qemu-devel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-01 18:00 ` Scott Wood
0 siblings, 0 replies; 32+ messages in thread
From: Scott Wood @ 2012-08-01 18:00 UTC (permalink / raw)
To: Bhushan Bharat-R65777
Cc: Alexander Graf, qemu-ppc@nongnu.org List, kvm-ppc@vger.kernel.org,
qemu-devel qemu-devel, KVM list
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
^ permalink raw reply [flat|nested] 32+ messages in thread* RE: [PATCH 4/4] Enable kvm emulated watchdog
2012-08-01 18:00 ` Scott Wood
(?)
@ 2012-08-02 15:56 ` Bhushan Bharat-R65777
-1 siblings, 0 replies; 32+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-08-02 15:56 UTC (permalink / raw)
To: Wood Scott-B07421
Cc: Alexander Graf, qemu-ppc@nongnu.org List, kvm-ppc@vger.kernel.org,
qemu-devel qemu-devel, KVM list
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogV29vZCBTY290dC1CMDc0
MjENCj4gU2VudDogV2VkbmVzZGF5LCBBdWd1c3QgMDEsIDIwMTIgMTE6MzEgUE0NCj4gVG86IEJo
dXNoYW4gQmhhcmF0LVI2NTc3Nw0KPiBDYzogQWxleGFuZGVyIEdyYWY7IHFlbXUtcHBjQG5vbmdu
dS5vcmcgTGlzdDsga3ZtLXBwY0B2Z2VyLmtlcm5lbC5vcmc7IHFlbXUtDQo+IGRldmVsIHFlbXUt
ZGV2ZWw7IEtWTSBsaXN0DQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggNC80XSBFbmFibGUga3ZtIGVt
dWxhdGVkIHdhdGNoZG9nDQo+IA0KPiBPbiAwOC8wMS8yMDEyIDEyOjI3IFBNLCBCaHVzaGFuIEJo
YXJhdC1SNjU3Nzcgd3JvdGU6DQo+ID4NCj4gPg0KPiA+PiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2Ut
LS0tLQ0KPiA+PiBGcm9tOiBBbGV4YW5kZXIgR3JhZiBbbWFpbHRvOmFncmFmQHN1c2UuZGVdDQo+
ID4+IFNlbnQ6IFdlZG5lc2RheSwgQXVndXN0IDAxLCAyMDEyIDc6NTcgQU0NCj4gPj4gVG86IEJo
dXNoYW4gQmhhcmF0LVI2NTc3Nw0KPiA+PiBDYzogcWVtdS1wcGNAbm9uZ251Lm9yZyBMaXN0OyBr
dm0tcHBjQHZnZXIua2VybmVsLm9yZzsgQmh1c2hhbg0KPiA+PiBCaGFyYXQtUjY1Nzc3OyBxZW11
LWRldmVsIHFlbXUtZGV2ZWw7IEtWTSBsaXN0DQo+ID4+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggNC80
XSBFbmFibGUga3ZtIGVtdWxhdGVkIHdhdGNoZG9nDQo+ID4+DQo+ID4+DQo+ID4+IE9uIDIwLjA3
LjIwMTIsIGF0IDA3OjIzLCBCaGFyYXQgQmh1c2hhbiB3cm90ZToNCj4gPj4+IEBAIC0zODQsNiAr
NDM3LDE1IEBAIGludCBrdm1fYXJjaF9pbml0X3ZjcHUoQ1BVUFBDU3RhdGUgKmNlbnYpDQo+ID4+
PiAgICAgICAgIHJldHVybiByZXQ7DQo+ID4+PiAgICAgfQ0KPiA+Pj4NCj4gPj4+ICsgICAgaWYg
KGVuYWJsZV93YXRjaGRvZ19zdXBwb3J0KSB7DQo+ID4+PiArICAgICAgICByZXQgPSBrdm1fd2F0
Y2hkb2dfZW5hYmxlKGNlbnYpOw0KPiA+Pg0KPiA+PiBEbyB5b3UgdGhpbmsgdGhpcyBpcyBhIGdv
b2QgaWRlYT8gV2h5IHdvdWxkIHJlYWwgaGFyZHdhcmUgbm90DQo+ID4+IGltcGxlbWVudCBhIHdh
dGNoZG9nIGp1c3QgYmVjYXVzZSB0aGUgdXNlciBkaWRuJ3Qgc2VsZWN0IGFuIGFjdGlvbj8NCj4g
Pg0KPiA+IElmIHRoZXJlIGlzIG5vIHdhdGNoZG9nIGFjdGlvbiB0aGVuIHdoeSB3ZSB3YW50IHRv
IHJ1biB3YXRjaGRvZyB0aW1lcj8NCj4gDQo+IE9uIHJlYWwgaGFyZHdhcmUsIGlmIHNvZnR3YXJl
IHNldHMgV1JDIHRvIGEgbm9uLXplcm8gdmFsdWUsIHRoZSB3YXRjaGRvZyBhY3Rpb24NCj4gaXMg
YSBzeXN0ZW0gcmVzZXQuICBUaGUgdXNlciBkb2Vzbid0IGhhdmUgdG8gZG8gYW55dGhpbmcgc3Bl
Y2lhbC4NCg0KU2NvdHQsIG1heWJlIEkgbWlzdW5kZXJzdG9vZCB5b3UgY29tbWVudCwgZGlkIG5v
dCB5b3UgY29tbWVudGVkIHRvIGVuYWJsZSBrdm0gd2F0Y2hkb2cgaWYgdGhlcmUgaXMgd2F0Y2hk
b2cgYWN0aW9uIHByb3ZpZGVkIGJ5IHVzZS4gDQoNClRoYW5rcw0KLUJoYXJhdA0KDQo
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-02 15:56 ` Bhushan Bharat-R65777
0 siblings, 0 replies; 32+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-08-02 15:56 UTC (permalink / raw)
To: Wood Scott-B07421
Cc: KVM list, qemu-ppc@nongnu.org List, Alexander Graf,
kvm-ppc@vger.kernel.org, qemu-devel qemu-devel
> -----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
^ permalink raw reply [flat|nested] 32+ messages in thread* RE: [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-02 15:56 ` Bhushan Bharat-R65777
0 siblings, 0 replies; 32+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-08-02 15:56 UTC (permalink / raw)
To: Wood Scott-B07421
Cc: Alexander Graf, qemu-ppc@nongnu.org List, kvm-ppc@vger.kernel.org,
qemu-devel qemu-devel, KVM list
> -----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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 4/4] Enable kvm emulated watchdog
2012-08-02 15:56 ` Bhushan Bharat-R65777
(?)
@ 2012-08-02 20:31 ` Scott Wood
-1 siblings, 0 replies; 32+ messages in thread
From: Scott Wood @ 2012-08-02 20:31 UTC (permalink / raw)
To: Bhushan Bharat-R65777
Cc: Wood Scott-B07421, KVM list, Alexander Graf,
kvm-ppc@vger.kernel.org, qemu-devel qemu-devel,
qemu-ppc@nongnu.org List
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-02 20:31 ` Scott Wood
0 siblings, 0 replies; 32+ messages in thread
From: Scott Wood @ 2012-08-02 20:31 UTC (permalink / raw)
To: Bhushan Bharat-R65777
Cc: Wood Scott-B07421, KVM list, Alexander Graf,
kvm-ppc@vger.kernel.org, qemu-devel qemu-devel,
qemu-ppc@nongnu.org List
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-02 20:31 ` Scott Wood
0 siblings, 0 replies; 32+ messages in thread
From: Scott Wood @ 2012-08-02 20:31 UTC (permalink / raw)
To: Bhushan Bharat-R65777
Cc: Wood Scott-B07421, KVM list, Alexander Graf,
kvm-ppc@vger.kernel.org, qemu-devel qemu-devel,
qemu-ppc@nongnu.org List
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
^ permalink raw reply [flat|nested] 32+ messages in thread* RE: [PATCH 4/4] Enable kvm emulated watchdog
2012-08-02 20:31 ` Scott Wood
(?)
@ 2012-08-03 0:51 ` Bhushan Bharat-R65777
-1 siblings, 0 replies; 32+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-08-03 0:51 UTC (permalink / raw)
To: Wood Scott-B07421
Cc: KVM list, qemu-ppc@nongnu.org List, Alexander Graf,
kvm-ppc@vger.kernel.org, qemu-devel qemu-devel
DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogV29vZCBTY290dC1CMDc0
MjENCj4gU2VudDogRnJpZGF5LCBBdWd1c3QgMDMsIDIwMTIgMjowMiBBTQ0KPiBUbzogQmh1c2hh
biBCaGFyYXQtUjY1Nzc3DQo+IENjOiBXb29kIFNjb3R0LUIwNzQyMTsgQWxleGFuZGVyIEdyYWY7
IHFlbXUtcHBjQG5vbmdudS5vcmcgTGlzdDsga3ZtLQ0KPiBwcGNAdmdlci5rZXJuZWwub3JnOyBx
ZW11LWRldmVsIHFlbXUtZGV2ZWw7IEtWTSBsaXN0DQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggNC80
XSBFbmFibGUga3ZtIGVtdWxhdGVkIHdhdGNoZG9nDQo+IA0KPiBPbiAwOC8wMi8yMDEyIDEwOjU2
IEFNLCBCaHVzaGFuIEJoYXJhdC1SNjU3Nzcgd3JvdGU6DQo+ID4NCj4gPg0KPiA+PiAtLS0tLU9y
aWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiA+PiBGcm9tOiBXb29kIFNjb3R0LUIwNzQyMQ0KPiA+PiBT
ZW50OiBXZWRuZXNkYXksIEF1Z3VzdCAwMSwgMjAxMiAxMTozMSBQTQ0KPiA+PiBUbzogQmh1c2hh
biBCaGFyYXQtUjY1Nzc3DQo+ID4+IENjOiBBbGV4YW5kZXIgR3JhZjsgcWVtdS1wcGNAbm9uZ251
Lm9yZyBMaXN0Ow0KPiA+PiBrdm0tcHBjQHZnZXIua2VybmVsLm9yZzsgcWVtdS0gZGV2ZWwgcWVt
dS1kZXZlbDsgS1ZNIGxpc3QNCj4gPj4gU3ViamVjdDogUmU6IFtQQVRDSCA0LzRdIEVuYWJsZSBr
dm0gZW11bGF0ZWQgd2F0Y2hkb2cNCj4gPj4NCj4gPj4gT24gMDgvMDEvMjAxMiAxMjoyNyBQTSwg
Qmh1c2hhbiBCaGFyYXQtUjY1Nzc3IHdyb3RlOg0KPiA+Pj4NCj4gPj4+DQo+ID4+Pj4gLS0tLS1P
cmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gPj4+PiBGcm9tOiBBbGV4YW5kZXIgR3JhZiBbbWFpbHRv
OmFncmFmQHN1c2UuZGVdDQo+ID4+Pj4gU2VudDogV2VkbmVzZGF5LCBBdWd1c3QgMDEsIDIwMTIg
Nzo1NyBBTQ0KPiA+Pj4+IFRvOiBCaHVzaGFuIEJoYXJhdC1SNjU3NzcNCj4gPj4+PiBDYzogcWVt
dS1wcGNAbm9uZ251Lm9yZyBMaXN0OyBrdm0tcHBjQHZnZXIua2VybmVsLm9yZzsgQmh1c2hhbg0K
PiA+Pj4+IEJoYXJhdC1SNjU3Nzc7IHFlbXUtZGV2ZWwgcWVtdS1kZXZlbDsgS1ZNIGxpc3QNCj4g
Pj4+PiBTdWJqZWN0OiBSZTogW1BBVENIIDQvNF0gRW5hYmxlIGt2bSBlbXVsYXRlZCB3YXRjaGRv
Zw0KPiA+Pj4+DQo+ID4+Pj4NCj4gPj4+PiBPbiAyMC4wNy4yMDEyLCBhdCAwNzoyMywgQmhhcmF0
IEJodXNoYW4gd3JvdGU6DQo+ID4+Pj4+IEBAIC0zODQsNiArNDM3LDE1IEBAIGludCBrdm1fYXJj
aF9pbml0X3ZjcHUoQ1BVUFBDU3RhdGUgKmNlbnYpDQo+ID4+Pj4+ICAgICAgICAgcmV0dXJuIHJl
dDsNCj4gPj4+Pj4gICAgIH0NCj4gPj4+Pj4NCj4gPj4+Pj4gKyAgICBpZiAoZW5hYmxlX3dhdGNo
ZG9nX3N1cHBvcnQpIHsNCj4gPj4+Pj4gKyAgICAgICAgcmV0ID0ga3ZtX3dhdGNoZG9nX2VuYWJs
ZShjZW52KTsNCj4gPj4+Pg0KPiA+Pj4+IERvIHlvdSB0aGluayB0aGlzIGlzIGEgZ29vZCBpZGVh
PyBXaHkgd291bGQgcmVhbCBoYXJkd2FyZSBub3QNCj4gPj4+PiBpbXBsZW1lbnQgYSB3YXRjaGRv
ZyBqdXN0IGJlY2F1c2UgdGhlIHVzZXIgZGlkbid0IHNlbGVjdCBhbiBhY3Rpb24/DQo+ID4+Pg0K
PiA+Pj4gSWYgdGhlcmUgaXMgbm8gd2F0Y2hkb2cgYWN0aW9uIHRoZW4gd2h5IHdlIHdhbnQgdG8g
cnVuIHdhdGNoZG9nIHRpbWVyPw0KPiA+Pg0KPiA+PiBPbiByZWFsIGhhcmR3YXJlLCBpZiBzb2Z0
d2FyZSBzZXRzIFdSQyB0byBhIG5vbi16ZXJvIHZhbHVlLCB0aGUNCj4gPj4gd2F0Y2hkb2cgYWN0
aW9uIGlzIGEgc3lzdGVtIHJlc2V0LiAgVGhlIHVzZXIgZG9lc24ndCBoYXZlIHRvIGRvIGFueXRo
aW5nDQo+IHNwZWNpYWwuDQo+ID4NCj4gPiBTY290dCwgbWF5YmUgSSBtaXN1bmRlcnN0b29kIHlv
dSBjb21tZW50LCBkaWQgbm90IHlvdSBjb21tZW50ZWQgdG8NCj4gPiBlbmFibGUga3ZtIHdhdGNo
ZG9nIGlmIHRoZXJlIGlzIHdhdGNoZG9nIGFjdGlvbiBwcm92aWRlZCBieSB1c2UuDQo+IA0KPiBJ
IGNoYW5nZWQgbXkgbWluZC4gOi0pDQo+IA0KPiBUaGUgbWFpbiBkaWZmZXJlbmNlIGJldHdlZW4g
dGhlIHVzZXIgc3BlY2lmaWNhbGx5IGFza2luZyBmb3IgYW4gYWN0aW9uIG9mIHN5c3RlbQ0KPiBy
ZXNldCwgYW5kIHRoZSB1c2VyIG5vdCBhc2tpbmcgZm9yIGFueXRoaW5nLCBpcyB0aGF0IG9ubHkg
aW4gdGhlIGZvcm1lciBjYXNlDQo+IHNob3VsZCB3ZSBlcnJvciBvdXQgaWYgd2F0Y2hkb2cgZnVu
Y3Rpb25hbGl0eSBpc24ndCBhdmFpbGFibGUgLS0gYnV0IGlmIGl0J3MgYQ0KPiBwYWluIHRvIGRp
c3Rpbmd1aXNoLCBkb24ndCBlcnJvciBvdXQgaW4gZWl0aGVyIGNhc2UuDQoNCjotKSAuLiAgb2sg
DQoNCi1CaGFyYXQNCg0KPiANCj4gLVNjb3R0DQoNCg=
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-03 0:51 ` Bhushan Bharat-R65777
0 siblings, 0 replies; 32+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-08-03 0:51 UTC (permalink / raw)
To: Wood Scott-B07421
Cc: KVM list, qemu-ppc@nongnu.org List, Alexander Graf,
kvm-ppc@vger.kernel.org, qemu-devel qemu-devel
> -----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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-03 0:51 ` Bhushan Bharat-R65777
0 siblings, 0 replies; 32+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-08-03 0:51 UTC (permalink / raw)
To: Wood Scott-B07421
Cc: KVM list, qemu-ppc@nongnu.org List, Alexander Graf,
kvm-ppc@vger.kernel.org, qemu-devel qemu-devel
> -----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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/4] Enable kvm emulated watchdog
2012-08-01 18:00 ` Scott Wood
(?)
@ 2012-08-02 15:58 ` Alexander Graf
-1 siblings, 0 replies; 32+ messages in thread
From: Alexander Graf @ 2012-08-02 15:58 UTC (permalink / raw)
To: Scott Wood
Cc: Bhushan Bharat-R65777, qemu-ppc@nongnu.org List,
kvm-ppc@vger.kernel.org, qemu-devel qemu-devel, KVM list
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-02 15:58 ` Alexander Graf
0 siblings, 0 replies; 32+ messages in thread
From: Alexander Graf @ 2012-08-02 15:58 UTC (permalink / raw)
To: Scott Wood
Cc: KVM list, qemu-ppc@nongnu.org List, qemu-devel qemu-devel,
kvm-ppc@vger.kernel.org, Bhushan Bharat-R65777
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-02 15:58 ` Alexander Graf
0 siblings, 0 replies; 32+ messages in thread
From: Alexander Graf @ 2012-08-02 15:58 UTC (permalink / raw)
To: Scott Wood
Cc: Bhushan Bharat-R65777, qemu-ppc@nongnu.org List,
kvm-ppc@vger.kernel.org, qemu-devel qemu-devel, KVM list
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 4/4] Enable kvm emulated watchdog
2012-08-01 17:27 ` Bhushan Bharat-R65777
(?)
@ 2012-08-02 15:56 ` Alexander Graf
-1 siblings, 0 replies; 32+ messages in thread
From: Alexander Graf @ 2012-08-02 15:56 UTC (permalink / raw)
To: Bhushan Bharat-R65777
Cc: qemu-ppc@nongnu.org List, qemu-devel qemu-devel,
kvm-ppc@vger.kernel.org, KVM list
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-02 15:56 ` Alexander Graf
0 siblings, 0 replies; 32+ messages in thread
From: Alexander Graf @ 2012-08-02 15:56 UTC (permalink / raw)
To: Bhushan Bharat-R65777
Cc: qemu-ppc@nongnu.org List, qemu-devel qemu-devel,
kvm-ppc@vger.kernel.org, KVM list
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-02 15:56 ` Alexander Graf
0 siblings, 0 replies; 32+ messages in thread
From: Alexander Graf @ 2012-08-02 15:56 UTC (permalink / raw)
To: Bhushan Bharat-R65777
Cc: qemu-ppc@nongnu.org List, qemu-devel qemu-devel,
kvm-ppc@vger.kernel.org, KVM list
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* RE: [PATCH 4/4] Enable kvm emulated watchdog
2012-08-01 2:27 ` Alexander Graf
(?)
@ 2012-08-01 17:36 ` Bhushan Bharat-R65777
-1 siblings, 0 replies; 32+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-08-01 17:36 UTC (permalink / raw)
To: Alexander Graf
Cc: qemu-ppc@nongnu.org List, kvm-ppc@vger.kernel.org,
qemu-devel qemu-devel, KVM list
> -----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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-01 17:36 ` Bhushan Bharat-R65777
0 siblings, 0 replies; 32+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-08-01 17:36 UTC (permalink / raw)
To: Alexander Graf
Cc: qemu-ppc@nongnu.org List, qemu-devel qemu-devel,
kvm-ppc@vger.kernel.org, KVM list
> -----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
^ permalink raw reply [flat|nested] 32+ messages in thread* RE: [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-01 17:36 ` Bhushan Bharat-R65777
0 siblings, 0 replies; 32+ messages in thread
From: Bhushan Bharat-R65777 @ 2012-08-01 17:36 UTC (permalink / raw)
To: Alexander Graf
Cc: qemu-ppc@nongnu.org List, kvm-ppc@vger.kernel.org,
qemu-devel qemu-devel, KVM list
> -----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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 4/4] Enable kvm emulated watchdog
2012-08-01 17:36 ` Bhushan Bharat-R65777
(?)
@ 2012-08-02 15:57 ` Alexander Graf
-1 siblings, 0 replies; 32+ messages in thread
From: Alexander Graf @ 2012-08-02 15:57 UTC (permalink / raw)
To: Bhushan Bharat-R65777
Cc: qemu-ppc@nongnu.org List, kvm-ppc@vger.kernel.org,
qemu-devel qemu-devel, KVM list
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [Qemu-devel] [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-02 15:57 ` Alexander Graf
0 siblings, 0 replies; 32+ messages in thread
From: Alexander Graf @ 2012-08-02 15:57 UTC (permalink / raw)
To: Bhushan Bharat-R65777
Cc: qemu-ppc@nongnu.org List, qemu-devel qemu-devel,
kvm-ppc@vger.kernel.org, KVM list
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 4/4] Enable kvm emulated watchdog
@ 2012-08-02 15:57 ` Alexander Graf
0 siblings, 0 replies; 32+ messages in thread
From: Alexander Graf @ 2012-08-02 15:57 UTC (permalink / raw)
To: Bhushan Bharat-R65777
Cc: qemu-ppc@nongnu.org List, kvm-ppc@vger.kernel.org,
qemu-devel qemu-devel, KVM list
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
^ permalink raw reply [flat|nested] 32+ messages in thread