On Tue, Aug 20, 2019 at 06:21:28PM +0200, Philippe Mathieu-Daudé wrote:
Cc'ing Eduardo, Paolo.
On 8/20/19 3:38 PM, Philippe Mathieu-Daudé wrote:
On 8/20/19 3:12 PM, John Snow wrote:
On 8/20/19 6:25 AM, Philippe Mathieu-Daudé wrote:
[cross posting QEMU & SeaBIOS]
Hello,
I'v been looking at a QEMU bug report [1] which bisection resulted in a SeaBIOS commit:
4a6dbcea3e412fe12effa2f812f50dd7eae90955 is the first bad commit commit 4a6dbcea3e412fe12effa2f812f50dd7eae90955 Author: Nikolay Nikolov nickysn@users.sourceforge.net Date: Sun Feb 4 17:27:01 2018 +0200
floppy: Use timer_check() in floppy_wait_irq() Use timer_check() instead of using floppy_motor_counter in BDA for the timeout check in floppy_wait_irq(). The problem with using floppy_motor_counter was that, after it reaches 0, it immediately stops the floppy motors, which is not what is supposed to happen on real hardware. Instead, after a timeout (like in the end of every floppy operation, regardless of the result - success, timeout or error), the floppy motors must be kept spinning for additional 2 seconds (the FLOPPY_MOTOR_TICKS). So, now the floppy_motor_counter is initialized to 255 (the max value) in the beginning of the floppy operation. For IRQ timeouts, a different timeout is used, specified by the new FLOPPY_IRQ_TIMEOUT constant (currently set to 5 seconds - a fairly conservative value, but should work reliably on most floppies). After the floppy operation, floppy_drive_pio() resets the floppy_motor_counter to 2 seconds (FLOPPY_MOTOR_TICKS). This is also consistent with what other PC BIOSes do.This commit improve behavior with real hardware, so maybe QEMU is not modelling something or modelling it incorrectly?
[...]
Well, that's unfortunate.
What version of QEMU shipped the SeaBIOS that caused the regression?
See https://bugs.launchpad.net/qemu/+bug/1840719/comments/3
QEMU commit 0b8f74488e, slighly before QEMU v3.1.0 (previous tag is v3.0.0).
But you can use v4.1.0 too, simply change the SeaBIOS bios.bin, i.e.:
qemu$ git checkout v4.1.0
qemu$ (cd roms/seabios && git checkout 4a6dbcea3e4~) && \ make -C roms bios
Now pc-bios/bios.bin is built using the last commit working,
qemu$ (cd roms/seabios && git checkout 4a6dbcea3e4) && \ make -C roms bios
And you can reproduce the error.
Back from here.
So the SeaBIOS patch is:
diff --git a/src/hw/floppy.c b/src/hw/floppy.c index 77dbade..3012b3a 100644 --- a/src/hw/floppy.c +++ b/src/hw/floppy.c @@ -34,6 +34,7 @@ #define FLOPPY_GAPLEN 0x1B #define FLOPPY_FORMAT_GAPLEN 0x6c #define FLOPPY_PIO_TIMEOUT 1000 +#define FLOPPY_IRQ_TIMEOUT 5000
#define FLOPPY_DOR_MOTOR_D 0x80 // Set to turn drive 3's motor ON #define FLOPPY_DOR_MOTOR_C 0x40 // Set to turn drive 2's motor ON @@ -221,8 +222,9 @@ floppy_wait_irq(void) { u8 frs = GET_BDA(floppy_recalibration_status); SET_BDA(floppy_recalibration_status, frs & ~FRS_IRQ);
- u32 end = timer_calc(FLOPPY_IRQ_TIMEOUT); for (;;) {
if (!GET_BDA(floppy_motor_counter)) {
if (timer_check(end)) { warn_timeout(); floppy_disable_controller(); return DISK_RET_ETIMEOUT;timer_calc() unit is milliseconds, so this patch should wait upto 5seconds before failing, and it seems the timeout is not used at all.
SeaBIOS timer.c:
// Return the TSC value that is 'msecs' time in the future. u32 timer_calc(u32 msecs) { return timer_read() + (GET_GLOBAL(TimerKHz) * msecs); }
static u32 timer_read(void) { u16 port = GET_GLOBAL(TimerPort); if (CONFIG_TSC_TIMER && !port) // Read from CPU TSC return rdtscll() >> GET_GLOBAL(ShiftTSC); if (CONFIG_PMTIMER && port != PORT_PIT_COUNTER0) // Read from PMTIMER return timer_adjust_bits(inl(port), 0xffffff); // 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); }
Using the default QEMU config, we build SeaBIOS to use the TSC timer:
builds/seabios-128k/.config:CONFIG_TSC_TIMER=y builds/seabios-256k/.config:CONFIG_TSC_TIMER=y
$ qemu-system-i386 -M isapc -cpu 486 \ -fda Windows\ 98\ Second\ Edition\ Boot.img \ -chardev stdio,id=seabios \ -device isa-debugcon,iobase=0x402,chardev=seabios Booting from Floppy... Floppy_drive_recal 0 Floppy_enable_controller WARNING - Timeout at floppy_wait_irq:228! Floppy_disable_controller Floppy_enable_controller WARNING - Timeout at floppy_wait_irq:228! Floppy_disable_controller Boot failed: could not read the boot disk
Now enabling the TSC feature:
$ qemu-system-i386 -M isapc -cpu 486,tsc \ -fda Windows\ 98\ Second\ Edition\ Boot.img \ -chardev stdio,id=seabios \ -device isa-debugcon,iobase=0x402,chardev=seabios Booting from Floppy... Floppy_drive_recal 0 Floppy_enable_controller Floppy_media_sense on drive 0 found rate 0 Booting from 0000:7c00 Floppy_disable_controller Floppy_enable_controller Floppy_drive_recal 0 Floppy_media_sense on drive 0 found rate 0
Do we need a cpu with TSC support to run SeaBIOS?
So we should use '-cpu Conroe' or '-cpu core2duo' minimum?
It's probably about time we update qemu64 (the default CPU model) to provide a more modern set of features. Once libvirt adapts to the CPU model alias/version interface we added in 4.1, this will become easier to do.