Re: [RFC] High resolution time support using x86 TSC
Colin D Bennett [EMAIL PROTECTED] writes: Hi Marco, On Thu, 03 Jul 2008 20:52:53 +0200 Marco Gerards [EMAIL PROTECTED] wrote: Hi Colin, Colin D Bennett [EMAIL PROTECTED] 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? That would be great. I do not want to drop 486 support just because of this. You could even drop back to a lower granularity of the timer or better: submit code that does the trick for the 486 as well. -- Marco ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RFC] High resolution time support using x86 TSC
On Thu, 03 Jul 2008 20:52:53 +0200 Marco Gerards [EMAIL PROTECTED] wrote: Hi Colin, Colin D Bennett [EMAIL PROTECTED] writes: + * 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? Originally, I had grub_machine_init() call grub_tsc_calibrate(), but these are comments Vesa made to me about that code: Vesa Jääskeläinen [EMAIL PROTECTED] wrote on Mon, 16 Jun 2008 20:34:54 +0300: Colin D Bennett wrote: This week I implemented high resolution timer support using the x86 TSC (see attached patch grub_tsc_2008-06-10.patch). It is calibrated at GRUB startup using the RTC as a reference. The core TSC function is ``grub_get_tsc()`` -- corresponding to ``grub_get_rtc()``, but returning a uint64 value representing the number of CPU cycles elapsed since boot. In most situations, you will want to use ``grub_get_time_ms()`` to get the system time in milliseconds based on the TSC value. The calibration function ``grub_tsc_calibrate()``, calculates the proper scale factor and absolute offset so that the millisecond value represents real time. The ``grub_get_time_ms()`` function is implemented for non-x86 platforms to simply call ``grub_get_time_ms_generic()`` (defined in kern/misc.c), which uses the RTC to get the time in milliseconds. We would probably want to leave that generic out from kernel, and let every platform either use this generic code or implement their own mechanism to do the job. Perhaps we should make own folder for generic stuff that can be included for arch specific build if there is no better replacement. How about calling function like grub_time_init() which would then be platform specific? Then platform can implement whatever way to calibrate (if needed) as long as it provides this grub_get_time_ms() function (also being platform specific). This would make initialization non-specific to arch while leaving more room for implementation. Therefore, I thought this was the right way to do it. Do you want me to instead call grub_time_init() from grub_machine_init()? Regards, Colin signature.asc Description: PGP signature ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RFC] High resolution time support using x86 TSC
Colin D Bennett wrote: On Thu, 03 Jul 2008 20:52:53 +0200 Marco Gerards [EMAIL PROTECTED] wrote: Hi Colin, Colin D Bennett [EMAIL PROTECTED] writes: + * 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? Originally, I had grub_machine_init() call grub_tsc_calibrate(), but these are comments Vesa made to me about that code: Vesa Jääskeläinen [EMAIL PROTECTED] wrote on Mon, 16 Jun 2008 20:34:54 +0300: Colin D Bennett wrote: This week I implemented high resolution timer support using the x86 TSC (see attached patch grub_tsc_2008-06-10.patch). It is calibrated at GRUB startup using the RTC as a reference. The core TSC function is ``grub_get_tsc()`` -- corresponding to ``grub_get_rtc()``, but returning a uint64 value representing the number of CPU cycles elapsed since boot. In most situations, you will want to use ``grub_get_time_ms()`` to get the system time in milliseconds based on the TSC value. The calibration function ``grub_tsc_calibrate()``, calculates the proper scale factor and absolute offset so that the millisecond value represents real time. The ``grub_get_time_ms()`` function is implemented for non-x86 platforms to simply call ``grub_get_time_ms_generic()`` (defined in kern/misc.c), which uses the RTC to get the time in milliseconds. We would probably want to leave that generic out from kernel, and let every platform either use this generic code or implement their own mechanism to do the job. Perhaps we should make own folder for generic stuff that can be included for arch specific build if there is no better replacement. How about calling function like grub_time_init() which would then be platform specific? Then platform can implement whatever way to calibrate (if needed) as long as it provides this grub_get_time_ms() function (also being platform specific). This would make initialization non-specific to arch while leaving more room for implementation. Therefore, I thought this was the right way to do it. Do you want me to instead call grub_time_init() from grub_machine_init()? Hi Colin, That is Ok. My comment was more of calling directly tsc_calibrate which is not generic function at all. You can call grub_time_init() and in there you decide that you use tsc (and then call tsc_calibrate) or something else depending what is available. This way time handling stuff can be reside easily on one module and it is easy to provide generic function to other platforms. Thanks, Vesa Jääskeläinen ___ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel
Re: [RFC] High resolution time support using x86 TSC
Hi Marco,
On Thu, 03 Jul 2008 20:52:53 +0200
Marco Gerards [EMAIL PROTECTED] wrote:
Hi Colin,
Colin D Bennett [EMAIL PROTECTED] 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 = 100;
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),
Re: [RFC] High resolution time support using x86 TSC
Hi Colin, Colin D Bennett [EMAIL PROTECTED] 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. This is great! I also extracted the grub_millisleep_generic() function into kern/generic/millisleep.c and renamed it to grub_millisleep() so that platforms linking to it don't need to define a grub_millisleep() just to call grub_millisleep_generic() anymore. Simply linking in kern/generic/millisleep.c will use the generic implementation. Ok. I have only tested this patch on the i386-pc platform, so if anyone tests it on another platform and finds problems, please let me know and I can try to fix it. Did someone else test it? I can't... === modified file 'ChangeLog' --- ChangeLog 2008-06-21 14:21:03 + +++ ChangeLog 2008-06-23 14:40:22 + @@ -1,3 +1,87 @@ +2008-06-22 Colin D Bennett [EMAIL PROTECTED] + + High resolution timer support. Implemented for i386 CPU using TSC. + Extracted generic grub_millisleep() so it's linked in only as needed. What if TSC is not available? + * conf/i386-efi.rmk: Added TSC high resolution time module, link with + generic grub_millisleep() function. + + * conf/i386-pc.rmk: Likewise. + + * conf/sparc64-ieee1275.rmk: Add kern/generic/millisleep.c and + kern/generic/get_time_ms.c to kernel, to use generic time functions. + + * conf/powerpc-ieee1275.rmk: Add kern/generic/millisleep.c to kernel, + to use generic grub_millisleep() function. + + * kern/generic/get_time_ms.c (grub_get_time_ms): New file. Platform + independent implementation of grub_get_time_ms() using the RTC that + can be linked into a platform's kernel when it does not implement its + own specialized grub_get_time_ms() function. + + * kern/generic/millisleep.c (grub_millisleep): New file. Extracted + from grub_millisleep_generic() in kern/misc.c and renamed. Now it is + not included in the kernel when a platform defines a specialized + grub_millisleep() implementation. + + * include/grub/i386/tsc.h (grub_get_tsc): New file. Inline function + grub_get_tsc() uses x86 RDTSC instruction (available on Pentium+ CPUs) + to read the counter value for the TSC. + + * kern/i386/tsc.c (grub_get_time_ms): x86 TSC support providing a high + resolution clock. + (grub_tsc_calibrate): Static function to calibrate the TSC using RTC. + (grub_time_init): Implemented to call the static grub_tsc_calibrate() + function to calibrate the TSC. + + * include/grub/kernel.h (grub_time_init): Add declaration for the + grub_time_init() platform specific time initialization function. This + function should be implemented by all platforms. If kern/i386/tsc.c + is linked in, it will provide grub_time_init(). + + * include/grub/time.h (grub_get_time_ms): Added grub_get_time_ms() + function to return the current time in millseconds since the epoch. + This supports higher resolution time than grub_get_rtc() on some + platforms such as i386-pc, where the RTC has only about 1/18 s + precision but a higher precision timer such as the TSC is available. + + * kern/i386/efi/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. + + * 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? + (grub_get_time_ms): Implement this required function. Simply uses the + prior implementation of grub_get_rtc(). + (grub_get_rtc): Now calls grub_get_time_ms(), which does the real + work. + (grub_time_init): Define this empty but required function. No further + initialization necessary. + + * kern/sparc64/ieee1275/init.c (grub_millisleep): Don't define + grub_millisleep(), just link with kern/generic/millisleep.c. + (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
