Re: [PATCH 2/6 V2] Add functions to check if the host has stopped the vm

[Eric B Munson <emunson@mgebm.net>]

[-- Attachment #1: Type: text/plain, Size: 2841 bytes --]

On Tue, 01 Nov 2011, Anthony Liguori wrote:

> On 10/31/2011 03:07 PM, Eric B Munson wrote:
> >When a host stops or suspends a VM it will set a flag to show this.  The
> >watchdog will use these functions to determine if a softlockup is real, or the
> >result of a suspended VM.
> >
> >Signed-off-by: Eric B Munson<emunson@mgebm.net>
> >---
> >  arch/x86/include/asm/pvclock.h |    2 ++
> >  arch/x86/kernel/kvmclock.c     |   19 +++++++++++++++++++
> >  2 files changed, 21 insertions(+), 0 deletions(-)
> >
> >diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
> >index c59cc97..7d3ba41 100644
> >--- a/arch/x86/include/asm/pvclock.h
> >+++ b/arch/x86/include/asm/pvclock.h
> >@@ -13,6 +13,8 @@ void pvclock_read_wallclock(struct pvclock_wall_clock *wall,
> >  			    struct timespec *ts);
> >  void pvclock_resume(void);
> >
> >+bool kvm_check_and_clear_host_stopped(int cpu);
> 
> A kvm-specific function shouldn't be declared in pvclock.h as this
> is shared code between KVM and Xen.

I'll move it for V3.

> 
> >+
> >  /*
> >   * Scale a 64-bit delta by scaling and multiplying by a 32-bit fraction,
> >   * yielding a 64-bit result.
> >diff --git a/arch/x86/kernel/kvmclock.c b/arch/x86/kernel/kvmclock.c
> >index c1a0188..8ddcfaf 100644
> >--- a/arch/x86/kernel/kvmclock.c
> >+++ b/arch/x86/kernel/kvmclock.c
> >@@ -113,6 +113,25 @@ static void kvm_get_preset_lpj(void)
> >  	preset_lpj = lpj;
> >  }
> >
> >+bool kvm_check_and_clear_host_stopped(int cpu)
> >+{
> >+	bool ret = false;
> >+	struct pvclock_vcpu_time_info *src;
> >+
> >+	/*
> >+	 * per_cpu() is safe here because this function is only called from
> >+	 * timer functions where preemption is already disabled.
> >+	 */
> >+	WARN_ON(!in_atomic());
> >+	src =&per_cpu(hv_clock, cpu);
> >+	if ((src->flags&  PVCLOCK_GUEST_STOPPED) != 0) {
> >+		src->flags = src->flags&  (~PVCLOCK_GUEST_STOPPED);
> >+		ret = true;
> >+	}
> 
> It would be helpful to separate out the patches for guest enablement
> from host enablement.  This is a guest function, correct?
> 

Yes, this is guest only.

> Don't you need an atomic getandset here?  Can the host clear this
> flag or only set it?

I didn't think they needed to be atomic because an over-write of this means that
a soft lockup message is delayed by the detector's timeout in the worst case.
If you disagree with that assessment, I can move to atomic get/set.

> 
> It's ashame we don't have a proper pvclock spec in Documentation/.
> I would suggest at least adding a description of how this bit works
> in the first patch.
> 



> Regards,
> 
> Anthony Liguori
> 
> >+
> >+	return ret;
> >+}
> >+
> >  static struct clocksource kvm_clock = {
> >  	.name = "kvm-clock",
> >  	.read = kvm_clock_get_cycles,
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]