On Tue, Jan 16, 2018 at 01:16:17PM -0500, Peter Jones wrote: > On my laptop running at 2.4GHz, if I run a VM where tsc calibration > using pmtimer will fail presuming a broken pmtimer, it takes ~51 seconds > to do so (as measured with the stopwatch on my phone), with a tsc delta > of 0x1cd1c85300, or around 125 billion cycles. > > If instead of trying to wait for 5-200ms to show up on the pmtimer, we try > to wait for 5-200us, it decides it's broken in ~0x7998f9e TSCs, aka ~2 > million cycles, or more or less instantly. > > Additionally, this reading the pmtimer was returning 0xffffffff anyway, > and that's obviously an invalid return. I've added a check for that and > 0 so we don't bother waiting for the test if what we're seeing is dead > pins with no response at all. > > Signed-off-by: Peter Jones <pjo...@redhat.com> > --- > grub-core/kern/i386/tsc_pmtimer.c | 43 > ++++++++++++++++++++++++++++++--------- > 1 file changed, 33 insertions(+), 10 deletions(-) > > diff --git a/grub-core/kern/i386/tsc_pmtimer.c > b/grub-core/kern/i386/tsc_pmtimer.c > index c9c36169978..609402b8376 100644 > --- a/grub-core/kern/i386/tsc_pmtimer.c > +++ b/grub-core/kern/i386/tsc_pmtimer.c > @@ -38,30 +38,53 @@ grub_pmtimer_wait_count_tsc (grub_port_t pmtimer, > grub_uint64_t start_tsc; > grub_uint64_t end_tsc; > int num_iter = 0; > + int bad_reads = 0; > > - start = grub_inl (pmtimer) & 0xffffff; > + start = grub_inl (pmtimer) & 0x3fff;
I am not sure why you are changing this to 0x3fff... > last = start; > end = start + num_pm_ticks; > start_tsc = grub_get_tsc (); > while (1) > { > - cur = grub_inl (pmtimer) & 0xffffff; What about 24-bit timers? I would leave this here... > + cur = grub_inl (pmtimer); > + > + /* If we get 10 reads in a row that are obviously dead pins, there's no > + reason to do this thousands of times. > + */ > + if (cur == 0xffffffff || cur == 0) ...and here I would check for 0xffffff and 0. > + { > + bad_reads++; > + grub_dprintf ("pmtimer", "cur: 0x%08x bad_reads: %d\n", cur, > bad_reads); > + > + if (bad_reads == 10) > + return 0; > + } > + else if (bad_reads) > + bad_reads = 0; Do we really need to reset this? > + cur &= 0x3fff; > + > if (cur < last) > - cur |= 0x1000000; > + cur |= 0x4000; > num_iter++; > if (cur >= end) > { > end_tsc = grub_get_tsc (); > + grub_dprintf ("pmtimer", "tsc delta is 0x%016lx\n", > + end_tsc - start_tsc); > return end_tsc - start_tsc; > } > - /* Check for broken PM timer. > - 50000000 TSCs is between 5 ms (10GHz) and 200 ms (250 MHz) > - if after this time we still don't have 1 ms on pmtimer, then > - pmtimer is broken. > + /* Check for broken PM timer. 5000 TSCs is between 5us (10GHz) and ^^^ 500ns? > + 200us (250 MHz). If after this time we still don't have 1us on ^^^^^ 20us? > + pmtimer, then pmtimer is broken. > */ > - if ((num_iter & 0xffffff) == 0 && grub_get_tsc () - start_tsc > > 5000000) { > - return 0; > - } > + end_tsc = grub_get_tsc(); > + if ((num_iter & 0x3fff) == 0 && end_tsc - start_tsc > 5000) Why 0x3fff here which means, AIUI, 4000 iterations? Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel