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@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; agraf@xxxxxxx; 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@xxxxxxxxxxxxx>
>>> Signed-off-by: Scott Wood <scottwood@xxxxxxxxxxxxx>
>>> Signed-off-by: Bharat Bhushan <bharat.bhushan@xxxxxxxxxxxxx>
>>
>> Please put a note before your signoff stating that it's been modified
>> since the previous signoffs, such as:
>>
>> [bharat.bhushan@xxxxxxxxxxxxx: 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
