> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, July 19, 2012 7:56 AM
> To: Bhushan Bharat-R65777
> Cc: [email protected]; [email protected]; [email protected]; Bhushan
> Bharat-
> R65777
> Subject: Re: [PATCH 2/2 v4] KVM: PPC: booke: Add watchdog emulation
>
> On 07/18/2012 04:39 AM, Bharat Bhushan wrote:
> > This patch adds the watchdog emulation in KVM. The watchdog
> > emulation is enabled by KVM_ENABLE_CAP(KVM_CAP_PPC_WDT) ioctl.
> > The kernel timer are used for watchdog emulation and emulates
> > h/w watchdog state machine. On watchdog timer expiry, it exit to QEMU
> > if TCR.WRC is non ZERO. QEMU can reset/shutdown etc depending upon how
> > it is configured.
> >
> > Signed-off-by: Liu Yu <[email protected]>
> > Signed-off-by: Scott Wood <[email protected]>
> > Signed-off-by: Bharat Bhushan <[email protected]>
>
> Please put a note before your signoff stating that it's been modified
> since the previous signoffs, such as:
>
> [[email protected]: reworked patch]
>
> > @@ -482,6 +484,7 @@ struct kvm_vcpu_arch {
> > u8 osi_needed;
> > u8 osi_enabled;
> > u8 papr_enabled;
> > + u8 watchdog_enable;
>
> s/enable;/enabled;/
>
> > +static void arm_next_watchdog(struct kvm_vcpu *vcpu)
> > +{
> > + unsigned long nr_jiffies;
> > +
> > + nr_jiffies = watchdog_next_timeout(vcpu);
> > + spin_lock(&vcpu->arch.wdt_lock);
>
> Do watchdog_next_timeout() from within the lock.
Why you think so? Is the next timeout calculation needed to be protected?
>
> > +void kvmppc_watchdog_func(unsigned long data)
> > +{
> > + struct kvm_vcpu *vcpu = (struct kvm_vcpu *)data;
> > + u32 tsr, new_tsr;
> > + int final;
> > +
> > + do {
> > + new_tsr = tsr = vcpu->arch.tsr;
> > + final = 0;
> > +
> > + /* Time out event */
> > + if (tsr & TSR_ENW) {
> > + if (tsr & TSR_WIS) {
> > + final = 1;
> > + } else {
> > + new_tsr = tsr | TSR_WIS;
> > + }
>
> Unnecessary braces on the inner if/else.
>
> > + } else {
> > + new_tsr = tsr | TSR_ENW;
> > + }
> > + } while (cmpxchg(&vcpu->arch.tsr, tsr, new_tsr) != tsr);
> > +
> > + if (new_tsr & TSR_WIS) {
> > + smp_wmb();
> > + kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu);
> > + kvm_vcpu_kick(vcpu);
> > + }
> > +
> > + /*
> > + * If this is final watchdog expiry and some action is required
> > + * then exit to userspace.
> > + */
> > + if (final && (vcpu->arch.tcr & TCR_WRC_MASK)) {
> > + smp_wmb();
> > + kvm_make_request(KVM_REQ_WATCHDOG, vcpu);
> > + kvm_vcpu_kick(vcpu);
> > + }
> > +
> > +
> > + /*
> > + * Stop running the watchdog timer after final expiration to
> > + * prevent the host from being flooded with timers if the
> > + * guest sets a short period.
> > + * Timers will resume when TSR/TCR is updated next time.
> > + */
> > + if (!final)
> > + arm_next_watchdog(vcpu);
> > +}
> > static void update_timer_ints(struct kvm_vcpu *vcpu)
>
> Blank line between functions
>
> > static void kvmppc_core_check_exceptions(struct kvm_vcpu *vcpu)
> > @@ -516,6 +627,13 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct
> kvm_vcpu *vcpu)
> > goto out;
> > }
> >
> > + if (kvm_check_request(KVM_REQ_WATCHDOG, vcpu) &&
> > + vcpu->arch.watchdog_enable) {
>
> Check .watchdog_enabled when you make the request, not here.
>
> > + kvm_run->exit_reason = KVM_EXIT_WATCHDOG;
> > + ret = 0;
> > + goto out;
> > + }
>
> I think we should check that ENW/WIS are still set. Now that we don't
> use TSR[WRS], this is the only way QEMU can preemptively clear a pending
> final expiration after leaving debug halt. If QEMU doesn't do this, and
> KVM comes back with a watchdog exit, QEMU won't know if it's a new
> legitimate one, or if it expired during the debug halt. This would be
> less ugly than QEMU doing a reset-TSR[WIS,ENW]-and-ignore on the first
> watchdog exit after a debug halt, and letting the expiration happen
> again if the guest is really stuck.
ahh, What about clearing KVM_REQ_WATCHDOG request when QEMU clears ENW|WIS (or
whenever KVM or QEMU clears ENW|WIS)?
Thanks
-Bharat
>
> > if (sregs->u.e.update_special & KVM_SREGS_E_UPDATE_TSR) {
> > + u32 old_tsr = vcpu->arch.tsr;
> > +
> > vcpu->arch.tsr = sregs->u.e.tsr;
> > +
> > + if ((old_tsr ^ vcpu->arch.tsr) &
> > + (TSR_ENW | TSR_WIS | TCR_WRC_MASK))
> > + arm_next_watchdog(vcpu);
>
> Do we still do anything with TSR[WRS] at all?
>
> > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> > index 87f4dc8..3c50b81 100644
> > --- a/arch/powerpc/kvm/powerpc.c
> > +++ b/arch/powerpc/kvm/powerpc.c
> > @@ -38,9 +38,11 @@
> >
> > int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
> > {
> > - return !(v->arch.shared->msr & MSR_WE) ||
> > - !!(v->arch.pending_exceptions) ||
> > - v->requests;
> > + bool ret = !(v->arch.shared->msr & MSR_WE) ||
> > + !!(v->arch.pending_exceptions) ||
> > + v->requests;
> > +
> > + return ret;
> > }
>
> There's no need to change this function anymore.
>
> > +#define KVM_CAP_PPC_WDT 81
>
> s/WDT/WATCHDOG/
>
> or better, s/WDT/BOOKE_WATCHDOG/
>
> -Scott
