From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Date: Thu, 19 Jul 2012 19:59:54 +0000 Subject: Re: [PATCH 2/2 v4] KVM: PPC: booke: Add watchdog emulation Message-Id: <5008673A.4010609@freescale.com> List-Id: References: <1342604355-22576-1-git-send-email-Bharat.Bhushan@freescale.com> <50077038.20207@freescale.com> <6A3DF150A5B70D4F9B66A25E3F7C888D03DCDCF0@039-SN2MPN1-023.039d.mgd.msft.net> In-Reply-To: <6A3DF150A5B70D4F9B66A25E3F7C888D03DCDCF0@039-SN2MPN1-023.039d.mgd.msft.net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Bhushan Bharat-R65777 Cc: Wood Scott-B07421 , "kvm-ppc@vger.kernel.org" , "kvm@vger.kernel.org" , "agraf@suse.de" 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 >>> Signed-off-by: Scott Wood >>> Signed-off-by: Bharat Bhushan >> >> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Subject: Re: [PATCH 2/2 v4] KVM: PPC: booke: Add watchdog emulation Date: Thu, 19 Jul 2012 14:59:54 -0500 Message-ID: <5008673A.4010609@freescale.com> References: <1342604355-22576-1-git-send-email-Bharat.Bhushan@freescale.com> <50077038.20207@freescale.com> <6A3DF150A5B70D4F9B66A25E3F7C888D03DCDCF0@039-SN2MPN1-023.039d.mgd.msft.net> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: Wood Scott-B07421 , "kvm-ppc@vger.kernel.org" , "kvm@vger.kernel.org" , "agraf@suse.de" To: Bhushan Bharat-R65777 Return-path: In-Reply-To: <6A3DF150A5B70D4F9B66A25E3F7C888D03DCDCF0@039-SN2MPN1-023.039d.mgd.msft.net> Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org 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 >>> Signed-off-by: Scott Wood >>> Signed-off-by: Bharat Bhushan >> >> 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