[SeaBIOS] [PATCH] timer: Handle decrements of PIT counter

Roman Bolshakov posted 1 patch 4 weeks ago
Failed in applying to current master (apply log)
src/hw/timer.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)

[SeaBIOS] [PATCH] timer: Handle decrements of PIT counter

Posted by Roman Bolshakov 4 weeks ago
There's a fallback to PIT if TSC is not present but it doesn't work
properly. It prevents boot from floppy on isapc and 486 cpu [1][2].

SeaBIOS configures PIT in Mode 2. PIT counter is decremented in the mode
but timer_adjust_bits() thinks that the counter overflows and increases
32-bit tick counter on each detected "overflow". Invalid overflow
detection results in 55ms time advance (1 / 18.2Hz) on each read from
PIT counter. So all timers expire much faster and 5-second floppy
timeout expires in 83 real microseconds (or just a bit longer).

Provide counter direction to timer_adjust_bits() and normalize the
counter to advance ticks in monotonically increasing TimerLast.

1. https://bugs.launchpad.net/seabios/+bug/1840719
2. https://lists.gnu.org/archive/html/qemu-devel/2019-08/msg03924.html

Fixes: eac11944019 ("Unify pmtimer_read() and pittimer_read() code.")
Reported-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Kevin O'Connor <kevin@koconnor.net>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 src/hw/timer.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/hw/timer.c b/src/hw/timer.c
index 56bb289..2441402 100644
--- a/src/hw/timer.c
+++ b/src/hw/timer.c
@@ -156,10 +156,15 @@ u32 TimerLast VARLOW;
 
 // Add extra high bits to timers that have less than 32bits of precision.
 static u32
-timer_adjust_bits(u32 value, u32 validbits)
+timer_adjust_bits(u32 value, u32 validbits, u8 countup)
 {
     u32 last = GET_LOW(TimerLast);
-    value = (last & ~validbits) | (value & validbits);
+    u32 validvalue;
+    if (countup)
+        validvalue = value & validbits;
+    else
+        validvalue = validbits - (value & validbits);
+    value = (last & ~validbits) | validvalue;
     if (value < last)
         value += validbits + 1;
     SET_LOW(TimerLast, value);
@@ -176,11 +181,11 @@ timer_read(void)
         return rdtscll() >> GET_GLOBAL(ShiftTSC);
     if (CONFIG_PMTIMER && port != PORT_PIT_COUNTER0)
         // Read from PMTIMER
-        return timer_adjust_bits(inl(port), 0xffffff);
+        return timer_adjust_bits(inl(port), 0xffffff, 1);
     // Read from PIT.
     outb(PM_SEL_READBACK | PM_READ_VALUE | PM_READ_COUNTER0, PORT_PIT_MODE);
     u16 v = inb(PORT_PIT_COUNTER0) | (inb(PORT_PIT_COUNTER0) << 8);
-    return timer_adjust_bits(v, 0xffff);
+    return timer_adjust_bits(v, 0xffff, 0);
 }
 
 // Return the TSC value that is 'msecs' time in the future.
-- 
2.26.1
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] timer: Handle decrements of PIT counter

Posted by Kevin O'Connor 2 weeks ago
On Sat, Jun 13, 2020 at 02:19:12PM +0300, Roman Bolshakov wrote:
> There's a fallback to PIT if TSC is not present but it doesn't work
> properly. It prevents boot from floppy on isapc and 486 cpu [1][2].
> 
> SeaBIOS configures PIT in Mode 2. PIT counter is decremented in the mode
> but timer_adjust_bits() thinks that the counter overflows and increases
> 32-bit tick counter on each detected "overflow". Invalid overflow
> detection results in 55ms time advance (1 / 18.2Hz) on each read from
> PIT counter. So all timers expire much faster and 5-second floppy
> timeout expires in 83 real microseconds (or just a bit longer).
> 
> Provide counter direction to timer_adjust_bits() and normalize the
> counter to advance ticks in monotonically increasing TimerLast.

Good catch.  Could we fix it using the patch below instead though?

-Kevin


--- a/src/hw/timer.c
+++ b/src/hw/timer.c
@@ -180,7 +180,7 @@ timer_read(void)
     // Read from PIT.
     outb(PM_SEL_READBACK | PM_READ_VALUE | PM_READ_COUNTER0, PORT_PIT_MODE);
     u16 v = inb(PORT_PIT_COUNTER0) | (inb(PORT_PIT_COUNTER0) << 8);
-    return timer_adjust_bits(v, 0xffff);
+    return timer_adjust_bits(-v, 0xffff);
 }
 
 // Return the TSC value that is 'msecs' time in the future.
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] timer: Handle decrements of PIT counter

Posted by Roman Bolshakov 2 weeks ago
On Tue, Jun 23, 2020 at 11:00:24PM -0400, Kevin O'Connor wrote:
> On Sat, Jun 13, 2020 at 02:19:12PM +0300, Roman Bolshakov wrote:
> > There's a fallback to PIT if TSC is not present but it doesn't work
> > properly. It prevents boot from floppy on isapc and 486 cpu [1][2].
> > 
> > SeaBIOS configures PIT in Mode 2. PIT counter is decremented in the mode
> > but timer_adjust_bits() thinks that the counter overflows and increases
> > 32-bit tick counter on each detected "overflow". Invalid overflow
> > detection results in 55ms time advance (1 / 18.2Hz) on each read from
> > PIT counter. So all timers expire much faster and 5-second floppy
> > timeout expires in 83 real microseconds (or just a bit longer).
> > 
> > Provide counter direction to timer_adjust_bits() and normalize the
> > counter to advance ticks in monotonically increasing TimerLast.
> 
> Good catch.  Could we fix it using the patch below instead though?
> 
> -Kevin
> 
> 
> --- a/src/hw/timer.c
> +++ b/src/hw/timer.c
> @@ -180,7 +180,7 @@ timer_read(void)
>      // Read from PIT.
>      outb(PM_SEL_READBACK | PM_READ_VALUE | PM_READ_COUNTER0, PORT_PIT_MODE);
>      u16 v = inb(PORT_PIT_COUNTER0) | (inb(PORT_PIT_COUNTER0) << 8);
> -    return timer_adjust_bits(v, 0xffff);
> +    return timer_adjust_bits(-v, 0xffff);
>  }
>  
>  // Return the TSC value that is 'msecs' time in the future.

Hi Kevin,

I like the approach much more. Initial count value is 0, PIT rearms the
timer when 1 is hit, unary negation on unsigned u16 fits perfectly, then
timer_adjust_bits recieves 0, 1, 2, ... and timer is rearmed at 0xffff.

Do you want me to send v2 or you plan to apply the fix on your own?

If you plan to apply it on your own, here are the tags:

Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
Tested-by: Roman Bolshakov <r.bolshakov@yadro.com>

Thanks,
Roman
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

[SeaBIOS] Re: [PATCH] timer: Handle decrements of PIT counter

Posted by Kevin O'Connor 2 weeks ago
On Fri, Jun 26, 2020 at 04:09:57PM +0300, Roman Bolshakov wrote:
> On Tue, Jun 23, 2020 at 11:00:24PM -0400, Kevin O'Connor wrote:
> > Good catch.  Could we fix it using the patch below instead though?
> > 
> > -Kevin
> > 
> > 
> > --- a/src/hw/timer.c
> > +++ b/src/hw/timer.c
> > @@ -180,7 +180,7 @@ timer_read(void)
> >      // Read from PIT.
> >      outb(PM_SEL_READBACK | PM_READ_VALUE | PM_READ_COUNTER0, PORT_PIT_MODE);
> >      u16 v = inb(PORT_PIT_COUNTER0) | (inb(PORT_PIT_COUNTER0) << 8);
> > -    return timer_adjust_bits(v, 0xffff);
> > +    return timer_adjust_bits(-v, 0xffff);
> >  }
> >  
> >  // Return the TSC value that is 'msecs' time in the future.
> 
> Hi Kevin,
> 
> I like the approach much more. Initial count value is 0, PIT rearms the
> timer when 1 is hit, unary negation on unsigned u16 fits perfectly, then
> timer_adjust_bits recieves 0, 1, 2, ... and timer is rearmed at 0xffff.
> 
> Do you want me to send v2 or you plan to apply the fix on your own?

I'm fine with either.

Thanks,
-Kevin
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org