[Qemu-devel] [PATCH 0/3] fdc: Fix FD_SR0_SEEK flag

[Qemu-devel] [PATCH 0/3] fdc: Fix FD_SR0_SEEK flag

[Kevin Wolf]

Pavel, does this make sense to you? It seems to fix the NT 3.1 floppy driver.

Kevin Wolf (3):
  fdc: Remove status0 parameter from fdctrl_set_fifo()
  fdc: Fix false FD_SR0_SEEK
  fdc-test: Check READ ID

 hw/fdc.c         |   38 ++++++++++++++----------------
 tests/fdc-test.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 85 insertions(+), 21 deletions(-)

-- 
1.7.6.5

[Qemu-devel] [PATCH 1/3] fdc: Remove status0 parameter from fdctrl_set_fifo()

[Kevin Wolf]

It decided whether an interrupt is triggered. Only one caller made use
of this functionality, so move the code there.

In this one caller, the interrupt must actually be triggered
unconditionally, like it was before commit 2fee0088. For example, a
successful read without an implied seek can result in st0 = 0, but still
triggers the interrupt.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/fdc.c |   33 ++++++++++++++++-----------------
 1 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 08830c1..78ae064 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -1079,15 +1079,12 @@ static void fdctrl_reset_fifo(FDCtrl *fdctrl)
 }
 
 /* Set FIFO status for the host to read */
-static void fdctrl_set_fifo(FDCtrl *fdctrl, int fifo_len, uint8_t status0)
+static void fdctrl_set_fifo(FDCtrl *fdctrl, int fifo_len)
 {
     fdctrl->data_dir = FD_DIR_READ;
     fdctrl->data_len = fifo_len;
     fdctrl->data_pos = 0;
     fdctrl->msr |= FD_MSR_CMDBUSY | FD_MSR_RQM | FD_MSR_DIO;
-    if (status0) {
-        fdctrl_raise_irq(fdctrl, status0);
-    }
 }
 
 /* Set an error: unimplemented/unknown command */
@@ -1096,7 +1093,7 @@ static void fdctrl_unimplemented(FDCtrl *fdctrl, int direction)
     qemu_log_mask(LOG_UNIMP, "fdc: unimplemented command 0x%02x\n",
                   fdctrl->fifo[0]);
     fdctrl->fifo[0] = FD_SR0_INVCMD;
-    fdctrl_set_fifo(fdctrl, 1, 0);
+    fdctrl_set_fifo(fdctrl, 1);
 }
 
 /* Seek to next sector
@@ -1170,7 +1167,9 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
     }
     fdctrl->msr |= FD_MSR_RQM | FD_MSR_DIO;
     fdctrl->msr &= ~FD_MSR_NONDMA;
-    fdctrl_set_fifo(fdctrl, 7, fdctrl->status0);
+
+    fdctrl_set_fifo(fdctrl, 7);
+    fdctrl_raise_irq(fdctrl, fdctrl->status0);
 }
 
 /* Prepare a data transfer (either DMA or FIFO) */
@@ -1538,7 +1537,7 @@ static void fdctrl_handle_lock(FDCtrl *fdctrl, int direction)
 {
     fdctrl->lock = (fdctrl->fifo[0] & 0x80) ? 1 : 0;
     fdctrl->fifo[0] = fdctrl->lock << 4;
-    fdctrl_set_fifo(fdctrl, 1, 0);
+    fdctrl_set_fifo(fdctrl, 1);
 }
 
 static void fdctrl_handle_dumpreg(FDCtrl *fdctrl, int direction)
@@ -1563,20 +1562,20 @@ static void fdctrl_handle_dumpreg(FDCtrl *fdctrl, int direction)
         (cur_drv->perpendicular << 2);
     fdctrl->fifo[8] = fdctrl->config;
     fdctrl->fifo[9] = fdctrl->precomp_trk;
-    fdctrl_set_fifo(fdctrl, 10, 0);
+    fdctrl_set_fifo(fdctrl, 10);
 }
 
 static void fdctrl_handle_version(FDCtrl *fdctrl, int direction)
 {
     /* Controller's version */
     fdctrl->fifo[0] = fdctrl->version;
-    fdctrl_set_fifo(fdctrl, 1, 0);
+    fdctrl_set_fifo(fdctrl, 1);
 }
 
 static void fdctrl_handle_partid(FDCtrl *fdctrl, int direction)
 {
     fdctrl->fifo[0] = 0x41; /* Stepping 1 */
-    fdctrl_set_fifo(fdctrl, 1, 0);
+    fdctrl_set_fifo(fdctrl, 1);
 }
 
 static void fdctrl_handle_restore(FDCtrl *fdctrl, int direction)
@@ -1629,7 +1628,7 @@ static void fdctrl_handle_save(FDCtrl *fdctrl, int direction)
     fdctrl->fifo[12] = fdctrl->pwrd;
     fdctrl->fifo[13] = 0;
     fdctrl->fifo[14] = 0;
-    fdctrl_set_fifo(fdctrl, 15, 0);
+    fdctrl_set_fifo(fdctrl, 15);
 }
 
 static void fdctrl_handle_readid(FDCtrl *fdctrl, int direction)
@@ -1695,7 +1694,7 @@ static void fdctrl_handle_sense_drive_status(FDCtrl *fdctrl, int direction)
         (cur_drv->head << 2) |
         GET_CUR_DRV(fdctrl) |
         0x28;
-    fdctrl_set_fifo(fdctrl, 1, 0);
+    fdctrl_set_fifo(fdctrl, 1);
 }
 
 static void fdctrl_handle_recalibrate(FDCtrl *fdctrl, int direction)
@@ -1720,7 +1719,7 @@ static void fdctrl_handle_sense_interrupt_status(FDCtrl *fdctrl, int direction)
         fdctrl->reset_sensei--;
     } else if (!(fdctrl->sra & FD_SRA_INTPEND)) {
         fdctrl->fifo[0] = FD_SR0_INVCMD;
-        fdctrl_set_fifo(fdctrl, 1, 0);
+        fdctrl_set_fifo(fdctrl, 1);
         return;
     } else {
         fdctrl->fifo[0] =
@@ -1729,7 +1728,7 @@ static void fdctrl_handle_sense_interrupt_status(FDCtrl *fdctrl, int direction)
     }
 
     fdctrl->fifo[1] = cur_drv->track;
-    fdctrl_set_fifo(fdctrl, 2, 0);
+    fdctrl_set_fifo(fdctrl, 2);
     fdctrl_reset_irq(fdctrl);
     fdctrl->status0 = FD_SR0_RDYCHG;
 }
@@ -1771,7 +1770,7 @@ static void fdctrl_handle_powerdown_mode(FDCtrl *fdctrl, int direction)
 {
     fdctrl->pwrd = fdctrl->fifo[1];
     fdctrl->fifo[0] = fdctrl->fifo[1];
-    fdctrl_set_fifo(fdctrl, 1, 0);
+    fdctrl_set_fifo(fdctrl, 1);
 }
 
 static void fdctrl_handle_option(FDCtrl *fdctrl, int direction)
@@ -1790,7 +1789,7 @@ static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direct
             fdctrl->fifo[0] = fdctrl->fifo[1];
             fdctrl->fifo[2] = 0;
             fdctrl->fifo[3] = 0;
-            fdctrl_set_fifo(fdctrl, 4, 0);
+            fdctrl_set_fifo(fdctrl, 4);
         } else {
             fdctrl_reset_fifo(fdctrl);
         }
@@ -1798,7 +1797,7 @@ static void fdctrl_handle_drive_specification_command(FDCtrl *fdctrl, int direct
         /* ERROR */
         fdctrl->fifo[0] = 0x80 |
             (cur_drv->head << 2) | GET_CUR_DRV(fdctrl);
-        fdctrl_set_fifo(fdctrl, 1, 0);
+        fdctrl_set_fifo(fdctrl, 1);
     }
 }
 
-- 
1.7.6.5

[Qemu-devel] [PATCH 2/3] fdc: Fix false FD_SR0_SEEK

[Kevin Wolf]

fdctrl_start/stop_transfer() used to set FD_SR0_SEEK no matter if
there actually was a seek or not. This is obviously wrong.

fdctrl_start_transfer() has this information because it performs the
seek itself. fdctrl_stop_transfer() gets status0 passed and callers
already take care of setting FD_SR0_SEEK where appropriate.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/fdc.c         |    5 ++---
 tests/fdc-test.c |    2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 78ae064..8ea3341 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -1149,8 +1149,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
     FDrive *cur_drv;
 
     cur_drv = get_cur_drv(fdctrl);
-    fdctrl->status0 = status0 | FD_SR0_SEEK | (cur_drv->head << 2) |
-                      GET_CUR_DRV(fdctrl);
+    fdctrl->status0 = status0 | (cur_drv->head << 2) | GET_CUR_DRV(fdctrl);
 
     FLOPPY_DPRINTF("transfer status: %02x %02x %02x (%02x)\n",
                    status0, status1, status2, fdctrl->status0);
@@ -1284,7 +1283,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
     if (direction != FD_DIR_WRITE)
         fdctrl->msr |= FD_MSR_DIO;
     /* IO based transfer: calculate len */
-    fdctrl_raise_irq(fdctrl, FD_SR0_SEEK);
+    fdctrl_raise_irq(fdctrl, did_seek ? FD_SR0_SEEK : 0);
 
     return;
 }
diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index fa74411..ff96560 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -152,7 +152,7 @@ static uint8_t send_read_command(void)
     }
 
     st0 = floppy_recv();
-    if (st0 != 0x60) {
+    if (st0 != 0x40) {
         ret = 1;
     }
 
-- 
1.7.6.5

[Qemu-devel] [PATCH 3/3] fdc-test: Check READ ID

[Kevin Wolf]

ST0 shouldn't include 0x20 (FD_SR0_SEEK) after READ ID.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/fdc-test.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index ff96560..b825959 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -48,6 +48,7 @@ enum {
 
 enum {
     CMD_SENSE_INT           = 0x08,
+    CMD_READ_ID             = 0x0a,
     CMD_SEEK                = 0x0f,
     CMD_READ                = 0xe6,
     CMD_RELATIVE_SEEK_OUT   = 0x8f,
@@ -320,6 +321,70 @@ static void test_relative_seek(void)
     g_assert(pcn == 0);
 }
 
+static void test_read_id(void)
+{
+    uint8_t drive = 0;
+    uint8_t head = 0;
+    uint8_t cyl;
+    uint8_t st0;
+
+    /* Seek to track 0 and check with READ ID */
+    send_seek(0);
+
+    floppy_send(CMD_READ_ID);
+    g_assert(!get_irq(FLOPPY_IRQ));
+    floppy_send(head << 2 | drive);
+
+    while (!get_irq(FLOPPY_IRQ)) {
+        /* qemu involves a timer with READ ID... */
+        clock_step(1000000000LL / 50);
+    }
+
+    st0 = floppy_recv();
+    floppy_recv();
+    floppy_recv();
+    cyl = floppy_recv();
+    head = floppy_recv();
+    floppy_recv();
+    floppy_recv();
+
+    g_assert_cmpint(cyl, ==, 0);
+    g_assert_cmpint(head, ==, 0);
+    g_assert_cmpint(st0, ==, head << 2);
+
+    /* Seek to track 8 on head 1 and check with READ ID */
+    head = 1;
+    cyl = 8;
+
+    floppy_send(CMD_SEEK);
+    floppy_send(head << 2 | drive);
+    g_assert(!get_irq(FLOPPY_IRQ));
+    floppy_send(cyl);
+    g_assert(get_irq(FLOPPY_IRQ));
+    ack_irq(NULL);
+
+    floppy_send(CMD_READ_ID);
+    g_assert(!get_irq(FLOPPY_IRQ));
+    floppy_send(head << 2 | drive);
+
+    while (!get_irq(FLOPPY_IRQ)) {
+        /* qemu involves a timer with READ ID... */
+        clock_step(1000000000LL / 50);
+    }
+
+    st0 = floppy_recv();
+    floppy_recv();
+    floppy_recv();
+    cyl = floppy_recv();
+    head = floppy_recv();
+    floppy_recv();
+    floppy_recv();
+
+    g_assert_cmpint(cyl, ==, 8);
+    g_assert_cmpint(head, ==, 1);
+    g_assert_cmpint(st0, ==, head << 2);
+}
+
 /* success if no crash or abort */
 static void fuzz_registers(void)
 {
@@ -369,6 +434,7 @@ int main(int argc, char **argv)
     qtest_add_func("/fdc/media_change", test_media_change);
     qtest_add_func("/fdc/sense_interrupt", test_sense_interrupt);
     qtest_add_func("/fdc/relative_seek", test_relative_seek);
+    qtest_add_func("/fdc/read_id", test_read_id);
     qtest_add_func("/fdc/fuzz-registers", fuzz_registers);
 
     ret = g_test_run();
-- 
1.7.6.5

Re: [Qemu-devel] [PATCH 0/3] fdc: Fix FD_SR0_SEEK flag

[Pavel Hrdina]

On 09/04/2012 11:46 AM, Kevin Wolf wrote:
> Pavel, does this make sense to you? It seems to fix the NT 3.1 floppy driver.
>
> Kevin Wolf (3):
>    fdc: Remove status0 parameter from fdctrl_set_fifo()
>    fdc: Fix false FD_SR0_SEEK
>    fdc-test: Check READ ID
>
>   hw/fdc.c         |   38 ++++++++++++++----------------
>   tests/fdc-test.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 85 insertions(+), 21 deletions(-)
>

Hi Kevin, yes, it make sense to me. It's definitely better now.

Re: [Qemu-devel] [PATCH 2/3] fdc: Fix false FD_SR0_SEEK

[Hervé Poussineau]

Kevin Wolf a écrit :
> fdctrl_start/stop_transfer() used to set FD_SR0_SEEK no matter if
> there actually was a seek or not. This is obviously wrong.
>
> fdctrl_start_transfer() has this information because it performs the
> seek itself. fdctrl_stop_transfer() gets status0 passed and callers
> already take care of setting FD_SR0_SEEK where appropriate.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/fdc.c         |    5 ++---
>  tests/fdc-test.c |    2 +-
>  2 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/fdc.c b/hw/fdc.c
> index 78ae064..8ea3341 100644
> --- a/hw/fdc.c
> +++ b/hw/fdc.c
> @@ -1149,8 +1149,7 @@ static void fdctrl_stop_transfer(FDCtrl *fdctrl, uint8_t status0,
>      FDrive *cur_drv;
>  
>      cur_drv = get_cur_drv(fdctrl);
> -    fdctrl->status0 = status0 | FD_SR0_SEEK | (cur_drv->head << 2) |
> -                      GET_CUR_DRV(fdctrl);
> +    fdctrl->status0 = status0 | (cur_drv->head << 2) | GET_CUR_DRV(fdctrl);
>  
>      FLOPPY_DPRINTF("transfer status: %02x %02x %02x (%02x)\n",
>                     status0, status1, status2, fdctrl->status0);
> @@ -1284,7 +1283,7 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
>      if (direction != FD_DIR_WRITE)
>          fdctrl->msr |= FD_MSR_DIO;
>      /* IO based transfer: calculate len */
> -    fdctrl_raise_irq(fdctrl, FD_SR0_SEEK);
> +    fdctrl_raise_irq(fdctrl, did_seek ? FD_SR0_SEEK : 0);
>  
>      return;
>  }
> diff --git a/tests/fdc-test.c b/tests/fdc-test.c
> index fa74411..ff96560 100644
> --- a/tests/fdc-test.c
> +++ b/tests/fdc-test.c
> @@ -152,7 +152,7 @@ static uint8_t send_read_command(void)
>      }
>  
>      st0 = floppy_recv();
> -    if (st0 != 0x60) {
> +    if (st0 != 0x40) {
>          ret = 1;
>      }
>  
>   

NACK for this patch.
It fixes the seek flags DMA transfers, but does not handles PIO transfer 
case.
I just sent a whole serie for floppy improvements on ML, with a fixed 
version of this patch.

Hervé