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

[Alexander Graf]

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

> 
> 
>> -----Original Message-----
>> From: Alexander Graf [mailto:address@hidden
>> Sent: Wednesday, August 01, 2012 7:57 AM
>> To: Bhushan Bharat-R65777
>> Cc: address@hidden List; address@hidden; 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 <address@hidden>
>>> ---
>>> 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