Re: [Qemu-devel] Regression with floppy drive controller

[Philippe Mathieu-Daudé]

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 <address@hidden>
>>> 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.

Looking at the fdc timer I noticed it use a static '50 ns' magic value.

Increasing this value allows the floppy image to boot again, using this
snippet:

-- >8 --
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 9b24cb9b85..5fc54073fd 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2134,7 +2134,7 @@ static void fdctrl_handle_readid(FDCtrl *fdctrl,
int direction)

     cur_drv->head = (fdctrl->fifo[1] >> 2) & 1;
     timer_mod(fdctrl->result_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
-             (NANOSECONDS_PER_SECOND / 50));
+             (NANOSECONDS_PER_SECOND / 5000));
 }

 static void fdctrl_handle_format_track(FDCtrl *fdctrl, int direction)
---

Any idea what is the correct value to use here?

Regards,

Phil.