Patchwork [2/3,v2] Reset qemu timers when guest reset

login
register
mail settings
Submitter Bharat Bhushan
Date Dec. 28, 2012, 5:16 a.m.
Message ID <1356671812-7634-3-git-send-email-bharat.bhushan@freescale.com>
Download mbox | patch
Permalink /patch/208413/
State New
Headers show

Comments

Bharat Bhushan - Dec. 28, 2012, 5:16 a.m.
This patch install the timer reset handler. This will be called when
the guest is reset.

Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
---
 v2: same as v1

 hw/ppc_booke.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
Alexander Graf - Jan. 3, 2013, 2:10 p.m.
On 28.12.2012, at 06:16, Bharat Bhushan wrote:

> This patch install the timer reset handler. This will be called when
> the guest is reset.
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>

Thanks, adjusted to the current QOM'ified code and applied this patch.


Alex
Scott Wood - Jan. 3, 2013, 8:20 p.m.
On 12/27/2012 11:16:51 PM, Bharat Bhushan wrote:
> This patch install the timer reset handler. This will be called when
> the guest is reset.
> 
> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> ---
>  v2: same as v1
> 
>  hw/ppc_booke.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/ppc_booke.c b/hw/ppc_booke.c
> index d51e7fa..837a5b6 100644
> --- a/hw/ppc_booke.c
> +++ b/hw/ppc_booke.c
> @@ -231,6 +231,16 @@ void store_booke_tcr(CPUPPCState *env,  
> target_ulong val)
> 
>  }
> 
> +static void ppc_booke_timer_reset_handle(void *opaque)
> +{
> +    CPUPPCState *env = opaque;
> +
> +    env->spr[SPR_BOOKE_TSR] = 0;
> +    env->spr[SPR_BOOKE_TCR] = 0;
> +
> +    booke_update_irq(env);
> +}

When does KVM_SET_SREGS get called?

-Scott
Alexander Graf - Jan. 3, 2013, 8:33 p.m.
Am 03.01.2013 um 21:20 schrieb Scott Wood <scottwood@freescale.com>:

> On 12/27/2012 11:16:51 PM, Bharat Bhushan wrote:
>> This patch install the timer reset handler. This will be called when
>> the guest is reset.
>> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>> ---
>> v2: same as v1
>> hw/ppc_booke.c |   12 ++++++++++++
>> 1 files changed, 12 insertions(+), 0 deletions(-)
>> diff --git a/hw/ppc_booke.c b/hw/ppc_booke.c
>> index d51e7fa..837a5b6 100644
>> --- a/hw/ppc_booke.c
>> +++ b/hw/ppc_booke.c
>> @@ -231,6 +231,16 @@ void store_booke_tcr(CPUPPCState *env, target_ulong val)
>> }
>> +static void ppc_booke_timer_reset_handle(void *opaque)
>> +{
>> +    CPUPPCState *env = opaque;
>> +
>> +    env->spr[SPR_BOOKE_TSR] = 0;
>> +    env->spr[SPR_BOOKE_TCR] = 0;
>> +
>> +    booke_update_irq(env);
>> +}
> 
> When does KVM_SET_SREGS get called?

That's up to the next patch. 

Alex

> 
> -Scott
Scott Wood - Jan. 3, 2013, 8:37 p.m.
On 01/03/2013 02:33:39 PM, Alexander Graf wrote:
> 
> 
> Am 03.01.2013 um 21:20 schrieb Scott Wood <scottwood@freescale.com>:
> 
> > On 12/27/2012 11:16:51 PM, Bharat Bhushan wrote:
> >> This patch install the timer reset handler. This will be called  
> when
> >> the guest is reset.
> >> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> >> ---
> >> v2: same as v1
> >> hw/ppc_booke.c |   12 ++++++++++++
> >> 1 files changed, 12 insertions(+), 0 deletions(-)
> >> diff --git a/hw/ppc_booke.c b/hw/ppc_booke.c
> >> index d51e7fa..837a5b6 100644
> >> --- a/hw/ppc_booke.c
> >> +++ b/hw/ppc_booke.c
> >> @@ -231,6 +231,16 @@ void store_booke_tcr(CPUPPCState *env,  
> target_ulong val)
> >> }
> >> +static void ppc_booke_timer_reset_handle(void *opaque)
> >> +{
> >> +    CPUPPCState *env = opaque;
> >> +
> >> +    env->spr[SPR_BOOKE_TSR] = 0;
> >> +    env->spr[SPR_BOOKE_TCR] = 0;
> >> +
> >> +    booke_update_irq(env);
> >> +}
> >
> > When does KVM_SET_SREGS get called?
> 
> That's up to the next patch.

The watchdog one?  I don't see any direct connection to this function.   
Does a reset always involve a CPU state change?  If that's what we're  
relying on, it least deserves a code comment in this function.   
Otherwise it looks like no special handling is required when setting  
SREG SPRs in KVM-compatible code (i.e. not code that is only used in  
TCG mode).

-Scott
Alexander Graf - Jan. 3, 2013, 8:48 p.m.
On 03.01.2013, at 21:37, Scott Wood wrote:

> On 01/03/2013 02:33:39 PM, Alexander Graf wrote:
>> Am 03.01.2013 um 21:20 schrieb Scott Wood <scottwood@freescale.com>:
>> > On 12/27/2012 11:16:51 PM, Bharat Bhushan wrote:
>> >> This patch install the timer reset handler. This will be called when
>> >> the guest is reset.
>> >> Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
>> >> ---
>> >> v2: same as v1
>> >> hw/ppc_booke.c |   12 ++++++++++++
>> >> 1 files changed, 12 insertions(+), 0 deletions(-)
>> >> diff --git a/hw/ppc_booke.c b/hw/ppc_booke.c
>> >> index d51e7fa..837a5b6 100644
>> >> --- a/hw/ppc_booke.c
>> >> +++ b/hw/ppc_booke.c
>> >> @@ -231,6 +231,16 @@ void store_booke_tcr(CPUPPCState *env, target_ulong val)
>> >> }
>> >> +static void ppc_booke_timer_reset_handle(void *opaque)
>> >> +{
>> >> +    CPUPPCState *env = opaque;
>> >> +
>> >> +    env->spr[SPR_BOOKE_TSR] = 0;
>> >> +    env->spr[SPR_BOOKE_TCR] = 0;
>> >> +
>> >> +    booke_update_irq(env);
>> >> +}
>> >
>> > When does KVM_SET_SREGS get called?
>> That's up to the next patch.
> 
> The watchdog one?  I don't see any direct connection to this function.  Does a reset always involve a CPU state change?  If that's what we're relying on, it least deserves a code comment in this function.  Otherwise it looks like no special handling is required when setting SREG SPRs in KVM-compatible code (i.e. not code that is only used in TCG mode).

That's why we want an indication to the lower levels that TSR changed. Basically the code would look like:

cpu_synchronize_register_set(BOOKE_TIMERS); /* fetches current TSR, indicates internally that TSR is potentially dirty */
env->spr[TSR] = 0;

at which point the kvm entry code would know that it needs to write back TSR. Today, the infrastructure for that is missing, but Bharat is working on it.


Alex
Bharat Bhushan - Jan. 4, 2013, 1:28 a.m.
> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Friday, January 04, 2013 1:51 AM
> To: Bhushan Bharat-R65777
> Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de; Bhushan Bharat-
> R65777
> Subject: Re: [Qemu-ppc] [PATCH 2/3 v2] Reset qemu timers when guest reset
> 
> On 12/27/2012 11:16:51 PM, Bharat Bhushan wrote:
> > This patch install the timer reset handler. This will be called when
> > the guest is reset.
> >
> > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > ---
> >  v2: same as v1
> >
> >  hw/ppc_booke.c |   12 ++++++++++++
> >  1 files changed, 12 insertions(+), 0 deletions(-)
> >
> > diff --git a/hw/ppc_booke.c b/hw/ppc_booke.c index d51e7fa..837a5b6
> > 100644
> > --- a/hw/ppc_booke.c
> > +++ b/hw/ppc_booke.c
> > @@ -231,6 +231,16 @@ void store_booke_tcr(CPUPPCState *env,
> > target_ulong val)
> >
> >  }
> >
> > +static void ppc_booke_timer_reset_handle(void *opaque) {
> > +    CPUPPCState *env = opaque;
> > +
> > +    env->spr[SPR_BOOKE_TSR] = 0;
> > +    env->spr[SPR_BOOKE_TCR] = 0;
> > +
> > +    booke_update_irq(env);
> > +}
> 
> When does KVM_SET_SREGS get called?

This is part of reset processing and is not cpu_synchronize_state() called before all reset handlers are called and after that post_synchronize will do the KVM_SET_SREGS in kvm_put_registers().

Thanks
-Bharat
Scott Wood - Jan. 4, 2013, 3:58 p.m.
On 01/03/2013 07:28:49 PM, Bhushan Bharat-R65777 wrote:
> 
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Friday, January 04, 2013 1:51 AM
> > To: Bhushan Bharat-R65777
> > Cc: qemu-devel@nongnu.org; qemu-ppc@nongnu.org; agraf@suse.de;  
> Bhushan Bharat-
> > R65777
> > Subject: Re: [Qemu-ppc] [PATCH 2/3 v2] Reset qemu timers when guest  
> reset
> >
> > On 12/27/2012 11:16:51 PM, Bharat Bhushan wrote:
> > > This patch install the timer reset handler. This will be called  
> when
> > > the guest is reset.
> > >
> > > Signed-off-by: Bharat Bhushan <bharat.bhushan@freescale.com>
> > > ---
> > >  v2: same as v1
> > >
> > >  hw/ppc_booke.c |   12 ++++++++++++
> > >  1 files changed, 12 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/hw/ppc_booke.c b/hw/ppc_booke.c index  
> d51e7fa..837a5b6
> > > 100644
> > > --- a/hw/ppc_booke.c
> > > +++ b/hw/ppc_booke.c
> > > @@ -231,6 +231,16 @@ void store_booke_tcr(CPUPPCState *env,
> > > target_ulong val)
> > >
> > >  }
> > >
> > > +static void ppc_booke_timer_reset_handle(void *opaque) {
> > > +    CPUPPCState *env = opaque;
> > > +
> > > +    env->spr[SPR_BOOKE_TSR] = 0;
> > > +    env->spr[SPR_BOOKE_TCR] = 0;
> > > +
> > > +    booke_update_irq(env);
> > > +}
> >
> > When does KVM_SET_SREGS get called?
> 
> This is part of reset processing and is not cpu_synchronize_state()  
> called before all reset handlers are called and after that  
> post_synchronize will do the KVM_SET_SREGS in kvm_put_registers().

cpu_synchronize_state() does not do KVM_SET_SREGS.  I don't see  
"post_synchronize" anywhere in the QEMU sources.

As Alex said, there needs to be a way for this function to set a flag  
that TCR and TSR have been dirtied.

-Scott

Patch

diff --git a/hw/ppc_booke.c b/hw/ppc_booke.c
index d51e7fa..837a5b6 100644
--- a/hw/ppc_booke.c
+++ b/hw/ppc_booke.c
@@ -231,6 +231,16 @@  void store_booke_tcr(CPUPPCState *env, target_ulong val)
 
 }
 
+static void ppc_booke_timer_reset_handle(void *opaque)
+{
+    CPUPPCState *env = opaque;
+
+    env->spr[SPR_BOOKE_TSR] = 0;
+    env->spr[SPR_BOOKE_TCR] = 0;
+
+    booke_update_irq(env);
+}
+
 void ppc_booke_timers_init(CPUPPCState *env, uint32_t freq, uint32_t flags)
 {
     ppc_tb_t *tb_env;
@@ -251,4 +261,6 @@  void ppc_booke_timers_init(CPUPPCState *env, uint32_t freq, uint32_t flags)
         qemu_new_timer_ns(vm_clock, &booke_fit_cb, env);
     booke_timer->wdt_timer =
         qemu_new_timer_ns(vm_clock, &booke_wdt_cb, env);
+
+    qemu_register_reset(ppc_booke_timer_reset_handle, env);
 }