From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: John Snow <jsnow@redhat.com>,
seabios@seabios.org,
Nikolay Nikolov <nickysn@users.sourceforge.net>,
QEMU Developers <qemu-devel@nongnu.org>,
Eduardo Habkost <ehabkost@redhat.com>
Cc: Alex <coderain@sdf.org>, Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [Qemu-devel] Regression with floppy drive controller
Date: Tue, 20 Aug 2019 18:21:28 +0200 [thread overview]
Message-ID: <7f6e8a5c-8262-ae39-333a-e8f18b3174f0@redhat.com> (raw)
In-Reply-To: <cc0b5a77-8bc4-070b-31e4-f29d5a174eb8@redhat.com>
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?
next prev parent reply other threads:[~2019-08-20 16:22 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-20 10:25 [Qemu-devel] Regression with floppy drive controller Philippe Mathieu-Daudé
2019-08-20 13:12 ` John Snow
2019-08-20 13:38 ` Philippe Mathieu-Daudé
2019-08-20 14:00 ` Philippe Mathieu-Daudé
2019-08-20 14:36 ` [Qemu-devel] [SeaBIOS] " Dr. David Alan Gilbert
2019-08-20 14:54 ` Philippe Mathieu-Daudé
2019-08-20 15:02 ` Philippe Mathieu-Daudé
2019-08-20 15:04 ` Dr. David Alan Gilbert
2019-08-20 16:21 ` Philippe Mathieu-Daudé [this message]
2019-08-20 20:37 ` [Qemu-devel] " Eduardo Habkost
2019-08-21 6:42 ` Gerd Hoffmann
2019-08-21 7:45 ` Paolo Bonzini
2019-08-21 13:31 ` [Qemu-devel] [SeaBIOS] " Kevin O'Connor
2019-08-22 8:32 ` Gerd Hoffmann
2019-08-22 8:42 ` Gerd Hoffmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7f6e8a5c-8262-ae39-333a-e8f18b3174f0@redhat.com \
--to=philmd@redhat.com \
--cc=coderain@sdf.org \
--cc=ehabkost@redhat.com \
--cc=jsnow@redhat.com \
--cc=nickysn@users.sourceforge.net \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=seabios@seabios.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).