Re: [PATCH] High resolution time/TSC patch v3

[Robert Millan <rmh@aybabtu.com>]

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

On Mon, Jul 28, 2008 at 10:05:33AM -0700, Colin D Bennett wrote:
> Another updated TSC patch.  Now it detects TSC support for x86 CPUs at
> runtime and selects either the TSC or RTC time source.  This way 386
> and 486 CPUs without RDTSC instruction support are supported.
> 
> Robert Millan was interested in getting this patch merged for the
> Coreboot port, so I decided to take another crack at it.

Very nice.  Thanks for taking the time to send this.

It needed some adjustments in order to build in Coreboot (see attached
patch).

Unfortunately, on runtime the value returned is always 0.  I didn't think
this would be related to firmware in some way, but it is.  Maybe some hardware
needs to be initialized?  Or perhaps it's a QEMU bug.

> +static __inline int
> +grub_cpu_is_cpuid_supported (void)
> +{
> +  grub_uint32_t id_supported;
> +
> +  __asm__ ("pushfl\n\t"
> +           "popl %%eax             /* Get EFLAGS into EAX */\n\t"
> +           "movl %%eax, %%ecx      /* Save original flags in ECX */\n\t"
> +           "xorl $0x200000, %%eax  /* Flip ID bit in EFLAGS */\n\t"
> +           "pushl %%eax            /* Store modified EFLAGS on stack */\n\t"
> +           "popfl                  /* Replace current EFLAGS */\n\t"
> +           "pushfl                 /* Read back the EFLAGS */\n\t"
> +           "popl %%eax             /* Get EFLAGS into EAX */\n\t"
> +           "xorl %%ecx, %%eax      /* Check if flag could be modified */\n\t"
> +           : "=a" (id_supported)
> +           : /* No inputs.  */
> +           : /* Clobbered:  */ "%rcx");
> +
> +  return id_supported != 0;
> +}

There's similar code in commands/i386/cpuid.c.  I'd suggest harmonizing them.

> +static __inline int
> +grub_cpu_is_tsc_supported (void)
> +{
> +  if (! grub_cpu_is_cpuid_supported ())
> +    return 0;
> +
> +  grub_uint32_t features;
> +  __asm__ ("movl $1, %%eax\n\t"
> +           "cpuid"
> +           : "=d" (features)
> +           : /* No inputs.  */
> +           : /* Clobbered:  */ "%rax", "%rbx", "%rcx");
> +  return (features & (1 << 4)) != 0;
> +}

Maybe the same would make sense here.  How about a generic "is_flag_supported"
function?

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

[-- Attachment #2: coreboot.diff --]
[-- Type: text/x-diff, Size: 1012 bytes --]

diff -x .svn -x '*.mk' -ur grub2.old/conf/i386-coreboot.rmk grub2/conf/i386-coreboot.rmk
--- grub2.old/conf/i386-coreboot.rmk	2008-07-28 19:51:19.000000000 +0200
+++ grub2/conf/i386-coreboot.rmk	2008-07-28 19:44:19.000000000 +0200
@@ -16,7 +16,7 @@
 	kern/main.c kern/device.c \
 	kern/disk.c kern/dl.c kern/file.c kern/fs.c kern/err.c \
 	kern/misc.c kern/mm.c kern/loader.c kern/rescue.c kern/term.c \
-	kern/time.c \
+	kern/time.c kern/generic/rtc_get_time_ms.c kern/generic/millisleep.c \
 	kern/i386/dl.c kern/parser.c kern/partition.c \
 	kern/env.c \
 	term/i386/pc/console.c \
diff -x .svn -x '*.mk' -ur grub2.old/kern/i386/linuxbios/init.c grub2/kern/i386/linuxbios/init.c
--- grub2.old/kern/i386/linuxbios/init.c	2008-07-28 19:51:19.000000000 +0200
+++ grub2/kern/i386/linuxbios/init.c	2008-07-28 19:44:02.000000000 +0200
@@ -61,11 +61,6 @@
 }
 
 void
-grub_millisleep (grub_uint32_t ms __attribute__ ((unused)))
-{
-}
-
-void
 grub_exit (void)
 {
   grub_printf ("grub_exit() is not implemented.\n");