dmsc / emu2

Simple x86 and DOS emulator for the Linux terminal.
GNU General Public License v2.0
406 stars 31 forks source link

Arithmetic Exception in get_actual_timer #45

Closed tsupplis closed 2 years ago

tsupplis commented 2 years ago

in

static uint16_t get_actual_timer(struct i8253_timer *t)
{
    int64_t elapsed = get_timer_clock() - t->load_time;
    debug(debug_int, "timer elapsed: %lld\n", elapsed);
    switch(t->op_mode & 7)
    {
    case 2: // RATE GENERATOR
    case 3: // SQUARE WAVE GENERATOR
        return t->load_value - (elapsed % (t->load_value));
    default:
        return t->load_value - elapsed;
    }
}

we get an arithmetic expression when t->load_value is 0. This happens incidentally when executing qconfig.exe or checkit. Not too sure how to fix it there.

tkchia commented 2 years ago

Hello @tsupplis,

I gather that t->load_value will be the 16-bit value written to one of the Programmable Interval Timer (PIT) ports 0x40, 0x41, or 0x42? In "rate generator" or "square wave generator" mode, the value will be the period of the timer (in units of ≈ 1/1,193,182 seconds). A value of 0 should be treated as requesting a period of 0x10000 units.

Ralf Brown's Interrupt List says,

----------P0040005F--------------------------
PORT 0040-005F - PIT - PROGRAMMABLE INTERVAL TIMER (8253, 8254)
Notes:  XT & AT use ports 40h-43h; PS/2 uses ports 40h, 42h-44h, and 47h
        the counter chip is driven with a 1.193 MHz clock (1/4 of the
        original PC's 4.77 MHz CPU clock)
SeeAlso: PORT 0044h,PORT 0048h

0040  RW  PIT  counter 0, counter divisor             (XT, AT, PS/2)
        Used to keep the system time; the default divisor of (1)0000h
        produces the 18.2Hz clock tick.
...

Thank you!

tsupplis commented 2 years ago

Thank you @tkchia, So should the code in timer.c be:

// Get actual value in timer
static uint16_t get_actual_timer(struct i8253_timer *t)
{
    int64_t elapsed = get_timer_clock() - t->load_time;
    debug(debug_int, "timer elapsed: %lld\n", elapsed);
    int64_t lv=t->load_value?t->load_value:0x10000;
    switch(t->op_mode & 7)
    {
    case 2: // RATE GENERATOR
    case 3: // SQUARE WAVE GENERATOR
        return lv - (elapsed % (lv));
    default:
        return lv - elapsed;
    }
}

BTW, @tkchia, it would be cool to use emu2 as an alternative to dosemu2 in build-ia16. It would make it portable to non-linux unices like macOS 😉 ....

Below, the benchmark of checkit working as a result.

image

@dmsc, If this is correct, I am happy to submit a PR.

BTW, the perf is quite ok ~ 66% of what you can get with dosbox-x. 😉

dmsc commented 2 years ago

Hi!

Thanks @tsupplis and @tkchia for the report.

Yes, it is a shameful bug, I should have seen the division by zero in that code... I committed a fix.

I tried running checkit 1.1 from winworldpc, but it failed because I don't implement full 286 instructions. What version are you using?

Have Fun!

tsupplis commented 2 years ago

Thank you @dmsc, I use version 3.0. https://winworldpc.com/product/checkit/30 The config page fails, this is my next investigation. BTW, with your fix instead of my clumsy one, the performance is

image

... x2 compared to dosbox-x.... super cool altogether.

tkchia commented 2 years ago

Thanks @dmsc for the fix!

@tsupplis:

BTW, @tkchia, it would be cool to use emu2 as an alternative to dosemu2 in build-ia16. It would make it portable to non-linux unices like macOS :wink: ....

dosemu2 is only needed when running the test suite for libi86, and this should be optional. If something inside the build-ia16 toolchain becomes broken when dosemu2 is not installed, do let me know.

Thank you!