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

2012-07-25 Thread Marcelo Tosatti 
On Fri, Jul 20, 2012 at 04:29:08PM -0500, Scott Wood wrote:
 On 07/20/2012 12:00 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.

Why can't you do this in QEMU, given the kernel exits to userspace on
timer configuration?

The QEMU timer handler used to emulate the watchdog then would have to
only read the register value from the vCPU.

  Signed-off-by: Liu Yu yu@freescale.com
  Signed-off-by: Scott Wood scottw...@freescale.com
  Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
  [bharat.bhus...@freescale.com: reworked patch]
 
 Typically the [] note goes immediately before your signoff (but after
 the others).
 
  +static void arm_next_watchdog(struct kvm_vcpu *vcpu)
  +{
  +   unsigned long nr_jiffies;
  +
  +   spin_lock(vcpu-arch.wdt_lock);
  +   nr_jiffies = watchdog_next_timeout(vcpu);
  +   /*
  +* If the number of jiffies of watchdog timer = NEXT_TIMER_MAX_DELTA
  +* then do not run the watchdog timer as this can break timer APIs.
  +*/
  +   if (nr_jiffies  NEXT_TIMER_MAX_DELTA)
  +   mod_timer(vcpu-arch.wdt_timer, jiffies + nr_jiffies);
  +   else
  +   del_timer(vcpu-arch.wdt_timer);
  +   spin_unlock(vcpu-arch.wdt_lock);
  +}
 
 This needs to be an irqsave lock.
 
  @@ -386,13 +387,23 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
   #ifdef CONFIG_KVM_EXIT_TIMING
  mutex_init(vcpu-arch.exit_timing_lock);
   #endif
  -
  +#ifdef CONFIG_BOOKE
  +   spin_lock_init(vcpu-arch.wdt_lock);
  +   /* setup watchdog timer once */
  +   setup_timer(vcpu-arch.wdt_timer, kvmppc_watchdog_func,
  +   (unsigned long)vcpu);
  +#endif
  return 0;
   }
 
 Can you do this in kvmppc_booke_init()?
 
   
   void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu)
   {
  kvmppc_mmu_destroy(vcpu);
  +#ifdef CONFIG_BOOKE
  +   spin_lock(vcpu-arch.wdt_lock);
  +   del_timer(vcpu-arch.wdt_timer);
  +   spin_unlock(vcpu-arch.wdt_lock);
  +#endif
   }
 
 Don't acquire the lock here, but use del_timer_sync().
 
 -Scott
 
 
 --
 To unsubscribe from this list: send the line unsubscribe kvm in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe kvm-ppc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2012-07-25 Thread Scott Wood 
On 07/25/2012 03:37 PM, Marcelo Tosatti wrote:
 On Fri, Jul 20, 2012 at 04:29:08PM -0500, Scott Wood wrote:
 On 07/20/2012 12:00 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.
 
 Why can't you do this in QEMU, given the kernel exits to userspace on
 timer configuration?

 The QEMU timer handler used to emulate the watchdog then would have to
 only read the register value from the vCPU.

Do what specifically in QEMU?  The whole watchdog mechanism?

It's only on final (third) expiry, when a reset is needed, that it exits
to QEMU.  The first expiry sets a bit in TSR that software can clear to
clear the watchdog.  If software doesn't clear it, the second expiry
sets a different bit in TSR that triggers a critical interrupt to the
guest.  If software doesn't clear either of those bits by the time it
expires again, it's reset time (if a reset action is enabled).

The TSR/TCR registers are shared with other timers that are implemented
in the kernel, and it would be more complicated to implement just the
watchdog in QEMU.  Plus, we currently have no interface for exiting to
QEMU for emulation of an SPR.

Currently QEMU on PPC leaves all timekeeping stuff to the kernel, and
I'd like to keep it that way.

-Scott


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

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

2012-07-25 Thread Marcelo Tosatti 
On Wed, Jul 25, 2012 at 06:55:16PM -0500, Scott Wood wrote:
 On 07/25/2012 03:37 PM, Marcelo Tosatti wrote:
  On Fri, Jul 20, 2012 at 04:29:08PM -0500, Scott Wood wrote:
  On 07/20/2012 12:00 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.
  
  Why can't you do this in QEMU, given the kernel exits to userspace on
  timer configuration?
 
  The QEMU timer handler used to emulate the watchdog then would have to
  only read the register value from the vCPU.
 
 Do what specifically in QEMU?  The whole watchdog mechanism?

 It's only on final (third) expiry, when a reset is needed, that it exits
 to QEMU.  The first expiry sets a bit in TSR that software can clear to
 clear the watchdog.  If software doesn't clear it, the second expiry
 sets a different bit in TSR that triggers a critical interrupt to the
 guest.  If software doesn't clear either of those bits by the time it
 expires again, it's reset time (if a reset action is enabled).
 
 The TSR/TCR registers are shared with other timers that are implemented
 in the kernel, and it would be more complicated to implement just the
 watchdog in QEMU.  Plus, we currently have no interface for exiting to
 QEMU for emulation of an SPR.
 
 Currently QEMU on PPC leaves all timekeeping stuff to the kernel, and
 I'd like to keep it that way.
 
 -Scott
 

OK.

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

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

2012-07-24 Thread Scott Wood 
On 07/24/2012 02:45 AM, Bhushan Bharat-R65777 wrote:
 
 
 -Original Message-
 From: Wood Scott-B07421
 Sent: Monday, July 23, 2012 10:00 PM
 To: Bhushan Bharat-R65777
 Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; k...@vger.kernel.org;
 ag...@suse.de
 Subject: Re: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation

 On 07/23/2012 11:04 AM, Bhushan Bharat-R65777 wrote:


 -Original Message-
 From: Wood Scott-B07421
 Sent: Monday, July 23, 2012 9:31 PM
 To: Bhushan Bharat-R65777
 Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; k...@vger.kernel.org;
 ag...@suse.de
 Subject: Re: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation

 On 07/23/2012 10:43 AM, Bhushan Bharat-R65777 wrote:


 -Original Message-
 From: Wood Scott-B07421
 Sent: Monday, July 23, 2012 9:02 PM
 To: Bhushan Bharat-R65777
 Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org;
 k...@vger.kernel.org; ag...@suse.de
 Subject: Re: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation

 On 07/22/2012 11:10 PM, Bhushan Bharat-R65777 wrote:
 @@ -386,13 +387,23 @@ int kvm_arch_vcpu_init(struct kvm_vcpu
 *vcpu) #ifdef CONFIG_KVM_EXIT_TIMING
   mutex_init(vcpu-arch.exit_timing_lock);
  #endif
 -
 +#ifdef CONFIG_BOOKE
 + spin_lock_init(vcpu-arch.wdt_lock);
 + /* setup watchdog timer once */
 + setup_timer(vcpu-arch.wdt_timer, kvmppc_watchdog_func,
 + (unsigned long)vcpu);
 +#endif
   return 0;
  }

 Can you do this in kvmppc_booke_init()?

 I do not think we can do this in kvmppc_booke_init(). Watchdog
 have association with vcpu, while there is no vcpu at 
 kvmppc_booke_init().

 Sorry, I meant kvm_arch_vcpu_setup() in booke.c.

 Any specific reason to move this in above mentioned function? Want
 to avoid
 #ifdef config_booke ?

 Yes, to avoid the ifdef.  We already have a (poorly named) place for
 booke- specific vcpu init.

 Where we want to delete watchdog timer?

 Kvm_arch_vcpu_setup() is in flow of CREATE_VCPU ioctl, I do not see
 DESTROY_VCPU type of code?

 So is it ok to let del_timer_sync() as is with #ifdef config_booke ?

 You could add such a function.  I suggest correcting the naming issue at the
 same time -- have kvm_subarch_vcpu_init() and kvm_subarch_vcpu_free().
 
 I am sorry Scott but I did not get which functions you want to
 rename. What I understood from current code for powerpc is:
 
 kvm_vm_ioctl_create_vcpu()
  |
  |- kvm_arch_vcpu_create()
 |- kvm_arch_vcpu_setup() - This is where you want me to call
 watchdog timer setup. We cannot (require more efforts) rename this as
 this is called from virt/kvm/kvm_main.c. Say if we add, now let us
 figure out where to uninit what is done in this function:

Sigh, I didn't realize this came from generic code.  What, from generic
code's perspective, is the difference between kvm_arch_vcpu_init() and
kvm_arch_vcpu_setup() supposed to be?  They're synonyms!  And as always
in Linux, there's no documentation (that I can find) on what's expected
of functions that have multiple implementations.

Inferring from what the ppc code does, apparently kvm_arch_vcpu_create()
is supposed to call kvm_vcpu_init(), which then calls back into arch
code with kvm_arch_vcpu_init().  Then, after the preempt notifier is
initialized (but not actually activated), generic code goes back to arch
code with kvm_arch_vcpu_setup().  So basically the setup version comes
later.  And PPC KVM arbitrarily decides to use one of these for PPC-wide
init, and the other for subarch init.  This could really use some
untangling (or documentation if there really is a method to the madness).

In any case, we can still put this init code in kvmppc_arch_vcpu_setup()
(i.e. the subarch init), and add a kvmppc_subarch_vcpu_uninit() that we
call from kvm_arch_vcpu_uninit().  Or just move kvm_arch_vcpu_uninit()
into subarch code -- the only thing it does is call kvmppc_mmu_destroy,
which is a subarch hook anyway.

-Scott


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

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

2012-07-23 Thread Scott Wood 
On 07/22/2012 11:10 PM, Bhushan Bharat-R65777 wrote:
 @@ -386,13 +387,23 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 #ifdef CONFIG_KVM_EXIT_TIMING
 mutex_init(vcpu-arch.exit_timing_lock);
  #endif
 -
 +#ifdef CONFIG_BOOKE
 +   spin_lock_init(vcpu-arch.wdt_lock);
 +   /* setup watchdog timer once */
 +   setup_timer(vcpu-arch.wdt_timer, kvmppc_watchdog_func,
 +   (unsigned long)vcpu);
 +#endif
 return 0;
  }

 Can you do this in kvmppc_booke_init()?
 
 I do not think we can do this in kvmppc_booke_init(). Watchdog have
 association with vcpu, while there is no vcpu at
 kvmppc_booke_init().

Sorry, I meant kvm_arch_vcpu_setup() in booke.c.

-Scott


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

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

2012-07-23 Thread Scott Wood 
On 07/21/2012 03:37 AM, Bhushan Bharat-R65777 wrote:
 
 
 -Original Message-
 From: Wood Scott-B07421
 Sent: Saturday, July 21, 2012 2:59 AM
 To: Bhushan Bharat-R65777
 Cc: kvm-ppc@vger.kernel.org; k...@vger.kernel.org; ag...@suse.de; Bhushan 
 Bharat-
 R65777
 Subject: Re: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation

 On 07/20/2012 12:00 AM, Bharat Bhushan wrote:
 +static void arm_next_watchdog(struct kvm_vcpu *vcpu) {
 +   unsigned long nr_jiffies;
 +
 +   spin_lock(vcpu-arch.wdt_lock);
 +   nr_jiffies = watchdog_next_timeout(vcpu);
 +   /*
 +* If the number of jiffies of watchdog timer = NEXT_TIMER_MAX_DELTA
 +* then do not run the watchdog timer as this can break timer APIs.
 +*/
 +   if (nr_jiffies  NEXT_TIMER_MAX_DELTA)
 +   mod_timer(vcpu-arch.wdt_timer, jiffies + nr_jiffies);
 +   else
 +   del_timer(vcpu-arch.wdt_timer);
 +   spin_unlock(vcpu-arch.wdt_lock);
 +}

 This needs to be an irqsave lock.
 
 Ok, I want to understood why irqsave lock should be used here?

 irqsave_lock is used when the critical section within lock can be
 referenced from interrupt context also. What part of critical section
 above can get affected from local irq? Is it jiffies here?

This can be called from the watchdog timer.  Timers run in interrupt
context.

-Scott


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

RE: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation

2012-07-23 Thread Bhushan Bharat-R65777 


 -Original Message-
 From: Wood Scott-B07421
 Sent: Monday, July 23, 2012 9:02 PM
 To: Bhushan Bharat-R65777
 Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; k...@vger.kernel.org;
 ag...@suse.de
 Subject: Re: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation
 
 On 07/22/2012 11:10 PM, Bhushan Bharat-R65777 wrote:
  @@ -386,13 +387,23 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
  #ifdef CONFIG_KVM_EXIT_TIMING
mutex_init(vcpu-arch.exit_timing_lock);
   #endif
  -
  +#ifdef CONFIG_BOOKE
  + spin_lock_init(vcpu-arch.wdt_lock);
  + /* setup watchdog timer once */
  + setup_timer(vcpu-arch.wdt_timer, kvmppc_watchdog_func,
  + (unsigned long)vcpu);
  +#endif
return 0;
   }
 
  Can you do this in kvmppc_booke_init()?
 
  I do not think we can do this in kvmppc_booke_init(). Watchdog have
  association with vcpu, while there is no vcpu at kvmppc_booke_init().
 
 Sorry, I meant kvm_arch_vcpu_setup() in booke.c.

Any specific reason to move this in above mentioned function? Want to avoid 
#ifdef config_booke ?


Thanks
-Bharat

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

2012-07-23 Thread Scott Wood 
On 07/23/2012 10:43 AM, Bhushan Bharat-R65777 wrote:
 
 
 -Original Message-
 From: Wood Scott-B07421
 Sent: Monday, July 23, 2012 9:02 PM
 To: Bhushan Bharat-R65777
 Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; k...@vger.kernel.org;
 ag...@suse.de
 Subject: Re: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation

 On 07/22/2012 11:10 PM, Bhushan Bharat-R65777 wrote:
 @@ -386,13 +387,23 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 #ifdef CONFIG_KVM_EXIT_TIMING
   mutex_init(vcpu-arch.exit_timing_lock);
  #endif
 -
 +#ifdef CONFIG_BOOKE
 + spin_lock_init(vcpu-arch.wdt_lock);
 + /* setup watchdog timer once */
 + setup_timer(vcpu-arch.wdt_timer, kvmppc_watchdog_func,
 + (unsigned long)vcpu);
 +#endif
   return 0;
  }

 Can you do this in kvmppc_booke_init()?

 I do not think we can do this in kvmppc_booke_init(). Watchdog have
 association with vcpu, while there is no vcpu at kvmppc_booke_init().

 Sorry, I meant kvm_arch_vcpu_setup() in booke.c.
 
 Any specific reason to move this in above mentioned function? Want to avoid 
 #ifdef config_booke ?

Yes, to avoid the ifdef.  We already have a (poorly named) place for
booke-specific vcpu init.

-Scott


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

RE: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation

2012-07-23 Thread Bhushan Bharat-R65777 


 -Original Message-
 From: Wood Scott-B07421
 Sent: Monday, July 23, 2012 9:31 PM
 To: Bhushan Bharat-R65777
 Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; k...@vger.kernel.org;
 ag...@suse.de
 Subject: Re: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation
 
 On 07/23/2012 10:43 AM, Bhushan Bharat-R65777 wrote:
 
 
  -Original Message-
  From: Wood Scott-B07421
  Sent: Monday, July 23, 2012 9:02 PM
  To: Bhushan Bharat-R65777
  Cc: Wood Scott-B07421; kvm-ppc@vger.kernel.org; k...@vger.kernel.org;
  ag...@suse.de
  Subject: Re: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation
 
  On 07/22/2012 11:10 PM, Bhushan Bharat-R65777 wrote:
  @@ -386,13 +387,23 @@ int kvm_arch_vcpu_init(struct kvm_vcpu
  *vcpu) #ifdef CONFIG_KVM_EXIT_TIMING
  mutex_init(vcpu-arch.exit_timing_lock);
   #endif
  -
  +#ifdef CONFIG_BOOKE
  +   spin_lock_init(vcpu-arch.wdt_lock);
  +   /* setup watchdog timer once */
  +   setup_timer(vcpu-arch.wdt_timer, kvmppc_watchdog_func,
  +   (unsigned long)vcpu);
  +#endif
  return 0;
   }
 
  Can you do this in kvmppc_booke_init()?
 
  I do not think we can do this in kvmppc_booke_init(). Watchdog have
  association with vcpu, while there is no vcpu at kvmppc_booke_init().
 
  Sorry, I meant kvm_arch_vcpu_setup() in booke.c.
 
  Any specific reason to move this in above mentioned function? Want to avoid
 #ifdef config_booke ?
 
 Yes, to avoid the ifdef.  We already have a (poorly named) place for booke-
 specific vcpu init.

Where we want to delete watchdog timer? 

Kvm_arch_vcpu_setup() is in flow of CREATE_VCPU ioctl, I do not see 
DESTROY_VCPU type of code?

So is it ok to let del_timer_sync() as is with #ifdef config_booke ?

Thanks
-Bharat


N�r��yb�X��ǧv�^�)޺{.n�+jir)w*jg����ݢj/���z�ޖ��2�ޙ�)ߡ�a�����G���h��j:+v���w��٥

RE: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation

2012-07-22 Thread Bhushan Bharat-R65777 


 -Original Message-
 From: Wood Scott-B07421
 Sent: Saturday, July 21, 2012 2:59 AM
 To: Bhushan Bharat-R65777
 Cc: kvm-ppc@vger.kernel.org; k...@vger.kernel.org; ag...@suse.de; Bhushan 
 Bharat-
 R65777
 Subject: Re: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation
 
 On 07/20/2012 12:00 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@freescale.com
  Signed-off-by: Scott Wood scottw...@freescale.com
  Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
  [bharat.bhus...@freescale.com: reworked patch]
 
 Typically the [] note goes immediately before your signoff (but after the
 others).
 
  +static void arm_next_watchdog(struct kvm_vcpu *vcpu) {
  +   unsigned long nr_jiffies;
  +
  +   spin_lock(vcpu-arch.wdt_lock);
  +   nr_jiffies = watchdog_next_timeout(vcpu);
  +   /*
  +* If the number of jiffies of watchdog timer = NEXT_TIMER_MAX_DELTA
  +* then do not run the watchdog timer as this can break timer APIs.
  +*/
  +   if (nr_jiffies  NEXT_TIMER_MAX_DELTA)
  +   mod_timer(vcpu-arch.wdt_timer, jiffies + nr_jiffies);
  +   else
  +   del_timer(vcpu-arch.wdt_timer);
  +   spin_unlock(vcpu-arch.wdt_lock);
  +}
 
 This needs to be an irqsave lock.
 
  @@ -386,13 +387,23 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
  #ifdef CONFIG_KVM_EXIT_TIMING
  mutex_init(vcpu-arch.exit_timing_lock);
   #endif
  -
  +#ifdef CONFIG_BOOKE
  +   spin_lock_init(vcpu-arch.wdt_lock);
  +   /* setup watchdog timer once */
  +   setup_timer(vcpu-arch.wdt_timer, kvmppc_watchdog_func,
  +   (unsigned long)vcpu);
  +#endif
  return 0;
   }
 
 Can you do this in kvmppc_booke_init()?

I do not think we can do this in kvmppc_booke_init(). Watchdog have association 
with vcpu, while there is no vcpu at kvmppc_booke_init().

Thanks
-Bharat

 
 
   void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu)  {
  kvmppc_mmu_destroy(vcpu);
  +#ifdef CONFIG_BOOKE
  +   spin_lock(vcpu-arch.wdt_lock);
  +   del_timer(vcpu-arch.wdt_timer);
  +   spin_unlock(vcpu-arch.wdt_lock);
  +#endif
   }
 
 Don't acquire the lock here, but use del_timer_sync().
 
 -Scott

N�r��yb�X��ǧv�^�)޺{.n�+jir)w*jg����ݢj/���z�ޖ��2�ޙ�)ߡ�a�����G���h��j:+v���w��٥

RE: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation

2012-07-21 Thread Bhushan Bharat-R65777 


 -Original Message-
 From: Wood Scott-B07421
 Sent: Saturday, July 21, 2012 2:59 AM
 To: Bhushan Bharat-R65777
 Cc: kvm-ppc@vger.kernel.org; k...@vger.kernel.org; ag...@suse.de; Bhushan 
 Bharat-
 R65777
 Subject: Re: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation
 
 On 07/20/2012 12:00 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@freescale.com
  Signed-off-by: Scott Wood scottw...@freescale.com
  Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
  [bharat.bhus...@freescale.com: reworked patch]
 
 Typically the [] note goes immediately before your signoff (but after the
 others).

ok

 
  +static void arm_next_watchdog(struct kvm_vcpu *vcpu) {
  +   unsigned long nr_jiffies;
  +
  +   spin_lock(vcpu-arch.wdt_lock);
  +   nr_jiffies = watchdog_next_timeout(vcpu);
  +   /*
  +* If the number of jiffies of watchdog timer = NEXT_TIMER_MAX_DELTA
  +* then do not run the watchdog timer as this can break timer APIs.
  +*/
  +   if (nr_jiffies  NEXT_TIMER_MAX_DELTA)
  +   mod_timer(vcpu-arch.wdt_timer, jiffies + nr_jiffies);
  +   else
  +   del_timer(vcpu-arch.wdt_timer);
  +   spin_unlock(vcpu-arch.wdt_lock);
  +}
 
 This needs to be an irqsave lock.

Ok, I want to understood why irqsave lock should be used here?
irqsave_lock is used when the critical section within lock can be referenced 
from interrupt context also. What part of critical section above can get 
affected from local irq? Is it jiffies here?


 
  @@ -386,13 +387,23 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
  #ifdef CONFIG_KVM_EXIT_TIMING
  mutex_init(vcpu-arch.exit_timing_lock);
   #endif
  -
  +#ifdef CONFIG_BOOKE
  +   spin_lock_init(vcpu-arch.wdt_lock);
  +   /* setup watchdog timer once */
  +   setup_timer(vcpu-arch.wdt_timer, kvmppc_watchdog_func,
  +   (unsigned long)vcpu);
  +#endif
  return 0;
   }
 
 Can you do this in kvmppc_booke_init()?

Ok, so the *_uninit part of code also needed to be moved in respective function.

 
 
   void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu)  {
  kvmppc_mmu_destroy(vcpu);
  +#ifdef CONFIG_BOOKE
  +   spin_lock(vcpu-arch.wdt_lock);
  +   del_timer(vcpu-arch.wdt_timer);
  +   spin_unlock(vcpu-arch.wdt_lock);
  +#endif
   }
 
 Don't acquire the lock here, but use del_timer_sync().

Ok.

Thanks
-Bharat

 
 -Scott

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

2012-07-20 Thread Scott Wood 
On 07/20/2012 12:00 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@freescale.com
 Signed-off-by: Scott Wood scottw...@freescale.com
 Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
 [bharat.bhus...@freescale.com: reworked patch]

Typically the [] note goes immediately before your signoff (but after
the others).

 +static void arm_next_watchdog(struct kvm_vcpu *vcpu)
 +{
 + unsigned long nr_jiffies;
 +
 + spin_lock(vcpu-arch.wdt_lock);
 + nr_jiffies = watchdog_next_timeout(vcpu);
 + /*
 +  * If the number of jiffies of watchdog timer = NEXT_TIMER_MAX_DELTA
 +  * then do not run the watchdog timer as this can break timer APIs.
 +  */
 + if (nr_jiffies  NEXT_TIMER_MAX_DELTA)
 + mod_timer(vcpu-arch.wdt_timer, jiffies + nr_jiffies);
 + else
 + del_timer(vcpu-arch.wdt_timer);
 + spin_unlock(vcpu-arch.wdt_lock);
 +}

This needs to be an irqsave lock.

 @@ -386,13 +387,23 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
  #ifdef CONFIG_KVM_EXIT_TIMING
   mutex_init(vcpu-arch.exit_timing_lock);
  #endif
 -
 +#ifdef CONFIG_BOOKE
 + spin_lock_init(vcpu-arch.wdt_lock);
 + /* setup watchdog timer once */
 + setup_timer(vcpu-arch.wdt_timer, kvmppc_watchdog_func,
 + (unsigned long)vcpu);
 +#endif
   return 0;
  }

Can you do this in kvmppc_booke_init()?

  
  void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu)
  {
   kvmppc_mmu_destroy(vcpu);
 +#ifdef CONFIG_BOOKE
 + spin_lock(vcpu-arch.wdt_lock);
 + del_timer(vcpu-arch.wdt_timer);
 + spin_unlock(vcpu-arch.wdt_lock);
 +#endif
  }

Don't acquire the lock here, but use del_timer_sync().

-Scott


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

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

2012-07-19 Thread Bharat Bhushan 
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@freescale.com
Signed-off-by: Scott Wood scottw...@freescale.com
Signed-off-by: Bharat Bhushan bharat.bhus...@freescale.com
[bharat.bhus...@freescale.com: reworked patch]

---
v5:
 - Checking that TSR_ENW/TSR_WIS are still set on KVM_EXIT_WATCHDOG.
 - Moved watchdog_next_timeout() in lock.

v4:
 - in v3 i forgot to add Scott Wood and Liu Yu signoff

v3:
 - Using KVM_REQ_WATCHDOG for userspace exit.
 - TSR changes left for vcpu thread.
 - Other review comments on v2

 arch/powerpc/include/asm/kvm_host.h  |3 +
 arch/powerpc/include/asm/kvm_ppc.h   |3 +
 arch/powerpc/include/asm/reg_booke.h |7 ++
 arch/powerpc/kvm/booke.c |  140 ++
 arch/powerpc/kvm/booke_emulate.c |8 ++
 arch/powerpc/kvm/powerpc.c   |   19 +-
 include/linux/kvm.h  |2 +
 include/linux/kvm_host.h |1 +
 8 files changed, 182 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_host.h 
b/arch/powerpc/include/asm/kvm_host.h
index 50ea12f..43cac48 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -467,6 +467,8 @@ struct kvm_vcpu_arch {
ulong fault_esr;
ulong queued_dear;
ulong queued_esr;
+   spinlock_t wdt_lock;
+   struct timer_list wdt_timer;
u32 tlbcfg[4];
u32 mmucfg;
u32 epr;
@@ -482,6 +484,7 @@ struct kvm_vcpu_arch {
u8 osi_needed;
u8 osi_enabled;
u8 papr_enabled;
+   u8 watchdog_enabled;
u8 sane;
u8 cpu_type;
u8 hcall_needed;
diff --git a/arch/powerpc/include/asm/kvm_ppc.h 
b/arch/powerpc/include/asm/kvm_ppc.h
index 0124937..e5cf4b9 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -67,6 +67,7 @@ extern int kvmppc_emulate_mmio(struct kvm_run *run, struct 
kvm_vcpu *vcpu);
 extern void kvmppc_emulate_dec(struct kvm_vcpu *vcpu);
 extern u32 kvmppc_get_dec(struct kvm_vcpu *vcpu, u64 tb);
 extern void kvmppc_decrementer_func(unsigned long data);
+extern void kvmppc_watchdog_func(unsigned long data);
 extern int kvmppc_sanity_check(struct kvm_vcpu *vcpu);
 
 /* Core-specific hooks */
@@ -104,6 +105,8 @@ extern void kvmppc_core_queue_external(struct kvm_vcpu 
*vcpu,
struct kvm_interrupt *irq);
 extern void kvmppc_core_dequeue_external(struct kvm_vcpu *vcpu,
  struct kvm_interrupt *irq);
+extern void kvmppc_core_queue_watchdog(struct kvm_vcpu *vcpu);
+extern void kvmppc_core_dequeue_watchdog(struct kvm_vcpu *vcpu);
 
 extern int kvmppc_core_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
   unsigned int op, int *advance);
diff --git a/arch/powerpc/include/asm/reg_booke.h 
b/arch/powerpc/include/asm/reg_booke.h
index 2d916c4..e07e6af 100644
--- a/arch/powerpc/include/asm/reg_booke.h
+++ b/arch/powerpc/include/asm/reg_booke.h
@@ -539,6 +539,13 @@
 #define TCR_FIE0x0080  /* FIT Interrupt Enable */
 #define TCR_ARE0x0040  /* Auto Reload Enable */
 
+#ifdef CONFIG_E500
+#define TCR_GET_WP(tcr)  tcr)  0xC000)  30) | \
+ (((tcr)  0x1E)  15))
+#else
+#define TCR_GET_WP(tcr)  (((tcr)  0xC000)  30)
+#endif
+
 /* Bit definitions for the TSR. */
 #define TSR_ENW0x8000  /* Enable Next Watchdog */
 #define TSR_WIS0x4000  /* WDT Interrupt Status */
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index d25a097..71b4ce9 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -206,6 +206,16 @@ void kvmppc_core_dequeue_external(struct kvm_vcpu *vcpu,
clear_bit(BOOKE_IRQPRIO_EXTERNAL_LEVEL, vcpu-arch.pending_exceptions);
 }
 
+void kvmppc_core_queue_watchdog(struct kvm_vcpu *vcpu)
+{
+   kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_WATCHDOG);
+}
+
+void kvmppc_core_dequeue_watchdog(struct kvm_vcpu *vcpu)
+{
+   clear_bit(BOOKE_IRQPRIO_WATCHDOG, vcpu-arch.pending_exceptions);
+}
+
 static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32 srr1)
 {
 #ifdef CONFIG_KVM_BOOKE_HV
@@ -325,6 +335,7 @@ static int kvmppc_booke_irqprio_deliver(struct kvm_vcpu 
*vcpu,
msr_mask = MSR_CE | MSR_ME | MSR_DE;
int_class = INT_CLASS_NONCRIT;
break;
+   case BOOKE_IRQPRIO_WATCHDOG:
case BOOKE_IRQPRIO_CRITICAL:
case BOOKE_IRQPRIO_DBELL_CRIT:
allowed = vcpu-arch.shared-msr  MSR_CE;
@@ -404,12 +415,113