> -----Original Message-----
> From: Alexander Graf [mailto:ag...@suse.de]
> Sent: Tuesday, August 07, 2012 3:38 PM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@vger.kernel.org; k...@vger.kernel.org; Bhushan Bharat-R65777
> Subject: Re: [PATCH 1/2] KVM: PPC: booke: Add watchdog emulation
>
>
> On 03.08.2012, at 07:30, Bharat Bhushan wrote:
>
> > This patch adds the watchdog emulation in KVM. The watchdog
> > emulation is enabled by KVM_ENABLE_CAP(KVM_CAP_PPC_BOOKE_WATCHDOG) 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>
> > [bharat.bhus...@freescale.com: reworked patch]
> > Signed-off-by: Bharat Bhushan <bharat.bhus...@freescale.com>
> > ---
> > v7:
> > - kvmppc_core_dequeue_watchdog() and kvmppc_core_enque_watchdog()
> > are made staic
> > - Clear the KVM_REQ_WATCHDOG request when TSR_ENW | ENW_WIS cleared
> > rather than checking these bits on handling the request.
> > Below changes (pasted for quick understanding)
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> > index 7eedaeb..a0f5922 100644
> > --- a/arch/powerpc/kvm/booke.c
> > +++ b/arch/powerpc/kvm/booke.c
> > @@ -629,8 +629,7 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct
> kvm_vcpu *vcpu)
> > goto out;
> > }
> >
> > - if (kvm_check_request(KVM_REQ_WATCHDOG, vcpu) &&
> > - (vcpu->arch.tsr & (TSR_ENW | TSR_WIS))) {
> > + if (kvm_check_request(KVM_REQ_WATCHDOG, vcpu)) {
> > kvm_run->exit_reason = KVM_EXIT_WATCHDOG;
> > ret = 0;
> > goto out;
> > @@ -1098,8 +1097,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct
> kvm_vcpu *vcpu,
> > }
> > }
> >
> > - if (kvm_check_request(KVM_REQ_WATCHDOG, vcpu) &&
> > - (vcpu->arch.tsr & (TSR_ENW | TSR_WIS))) {
> > + if (kvm_check_request(KVM_REQ_WATCHDOG, vcpu)) {
> > run->exit_reason = KVM_EXIT_WATCHDOG;
> > r = RESUME_HOST | (r & RESUME_FLAG_NV);
> > }
> > @@ -1251,8 +1249,10 @@ static int set_sregs_base(struct kvm_vcpu *vcpu,
> >
> > vcpu->arch.tsr = sregs->u.e.tsr;
> >
> > - if ((old_tsr ^ vcpu->arch.tsr) & (TSR_ENW | TSR_WIS))
> > + if ((old_tsr ^ vcpu->arch.tsr) & (TSR_ENW | TSR_WIS)) {
> > + clear_bit(KVM_REQ_WATCHDOG, &vcpu->requests);
> > arm_next_watchdog(vcpu);
> > + }
> >
> > update_timer_ints(vcpu);
> > }
> > @@ -1434,8 +1434,10 @@ void kvmppc_clr_tsr_bits(struct kvm_vcpu *vcpu, u32
> tsr_bits)
> > * We may have stopped the watchdog due to
> > * being stuck on final expiration.
> > */
> > - if (tsr_bits & (TSR_ENW | TSR_WIS))
> > + if (tsr_bits & (TSR_ENW | TSR_WIS)) {
> > + clear_bit(KVM_REQ_WATCHDOG, &vcpu->requests);
> > arm_next_watchdog(vcpu);
> > + }
> > ----------
> >
> > v6:
> > - Added kvmppc_subarch_vcpu_unit/uninit() for subarch specific
> > initialization
> > - Using spin_lock_irqsave()
> > - Using del_timer_sync().
> >
> > v5:
> > - Checking that TSR_ENW/TSR_WIS are still set on KVM_EXIT_WATCHDOG.
> > - Moved watchdog_next_timeout() in lock.
> >
> > arch/powerpc/include/asm/kvm_host.h | 3 +
> > arch/powerpc/include/asm/kvm_ppc.h | 2 +
> > arch/powerpc/include/asm/reg_booke.h | 7 ++
> > arch/powerpc/kvm/book3s.c | 9 ++
> > arch/powerpc/kvm/booke.c | 158
> > ++++++++++++++++++++++++++++++++++
> > arch/powerpc/kvm/booke_emulate.c | 8 ++
> > arch/powerpc/kvm/powerpc.c | 14 +++-
> > include/linux/kvm.h | 2 +
> > include/linux/kvm_host.h | 1 +
> > 9 files changed, 202 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/kvm_host.h
> b/arch/powerpc/include/asm/kvm_host.h
> > index 572ad01..873ec11 100644
> > --- a/arch/powerpc/include/asm/kvm_host.h
> > +++ b/arch/powerpc/include/asm/kvm_host.h
> > @@ -469,6 +469,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;
> > @@ -484,6 +486,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..1438a5e 100644
> > --- a/arch/powerpc/include/asm/kvm_ppc.h
> > +++ b/arch/powerpc/include/asm/kvm_ppc.h
> > @@ -68,6 +68,8 @@ 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 int kvmppc_sanity_check(struct kvm_vcpu *vcpu);
> > +extern int kvmppc_subarch_vcpu_init(struct kvm_vcpu *vcpu);
> > +extern void kvmppc_subarch_vcpu_uninit(struct kvm_vcpu *vcpu);
> >
> > /* Core-specific hooks */
> >
> > 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_FIE 0x00800000 /* FIT Interrupt Enable */
> > #define TCR_ARE 0x00400000 /* Auto Reload Enable */
> >
> > +#ifdef CONFIG_E500
> > +#define TCR_GET_WP(tcr) ((((tcr) & 0xC0000000) >> 30) | \
> > + (((tcr) & 0x1E0000) >> 15))
> > +#else
> > +#define TCR_GET_WP(tcr) (((tcr) & 0xC0000000) >> 30)
> > +#endif
> > +
> > /* Bit definitions for the TSR. */
> > #define TSR_ENW 0x80000000 /* Enable Next Watchdog */
> > #define TSR_WIS 0x40000000 /* WDT Interrupt Status */
> > diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> > index 3f2a836..e946665 100644
> > --- a/arch/powerpc/kvm/book3s.c
> > +++ b/arch/powerpc/kvm/book3s.c
> > @@ -411,6 +411,15 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
> > return 0;
> > }
> >
> > +int kvmppc_subarch_vcpu_init(struct kvm_vcpu *vcpu)
> > +{
> > + return 0;
> > +}
> > +
> > +void kvmppc_subarch_vcpu_uninit(struct kvm_vcpu *vcpu)
> > +{
> > +}
> > +
> > int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs
> > *regs)
> > {
> > int i;
> > diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> > index d25a097..a0f5922 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);
> > }
> >
> > +static void kvmppc_core_queue_watchdog(struct kvm_vcpu *vcpu)
> > +{
> > + kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_WATCHDOG);
> > +}
> > +
> > +static 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,114 @@ static int kvmppc_booke_irqprio_deliver(struct
> > kvm_vcpu
> *vcpu,
> > return allowed;
> > }
> >
> > +/*
> > + * Return the number of jiffies until the next timeout. If the timeout is
> > + * longer than the NEXT_TIMER_MAX_DELTA, then return NEXT_TIMER_MAX_DELTA
> > + * because the larger value can break the timer APIs.
> > + */
> > +static unsigned long watchdog_next_timeout(struct kvm_vcpu *vcpu)
> > +{
> > + u64 tb, wdt_tb, wdt_ticks = 0;
> > + u64 nr_jiffies = 0;
> > + u32 period = TCR_GET_WP(vcpu->arch.tcr);
> > +
> > + wdt_tb = 1ULL << (63 - period);
> > + tb = get_tb();
> > + /*
> > + * The watchdog timeout will hapeen when TB bit corresponding
> > + * to watchdog will toggle from 0 to 1.
> > + */
> > + if (tb & wdt_tb)
> > + wdt_ticks = wdt_tb;
> > +
> > + wdt_ticks += wdt_tb - (tb & (wdt_tb - 1));
> > +
> > + /* Convert timebase ticks to jiffies */
> > + nr_jiffies = wdt_ticks;
> > +
> > + if (do_div(nr_jiffies, tb_ticks_per_jiffy))
> > + nr_jiffies++;
> > +
> > + return min_t(unsigned long long, nr_jiffies, NEXT_TIMER_MAX_DELTA);
> > +}
> > +
> > +static void arm_next_watchdog(struct kvm_vcpu *vcpu)
> > +{
> > + unsigned long nr_jiffies;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&vcpu->arch.wdt_lock, flags);
> > + 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_irqrestore(&vcpu->arch.wdt_lock, flags);
> > +}
> > +
> > +void kvmppc_watchdog_func(unsigned long data)
> > +{
> > + struct kvm_vcpu *vcpu = (struct kvm_vcpu *)data;
> > + u32 tsr, new_tsr;
> > + int final;
> > +
> > + do {
> > + new_tsr = tsr = vcpu->arch.tsr;
> > + final = 0;
> > +
> > + /* Time out event */
> > + if (tsr & TSR_ENW) {
> > + if (tsr & TSR_WIS)
> > + final = 1;
> > + else
> > + new_tsr = tsr | TSR_WIS;
> > + } else {
> > + new_tsr = tsr | TSR_ENW;
> > + }
> > + } while (cmpxchg(&vcpu->arch.tsr, tsr, new_tsr) != tsr);
> > +
> > + if (new_tsr & TSR_WIS) {
> > + smp_wmb();
> > + kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu);
> > + kvm_vcpu_kick(vcpu);
> > + }
> > +
> > + /*
> > + * If this is final watchdog expiry and some action is required
> > + * then exit to userspace.
> > + */
> > + if (final && (vcpu->arch.tcr & TCR_WRC_MASK) &&
> > + vcpu->arch.watchdog_enabled) {
> > + smp_wmb();
> > + kvm_make_request(KVM_REQ_WATCHDOG, vcpu);
> > + kvm_vcpu_kick(vcpu);
> > + }
> > +
> > + /*
> > + * Stop running the watchdog timer after final expiration to
> > + * prevent the host from being flooded with timers if the
> > + * guest sets a short period.
> > + * Timers will resume when TSR/TCR is updated next time.
> > + */
> > + if (!final)
> > + arm_next_watchdog(vcpu);
> > +}
> > +
> > static void update_timer_ints(struct kvm_vcpu *vcpu)
> > {
> > if ((vcpu->arch.tcr & TCR_DIE) && (vcpu->arch.tsr & TSR_DIS))
> > kvmppc_core_queue_dec(vcpu);
> > else
> > kvmppc_core_dequeue_dec(vcpu);
> > +
> > + if ((vcpu->arch.tcr & TCR_WIE) && (vcpu->arch.tsr & TSR_WIS))
> > + kvmppc_core_queue_watchdog(vcpu);
> > + else
> > + kvmppc_core_dequeue_watchdog(vcpu);
> > }
> >
> > static void kvmppc_core_check_exceptions(struct kvm_vcpu *vcpu)
> > @@ -516,6 +629,12 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct
> kvm_vcpu *vcpu)
> > goto out;
> > }
> >
> > + if (kvm_check_request(KVM_REQ_WATCHDOG, vcpu)) {
> > + kvm_run->exit_reason = KVM_EXIT_WATCHDOG;
> > + ret = 0;
> > + goto out;
> > + }
> > +
> > kvm_guest_enter();
> >
> > #ifdef CONFIG_PPC_FPU
> > @@ -978,6 +1097,11 @@ int kvmppc_handle_exit(struct kvm_run *run, struct
> kvm_vcpu *vcpu,
> > }
> > }
> >
> > + if (kvm_check_request(KVM_REQ_WATCHDOG, vcpu)) {
> > + run->exit_reason = KVM_EXIT_WATCHDOG;
> > + r = RESUME_HOST | (r & RESUME_FLAG_NV);
> > + }
> > +
> > return r;
> > }
> >
> > @@ -1011,6 +1135,21 @@ int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
> > return r;
> > }
> >
> > +int kvmppc_subarch_vcpu_init(struct kvm_vcpu *vcpu)
> > +{
> > + /* setup watchdog timer once */
> > + spin_lock_init(&vcpu->arch.wdt_lock);
> > + setup_timer(&vcpu->arch.wdt_timer, kvmppc_watchdog_func,
> > + (unsigned long)vcpu);
> > +
> > + return 0;
> > +}
> > +
> > +void kvmppc_subarch_vcpu_uninit(struct kvm_vcpu *vcpu)
> > +{
> > + del_timer_sync(&vcpu->arch.wdt_timer);
> > +}
> > +
> > int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs
> > *regs)
> > {
> > int i;
> > @@ -1106,7 +1245,15 @@ static int set_sregs_base(struct kvm_vcpu *vcpu,
> > }
> >
> > if (sregs->u.e.update_special & KVM_SREGS_E_UPDATE_TSR) {
> > + u32 old_tsr = vcpu->arch.tsr;
> > +
> > vcpu->arch.tsr = sregs->u.e.tsr;
> > +
> > + if ((old_tsr ^ vcpu->arch.tsr) & (TSR_ENW | TSR_WIS)) {
> > + clear_bit(KVM_REQ_WATCHDOG, &vcpu->requests);
> > + arm_next_watchdog(vcpu);
> > + }
> > +
> > update_timer_ints(vcpu);
> > }
> >
> > @@ -1267,6 +1414,7 @@ void kvmppc_core_commit_memory_region(struct kvm *kvm,
> > void kvmppc_set_tcr(struct kvm_vcpu *vcpu, u32 new_tcr)
> > {
> > vcpu->arch.tcr = new_tcr;
> > + arm_next_watchdog(vcpu);
> > update_timer_ints(vcpu);
> > }
> >
> > @@ -1281,6 +1429,16 @@ void kvmppc_set_tsr_bits(struct kvm_vcpu *vcpu, u32
> tsr_bits)
> > void kvmppc_clr_tsr_bits(struct kvm_vcpu *vcpu, u32 tsr_bits)
> > {
> > clear_bits(tsr_bits, &vcpu->arch.tsr);
> > +
> > + /*
> > + * We may have stopped the watchdog due to
> > + * being stuck on final expiration.
> > + */
> > + if (tsr_bits & (TSR_ENW | TSR_WIS)) {
> > + clear_bit(KVM_REQ_WATCHDOG, &vcpu->requests);
> > + arm_next_watchdog(vcpu);
>
> Just a thought: Would it make sense to fold the clear_bit into
> arm_next_watchdog()?
We want to clear_bit(REQ_WATCHDOG) only when ENW or WIS are cleared.
Arm_next_watchdog() is called from some more places where these bit are not
cleared.
We can do in arm_next_watchdog() in something like this:
if (vcpu->arch.tsr & (TSR_ENW | TSR_WIS) != TSR_ENW | TSR_WIS)
clear_bit(KVM_REQ_WATCHDOG, &vcpu->requests);
Thanks
-Bharat
>
> Otherwise looks good to me :)
>
>
> Alex
>
--
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
