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

[Marcelo Tosatti <mtosatti@redhat.com>]

On Fri, Jul 20, 2012 at 04:29:08PM -0500, Scott Wood wrote:
> On 07/20/2012 12:00 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.

Why can't you do this in QEMU, given the kernel exits to userspace on
timer configuration?

The QEMU timer handler used to emulate the watchdog then would have to
only read the register value from the vCPU.

> > 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>
> > [bharat.bhushan@freescale.com: reworked patch]
> 
> Typically the [] note goes immediately before your signoff (but after
> the others).
> 
> > +static void arm_next_watchdog(struct kvm_vcpu *vcpu)
> > +{
> > +	unsigned long nr_jiffies;
> > +
> > +	spin_lock(&vcpu->arch.wdt_lock);
> > +	nr_jiffies = watchdog_next_timeout(vcpu);
> > +	/*
> > +	 * If the number of jiffies of watchdog timer >= NEXT_TIMER_MAX_DELTA
> > +	 * then do not run the watchdog timer as this can break timer APIs.
> > +	 */
> > +	if (nr_jiffies < NEXT_TIMER_MAX_DELTA)
> > +		mod_timer(&vcpu->arch.wdt_timer, jiffies + nr_jiffies);
> > +	else
> > +		del_timer(&vcpu->arch.wdt_timer);
> > +	spin_unlock(&vcpu->arch.wdt_lock);
> > +}
> 
> This needs to be an irqsave lock.
> 
> > @@ -386,13 +387,23 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
> >  #ifdef CONFIG_KVM_EXIT_TIMING
> >  	mutex_init(&vcpu->arch.exit_timing_lock);
> >  #endif
> > -
> > +#ifdef CONFIG_BOOKE
> > +	spin_lock_init(&vcpu->arch.wdt_lock);
> > +	/* setup watchdog timer once */
> > +	setup_timer(&vcpu->arch.wdt_timer, kvmppc_watchdog_func,
> > +		    (unsigned long)vcpu);
> > +#endif
> >  	return 0;
> >  }
> 
> Can you do this in kvmppc_booke_init()?
> 
> >  
> >  void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu)
> >  {
> >  	kvmppc_mmu_destroy(vcpu);
> > +#ifdef CONFIG_BOOKE
> > +	spin_lock(&vcpu->arch.wdt_lock);
> > +	del_timer(&vcpu->arch.wdt_timer);
> > +	spin_unlock(&vcpu->arch.wdt_lock);
> > +#endif
> >  }
> 
> Don't acquire the lock here, but use del_timer_sync().
> 
> -Scott
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html