[PATCH 1/2] lguest: handle dodgy/non-existent TSC. Host code.

[PATCH 1/2] lguest: handle dodgy/non-existent TSC. Host code.

[Rusty Russell]

Lguest currently requires a TSC, which breaks older machines and Matt
Mackall who boots the host with "notsc".  In addition, there is no
good solution to changing TSC speeds (informing all the guests about
the TSC impending change before it happens would be a great deal of
code and have issues with disobedient guests).

This patch makes the host determine if the TSC is both constant and
stable: if not, it doesn't tell the guest to use it.  We export
check_tsc_unstable() for this.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 arch/i386/kernel/tsc.c      |    3 ++-
 drivers/lguest/core.c       |    8 --------
 drivers/lguest/hypercalls.c |    9 +++++++++
 include/asm-i386/tsc.h      |    1 +
 4 files changed, 12 insertions(+), 9 deletions(-)

===================================================================
--- a/arch/i386/kernel/tsc.c
+++ b/arch/i386/kernel/tsc.c
@@ -59,10 +59,11 @@ __setup("notsc", tsc_setup);
  */
 static int tsc_unstable;
 
-static inline int check_tsc_unstable(void)
+int check_tsc_unstable(void)
 {
 	return tsc_unstable;
 }
+EXPORT_SYMBOL_GPL(check_tsc_unstable);
 
 unsigned long native_calculate_cpu_khz(void)
 {
===================================================================
--- a/drivers/lguest/core.c
+++ b/drivers/lguest/core.c
@@ -332,14 +332,6 @@ int run_guest(struct lguest *lg, unsigne
 			continue;
 		}
 
-		/* If the Guest hasn't been told the clock multiplier to use or
-		 * it's changed, we update it here. */
-		if (unlikely(lg->tsc_khz != tsc_khz) && lg->lguest_data) {
-			lg->tsc_khz = tsc_khz;
-			if (put_user(lg->tsc_khz, &lg->lguest_data->tsc_khz))
-				return -EFAULT;
-		}
-
 		local_irq_disable();
 
 		/* Even if *we* don't want FPU trap, guest might... */
===================================================================
--- a/drivers/lguest/hypercalls.c
+++ b/drivers/lguest/hypercalls.c
@@ -132,11 +132,19 @@ static void do_async_hcalls(struct lgues
 
 static void initialize(struct lguest *lg)
 {
+	u32 tsc_speed;
+
 	if (lg->regs->eax != LHCALL_LGUEST_INIT) {
 		kill_guest(lg, "hypercall %li before LGUEST_INIT",
 			   lg->regs->eax);
 		return;
 	}
+
+	/* We only tell the guest to use the TSC if it's reliable. */
+	if (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) && !check_tsc_unstable())
+		tsc_speed = tsc_khz;
+	else
+		tsc_speed = 0;
 
 	lg->lguest_data = (struct lguest_data __user *)lg->regs->edx;
 	/* We check here so we can simply copy_to_user/from_user */
@@ -148,6 +156,7 @@ static void initialize(struct lguest *lg
 	    || get_user(lg->noirq_end, &lg->lguest_data->noirq_end)
 	    /* We reserve the top pgd entry. */
 	    || put_user(4U*1024*1024, &lg->lguest_data->reserve_mem)
+	    || put_user(tsc_speed, &lg->lguest_data->tsc_khz)
 	    || put_user(lg->guestid, &lg->lguest_data->guestid))
 		kill_guest(lg, "bad guest page %p", lg->lguest_data);
 
===================================================================
--- a/include/asm-i386/tsc.h
+++ b/include/asm-i386/tsc.h
@@ -63,6 +63,7 @@ extern void mark_tsc_unstable(char *reas
 extern void mark_tsc_unstable(char *reason);
 extern int unsynchronized_tsc(void);
 extern void init_tsc_clocksource(void);
+int check_tsc_unstable(void);
 
 /*
  * Boot-time check whether the TSCs are synchronized across

[PATCH 2/2] lguest: handle dodgy/non-existent TSC. Guest code.

[Rusty Russell]

We create an "lguest_clock" which the guest uses: either TSC or
jiffies, depending on whether the host tells us the TSC frequency.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/lguest/lguest.c |   51 +++++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 25 deletions(-)

===================================================================
--- a/drivers/lguest/lguest.c
+++ b/drivers/lguest/lguest.c
@@ -212,8 +212,8 @@ static void lguest_cpuid(unsigned int *e
 	case 1:	/* Basic feature request. */
 		/* We only allow kernel to see SSE3, CMPXCHG16B and SSSE3 */
 		*ecx &= 0x00002201;
-		/* SSE, SSE2, FXSR, MMX, CMOV, CMPXCHG8B, TSC, FPU. */
-		*edx &= 0x07808111;
+		/* SSE, SSE2, FXSR, MMX, CMOV, CMPXCHG8B, FPU. */
+		*edx &= 0x07808101;
 		/* Host wants to know when we flush kernel pages: set PGE. */
 		*edx |= 0x00002000;
 		break;
@@ -348,16 +348,19 @@ static unsigned long lguest_get_wallcloc
 	return hcall(LHCALL_GET_WALLCLOCK, 0, 0, 0);
 }
 
-/* This is what we tell the kernel is our clocksource.  It's the normal "Time
- * Stamp Counter": the Host tells us what speed it's going at. */
+static cycle_t lguest_clock_read(void)
+{
+	if (lguest_data.tsc_khz)
+		return native_read_tsc();
+	else
+		return jiffies;
+}
+
+/* This is what we tell the kernel is our clocksource.  */
 static struct clocksource lguest_clock = {
 	.name		= "lguest",
 	.rating		= 400,
-	.read		= native_read_tsc,
-	.mask		= CLOCKSOURCE_MASK(64),
-	.mult		= 0, /* to be set */
-	.shift		= 22,
-	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
+	.read		= lguest_clock_read,
 };
 
 /* We also need a "struct clock_event_device": Linux asks us to set it to go
@@ -414,14 +417,6 @@ static void lguest_time_irq(unsigned int
 {
 	unsigned long flags;
 
-	/* Check in case host TSC has changed rate. */
-	if (unlikely(tsc_khz != lguest_data.tsc_khz)) {
-		tsc_khz = lguest_data.tsc_khz;
-		lguest_clock.mult = clocksource_khz2mult(tsc_khz, 22);
-		__get_cpu_var(sc_data).cyc2ns_scale
-			= (1000000 << CYC2NS_SCALE_FACTOR) / tsc_khz;
-	}
-
 	/* Don't interrupt us while this is running. */
 	local_irq_save(flags);
 	lguest_clockevent.event_handler(&lguest_clockevent);
@@ -432,7 +427,20 @@ static void lguest_time_init(void)
 {
 	set_irq_handler(0, lguest_time_irq);
 
-	lguest_clock.mult = clocksource_khz2mult(tsc_khz, 22);
+	/* We use the TSC if the Host tells us we can, otherwise a dumb
+	 * jiffies-based clock. */
+	if (lguest_data.tsc_khz) {
+		lguest_clock.shift = 22;
+		lguest_clock.mult = clocksource_khz2mult(lguest_data.tsc_khz,
+							 lguest_clock.shift);
+		lguest_clock.mask = CLOCKSOURCE_MASK(64);
+		lguest_clock.flags = CLOCK_SOURCE_IS_CONTINUOUS;
+	} else {
+		/* To understand this, start at kernel/time/jiffies.c... */
+		lguest_clock.shift = 8;
+		lguest_clock.mult = (((u64)NSEC_PER_SEC<<8)/ACTHZ) << 8;
+		lguest_clock.mask = CLOCKSOURCE_MASK(32);
+	}
 	clocksource_register(&lguest_clock);
 
 	/* We can't set cpumask in the initializer: damn C limitations! */
@@ -440,12 +448,6 @@ static void lguest_time_init(void)
 	clockevents_register_device(&lguest_clockevent);
 
 	enable_lguest_irq(0);
-}
-
-static unsigned long lguest_get_cpu_khz(void)
-{
-	/* The Host tells us the TSC speed */
-	return lguest_data.tsc_khz;
 }
 
 static void lguest_load_esp0(struct tss_struct *tss,
@@ -584,7 +586,6 @@ __init void lguest_init(void *boot)
 	paravirt_ops.time_init = lguest_time_init;
 	paravirt_ops.set_lazy_mode = lguest_lazy_mode;
 	paravirt_ops.wbinvd = lguest_wbinvd;
-	paravirt_ops.get_cpu_khz = lguest_get_cpu_khz;
 
 	hcall(LHCALL_LGUEST_INIT, __pa(&lguest_data), 0, 0);

Re: [PATCH 2/2] lguest: handle dodgy/non-existent TSC. Guest code.

[Tony Breeds]

On Wed, Jul 04, 2007 at 04:20:33PM +1000, Rusty Russell wrote:
> We create an "lguest_clock" which the guest uses: either TSC or
> jiffies, depending on whether the host tells us the TSC frequency.
> 
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  drivers/lguest/lguest.c |   51 +++++++++++++++++++++++------------------------
>  1 file changed, 26 insertions(+), 25 deletions(-)
> 
> ===================================================================

Hi Rusty,

<snip>

> +static cycle_t lguest_clock_read(void)
> +{
> +	if (lguest_data.tsc_khz)
> +		return native_read_tsc();
> +	else
> +		return jiffies;
> +}
> +
> +/* This is what we tell the kernel is our clocksource.  */

<snip>

> +	/* We use the TSC if the Host tells us we can, otherwise a dumb
> +	 * jiffies-based clock. */
> +	if (lguest_data.tsc_khz) {
> +		lguest_clock.shift = 22;
> +		lguest_clock.mult = clocksource_khz2mult(lguest_data.tsc_khz,
> +							 lguest_clock.shift);
> +		lguest_clock.mask = CLOCKSOURCE_MASK(64);
> +		lguest_clock.flags = CLOCK_SOURCE_IS_CONTINUOUS;
> +	} else {
> +		/* To understand this, start at kernel/time/jiffies.c... */
> +		lguest_clock.shift = 8;
> +		lguest_clock.mult = (((u64)NSEC_PER_SEC<<8)/ACTHZ) << 8;
> +		lguest_clock.mask = CLOCKSOURCE_MASK(32);
> +	}
>  	clocksource_register(&lguest_clock);

Why bother installing an lguest_clock which is the same as jiffies?
Wouldn't it be better to just use the system provided jiffies
clocksource in the case where you haven't been provided with
lguest_data.tsc_khz ?

Yours Tony

  linux.conf.au        http://linux.conf.au/ || http://lca2008.linux.org.au/
  Jan 28 - Feb 02 2008 The Australian Linux Technical Conference!

Re: [PATCH 2/2] lguest: handle dodgy/non-existent TSC. Guest code.

[Rusty Russell]

On Wed, 2007-07-04 at 16:44 +1000, Tony Breeds wrote:
> Why bother installing an lguest_clock which is the same as jiffies?
> Wouldn't it be better to just use the system provided jiffies
> clocksource in the case where you haven't been provided with
> lguest_data.tsc_khz ?

Hi Tony!

	Yes, but jiffies is the lowest-rated clock (so PIT will get chosen).  I
initially mugged clocksource_jiffies.rating, but that felt wrong: it's
currently not registered when this code runs so it's a simple
assignment, but if code order was to change it would have to be a call
to clocksource_change_rating().

There's an internal clocksource_override which I could use, but I'd have
to make it non-static.

Maybe John has thoughts?

Thanks,
Rusty.

Re: [PATCH 2/2] lguest: handle dodgy/non-existent TSC. Guest code.

[john stultz]

On Wed, 2007-07-04 at 18:10 +1000, Rusty Russell wrote:
> On Wed, 2007-07-04 at 16:44 +1000, Tony Breeds wrote:
> > Why bother installing an lguest_clock which is the same as jiffies?
> > Wouldn't it be better to just use the system provided jiffies
> > clocksource in the case where you haven't been provided with
> > lguest_data.tsc_khz ?
> 
> 	Yes, but jiffies is the lowest-rated clock (so PIT will get chosen).  I
> initially mugged clocksource_jiffies.rating, but that felt wrong: it's
> currently not registered when this code runs so it's a simple
> assignment, but if code order was to change it would have to be a call
> to clocksource_change_rating().
> 
> There's an internal clocksource_override which I could use, but I'd have
> to make it non-static.
> 
> Maybe John has thoughts?

I tend to prefer to have many simple clocksources then a few complex
ones that try to handle everything. So adding the lguest clocksource,
even if its the same as jiffies, isn't an issue to me. They're small
anyway, and this allows you to add changes as needed w/o affecting the
normal jiffies clocksource.

Hijacking the rating value would work, but that further complicates the
normal selection paths (which is already complex enough), so I'd avoid
it.

The last apprach of making boot_override_clocksource non-static and
using it to force the clocksource wanted would work. But I think that
opens it up too much for abuse.

thanks
-john

Re: [PATCH 2/2] lguest: handle dodgy/non-existent TSC. Guest code.

[Jeremy Fitzhardinge]

Rusty Russell wrote:
> 	Yes, but jiffies is the lowest-rated clock (so PIT will get chosen).  I
> initially mugged clocksource_jiffies.rating, but that felt wrong: it's
> currently not registered when this code runs so it's a simple
> assignment, but if code order was to change it would have to be a call
> to clocksource_change_rating().
>
> There's an internal clocksource_override which I could use, but I'd have
> to make it non-static.
>
> Maybe John has thoughts?

How about having lguest_tsc and lguest_jiffies clocksources, with the 
approprate ratings to get them selected when needed?  And lguest_jiffies 
could probably just be a new structure which points to the normal 
jiffies clocksource function(s).

    J

Re: [PATCH 1/2] lguest: handle dodgy/non-existent TSC. Host code.

[Matt Mackall]

On Wed, Jul 04, 2007 at 04:19:32PM +1000, Rusty Russell wrote:
> Lguest currently requires a TSC, which breaks older machines and Matt
> Mackall who boots the host with "notsc".

I do? I temporarily had a "notsc" arg in my lguest launcher script,
but removing it made no difference. I do* boot the host with NO_HZ though.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 1/2] lguest: handle dodgy/non-existent TSC. Host code.

[Rusty Russell]

On Wed, 2007-07-04 at 08:20 -0500, Matt Mackall wrote:
> On Wed, Jul 04, 2007 at 04:19:32PM +1000, Rusty Russell wrote:
> > Lguest currently requires a TSC, which breaks older machines and Matt
> > Mackall who boots the host with "notsc".
> 
> I do? I temporarily had a "notsc" arg in my lguest launcher script,
> but removing it made no difference. I do* boot the host with NO_HZ though.

NO_HZ should be OK, but last we spoke, "notsc" in the host caused
lguest_data.tsc_khz to be zero => divide by zero errors (the previous
fixes should sort out the other TSC issues).

Anyway, if the host decides not to use the TSC, the guest shouldn't use
it, which is the basis of this patch...

Hope that clarifies,
Rusty.

Re: [PATCH 1/2] lguest: handle dodgy/non-existent TSC. Host code.

[Matt Mackall]

On Thu, Jul 05, 2007 at 11:38:45AM +1000, Rusty Russell wrote:
> On Wed, 2007-07-04 at 08:20 -0500, Matt Mackall wrote:
> > On Wed, Jul 04, 2007 at 04:19:32PM +1000, Rusty Russell wrote:
> > > Lguest currently requires a TSC, which breaks older machines and Matt
> > > Mackall who boots the host with "notsc".
> > 
> > I do? I temporarily had a "notsc" arg in my lguest launcher script,
> > but removing it made no difference. I do* boot the host with NO_HZ though.
> 
> NO_HZ should be OK, but last we spoke, "notsc" in the host caused
> lguest_data.tsc_khz to be zero => divide by zero errors (the previous
> fixes should sort out the other TSC issues).

I did indeed have a zero tsc_khz, and my system does mark TSC unstable
(falling back to acpi-pm). But I've never once booted the host kernel
on this laptop with "notsc". I've experimented with booting a guest
with notsc to see if it would help.

As a further datapoint, rc6-mm1 host and guest works. 

> Anyway, if the host decides not to use the TSC, the guest shouldn't use
> it, which is the basis of this patch...

Sure.

-- 
Mathematics is the supreme nostalgia of our time.