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 <nick...@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. -- Eduardo _______________________________________________ SeaBIOS mailing list -- seabios@seabios.org To unsubscribe send an email to seabios-le...@seabios.org
