Re: [RFC] High resolution time support using x86 TSC

[Colin D Bennett]

Hi Marco,

On Thu, 03 Jul 2008 20:52:53 +0200
Marco Gerards <address@hidden> wrote:

> Hi Colin,
> 
> Colin D Bennett <address@hidden> writes:
> 
> > I have implemented high resolution time support (through the
> > new grub_get_time_ms() function) using the RDTSC instruction
> > available on Pentium and higher x86 CPUs.  The TSC value is simply
> > a 64-bit block cycle counter that is zeroed at bootup, so
> > grub_main() calls grub_time_init(), which is defined by each
> > platform.  If a platform links to kern/i386/tsc.c, then the
> > grub_time_init() function from tsc.c is used, which calibrates the
> > TSC rate and absolute zero reference using the RTC.
> What if TSC is not available?

I updated the changelog entry to indicate that running on a 386 or 486
will fail, since TSC is provided in Pentium+.  Do we support running on
386 or 386?  Should I check for this?  If so, the code will have to
change a bit, and be able to select between the generic implementation
and the TSC implementation at runtime.

I think this would be best done letting the "grub_get_time_ms"
implementation be selected by setting a function pointer in
grub_machine_init() depending on the machine's capabilities.  I would
have to significantly re-structure my patch, but it might be
necessary (and could lead to more understandable code?).
What do you think?

>...
> > +   * kern/i386/pc/init.c (grub_millisleep): Likewise.
> > +
> > +   * kern/ieee1275/init.c (grub_millisleep): Don't define
> > +   grub_millisleep() -- it just called
> > grub_millisleep_generic() but now
> > +   we just need to link with kern/generic/millisleep.c to use
> > the generic
> > +   implementation.
> 
> No need to mention how it has to be linked.  I just assume you made
> this change already?

Yes.  I removed that part of the changelog comment.

>...
> > +   (grub_time_init): Define this required function. Does
> > nothing since
> > +   the generic RTC-based functions are used.
> 
> No need to mention what a function does.  Only describe the changes
> you make.  Other information should go into the file itself as
> comments, if it is important enough.  Here it is only noise...

Ok.  I tried to clean this up.

> > +   * kern/i386/linuxbios/init.c (grub_get_time_ms):
> > +   Define grub_get_time_ms() to always return 0.  This should
> > be fixed
> > +   when RTC functionality is implemented.
> > +   (grub_time_init): Define this required function as a
> > no-op. Inserted
> > +   a comment to remind us delete this function when proper
> > time support 
> > +   is added to i386-linuxbios.
> > +
> > +   * kern/main.c (grub_main): Call grub_time_init() right
> > after
> > +   grub_machine_init().
> 
> I think this should go into grub_machine_init?  Why didn't you just
> add it there?

I commented on this in a separate message a few minutes ago.

> > +grub_uint64_t
> > +grub_get_time_ms (void)
> > +{
> > +  /* FIXME: Delete this function and link to `kern/i386/tsc.c'
> > once RTC
> > +   * is implemented.  See also comment below in grub_time_init().
> > */
> > +  return 0;
> > +}
> 
> Please do not use comments with *'s on each line.

Sorry, it was a habit of mine.  Fixed.

>...
> > +void
> > +grub_time_init (void)
> > +{
> > +  /* FIXME: Delete this function and link with `kern/i386/tsc.c'
> > once RTC
> > +   * is implemented.  Until then, this dummy function simply
> > defines 
> > +   * grub_time_init(), which is called by grub_main() in
> > `kern/main.c'.
> > +   * Without RTC, TSC calibration will hang waiting for an RTC
> > edge. */ +}
> > +
> 
> Same here.  Can you explain what is going on here?

Since grub_main() calls grub_time_init(), it must be defined.  However,
we can't use the TSC implementation of grub_time_init() from
kern/i386/tsc.c, since it will not be able to calibrate (it will hang)
if the RTC always returns the same value, which the i386-linuxbios
kernel does.

>...
> > +/* Reference for TSC=0.  This defines the time since the epoch
>in 
> > + * milliseconds that TSC=0 refers to. */
> > +static grub_uint64_t tsc_boot_time = 0;
> 
> Please do not use a * on each line for comments.  No need to set this
> to zero explicitly.

Ok.

> 
> > +/* TSC rate. TSC ticks per millisecond. 
> > + * Begin with default (incorrect) value of assuming a 1 GHz
> > machine.
> > + * grub_tsc_calibrate() must be called to set this properly. */
> > +static grub_uint64_t tsc_ticks_per_ms = 1000000;
> 
> Same as above.
> 
> The value is not correct.  Why can't we just set it to 0?

I figured that in case calibration was not done, we could at least fake
partial functionality by going with an incorrect value.  However, I
don't initialize tsc_boot_time, then there is no sense in initializing
tsc_ticks_per_ms either, since without both of them set to a valid
value, grub_get_time_ms() will not work.


I just realize I should probably not have chopped up the patch above in
my reply in an attempt to make it short. I hope I didn't make it too
hard to follow. I'll send a new patch shortly for your comments -- it
will include the minor changes you mentioned, but will not address the
issue of supporting 386/486 machines (no TSC), as I discussed at the
beginning of this message.  I await your comments regarding that point.

Regards,
Colin

signature.asc
Description: PGP signature