On 20.07.2012, at 07:23, Bharat Bhushan wrote:
> Enable the KVM emulated watchdog if KVM supports (use the
> capability enablement in watchdog handler). Also watchdog exit
> (KVM_EXIT_WATCHDOG) handling is added.
> Watchdog state machine is cleared whenever VM state changes to running.
> This is to handle the cases like return from debug halt etc.
>
> Signed-off-by: Bharat Bhushan <bharat.bhushan@xxxxxxxxxxxxx>
> ---
> v4: Enbale watchdog support only when user specifies watchdog-action
> Earlier this was [3/3] of the patch series. Now because i separated
> linux-header updation in separate patch, so this become [4/4]
>
> v3:
> - TSR clearing is removed in whatchdog exit handling as this is
> no more needed.
>
> v2:
> - Merged ([PATCH 3/4] Watchdog exit handling support) and
> ([PATCH 4/4] Enable to use kvm emulated watchdog)
> - Clear watchdog state machine when VM state changes to running.
>
> hw/ppc_booke.c | 5 ++++
> sysemu.h | 1 +
> target-ppc/cpu.h | 1 +
> target-ppc/kvm.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> vl.c | 2 +
> 5 files changed, 78 insertions(+), 0 deletions(-)
>
> diff --git a/hw/ppc_booke.c b/hw/ppc_booke.c
> index 837a5b6..478dbcd 100644
> --- a/hw/ppc_booke.c
> +++ b/hw/ppc_booke.c
> @@ -203,6 +203,11 @@ static void booke_wdt_cb(void *opaque)
> booke_timer->wdt_timer);
> }
>
> +void ppc_booke_watchdog_clear_tsr(CPUPPCState *env, target_ulong tsr)
> +{
> + env->spr[SPR_BOOKE_TSR] = tsr & ~(TSR_ENW | TSR_WIS | TSR_WRS_MASK);
> +}
> +
> void store_booke_tsr(CPUPPCState *env, target_ulong val)
> {
> env->spr[SPR_BOOKE_TSR] &= ~val;
> diff --git a/sysemu.h b/sysemu.h
> index bc2c788..fc388b7 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -131,6 +131,7 @@ extern uint8_t *boot_splash_filedata;
> extern int boot_splash_filedata_size;
> extern uint8_t qemu_extra_params_fw[2];
> extern QEMUClock *rtc_clock;
> +extern int enable_watchdog_support;
>
> #define MAX_NODES 64
> extern int nb_numa_nodes;
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index ca2fc21..163389a 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1191,6 +1191,7 @@ void store_40x_dbcr0 (CPUPPCState *env, uint32_t val);
> void store_40x_sler (CPUPPCState *env, uint32_t val);
> void store_booke_tcr (CPUPPCState *env, target_ulong val);
> void store_booke_tsr (CPUPPCState *env, target_ulong val);
> +void ppc_booke_watchdog_clear_tsr(CPUPPCState *env, target_ulong tsr);
> void booke206_flush_tlb(CPUPPCState *env, int flags, const int check_iprot);
> target_phys_addr_t booke206_tlb_to_page_size(CPUPPCState *env, ppcmas_tlb_t *tlb);
> int ppcmas_tlb_check(CPUPPCState *env, ppcmas_tlb_t *tlb,
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index b6ef72d..0226b5e 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -32,6 +32,7 @@
> #include "device_tree.h"
> #include "hw/sysbus.h"
> #include "hw/spapr.h"
> +#include "hw/watchdog.h"
>
> #include "hw/sysbus.h"
> #include "hw/spapr.h"
> @@ -60,6 +61,7 @@ static int cap_booke_sregs;
> static int cap_ppc_smt;
> static int cap_ppc_rma;
> static int cap_spapr_tce;
> +static int cap_ppc_watchdog;
>
> /* XXX We have a race condition where we actually have a level triggered
> * interrupt, but the infrastructure can't expose that yet, so the guest
> @@ -86,6 +88,7 @@ int kvm_arch_init(KVMState *s)
> cap_ppc_smt = kvm_check_extension(s, KVM_CAP_PPC_SMT);
> cap_ppc_rma = kvm_check_extension(s, KVM_CAP_PPC_RMA);
> cap_spapr_tce = kvm_check_extension(s, KVM_CAP_SPAPR_TCE);
> + cap_ppc_watchdog = kvm_check_extension(s, KVM_CAP_PPC_BOOKE_WATCHDOG);
>
> if (!cap_interrupt_level) {
> fprintf(stderr, "KVM: Couldn't find level irq capability. Expect the "
> @@ -168,6 +171,30 @@ static int kvm_booke206_tlb_init(CPUPPCState *env)
> return 0;
> }
>
> +static int kvm_watchdog_enable(CPUPPCState *env)
> +{
> + int ret;
> + struct kvm_enable_cap encap = {};
> +
> + if (!kvm_enabled()) {
> + return 0;
> + }
> +
> + if (!cap_ppc_watchdog) {
> + printf("warning: KVM does not support watchdog");
> + return 0;
> + }
> +
> + encap.cap = KVM_CAP_PPC_BOOKE_WATCHDOG;
> + ret = kvm_vcpu_ioctl(env, KVM_ENABLE_CAP, &encap);
> + if (ret < 0) {
> + fprintf(stderr, "%s: couldn't enable KVM_CAP_PPC_BOOKE_WATCHDOG: %s\n",
> + __func__, strerror(-ret));
> + return ret;
> + }
> +
> + return ret;
> +}
>
> #if defined(TARGET_PPC64)
> static void kvm_get_fallback_smmu_info(CPUPPCState *env,
> @@ -371,6 +398,32 @@ static inline void kvm_fixup_page_sizes(CPUPPCState *env)
>
> #endif /* !defined (TARGET_PPC64) */
>
> +static void cpu_state_change_handler(void *opaque, int running, RunState state)
> +{
> + CPUPPCState *env = opaque;
> +
> + struct kvm_sregs sregs;
= { };
> +
> + if (!running)
> + return;
Did you run checkpatch.pl? You're also missing a comment here which case we want to act upon.
> +
> + /*
> + * Clear watchdog interrupt condition by clearing TSR.
> + * Similar logic needed to be implemented for watchdog
> + * emulation in qemu.
> + */
> + if (cap_booke_sregs && cap_ppc_watchdog) {
> + kvm_vcpu_ioctl(env, KVM_GET_SREGS, &sregs);
> +
> + /* Clear TSR.ENW, TSR.WIS and TSR.WRS */
> + ppc_booke_watchdog_clear_tsr(env, sregs.u.e.tsr);
> + sregs.u.e.tsr = env->spr[SPR_BOOKE_TSR];
> + sregs.u.e.update_special = KVM_SREGS_E_UPDATE_TSR;
> +
> + kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs);
I'd prefer to see that logic implemented in the booke timer code with some special case around it for the kvm enabled case. But the event action is independent of kvm.
> + }
> +}
> +
> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
