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

[Scott Wood <scottwood@freescale.com>]

On 07/19/2012 12:35 AM, Bhushan Bharat-R65777 wrote:
> 
> 
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Thursday, July 19, 2012 7:56 AM
>> To: Bhushan Bharat-R65777
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; agraf@suse.de; 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 <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.
> 
> Why you think so? Is the next timeout calculation needed to be protected?

The point is to make sure that the timeout is still valid:

	CPU 0				CPU 1
	arm_next_watchdog
	  watchdog_next_timeout
	  spin_lock
					update TCR
					arm_next_watchdog
					  watchdog_next_timeout
					  spin_lock
					  mod timer
					  spin_unlock
	  lock acquired
	  mod_timer
	  spin_unlock

In the above race, you'll end up with the watchdog armed based on the
old TCR.

>> 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)?

Whichever's easier.

-Scott