Patchwork [3/4] Watchdog exit handling support

login
register
mail settings
Submitter Bharat Bhushan
Date June 28, 2012, 5:39 a.m.
Message ID <1340861999-31281-3-git-send-email-Bharat.Bhushan@freescale.com>
Download mbox | patch
Permalink /patch/167793/
State New
Headers show

Comments

Bharat Bhushan - June 28, 2012, 5:39 a.m.
This patch adds the support to handle the exit caused by watchdog
(KVM_EXIT_WDT). In the handling we clear the TSR register.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 hw/ppc_booke.c            |    5 +++++
 linux-headers/linux/kvm.h |    1 +
 target-ppc/cpu.h          |    1 +
 target-ppc/kvm.c          |   38 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 45 insertions(+), 0 deletions(-)
Scott Wood - June 28, 2012, 10:26 p.m.
On 06/28/2012 12:39 AM, Bharat Bhushan wrote:
> This patch adds the support to handle the exit caused by watchdog
> (KVM_EXIT_WDT). In the handling we clear the TSR register.

I'm not sure what the logical split is between this patch and 4/4...

> diff --git a/hw/ppc_booke.c b/hw/ppc_booke.c
> index 837a5b6..a9fba15 100644
> --- a/hw/ppc_booke.c
> +++ b/hw/ppc_booke.c
> @@ -203,6 +203,11 @@ static void booke_wdt_cb(void *opaque)
>                               booke_timer->wdt_timer);
>  }
>  
> +void ppc_booke_wdt_clear_tsr(CPUPPCState *env, target_ulong tsr)
> +{
> +    env->spr[SPR_BOOKE_TSR] = tsr & ~(TSR_ENW | TSR_WIS | TSR_WRS_MASK);
> +}
> +

We should probably call this function before returning to KVM, at least
after we halt for debug, possibly other times.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood - July 5, 2012, 8:27 p.m.
On 07/04/2012 06:13 AM, Bhushan Bharat-R65777 wrote:
> 
> 
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Friday, June 29, 2012 3:57 AM
>> To: Bhushan Bharat-R65777
>> Cc: qemu-ppc@nongnu.org; kvm-ppc@vger.kernel.org; agraf@suse.de; Bhushan Bharat-
>> R65777
>> Subject: Re: [PATCH 3/4] Watchdog exit handling support
>>
>> On 06/28/2012 12:39 AM, Bharat Bhushan wrote:
>>> This patch adds the support to handle the exit caused by watchdog
>>> (KVM_EXIT_WDT). In the handling we clear the TSR register.
>>
>> I'm not sure what the logical split is between this patch and 4/4...
> 
> Ok I will merge 3/4 and 4/4.
> 
>>
>>> diff --git a/hw/ppc_booke.c b/hw/ppc_booke.c index 837a5b6..a9fba15
>>> 100644
>>> --- a/hw/ppc_booke.c
>>> +++ b/hw/ppc_booke.c
>>> @@ -203,6 +203,11 @@ static void booke_wdt_cb(void *opaque)
>>>                               booke_timer->wdt_timer);  }
>>>
>>> +void ppc_booke_wdt_clear_tsr(CPUPPCState *env, target_ulong tsr) {
>>> +    env->spr[SPR_BOOKE_TSR] = tsr & ~(TSR_ENW | TSR_WIS |
>>> +TSR_WRS_MASK); }
>>> +
>>
>> We should probably call this function before returning to KVM, at least after we
>> halt for debug, possibly other times.
> 
> Why clearing watchdog related bits in TSR? This will just reset the
> watchdog state machine. Do we actually want to reset the state
> machine or want watchdog interrupt to not occur when exiting to KVM
> for debug.

We want to prevent long delays in QEMU (especially for debug halt) from
causing watchdog actions (interrupt, reset, etc).  Resetting the state
machine seems like the way to do that.

> Does possibly other times mean except KVM_RUN? 

Not sure what you mean here.  Don't we always use KVM_RUN to enter the
guest?  My point was we definitely want this after we had the guest
halted, and we may want to consider doing it for some other heavyweight
exits.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bharat Bhushan - July 6, 2012, 12:43 a.m.
> -----Original Message-----

> From: Wood Scott-B07421

> Sent: Friday, July 06, 2012 1:57 AM

> To: Bhushan Bharat-R65777

> Cc: Wood Scott-B07421; qemu-ppc@nongnu.org; kvm-ppc@vger.kernel.org;

> agraf@suse.de

> Subject: Re: [PATCH 3/4] Watchdog exit handling support

> 

> On 07/04/2012 06:13 AM, Bhushan Bharat-R65777 wrote:

> >

> >

> >> -----Original Message-----

> >> From: Wood Scott-B07421

> >> Sent: Friday, June 29, 2012 3:57 AM

> >> To: Bhushan Bharat-R65777

> >> Cc: qemu-ppc@nongnu.org; kvm-ppc@vger.kernel.org; agraf@suse.de;

> >> Bhushan Bharat-

> >> R65777

> >> Subject: Re: [PATCH 3/4] Watchdog exit handling support

> >>

> >> On 06/28/2012 12:39 AM, Bharat Bhushan wrote:

> >>> This patch adds the support to handle the exit caused by watchdog

> >>> (KVM_EXIT_WDT). In the handling we clear the TSR register.

> >>

> >> I'm not sure what the logical split is between this patch and 4/4...

> >

> > Ok I will merge 3/4 and 4/4.

> >

> >>

> >>> diff --git a/hw/ppc_booke.c b/hw/ppc_booke.c index 837a5b6..a9fba15

> >>> 100644

> >>> --- a/hw/ppc_booke.c

> >>> +++ b/hw/ppc_booke.c

> >>> @@ -203,6 +203,11 @@ static void booke_wdt_cb(void *opaque)

> >>>                               booke_timer->wdt_timer);  }

> >>>

> >>> +void ppc_booke_wdt_clear_tsr(CPUPPCState *env, target_ulong tsr) {

> >>> +    env->spr[SPR_BOOKE_TSR] = tsr & ~(TSR_ENW | TSR_WIS |

> >>> +TSR_WRS_MASK); }

> >>> +

> >>

> >> We should probably call this function before returning to KVM, at

> >> least after we halt for debug, possibly other times.

> >

> > Why clearing watchdog related bits in TSR? This will just reset the

> > watchdog state machine. Do we actually want to reset the state machine

> > or want watchdog interrupt to not occur when exiting to KVM for debug.

> 

> We want to prevent long delays in QEMU (especially for debug halt) from causing

> watchdog actions (interrupt, reset, etc).  Resetting the state machine seems

> like the way to do that.

> 

> > Does possibly other times mean except KVM_RUN?

> 

> Not sure what you mean here.  Don't we always use KVM_RUN to enter the guest?

> My point was we definitely want this after we had the guest halted, and we may

> want to consider doing it for some other heavyweight exits.


Your initial comment was "We should probably call this function before returning to KVM". Which suggest that ioctls calls made by QEMU to enter KVM. I am a bit confused, now you want to avoid long delays in QEMU? This suggest whenever we exit to qemu, right?

Thanks
=Bharat
> 

> -Scott
Scott Wood - July 6, 2012, 1 a.m.
On 07/05/2012 07:43 PM, Bhushan Bharat-R65777 wrote:
> 
> 
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Friday, July 06, 2012 1:57 AM
>> To: Bhushan Bharat-R65777
>> Cc: Wood Scott-B07421; qemu-ppc@nongnu.org; kvm-ppc@vger.kernel.org;
>> agraf@suse.de
>> Subject: Re: [PATCH 3/4] Watchdog exit handling support
>>
>> On 07/04/2012 06:13 AM, Bhushan Bharat-R65777 wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Wood Scott-B07421
>>>> Sent: Friday, June 29, 2012 3:57 AM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: qemu-ppc@nongnu.org; kvm-ppc@vger.kernel.org; agraf@suse.de;
>>>> Bhushan Bharat-
>>>> R65777
>>>> Subject: Re: [PATCH 3/4] Watchdog exit handling support
>>>>
>>>> On 06/28/2012 12:39 AM, Bharat Bhushan wrote:
>>>>> This patch adds the support to handle the exit caused by watchdog
>>>>> (KVM_EXIT_WDT). In the handling we clear the TSR register.
>>>>
>>>> I'm not sure what the logical split is between this patch and 4/4...
>>>
>>> Ok I will merge 3/4 and 4/4.
>>>
>>>>
>>>>> diff --git a/hw/ppc_booke.c b/hw/ppc_booke.c index 837a5b6..a9fba15
>>>>> 100644
>>>>> --- a/hw/ppc_booke.c
>>>>> +++ b/hw/ppc_booke.c
>>>>> @@ -203,6 +203,11 @@ static void booke_wdt_cb(void *opaque)
>>>>>                               booke_timer->wdt_timer);  }
>>>>>
>>>>> +void ppc_booke_wdt_clear_tsr(CPUPPCState *env, target_ulong tsr) {
>>>>> +    env->spr[SPR_BOOKE_TSR] = tsr & ~(TSR_ENW | TSR_WIS |
>>>>> +TSR_WRS_MASK); }
>>>>> +
>>>>
>>>> We should probably call this function before returning to KVM, at
>>>> least after we halt for debug, possibly other times.
>>>
>>> Why clearing watchdog related bits in TSR? This will just reset the
>>> watchdog state machine. Do we actually want to reset the state machine
>>> or want watchdog interrupt to not occur when exiting to KVM for debug.
>>
>> We want to prevent long delays in QEMU (especially for debug halt) from causing
>> watchdog actions (interrupt, reset, etc).  Resetting the state machine seems
>> like the way to do that.
>>
>>> Does possibly other times mean except KVM_RUN?
>>
>> Not sure what you mean here.  Don't we always use KVM_RUN to enter the guest?
>> My point was we definitely want this after we had the guest halted, and we may
>> want to consider doing it for some other heavyweight exits.
> 
> Your initial comment was "We should probably call this function
> before returning to KVM". Which suggest that ioctls calls made by
> QEMU to enter KVM. I am a bit confused, now you want to avoid long
> delays in QEMU? This suggest whenever we exit to qemu, right?

Whenever we exit to QEMU is one possibility, though it's probably too
aggressive (if the guest gets stuck in a loop that contains a QEMU exit,
the watchdog won't catch it).  Another option is to reset the state
machine only when the guest resumes from a debug halt.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood - July 6, 2012, 1:18 a.m.
On 07/05/2012 08:16 PM, Bhushan Bharat-R65777 wrote:
> 
> 
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Friday, July 06, 2012 6:30 AM
>> To: Bhushan Bharat-R65777
>> Cc: Wood Scott-B07421; qemu-ppc@nongnu.org; kvm-ppc@vger.kernel.org;
>> agraf@suse.de
>> Subject: Re: [PATCH 3/4] Watchdog exit handling support
>>
>> On 07/05/2012 07:43 PM, Bhushan Bharat-R65777 wrote:
>>> Your initial comment was "We should probably call this function before
>>> returning to KVM". Which suggest that ioctls calls made by QEMU to
>>> enter KVM. I am a bit confused, now you want to avoid long delays in
>>> QEMU? This suggest whenever we exit to qemu, right?
>>
>> Whenever we exit to QEMU is one possibility, though it's probably too aggressive
>> (if the guest gets stuck in a loop that contains a QEMU exit, the watchdog won't
>> catch it).  Another option is to reset the state machine only when the guest
>> resumes from a debug halt.
> 
> Ok.
>
> Resetting the state machine does not disable watchdog, it will just
> ensure the longest time for debug halt etc. Is that ok ?

I'm talking about TSR, not TCR.  It's not about disabling the watchdog
or changing the period, but clearing any event that has happened when
leaving debug halt.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/hw/ppc_booke.c b/hw/ppc_booke.c
index 837a5b6..a9fba15 100644
--- a/hw/ppc_booke.c
+++ b/hw/ppc_booke.c
@@ -203,6 +203,11 @@  static void booke_wdt_cb(void *opaque)
                              booke_timer->wdt_timer);
 }
 
+void ppc_booke_wdt_clear_tsr(CPUPPCState *env, target_ulong tsr)
+{
+    env->spr[SPR_BOOKE_TSR] = tsr & ~(TSR_ENW | TSR_WIS | TSR_WRS_MASK);
+}
+
 void store_booke_tsr(CPUPPCState *env, target_ulong val)
 {
     env->spr[SPR_BOOKE_TSR] &= ~val;
diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 4b9e575..c1169ff 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -163,6 +163,7 @@  struct kvm_pit_config {
 #define KVM_EXIT_OSI              18
 #define KVM_EXIT_PAPR_HCALL	  19
 #define KVM_EXIT_S390_UCONTROL	  20
+#define KVM_EXIT_WDT              21
 
 /* For KVM_EXIT_INTERNAL_ERROR */
 #define KVM_INTERNAL_ERROR_EMULATION 1
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index ca2fc21..78212b4 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1191,6 +1191,7 @@  void store_40x_dbcr0 (CPUPPCState *env, uint32_t val);
 void store_40x_sler (CPUPPCState *env, uint32_t val);
 void store_booke_tcr (CPUPPCState *env, target_ulong val);
 void store_booke_tsr (CPUPPCState *env, target_ulong val);
+void ppc_booke_wdt_clear_tsr(CPUPPCState *env, target_ulong tsr);
 void booke206_flush_tlb(CPUPPCState *env, int flags, const int check_iprot);
 target_phys_addr_t booke206_tlb_to_page_size(CPUPPCState *env, ppcmas_tlb_t *tlb);
 int ppcmas_tlb_check(CPUPPCState *env, ppcmas_tlb_t *tlb,
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index b6ef72d..6a37e94 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -32,6 +32,7 @@ 
 #include "device_tree.h"
 #include "hw/sysbus.h"
 #include "hw/spapr.h"
+#include "hw/watchdog.h"
 
 #include "hw/sysbus.h"
 #include "hw/spapr.h"
@@ -743,6 +744,37 @@  static int kvmppc_handle_dcr_write(CPUPPCState *env, uint32_t dcrn, uint32_t dat
     return 0;
 }
 
+static int kvm_arch_handle_watchdog(CPUPPCState *env)
+{
+    int ret;
+    struct kvm_sregs sregs;
+
+    watchdog_perform_action();
+
+    /*
+     * Clear watchdog interrupt condition by clearing TSR.
+     * Similar logic needed to be implemented for watchdog emulation in qemu
+     */
+    if (cap_booke_sregs) {
+        ret = kvm_vcpu_ioctl(env, KVM_GET_SREGS, &sregs);
+        if (ret < 0) {
+            return ret;
+        }
+
+        /* Clear TSR.ENW, TSR.WIS and TSR.WRS */
+        ppc_booke_wdt_clear_tsr(env, sregs.u.e.tsr);
+        sregs.u.e.tsr = env->spr[SPR_BOOKE_TSR];
+
+        sregs.u.e.update_special = KVM_SREGS_E_BASE | KVM_SREGS_E_UPDATE_TSR;
+        ret = kvm_vcpu_ioctl(env, KVM_SET_SREGS, &sregs);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
+    return 0;
+}
+
 int kvm_arch_handle_exit(CPUPPCState *env, struct kvm_run *run)
 {
     int ret;
@@ -769,6 +801,12 @@  int kvm_arch_handle_exit(CPUPPCState *env, struct kvm_run *run)
         ret = 1;
         break;
 #endif
+#ifdef KVM_EXIT_WDT
+    case KVM_EXIT_WDT:
+        dprintf("booke watchdog action\n");
+        ret = kvm_arch_handle_watchdog(env);
+        break;
+#endif
     default:
         fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
         ret = -1;