[Qemu-devel] Regression with floppy drive controller

[Qemu-devel] Regression with floppy drive controller

[Philippe Mathieu-Daudé]

[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?


Regards,

Phil.


PD: How to reproduce:

- Download Windows 98 SE floppy image from [2]

- Run QEMU using the 'isapc' machine:

  $ qemu-system-i386 -M isapc \
     -fda Windows\ 98\ Second\ Edition\ Boot.img

  SeaBIOS (version rel-1.11.0-11-g4a6dbce-prebuilt.qemu.org)
  Booting from Floppy...
  Boot failed: could not read the boot disk

[1] https://bugs.launchpad.net/qemu/+bug/1840719
[2] https://winworldpc.com/download/417d71c2-ae18-c39a-11c3-a4e284a2c3a5

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

[John Snow]

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?
> 
> 
> Regards,
> 
> Phil.
> 
> 
> PD: How to reproduce:
> 
> - Download Windows 98 SE floppy image from [2]
> 
> - Run QEMU using the 'isapc' machine:
> 
>   $ qemu-system-i386 -M isapc \
>      -fda Windows\ 98\ Second\ Edition\ Boot.img
> 
>   SeaBIOS (version rel-1.11.0-11-g4a6dbce-prebuilt.qemu.org)
>   Booting from Floppy...
>   Boot failed: could not read the boot disk
> 
> [1] https://bugs.launchpad.net/qemu/+bug/1840719
> [2] https://winworldpc.com/download/417d71c2-ae18-c39a-11c3-a4e284a2c3a5
> 

Well, that's unfortunate.

What version of QEMU shipped the SeaBIOS that caused the regression?

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

[Philippe Mathieu-Daudé]

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.

Regards,

Phil.

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 <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.

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.

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

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> 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.
> 
> Looking at the fdc timer I noticed it use a static '50 ns' magic value.

That's not 50ns

> 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));

That's 1/50th of a second in ns.

> +             (NANOSECONDS_PER_SECOND / 5000));

I'm not too sure about readid; but assuming we're rotating at 360rpm,
that's 6 revolutions/second, and 18 sectors/track = 108 sectors/second
(half of that for a double density disk).

So, the wait for a sector to spin around and read feels like it should
be in the region of 1/108 of a second + some latency - so 1/50th of a
second would seem to be in the ballpark or being right, where as 1/5000
of a second is way too fast for a poor old floppy.

Dave

>  }
> 
>  static void fdctrl_handle_format_track(FDCtrl *fdctrl, int direction)
> ---
> 
> Any idea what is the correct value to use here?
> 
> Regards,
> 
> Phil.
> _______________________________________________
> SeaBIOS mailing list -- seabios@seabios.org
> To unsubscribe send an email to seabios-leave@seabios.org
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Philippe Mathieu-Daudé]

On 8/20/19 4:36 PM, Dr. David Alan Gilbert wrote:
> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
>> 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?
[...]
>> Looking at the fdc timer I noticed it use a static '50 ns' magic value.
> 
> That's not 50ns
> 
>> 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));
> 
> That's 1/50th of a second in ns.

Just noticed that too, so we have here 20ms.

>> +             (NANOSECONDS_PER_SECOND / 5000));
> 
> I'm not too sure about readid; but assuming we're rotating at 360rpm,
> that's 6 revolutions/second, and 18 sectors/track = 108 sectors/second
> (half of that for a double density disk).
> 
> So, the wait for a sector to spin around and read feels like it should
> be in the region of 1/108 of a second + some latency - so 1/50th of a
> second would seem to be in the ballpark or being right, where as 1/5000
> of a second is way too fast for a poor old floppy.

The first command sent is READ_ID.

Reading the Intel 82077AA datasheet:

  The READ ID command is used to find the present
  position of the recording heads. The 82077AA
  stores the values from the first ID Field it is able to
  read into its registers. If the 82077AA does not find
  an ID Address Mark on the diskette after the second
  occurrence of a pulse on the IDX pin, it then sets the
  IC code in Status Register 0 to ‘‘01’’ (Abnormal ter-
  mination), sets the MA bit in Status Register 1 to
  ‘’1’’, and terminates the command.

Then later the SPECIFICATIONS table:

  nRD/nWR Pulse Width: min 90ns
  INDEX Pulse Width: min 5 'Internal Clock Period'

  The nominal values for the 'internal clock period' for the various
  data rates are:

    1 Mbps:  3 * osc period = 125ns
  500 Kbps:  6 * osc period = 250ns
  300 Kbps: 10 * osc period = 420ns
  250 Kbps: 12 * osc period = 500ns

IIUC the model we have DATARATE SELECT REGISTER (DSR) = 0

So DRATESEL=0 => datarate = 500 Kbps

So we should wait at least 250ns.

Trying the following snippet it also works:

-- >8 --
@@ -2133,8 +2133,8 @@ static void fdctrl_handle_readid(FDCtrl *fdctrl,
int direction)
     FDrive *cur_drv = get_cur_drv(fdctrl);

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

Note this is not the spining-up delay on reset:

  Before data can be transferred to or from the disk-
  ette, the disk drive motor must be brought up to
  speed. For most 3(/2 × disk drives, the spin-up time is
  300 ms, while the 5(/4 × drive usually requires about
  500 ms due to the increased moment of inertia asso-
  ciated with the larger diameter diskette.

This looks more closer to the 20ms order. So maybe what we miss
here is a RESET delay (of 500ms?) previous to the READ_ID?

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

[Philippe Mathieu-Daudé]

[Sorry seabios@ list for the noise!]

On 8/20/19 4:54 PM, Philippe Mathieu-Daudé wrote:
> On 8/20/19 4:36 PM, Dr. David Alan Gilbert wrote:
>> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
>>> 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?
> [...]
>>> Looking at the fdc timer I noticed it use a static '50 ns' magic value.
>>
>> That's not 50ns
>>
>>> 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));
>>
>> That's 1/50th of a second in ns.
> 
> Just noticed that too, so we have here 20ms.
> 
>>> +             (NANOSECONDS_PER_SECOND / 5000));
>>
>> I'm not too sure about readid; but assuming we're rotating at 360rpm,
>> that's 6 revolutions/second, and 18 sectors/track = 108 sectors/second
>> (half of that for a double density disk).
>>
>> So, the wait for a sector to spin around and read feels like it should
>> be in the region of 1/108 of a second + some latency - so 1/50th of a
>> second would seem to be in the ballpark or being right, where as 1/5000
>> of a second is way too fast for a poor old floppy.
> 
> The first command sent is READ_ID.
> 
> Reading the Intel 82077AA datasheet:
> 
>   The READ ID command is used to find the present
>   position of the recording heads. The 82077AA
>   stores the values from the first ID Field it is able to
>   read into its registers. If the 82077AA does not find
>   an ID Address Mark on the diskette after the second
>   occurrence of a pulse on the IDX pin, it then sets the
>   IC code in Status Register 0 to ‘‘01’’ (Abnormal ter-
>   mination), sets the MA bit in Status Register 1 to
>   ‘’1’’, and terminates the command.
> 
> Then later the SPECIFICATIONS table:
> 
>   nRD/nWR Pulse Width: min 90ns
>   INDEX Pulse Width: min 5 'Internal Clock Period'
> 
>   The nominal values for the 'internal clock period' for the various
>   data rates are:
> 
>     1 Mbps:  3 * osc period = 125ns
>   500 Kbps:  6 * osc period = 250ns
>   300 Kbps: 10 * osc period = 420ns
>   250 Kbps: 12 * osc period = 500ns
> 
> IIUC the model we have DATARATE SELECT REGISTER (DSR) = 0
> 
> So DRATESEL=0 => datarate = 500 Kbps
> 
> So we should wait at least 250ns.
> 
> Trying the following snippet it also works:
> 
> -- >8 --
> @@ -2133,8 +2133,8 @@ static void fdctrl_handle_readid(FDCtrl *fdctrl,
> int direction)
>      FDrive *cur_drv = get_cur_drv(fdctrl);
> 
>      cur_drv->head = (fdctrl->fifo[1] >> 2) & 1;
> -    timer_mod(fdctrl->result_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> -             (NANOSECONDS_PER_SECOND / 50));
> +    timer_mod(fdctrl->result_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)
> +                                    + 250);
>  }
> ---
> 
> Note this is not the spining-up delay on reset:
> 
>   Before data can be transferred to or from the disk-
>   ette, the disk drive motor must be brought up to
>   speed. For most 3(/2 × disk drives, the spin-up time is
>   300 ms, while the 5(/4 × drive usually requires about
>   500 ms due to the increased moment of inertia asso-
>   ciated with the larger diameter diskette.
> 
> This looks more closer to the 20ms order. So maybe what we miss
> here is a RESET delay (of 500ms?) previous to the READ_ID?

From the datasheet:

  After the motor has been turned on, the matching
  data rate for the media inserted into the disk drive
  should then be programmed to the 82077AA via the
  Configuration Control Register (CCR). The 82077AA
  is designed to allow a different data rate to be pro-
  grammed arbitrarily without disrupting the integrity of
  the device. In some applications, it is required to au-
  tomatically determine the recorded data rate of the
  inserted media. One technique for doing this is to
  perform a READ ID operation at each available data
  rate until a successful status is returned in the result
  phase.

However QEMU doesn't model this delay:

static void fdctrl_result_timer(void *opaque)
{
    FDCtrl *fdctrl = opaque;
    FDrive *cur_drv = get_cur_drv(fdctrl);

    /* Pretend we are spinning.
     * This is needed for Coherent, which uses READ ID to check for
     * sector interleaving.
     */

So latest snippet could be OK.

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

[Dr. David Alan Gilbert]

* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> On 8/20/19 4:36 PM, Dr. David Alan Gilbert wrote:
> > * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> >> 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?
> [...]
> >> Looking at the fdc timer I noticed it use a static '50 ns' magic value.
> > 
> > That's not 50ns
> > 
> >> 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));
> > 
> > That's 1/50th of a second in ns.
> 
> Just noticed that too, so we have here 20ms.
> 
> >> +             (NANOSECONDS_PER_SECOND / 5000));
> > 
> > I'm not too sure about readid; but assuming we're rotating at 360rpm,
> > that's 6 revolutions/second, and 18 sectors/track = 108 sectors/second
> > (half of that for a double density disk).
> > 
> > So, the wait for a sector to spin around and read feels like it should
> > be in the region of 1/108 of a second + some latency - so 1/50th of a
> > second would seem to be in the ballpark or being right, where as 1/5000
> > of a second is way too fast for a poor old floppy.
> 
> The first command sent is READ_ID.
> 
> Reading the Intel 82077AA datasheet:
> 
>   The READ ID command is used to find the present
>   position of the recording heads. The 82077AA
>   stores the values from the first ID Field it is able to
>   read into its registers. If the 82077AA does not find
>   an ID Address Mark on the diskette after the second
>   occurrence of a pulse on the IDX pin, it then sets the
>   IC code in Status Register 0 to ‘‘01’’ (Abnormal ter-
>   mination), sets the MA bit in Status Register 1 to
>   ‘’1’’, and terminates the command.
> 
> Then later the SPECIFICATIONS table:
> 
>   nRD/nWR Pulse Width: min 90ns
>   INDEX Pulse Width: min 5 'Internal Clock Period'

Note that's the pulse width, not the gap between the idx pulses.
My understanding is that an index pulse is once per rotation; ie. every
1/60th of a second.

The failure after 2 IDX pin pulses makes sense, that's saying if you've
not found a sector after spinning the disk twice then you fail.

So, your time to deliver a good result to a readid shoukd be the
rotational time for 1 or 2 sectors, where as the time to fail should
be the rotational time for about 2 whole rotations (ie 1/30 of a
second).

Dave

>   The nominal values for the 'internal clock period' for the various
>   data rates are:
> 
>     1 Mbps:  3 * osc period = 125ns
>   500 Kbps:  6 * osc period = 250ns
>   300 Kbps: 10 * osc period = 420ns
>   250 Kbps: 12 * osc period = 500ns
> 
> IIUC the model we have DATARATE SELECT REGISTER (DSR) = 0
> 
> So DRATESEL=0 => datarate = 500 Kbps
> 
> So we should wait at least 250ns.
> 
> Trying the following snippet it also works:
> 
> -- >8 --
> @@ -2133,8 +2133,8 @@ static void fdctrl_handle_readid(FDCtrl *fdctrl,
> int direction)
>      FDrive *cur_drv = get_cur_drv(fdctrl);
> 
>      cur_drv->head = (fdctrl->fifo[1] >> 2) & 1;
> -    timer_mod(fdctrl->result_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> -             (NANOSECONDS_PER_SECOND / 50));
> +    timer_mod(fdctrl->result_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)
> +                                    + 250);
>  }
> ---
> 
> Note this is not the spining-up delay on reset:
> 
>   Before data can be transferred to or from the disk-
>   ette, the disk drive motor must be brought up to
>   speed. For most 3(/2 × disk drives, the spin-up time is
>   300 ms, while the 5(/4 × drive usually requires about
>   500 ms due to the increased moment of inertia asso-
>   ciated with the larger diameter diskette.
> 
> This looks more closer to the 20ms order. So maybe what we miss
> here is a RESET delay (of 500ms?) previous to the READ_ID?
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Philippe Mathieu-Daudé]

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?

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

[Eduardo Habkost]

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.

-- 
Eduardo

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

[Gerd Hoffmann]

  Hi,

> 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

> Do we need a cpu with TSC support to run SeaBIOS?

Hmm.  seabios uses pmtimer if available.  isapc has no pmtimer though,
so it uses TSC instead.

> So we should use '-cpu Conroe' or '-cpu core2duo' minimum?

-cpu Conroe for -M isapc is kida silly though ...

Maybe we should simply build seabios with CONFIG_TSC_TIMER=n ?

Using the TSC in a virtual machine is problematic anyway, the
calibration can be _way_ off on a loaded host, this is why seabios
prefers the (fixed frequency) pmtimer. 

cheers,
  Gerd

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

[Paolo Bonzini]

On 21/08/19 08:42, Gerd Hoffmann wrote:
>> So we should use '-cpu Conroe' or '-cpu core2duo' minimum?
> -cpu Conroe for -M isapc is kida silly though ...
> 
> Maybe we should simply build seabios with CONFIG_TSC_TIMER=n ?
> 
> Using the TSC in a virtual machine is problematic anyway, the
> calibration can be _way_ off on a loaded host, this is why seabios
> prefers the (fixed frequency) pmtimer. 

Yeah, that falls back to the PIT which is a good enough source for
SeasBIOS's needs (1 MHz, one third of the pmtimer).  It overflows a bit
too easily but it shouldn't be a big deal for users of isapc.

Paolo

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

[Kevin O'Connor]

On Wed, Aug 21, 2019 at 08:42:08AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > 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
> 
> > Do we need a cpu with TSC support to run SeaBIOS?
> 
> Hmm.  seabios uses pmtimer if available.  isapc has no pmtimer though,
> so it uses TSC instead.

But, SeaBIOS should have automatically detected no TSC and then fallen
back to the PIT.  And the check does work correctly with "-cpu 486" in
my tests.

Is the PIT not working for some reason in the original setup?  (Any
time I attempt to run with "-M isapc" I just get an "Unable to unlock
ram - bridge not found" error.)  The PIT seems to work okay in my
setup.

-Kevin

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

[Gerd Hoffmann]

On Wed, Aug 21, 2019 at 09:31:48AM -0400, Kevin O'Connor wrote:
> On Wed, Aug 21, 2019 at 08:42:08AM +0200, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > 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
> > 
> > > Do we need a cpu with TSC support to run SeaBIOS?
> > 
> > Hmm.  seabios uses pmtimer if available.  isapc has no pmtimer though,
> > so it uses TSC instead.
> 
> But, SeaBIOS should have automatically detected no TSC and then fallen
> back to the PIT.  And the check does work correctly with "-cpu 486" in
> my tests.
> 
> Is the PIT not working for some reason in the original setup?  (Any
> time I attempt to run with "-M isapc" I just get an "Unable to unlock
> ram - bridge not found" error.)

Yep, that should be no problem, with isapc the ram is not locked in
the first place.

> The PIT seems to work okay in my
> setup.

Using the floppy image from
  https://winworldpc.com/download/49c380c2-a9c3-af25-c389-11c3a6e28094

qemu-system-x86_64 -cpu 486 -M isapc -fda win98se.raw         => fails
qemu-system-x86_64 -cpu 486,+tsc -M isapc -fda win98se.raw    => works

cheers,
  Gerd

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

[Gerd Hoffmann]

  Hi,

> > Is the PIT not working for some reason in the original setup?  (Any
> > time I attempt to run with "-M isapc" I just get an "Unable to unlock
> > ram - bridge not found" error.)
> 
> Yep, that should be no problem, with isapc the ram is not locked in
> the first place.

Oh, and you need a config small enough to fit into 128k, 256k roms don't
work for isapc.  When you turn off all PCI drivers this should be easy ;)

cheers,
  Gerd