Re: [SeaBIOS] [PATCH] Increase precision of usec timer calculation

[Stefan Berger]

On 11/01/2017 11:51 AM, Kevin O'Connor wrote:
> On Wed, Nov 01, 2017 at 10:40:57AM -0400, Stefan Berger wrote:
> > When timer_calc_usec() is used with large timeout falues, such as 60s,
> > the function lacks precision and produces different results than when
> > using timer_calc(time / 1000) for the same timeout. This patch fixes
> > the precision issue by falling back to timer_calc(time / 1000) for
> > usec values over one second.
>
> Makes sense.  The code would need to use DIV_ROUND_UP though (it's
> okay to go slightly longer, but it must not calculate a shorter time).
> Also, I think the check should be extended to
> nsleep/usleep/ndelay/udelay as well - see the patch below.
>
> Is this needed for the current code, or is this only an issue for the
> future TPM CRB patches?  I was planning to make a release this week -

I do see wrong calculations of course when using the usec function, but 
I am not running into actual timeouts (because the emulated device is 
fast). I think it also depends on the values of the timeouts and what 
bit patterns the values have and then what bits get lost during the 
calculation that it may or may not matter. For the general case I would 
say we have an active bug that, depending on timeout values, may cause 
unwanted behavior.

With your patch applied the values returned from the usec function is 
correct.

Stefan
> I'd prefer to add this to the next release unless it fixes an active
> bug.
>
> -Kevin
>
> --- a/src/hw/timer.c
> +++ b/src/hw/timer.c
> @@ -167,53 +167,60 @@ timer_check(u32 end)
>   }
>
> static void
> -timer_delay(u32 diff)
> +timer_delay(u32 end)
>   {
>
> u32 start = timer_read();
>
> u32 end = start + diff;
>  while (!timer_check(end))
>      cpu_relax();
> }
>
> static void
>
> -timer_sleep(u32 diff)
> +timer_sleep(u32 end)
>   {
> u32 start = timer_read();
> u32 end = start + diff;
>  while (!timer_check(end))
>      yield();
> }
>
> +// Return the TSC value that is 'msecs' time in the future.
> +u32
> +timer_calc(u32 msecs)
> +{
> return timer_read() + (GET_GLOBAL(TimerKHz) * msecs);
>
> +}
> +u32
> +timer_calc_usec(u32 usecs)
> +{
> u32 cur = timer_read(), khz = GET_GLOBAL(TimerKHz);
> if (usecs > 500000)
>    return cur + DIV_ROUND_UP(usecs, 1000) * khz;
> return cur + DIV_ROUND_UP(usecs * khz, 1000);
>
> +}
> +static u32
> +timer_calc_nsec(u32 nsecs)
> +{
> u32 cur = timer_read(), khz = GET_GLOBAL(TimerKHz);
> if (nsecs > 500000)
>    return cur + DIV_ROUND_UP(nsecs, 1000000) * khz;
> return cur + DIV_ROUND_UP(nsecs * khz, 1000000);
>
> +}
> void ndelay(u32 count) {
> timer_delay(DIV_ROUND_UP(count * GET_GLOBAL(TimerKHz), 1000000));
> timer_delay(timer_calc_nsec(count));
> }
> void udelay(u32 count) {
> timer_delay(DIV_ROUND_UP(count * GET_GLOBAL(TimerKHz), 1000));
> timer_delay(timer_calc_usec(count));
> }
> void mdelay(u32 count) {
> timer_delay(count * GET_GLOBAL(TimerKHz));
>
> timer_delay(timer_calc(count));
> }
>
> void nsleep(u32 count) {
> timer_sleep(DIV_ROUND_UP(count * GET_GLOBAL(TimerKHz), 1000000));
> timer_sleep(timer_calc_nsec(count));
> }
> void usleep(u32 count) {
> timer_sleep(DIV_ROUND_UP(count * GET_GLOBAL(TimerKHz), 1000));
> timer_sleep(timer_calc_usec(count));
> }
> void msleep(u32 count) {
> timer_sleep(count * GET_GLOBAL(TimerKHz));
>
> -}
>
> -// Return the TSC value that is 'msecs' time in the future.
> -u32
> -timer_calc(u32 msecs)
> -{
> return timer_read() + (GET_GLOBAL(TimerKHz) * msecs);
>
> -}
> -u32
> -timer_calc_usec(u32 usecs)
> -{
> return timer_read() + DIV_ROUND_UP(GET_GLOBAL(TimerKHz) * usecs, 1000);
> timer_sleep(timer_calc(count));
> }