Re: [PATCH 2/2 v4] KVM: PPC: booke: Add watchdog emulation

[Scott Wood <scottwood@freescale.com>]

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 <yu.liu@freescale.com>
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>

Please put a note before your signoff stating that it's been modified
since the previous signoffs, such as:

[bharat.bhushan@freescale.com: 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.

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

>  	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