* [PATCH] Make pmtimer tsc calibration not take 51 seconds to fail. @ 2018-01-16 18:16 Peter Jones 2018-01-18 22:52 ` Daniel Kiper 0 siblings, 1 reply; 6+ messages in thread From: Peter Jones @ 2018-01-16 18:16 UTC (permalink / raw) To: grub-devel; +Cc: Daniel Kiper, David E . Box, Peter Jones 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 <pjones@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; last = start; end = start + num_pm_ticks; start_tsc = grub_get_tsc (); while (1) { - cur = grub_inl (pmtimer) & 0xffffff; + 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) + { + 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; + + 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 + 200us (250 MHz). If after this time we still don't have 1us on + 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) + { + grub_dprintf ("pmtimer", "tsc delta is 0x%016lx\n", + end_tsc - start_tsc); + return 0; + } } } -- 2.15.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Make pmtimer tsc calibration not take 51 seconds to fail. 2018-01-16 18:16 [PATCH] Make pmtimer tsc calibration not take 51 seconds to fail Peter Jones @ 2018-01-18 22:52 ` Daniel Kiper 2018-01-19 21:10 ` Peter Jones 0 siblings, 1 reply; 6+ messages in thread From: Daniel Kiper @ 2018-01-18 22:52 UTC (permalink / raw) To: Peter Jones; +Cc: grub-devel, David E . Box 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 <pjones@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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Make pmtimer tsc calibration not take 51 seconds to fail. 2018-01-18 22:52 ` Daniel Kiper @ 2018-01-19 21:10 ` Peter Jones 2018-01-29 16:42 ` Daniel Kiper 0 siblings, 1 reply; 6+ messages in thread From: Peter Jones @ 2018-01-19 21:10 UTC (permalink / raw) To: Daniel Kiper; +Cc: grub-devel, David E . Box On Thu, Jan 18, 2018 at 11:52:53PM +0100, Daniel Kiper wrote: > 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 <pjones@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... Upon re-reading, I am not either. (As you see I wrote this patch 2+ months ago while working on something else, so the memory has already faded.) Obviously I need to make a version 2 of this patch to fix some issues. > > 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... I was blisfully unaware of 24-bit timers. I'll put that back and add a comment for future readers of the code, and check for 0xffffff below. > > + 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. Sure. > > > + { > > + 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? I mean, it's technically correct, but I don't think you're going to see 0 and 0xffffff as valid values often enough to actually care. > > + cur &= 0x3fff; > > + > > if (cur < last) > > - cur |= 0x1000000; > > + cur |= 0x4000; > > num_iter++; You didn't spot this, but it's wrong for exactly the same reason the initialization of start is. I'm pretty sure when I did this part, I was letting the comment below tell me what was going on, and completely missed that the comment doesn't match the conditional at all. > > 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? You know, I stared at this for a surprising amount of time when writing it trying to figure out why these numbers seemed weird. While I have the diff in front of me rather than the code as it is now, I see that it's because I was starting with the comment that was previously there, which had 50M instead of 5M for the TSC count. So I wound up dividing these by 10k instead of 1k. Woops. Will fix in v2. > > > + 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? Er... 16384, surely? But basically I just wanted to scale down the limit by approximately the same order as I scaled down the TSC delta, and I wanted to leave the code basically working the same way (not for any good reason), so I still wanted a number with only low-order bits set. TSC is scaled down by 1000, so I scaled the PM tick iteration down by 1024. I could make it twice as wide of a window instead, if you're worried about that particular race, but I don't think it's going to matter. If our loop takes 1 TSC tick per iteration, which is quite impluasibly fast, and we're on a 10GHz machine, then we should normally see pmtimer change by 3580 in ~2800 iterations. If it takes 16k iterations (again, with perfectly executing code that takes no time), the pmtimer is running at 625KHz instead of 3.58MHz. If we're on a 250MHz machine, that number is ~70 iterations. If we get to 16000 iterations without seeing pmtimer change by 3580 then pmtimer is either broken or we're on a 60GHz+ machine. That said the logic here did not match the comment and it still doesn't; it should be comparing cur and start, not the number of iterations. On the next version of the patch I'm going to do something like: if ((++num_iter > (grub_uint32_t)num_pm_ticks << 3UL) || (cur == start && end_tsc - start_tsc > 5000)) So it'll have a fairly widely ranged total limit but also actually check if pmtimer results have changed at all. Tentative patch below, but I haven't had a chance to test it yet: From 97e59bbe000961db5f823c4df074835d1165bd9d Mon Sep 17 00:00:00 2001 From: Peter Jones <pjones@redhat.com> Date: Tue, 7 Nov 2017 17:12:17 -0500 Subject: [PATCH] Make pmtimer tsc calibration not take 51 seconds to fail. 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 500ns-20us, 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. --- grub-core/kern/i386/tsc_pmtimer.c | 56 ++++++++++++++++++++++++++++++++------- 1 file changed, 46 insertions(+), 10 deletions(-) diff --git a/grub-core/kern/i386/tsc_pmtimer.c b/grub-core/kern/i386/tsc_pmtimer.c index c9c36169978..b09c00316df 100644 --- a/grub-core/kern/i386/tsc_pmtimer.c +++ b/grub-core/kern/i386/tsc_pmtimer.c @@ -37,8 +37,14 @@ grub_pmtimer_wait_count_tsc (grub_port_t pmtimer, grub_uint32_t cur, end; grub_uint64_t start_tsc; grub_uint64_t end_tsc; - int num_iter = 0; + unsigned int num_iter = 0; + int bad_reads = 0; + /* Some timers are 24-bit and some are 32-bit, but it doesn't make much + * difference to us. Caring which one we have isn't really worth it since + * the low-order digits will give us enough data to calibrate TSC. So just + * mask the top-order byte off. + */ start = grub_inl (pmtimer) & 0xffffff; last = start; end = start + num_pm_ticks; @@ -46,22 +52,52 @@ grub_pmtimer_wait_count_tsc (grub_port_t pmtimer, while (1) { cur = grub_inl (pmtimer) & 0xffffff; + + /* 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 == 0xffffff || cur == 0) + { + bad_reads++; + grub_dprintf ("pmtimer", "pmtimer value: 0x%08x bad_reads: %d\n", + cur, bad_reads); + + if (bad_reads == 10) + return 0; + } + else if (bad_reads) + bad_reads = 0; + + end_tsc = grub_get_tsc(); + if (cur < last) cur |= 0x1000000; - num_iter++; if (cur >= end) { - end_tsc = grub_get_tsc (); + grub_dprintf ("pmtimer", "tsc delta is 0x%016llx\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 500ns (10GHz) and + 20us (250 MHz). If after this time we still don't have 1us on + pmtimer, then pmtimer is broken. + + Likewise, if our code is perfectly efficient and introduces no delays + whatsoever, on a 10GHz system we should see a TSC delta of 3580 in + ~2800 iterations. On a 250MHz machine that should be ~70 iterations. + So there's a hard limit here at 8x our desired pm timer delta, because + if we get that far, we're either on a 60GHz+ machine or we're never + hitting our threshold. */ - if ((num_iter & 0xffffff) == 0 && grub_get_tsc () - start_tsc > 5000000) { - return 0; - } + if ((++num_iter > (grub_uint32_t)num_pm_ticks << 3UL) || + (cur == start && end_tsc - start_tsc > 5000)) + { + grub_dprintf ("pmtimer", + "tsc delta is 0x%016llx, which is implausible\n", + end_tsc - start_tsc); + return 0; + } } } -- 2.15.0 -- Peter ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Make pmtimer tsc calibration not take 51 seconds to fail. 2018-01-19 21:10 ` Peter Jones @ 2018-01-29 16:42 ` Daniel Kiper 2018-02-19 22:37 ` Peter Jones 0 siblings, 1 reply; 6+ messages in thread From: Daniel Kiper @ 2018-01-29 16:42 UTC (permalink / raw) To: Peter Jones; +Cc: grub-devel, David E . Box On Fri, Jan 19, 2018 at 04:10:01PM -0500, Peter Jones wrote: > On Thu, Jan 18, 2018 at 11:52:53PM +0100, Daniel Kiper wrote: > > 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 <pjones@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... > > Upon re-reading, I am not either. (As you see I wrote this patch 2+ > months ago while working on something else, so the memory has already > faded.) Yeah, I know how it works... > Obviously I need to make a version 2 of this patch to fix some issues. Great! > > > 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... > > I was blisfully unaware of 24-bit timers. I'll put that back and add a > comment for future readers of the code, and check for 0xffffff below. Thanks! > > > + 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. > > Sure. > > > > > > + { > > > + 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? > > I mean, it's technically correct, but I don't think you're going to see > 0 and 0xffffff as valid values often enough to actually care. > > > > + cur &= 0x3fff; > > > + > > > if (cur < last) > > > - cur |= 0x1000000; > > > + cur |= 0x4000; > > > num_iter++; > > You didn't spot this, but it's wrong for exactly the same reason the > initialization of start is. I'm pretty sure when I did this part, I was > letting the comment below tell me what was going on, and completely > missed that the comment doesn't match the conditional at all. > > > > 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? > > You know, I stared at this for a surprising amount of time when writing > it trying to figure out why these numbers seemed weird. While I have > the diff in front of me rather than the code as it is now, I see that > it's because I was starting with the comment that was previously there, > which had 50M instead of 5M for the TSC count. So I wound up dividing > these by 10k instead of 1k. Woops. Will fix in v2. > > > > > > + 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? > > Er... 16384, surely? But basically I just wanted to scale down the Yep, or I simply forgot 0x before 4000... > limit by approximately the same order as I scaled down the TSC delta, > and I wanted to leave the code basically working the same way (not for > any good reason), so I still wanted a number with only low-order bits > set. TSC is scaled down by 1000, so I scaled the PM tick iteration down > by 1024. I could make it twice as wide of a window instead, if you're > worried about that particular race, but I don't think it's going to > matter. > > If our loop takes 1 TSC tick per iteration, which is quite impluasibly > fast, and we're on a 10GHz machine, then we should normally see pmtimer > change by 3580 in ~2800 iterations. If it takes 16k iterations (again, I think that number of iterations is wrong here. Let's see... f_pm = n_pm / t_pm, n_pm = 3580, f_pm = 3.58MHz => t_pm = 3580 / 3.58MHz t_pm = 0,001s = 1ms => num_iter = 1ms * 10GHz = 10.000.000 Then below calculations are wrong too... > with perfectly executing code that takes no time), the pmtimer is > running at 625KHz instead of 3.58MHz. If we're on a 250MHz machine, > that number is ~70 iterations. If we get to 16000 iterations without > seeing pmtimer change by 3580 then pmtimer is either broken or we're on > a 60GHz+ machine. > > That said the logic here did not match the comment and it still doesn't; > it should be comparing cur and start, not the number of iterations. On > the next version of the patch I'm going to do something like: > > if ((++num_iter > (grub_uint32_t)num_pm_ticks << 3UL) || Taking into account above I am not sure why you multiply by 8 here... > (cur == start && end_tsc - start_tsc > 5000)) > > So it'll have a fairly widely ranged total limit but also actually check if > pmtimer results have changed at all. > > Tentative patch below, but I haven't had a chance to test it yet: In general I am not against the idea itself but I think that some calculations should be fixed here and there. > From 97e59bbe000961db5f823c4df074835d1165bd9d Mon Sep 17 00:00:00 2001 > From: Peter Jones <pjones@redhat.com> > Date: Tue, 7 Nov 2017 17:12:17 -0500 > Subject: [PATCH] Make pmtimer tsc calibration not take 51 seconds to fail. > > 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 500ns-20us, 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. > --- > grub-core/kern/i386/tsc_pmtimer.c | 56 ++++++++++++++++++++++++++++++++------- > 1 file changed, 46 insertions(+), 10 deletions(-) > > diff --git a/grub-core/kern/i386/tsc_pmtimer.c b/grub-core/kern/i386/tsc_pmtimer.c > index c9c36169978..b09c00316df 100644 > --- a/grub-core/kern/i386/tsc_pmtimer.c > +++ b/grub-core/kern/i386/tsc_pmtimer.c > @@ -37,8 +37,14 @@ grub_pmtimer_wait_count_tsc (grub_port_t pmtimer, > grub_uint32_t cur, end; > grub_uint64_t start_tsc; > grub_uint64_t end_tsc; > - int num_iter = 0; > + unsigned int num_iter = 0; > + int bad_reads = 0; > > + /* Some timers are 24-bit and some are 32-bit, but it doesn't make much Please leave empty line after "/*". > + * difference to us. Caring which one we have isn't really worth it since > + * the low-order digits will give us enough data to calibrate TSC. So just > + * mask the top-order byte off. > + */ > start = grub_inl (pmtimer) & 0xffffff; > last = start; > end = start + num_pm_ticks; > @@ -46,22 +52,52 @@ grub_pmtimer_wait_count_tsc (grub_port_t pmtimer, > while (1) > { > cur = grub_inl (pmtimer) & 0xffffff; > + > + /* If we get 10 reads in a row that are obviously dead pins, there's no > + reason to do this thousands of times. Please be consistent with the comments format. I prefer one given above. > + */ > + if (cur == 0xffffff || cur == 0) > + { > + bad_reads++; > + grub_dprintf ("pmtimer", "pmtimer value: 0x%08x bad_reads: %d\n", > + cur, bad_reads); > + > + if (bad_reads == 10) > + return 0; > + } > + else if (bad_reads) > + bad_reads = 0; > + > + end_tsc = grub_get_tsc(); > + > if (cur < last) > cur |= 0x1000000; > - num_iter++; > if (cur >= end) > { > - end_tsc = grub_get_tsc (); > + grub_dprintf ("pmtimer", "tsc delta is 0x%016llx\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 500ns (10GHz) and > + 20us (250 MHz). If after this time we still don't have 1us on > + pmtimer, then pmtimer is broken. > + > + Likewise, if our code is perfectly efficient and introduces no delays > + whatsoever, on a 10GHz system we should see a TSC delta of 3580 in > + ~2800 iterations. On a 250MHz machine that should be ~70 iterations. Incorrect calculations? > + So there's a hard limit here at 8x our desired pm timer delta, because > + if we get that far, we're either on a 60GHz+ machine or we're never > + hitting our threshold. And please use same comment format as in first one. Thanks, Daniel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Make pmtimer tsc calibration not take 51 seconds to fail. 2018-01-29 16:42 ` Daniel Kiper @ 2018-02-19 22:37 ` Peter Jones 2018-02-20 19:39 ` Daniel Kiper 0 siblings, 1 reply; 6+ messages in thread From: Peter Jones @ 2018-02-19 22:37 UTC (permalink / raw) To: grub-devel; +Cc: Daniel Kiper, Peter Jones 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 ~0x2626aa0 TSCs, aka ~2.4 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. If "debug" is includes "pmtimer", you will see one of the following three outcomes. If pmtimer gives all 0 or all 1 bits, you will see: kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 1 kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 2 kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 3 kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 4 kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 5 kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 6 kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 7 kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 8 kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 9 kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 10 kern/i386/tsc_pmtimer.c:78: timer is broken; giving up. This outcome was tested using qemu+kvm with UEFI (OVMF) firmware and these options: -machine pc-q35-2.10 -cpu Broadwell-noTSX If pmtimer gives any other bit patterns but is not actually marching forward fast enough to use for clock calibration, you will see: kern/i386/tsc_pmtimer.c:121: pmtimer delta is 0x0 (1904 iterations) kern/i386/tsc_pmtimer.c:124: tsc delta is implausible: 0x2626aa0 This outcome was tested using grub compiled with GRUB_PMTIMER_IGNORE_BAD_READS defined (so as not to trip the bad read test) using qemu+kvm with UEFI (OVMF) firmware, and these options: -machine pc-q35-2.10 -cpu Broadwell-noTSX If pmtimer actually works, you'll see something like: kern/i386/tsc_pmtimer.c:121: pmtimer delta is 0x0 (1904 iterations) kern/i386/tsc_pmtimer.c:124: tsc delta is implausible: 0x2626aa0 This outcome was tested using qemu+kvm with UEFI (OVMF) firmware, and these options: -machine pc-i440fx-2.4 -cpu Broadwell-noTSX I've also tested this outcome on a real Intel Xeon E3-1275v3 on an Intel Server Board S1200V3RPS using the SDV.RP.B8 "Release" build here: https://firmware.intel.com/sites/default/files/UEFIDevKit_S1200RP_vB8.zip Signed-off-by: Peter Jones <pjones@redhat.com> --- grub-core/kern/i386/tsc_pmtimer.c | 109 +++++++++++++++++++++++++++++++------- 1 file changed, 89 insertions(+), 20 deletions(-) diff --git a/grub-core/kern/i386/tsc_pmtimer.c b/grub-core/kern/i386/tsc_pmtimer.c index c9c36169978..ca15c3aacd7 100644 --- a/grub-core/kern/i386/tsc_pmtimer.c +++ b/grub-core/kern/i386/tsc_pmtimer.c @@ -28,40 +28,101 @@ #include <grub/acpi.h> #include <grub/cpu/io.h> +/* + * Define GRUB_PMTIMER_IGNORE_BAD_READS if you're trying to test a timer that's + * present but doesn't keep time well. + */ +// #define GRUB_PMTIMER_IGNORE_BAD_READS + grub_uint64_t grub_pmtimer_wait_count_tsc (grub_port_t pmtimer, grub_uint16_t num_pm_ticks) { grub_uint32_t start; - grub_uint32_t last; - grub_uint32_t cur, end; + grub_uint64_t cur, end; grub_uint64_t start_tsc; grub_uint64_t end_tsc; - int num_iter = 0; + unsigned int num_iter = 0; +#ifndef GRUB_PMTIMER_IGNORE_BAD_READS + int bad_reads = 0; +#endif - start = grub_inl (pmtimer) & 0xffffff; - last = start; + /* + * Some timers are 24-bit and some are 32-bit, but it doesn't make much + * difference to us. Caring which one we have isn't really worth it since + * the low-order digits will give us enough data to calibrate TSC. So just + * mask the top-order byte off. + */ + cur = start = grub_inl (pmtimer) & 0xffffffUL; end = start + num_pm_ticks; start_tsc = grub_get_tsc (); while (1) { - cur = grub_inl (pmtimer) & 0xffffff; - if (cur < last) - cur |= 0x1000000; - num_iter++; + cur &= 0xffffffffff000000ULL; + cur |= grub_inl (pmtimer) & 0xffffffUL; + + end_tsc = grub_get_tsc(); + +#ifndef GRUB_PMTIMER_IGNORE_BAD_READS + /* + * 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 == 0xffffffUL || cur == 0) + { + bad_reads++; + grub_dprintf ("pmtimer", + "pmtimer: 0x%"PRIxGRUB_UINT64_T" bad_reads: %d\n", + cur, bad_reads); + grub_dprintf ("pmtimer", "timer is broken; giving up.\n"); + + if (bad_reads == 10) + return 0; + } +#endif + + if (cur < start) + cur += 0x1000000; + if (cur >= end) { - end_tsc = grub_get_tsc (); + grub_dprintf ("pmtimer", "pmtimer delta is 0x%"PRIxGRUB_UINT64_T"\n", + cur - start); + grub_dprintf ("pmtimer", "tsc delta is 0x%"PRIxGRUB_UINT64_T"\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. 1ms at 10GHz should be 1E+7 TSCs; at + * 250MHz it should be 2.5E6. So if after 4E+7 TSCs on a 10GHz machine, + * we should have seen pmtimer show 4ms of change (i.e. cur =~ + * start+14320); on a 250MHz machine that should be 16ms (start+57280). + * If after this a time we still don't have 1ms on pmtimer, then pmtimer + * is broken. + * + * Likewise, if our code is perfectly efficient and introduces no delays + * whatsoever, on a 10GHz system we should see a TSC delta of 3580 in + * ~3580 iterations. On a 250MHz machine that should be ~900 iterations. + * + * With those factors in mind, there are two limits here. There's a hard + * limit here at 8x our desired pm timer delta, picked as an arbitrarily + * large value that's still not a lot of time to humans, because if we + * get that far this is either an implausibly fast machine or the pmtimer + * is not running. And there's another limit on 4x our 10GHz tsc delta + * without seeing cur converge on our target value. */ - if ((num_iter & 0xffffff) == 0 && grub_get_tsc () - start_tsc > 5000000) { - return 0; - } + if ((++num_iter > (grub_uint32_t)num_pm_ticks << 3UL) || + end_tsc - start_tsc > 40000000) + { + grub_dprintf ("pmtimer", + "pmtimer delta is 0x%"PRIxGRUB_UINT64_T" (%u iterations)\n", + cur - start, num_iter); + grub_dprintf ("pmtimer", + "tsc delta is implausible: 0x%"PRIxGRUB_UINT64_T"\n", + end_tsc - start_tsc); + return 0; + } } } @@ -74,12 +135,20 @@ grub_tsc_calibrate_from_pmtimer (void) fadt = grub_acpi_find_fadt (); if (!fadt) - return 0; + { + grub_dprintf ("pmtimer", "No FADT found; not using pmtimer.\n"); + return 0; + } pmtimer = fadt->pmtimer; if (!pmtimer) - return 0; + { + grub_dprintf ("pmtimer", "FADT does not specify pmtimer; skipping.\n"); + return 0; + } - /* It's 3.579545 MHz clock. Wait 1 ms. */ + /* + * It's 3.579545 MHz clock. Wait 1 ms. + */ tsc_diff = grub_pmtimer_wait_count_tsc (pmtimer, 3580); if (tsc_diff == 0) return 0; -- 2.15.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Make pmtimer tsc calibration not take 51 seconds to fail. 2018-02-19 22:37 ` Peter Jones @ 2018-02-20 19:39 ` Daniel Kiper 0 siblings, 0 replies; 6+ messages in thread From: Daniel Kiper @ 2018-02-20 19:39 UTC (permalink / raw) To: Peter Jones; +Cc: grub-devel On Mon, Feb 19, 2018 at 05:37:56PM -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 ~0x2626aa0 TSCs, aka ~2.4 > 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. > > If "debug" is includes "pmtimer", you will see one of the following > three outcomes. If pmtimer gives all 0 or all 1 bits, you will see: > > kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 1 > kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 2 > kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 3 > kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 4 > kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 5 > kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 6 > kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 7 > kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 8 > kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 9 > kern/i386/tsc_pmtimer.c:77: pmtimer: 0xffffff bad_reads: 10 > kern/i386/tsc_pmtimer.c:78: timer is broken; giving up. OK. > This outcome was tested using qemu+kvm with UEFI (OVMF) firmware and > these options: -machine pc-q35-2.10 -cpu Broadwell-noTSX > > If pmtimer gives any other bit patterns but is not actually marching > forward fast enough to use for clock calibration, you will see: > > kern/i386/tsc_pmtimer.c:121: pmtimer delta is 0x0 (1904 iterations) > kern/i386/tsc_pmtimer.c:124: tsc delta is implausible: 0x2626aa0 OK. > This outcome was tested using grub compiled with GRUB_PMTIMER_IGNORE_BAD_READS > defined (so as not to trip the bad read test) using qemu+kvm with UEFI > (OVMF) firmware, and these options: -machine pc-q35-2.10 -cpu Broadwell-noTSX > > If pmtimer actually works, you'll see something like: > > kern/i386/tsc_pmtimer.c:121: pmtimer delta is 0x0 (1904 iterations) > kern/i386/tsc_pmtimer.c:124: tsc delta is implausible: 0x2626aa0 Hmmm... Same as above? > This outcome was tested using qemu+kvm with UEFI (OVMF) firmware, and > these options: -machine pc-i440fx-2.4 -cpu Broadwell-noTSX > > I've also tested this outcome on a real Intel Xeon E3-1275v3 on an Intel > Server Board S1200V3RPS using the SDV.RP.B8 "Release" build here: > https://firmware.intel.com/sites/default/files/UEFIDevKit_S1200RP_vB8.zip > > Signed-off-by: Peter Jones <pjones@redhat.com> > --- > grub-core/kern/i386/tsc_pmtimer.c | 109 +++++++++++++++++++++++++++++++------- > 1 file changed, 89 insertions(+), 20 deletions(-) > > diff --git a/grub-core/kern/i386/tsc_pmtimer.c b/grub-core/kern/i386/tsc_pmtimer.c > index c9c36169978..ca15c3aacd7 100644 > --- a/grub-core/kern/i386/tsc_pmtimer.c > +++ b/grub-core/kern/i386/tsc_pmtimer.c > @@ -28,40 +28,101 @@ > #include <grub/acpi.h> > #include <grub/cpu/io.h> > > +/* > + * Define GRUB_PMTIMER_IGNORE_BAD_READS if you're trying to test a timer that's > + * present but doesn't keep time well. > + */ > +// #define GRUB_PMTIMER_IGNORE_BAD_READS > + > grub_uint64_t > grub_pmtimer_wait_count_tsc (grub_port_t pmtimer, > grub_uint16_t num_pm_ticks) > { > grub_uint32_t start; > - grub_uint32_t last; > - grub_uint32_t cur, end; > + grub_uint64_t cur, end; > grub_uint64_t start_tsc; > grub_uint64_t end_tsc; > - int num_iter = 0; > + unsigned int num_iter = 0; > +#ifndef GRUB_PMTIMER_IGNORE_BAD_READS > + int bad_reads = 0; > +#endif > > - start = grub_inl (pmtimer) & 0xffffff; > - last = start; > + /* > + * Some timers are 24-bit and some are 32-bit, but it doesn't make much > + * difference to us. Caring which one we have isn't really worth it since > + * the low-order digits will give us enough data to calibrate TSC. So just > + * mask the top-order byte off. > + */ > + cur = start = grub_inl (pmtimer) & 0xffffffUL; Just for the sake of readability I would do s/0xffffffUL/0x00ffffffUL/ here and below. > end = start + num_pm_ticks; > start_tsc = grub_get_tsc (); > while (1) > { > - cur = grub_inl (pmtimer) & 0xffffff; > - if (cur < last) > - cur |= 0x1000000; > - num_iter++; > + cur &= 0xffffffffff000000ULL; Could you put a comment before this line? It took me some time to get it and required to take a look below. This is not obvious at first sight. > + cur |= grub_inl (pmtimer) & 0xffffffUL; > + > + end_tsc = grub_get_tsc(); > + > +#ifndef GRUB_PMTIMER_IGNORE_BAD_READS > + /* > + * 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 == 0xffffffUL || cur == 0) > + { > + bad_reads++; > + grub_dprintf ("pmtimer", > + "pmtimer: 0x%"PRIxGRUB_UINT64_T" bad_reads: %d\n", > + cur, bad_reads); > + grub_dprintf ("pmtimer", "timer is broken; giving up.\n"); > + > + if (bad_reads == 10) > + return 0; > + } > +#endif > + > + if (cur < start) > + cur += 0x1000000; > + > if (cur >= end) > { > - end_tsc = grub_get_tsc (); > + grub_dprintf ("pmtimer", "pmtimer delta is 0x%"PRIxGRUB_UINT64_T"\n", > + cur - start); > + grub_dprintf ("pmtimer", "tsc delta is 0x%"PRIxGRUB_UINT64_T"\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. 1ms at 10GHz should be 1E+7 TSCs; at > + * 250MHz it should be 2.5E6. So if after 4E+7 TSCs on a 10GHz machine, s/2.5E6/2.5E5/ > + * we should have seen pmtimer show 4ms of change (i.e. cur =~ > + * start+14320); on a 250MHz machine that should be 16ms (start+57280). s/16ms/160ms/ s/start+57280/start+572800/ > + * If after this a time we still don't have 1ms on pmtimer, then pmtimer > + * is broken. > + * > + * Likewise, if our code is perfectly efficient and introduces no delays Just to be sure. Do you mean 1 CPU clock tick per loop? > + * whatsoever, on a 10GHz system we should see a TSC delta of 3580 in s/TSC delta/pm timer delta/? > + * ~3580 iterations. On a 250MHz machine that should be ~900 iterations. Hmmm... How come? AIUI the number of iterations is the same but the time needed to reach that value changes. > + * With those factors in mind, there are two limits here. There's a hard > + * limit here at 8x our desired pm timer delta, picked as an arbitrarily I am afraid that without proper comment above I am not able to understand why "8x" here. > + * large value that's still not a lot of time to humans, because if we > + * get that far this is either an implausibly fast machine or the pmtimer > + * is not running. And there's another limit on 4x our 10GHz tsc delta > + * without seeing cur converge on our target value. I buy this but I would tell "4 ms TSC delta on CPU with 10 GHz clock" or something like that. Daniel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-02-20 19:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-01-16 18:16 [PATCH] Make pmtimer tsc calibration not take 51 seconds to fail Peter Jones 2018-01-18 22:52 ` Daniel Kiper 2018-01-19 21:10 ` Peter Jones 2018-01-29 16:42 ` Daniel Kiper 2018-02-19 22:37 ` Peter Jones 2018-02-20 19:39 ` Daniel Kiper
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.