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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, July 19, 2012 7:56 AM
> To: Bhushan Bharat-R65777
> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; agraf@xxxxxxx; 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 <yu.liu@xxxxxxxxxxxxx>
> > Signed-off-by: Scott Wood <scottwood@xxxxxxxxxxxxx>
> > Signed-off-by: Bharat Bhushan <bharat.bhushan@xxxxxxxxxxxxx>
> 
> Please put a note before your signoff stating that it's been modified
> since the previous signoffs, such as:
> 
> [bharat.bhushan@xxxxxxxxxxxxx: 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?

> 
> > +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;
> > +			}
> 
> Unnecessary braces on the inner if/else.
> 
> > +		} 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)) {
> > +		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)
> 
> Blank line between functions
> 
> >  static void kvmppc_core_check_exceptions(struct kvm_vcpu *vcpu)
> > @@ -516,6 +627,13 @@ 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.watchdog_enable) {
> 
> Check .watchdog_enabled when you make the request, not here.
> 
> > +		kvm_run->exit_reason = KVM_EXIT_WATCHDOG;
> > +		ret = 0;
> > +		goto out;
> > +	}
> 
> 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)?

Thanks
-Bharat

> 
> >  	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 | TCR_WRC_MASK))
> > +			arm_next_watchdog(vcpu);
> 
> Do we still do anything with TSR[WRS] at all?
> 
> > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> > index 87f4dc8..3c50b81 100644
> > --- a/arch/powerpc/kvm/powerpc.c
> > +++ b/arch/powerpc/kvm/powerpc.c
> > @@ -38,9 +38,11 @@
> >
> >  int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
> >  {
> > -	return !(v->arch.shared->msr & MSR_WE) ||
> > -	       !!(v->arch.pending_exceptions) ||
> > -	       v->requests;
> > +	bool ret = !(v->arch.shared->msr & MSR_WE) ||
> > +		   !!(v->arch.pending_exceptions) ||
> > +		   v->requests;
> > +
> > +	return ret;
> >  }
> 
> There's no need to change this function anymore.
> 
> > +#define KVM_CAP_PPC_WDT	81
> 
> s/WDT/WATCHDOG/
> 
> or better, s/WDT/BOOKE_WATCHDOG/
> 
> -Scott

��.n��������+%������w��{.n�����o�^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�

[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux