[Qemu-devel] [PATCH v2 00/10] Misc fixes for floppy emulation

[Qemu-devel] [PATCH v2 00/10] Misc fixes for floppy emulation

[Hervé Poussineau]

Here are misc fixes done by VirtualBox team.
With these patches, floppy emulation is now good enough to run Xenix.

Changes v1->v2:
- updated commit messages
- added missing 'break' and braces
- moved patch 8 before patch 6

Hervé Poussineau (10):
  fdc: take side count into account
  fdc: set busy bit when starting a command
  fdc: most control commands do not generate interrupts
  fdc: emulate stepping 0
  fdc: handle read-only floppies (abort early on write commands)
  fdc: add CCR (Configuration Control Register) write register
  block: add a transfer rate for floppy types
  fdc: check if media rate is correct before doing any transfer
  fdc: fix seek command, which shouldn't check tracks
  fdc: DIR (Digital Input Register) should return status of current
    drive...

 block.c  |   74 ++++++++++++++++++++------------------
 block.h  |   10 +++++-
 hw/fdc.c |  117 ++++++++++++++++++++++++++++++++++++++++++++++----------------
 hw/pc.c  |    3 +-
 4 files changed, 137 insertions(+), 67 deletions(-)

-- 
1.7.7.3

[Qemu-devel] [PATCH v2 01/10] fdc: take side count into account

[Hervé Poussineau]

Floppies can be simple or double-sided. However, current code
was only taking the common case into account (ie 2 sides).

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/fdc.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 70aa5c7..c1898a6 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -95,16 +95,19 @@ static void fd_init(FDrive *drv)
     drv->max_track = 0;
 }
 
+#define NUM_SIDES(drv) ((drv)->flags & FDISK_DBL_SIDES ? 2 : 1)
+
 static int fd_sector_calc(uint8_t head, uint8_t track, uint8_t sect,
-                          uint8_t last_sect)
+                          uint8_t last_sect, uint8_t num_sides)
 {
-    return (((track * 2) + head) * last_sect) + sect - 1;
+    return (((track * num_sides) + head) * last_sect) + sect - 1;
 }
 
 /* Returns current position, in sectors, for given drive */
 static int fd_sector(FDrive *drv)
 {
-    return fd_sector_calc(drv->head, drv->track, drv->sect, drv->last_sect);
+    return fd_sector_calc(drv->head, drv->track, drv->sect, drv->last_sect,
+                          NUM_SIDES(drv));
 }
 
 /* Seek to a new position:
@@ -135,7 +138,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t track, uint8_t sect,
                        drv->max_track, drv->last_sect);
         return 3;
     }
-    sector = fd_sector_calc(head, track, sect, drv->last_sect);
+    sector = fd_sector_calc(head, track, sect, drv->last_sect, NUM_SIDES(drv));
     ret = 0;
     if (sector != fd_sector(drv)) {
 #if 0
@@ -1019,7 +1022,8 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
     ks = fdctrl->fifo[4];
     FLOPPY_DPRINTF("Start transfer at %d %d %02x %02x (%d)\n",
                    GET_CUR_DRV(fdctrl), kh, kt, ks,
-                   fd_sector_calc(kh, kt, ks, cur_drv->last_sect));
+                   fd_sector_calc(kh, kt, ks, cur_drv->last_sect,
+                                  NUM_SIDES(cur_drv)));
     switch (fd_seek(cur_drv, kh, kt, ks, fdctrl->config & FD_CONFIG_EIS)) {
     case 2:
         /* sect too big */
@@ -1289,7 +1293,8 @@ static void fdctrl_format_sector(FDCtrl *fdctrl)
     ks = fdctrl->fifo[8];
     FLOPPY_DPRINTF("format sector at %d %d %02x %02x (%d)\n",
                    GET_CUR_DRV(fdctrl), kh, kt, ks,
-                   fd_sector_calc(kh, kt, ks, cur_drv->last_sect));
+                   fd_sector_calc(kh, kt, ks, cur_drv->last_sect,
+                                  NUM_SIDES(cur_drv)));
     switch (fd_seek(cur_drv, kh, kt, ks, fdctrl->config & FD_CONFIG_EIS)) {
     case 2:
         /* sect too big */
-- 
1.7.7.3

[Qemu-devel] [PATCH v2 02/10] fdc: set busy bit when starting a command

[Hervé Poussineau]

This bit must be active while a command is currently executed.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/fdc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index c1898a6..1b9f303 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -1446,7 +1446,6 @@ static void fdctrl_handle_readid(FDCtrl *fdctrl, int direction)
 {
     FDrive *cur_drv = get_cur_drv(fdctrl);
 
-    /* XXX: should set main status register to busy */
     cur_drv->head = (fdctrl->fifo[1] >> 2) & 1;
     qemu_mod_timer(fdctrl->result_timer,
                    qemu_get_clock_ns(vm_clock) + (get_ticks_per_sec() / 50));
@@ -1734,6 +1733,7 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
         pos = command_to_handler[value & 0xff];
         FLOPPY_DPRINTF("%s command\n", handlers[pos].name);
         fdctrl->data_len = handlers[pos].parameters + 1;
+        fdctrl->msr |= FD_MSR_CMDBUSY;
     }
 
     FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
-- 
1.7.7.3

Re: [Qemu-devel] [PATCH v2 02/10] fdc: set busy bit when starting a command

[Kevin Wolf]

Am 15.01.2012 08:51, schrieb Hervé Poussineau:
> This bit must be active while a command is currently executed.
> 
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>

I believe this could use some more cleanup. The flag is set and reset
multiple times in places that aren't quite obvious. Not saying it should
be done in this patch, but maybe in a follow-up series.

Kevin

[Qemu-devel] [PATCH v2 03/10] fdc: most control commands do not generate interrupts

[Hervé Poussineau]

In fact, only three control commands generate an interrupt:
read_id, recalibrate and seek

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/fdc.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 1b9f303..bedaeca 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -1348,7 +1348,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, fdctrl->lock);
+    fdctrl_set_fifo(fdctrl, 1, 0);
 }
 
 static void fdctrl_handle_dumpreg(FDCtrl *fdctrl, int direction)
@@ -1380,7 +1380,7 @@ static void fdctrl_handle_version(FDCtrl *fdctrl, int direction)
 {
     /* Controller's version */
     fdctrl->fifo[0] = fdctrl->version;
-    fdctrl_set_fifo(fdctrl, 1, 1);
+    fdctrl_set_fifo(fdctrl, 1, 0);
 }
 
 static void fdctrl_handle_partid(FDCtrl *fdctrl, int direction)
@@ -1439,7 +1439,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, 1);
+    fdctrl_set_fifo(fdctrl, 15, 0);
 }
 
 static void fdctrl_handle_readid(FDCtrl *fdctrl, int direction)
@@ -1580,7 +1580,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, 1);
+    fdctrl_set_fifo(fdctrl, 1, 0);
 }
 
 static void fdctrl_handle_option(FDCtrl *fdctrl, int direction)
@@ -1599,7 +1599,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, 1);
+            fdctrl_set_fifo(fdctrl, 4, 0);
         } else {
             fdctrl_reset_fifo(fdctrl);
         }
@@ -1607,7 +1607,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, 1);
+        fdctrl_set_fifo(fdctrl, 1, 0);
     }
 }
 
-- 
1.7.7.3

[Qemu-devel] [PATCH v2 04/10] fdc: emulate stepping 0

[Hervé Poussineau]

Stepping 1 (S82078B) is not fully i82078 compatible, so better stick to initial revision

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/fdc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index bedaeca..0e167f8 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -1385,7 +1385,7 @@ static void fdctrl_handle_version(FDCtrl *fdctrl, int direction)
 
 static void fdctrl_handle_partid(FDCtrl *fdctrl, int direction)
 {
-    fdctrl->fifo[0] = 0x41; /* Stepping 1 */
+    fdctrl->fifo[0] = 0x01; /* Stepping 0 */
     fdctrl_set_fifo(fdctrl, 1, 0);
 }
 
-- 
1.7.7.3

Re: [Qemu-devel] [PATCH v2 04/10] fdc: emulate stepping 0

[Kevin Wolf]

Am 15.01.2012 08:51, schrieb Hervé Poussineau:
> Stepping 1 (S82078B) is not fully i82078 compatible, so better stick to initial revision
> 
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> ---
>  hw/fdc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/fdc.c b/hw/fdc.c
> index bedaeca..0e167f8 100644
> --- a/hw/fdc.c
> +++ b/hw/fdc.c
> @@ -1385,7 +1385,7 @@ static void fdctrl_handle_version(FDCtrl *fdctrl, int direction)
>  
>  static void fdctrl_handle_partid(FDCtrl *fdctrl, int direction)
>  {
> -    fdctrl->fifo[0] = 0x41; /* Stepping 1 */
> +    fdctrl->fifo[0] = 0x01; /* Stepping 0 */
>      fdctrl_set_fifo(fdctrl, 1, 0);
>  }
>  

Hm, this is the kind of change that I'm hesitant to make because I don't
understand the implications. Can you give some more details what is
fixed by this and why you think it's harmless for currently working OSes?

My spec says this:

     6 3 14 PART ID COMMAND
    This command can be used to identify the floppy
    disk controller as an enhanced controller The first
    stepping of the 82078 (all 44 pin versions) will yield
    0x41 in the result phase of this command Any future
    enhancements on these parts will be denoted by the
    5 LSBs (0x01 to 0x1F)

It doesn't even define what 0x01 would mean.

Kevin

Re: [Qemu-devel] [PATCH v2 04/10] fdc: emulate stepping 0

[Hervé Poussineau]

Hi,

Kevin Wolf a écrit :
> Am 15.01.2012 08:51, schrieb Hervé Poussineau:
>> Stepping 1 (S82078B) is not fully i82078 compatible, so better stick to initial revision
>>> 
> Hm, this is the kind of change that I'm hesitant to make because I don't
> understand the implications. Can you give some more details what is
> fixed by this and why you think it's harmless for currently working OSes?
> 

I was thinking at comments like
https://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=blob;f=drivers/char/ftape/lowlevel/fdc-io.c;h=65c9d2ec60bdd7221a30a776fba377c37b89c078;hb=refs/heads/linux-2.6.18.y#l1119

Anyway, I didn't see any real problem yet with current code.

I'll resend this serie without this patch.

Regards

Hervé

[Qemu-devel] [PATCH v2 05/10] fdc: handle read-only floppies (abort early on write commands)

[Hervé Poussineau]

A real floppy doesn't attempt to write to read-only media either.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/fdc.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 0e167f8..078ff0c 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -300,6 +300,7 @@ enum {
 };
 
 enum {
+    FD_SR1_NW       = 0x02, /* Not writable */
     FD_SR1_EC       = 0x80, /* End of cylinder */
 };
 
@@ -1179,6 +1180,16 @@ static int fdctrl_transfer_handler (void *opaque, int nchan,
             break;
         case FD_DIR_WRITE:
             /* WRITE commands */
+            if (cur_drv->ro) {
+                /* Handle readonly medium early, no need to do DMA, touch the
+                 * LED or attempt any writes. A real floppy doesn't attempt
+                 * to write to readonly media either. */
+                fdctrl_stop_transfer(fdctrl,
+                                     FD_SR0_ABNTERM | FD_SR0_SEEK, FD_SR1_NW,
+                                     0x00);
+                goto transfer_error;
+            }
+
             DMA_read_memory (nchan, fdctrl->fifo + rel_pos,
                              fdctrl->data_pos, len);
             if (bdrv_write(cur_drv->bs, fd_sector(cur_drv),
-- 
1.7.7.3

[Qemu-devel] [PATCH v2 06/10] fdc: add CCR (Configuration Control Register) write register

[Hervé Poussineau]

DIR and CCR registers share the same address ; DIR is read-only
while CCR is write-only

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/fdc.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 078ff0c..6726450 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -224,6 +224,7 @@ static void fdctrl_write_rate(FDCtrl *fdctrl, uint32_t value);
 static uint32_t fdctrl_read_data(FDCtrl *fdctrl);
 static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value);
 static uint32_t fdctrl_read_dir(FDCtrl *fdctrl);
+static void fdctrl_write_ccr(FDCtrl *fdctrl, uint32_t value);
 
 enum {
     FD_DIR_WRITE   = 0,
@@ -248,6 +249,7 @@ enum {
     FD_REG_DSR = 0x04,
     FD_REG_FIFO = 0x05,
     FD_REG_DIR = 0x07,
+    FD_REG_CCR = 0x07,
 };
 
 enum {
@@ -491,6 +493,9 @@ static void fdctrl_write (void *opaque, uint32_t reg, uint32_t value)
     case FD_REG_FIFO:
         fdctrl_write_data(fdctrl, value);
         break;
+    case FD_REG_CCR:
+        fdctrl_write_ccr(fdctrl, value);
+        break;
     default:
         break;
     }
@@ -881,6 +886,23 @@ static void fdctrl_write_rate(FDCtrl *fdctrl, uint32_t value)
     fdctrl->dsr = value;
 }
 
+/* Configuration control register: 0x07 (write) */
+static void fdctrl_write_ccr(FDCtrl *fdctrl, uint32_t value)
+{
+    /* Reset mode */
+    if (!(fdctrl->dor & FD_DOR_nRESET)) {
+        FLOPPY_DPRINTF("Floppy controller in RESET state !\n");
+        return;
+    }
+    FLOPPY_DPRINTF("configuration control register set to 0x%02x\n", value);
+
+    /* Only the rate selection bits used in AT mode, and we
+     * store those in the DSR.
+     */
+    fdctrl->dsr = (fdctrl->dsr & ~FD_DSR_DRATEMASK) |
+                  (value & FD_DSR_DRATEMASK);
+}
+
 static int fdctrl_media_changed(FDrive *drv)
 {
     int ret;
-- 
1.7.7.3

[Qemu-devel] [PATCH v2 07/10] block: add a transfer rate for floppy types

[Hervé Poussineau]

Floppies must be read at a specific transfer rate, depending of its own format.
Update floppy description table to include required transfer rate.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 block.c  |   74 ++++++++++++++++++++++++++++++++-----------------------------
 block.h  |   10 +++++++-
 hw/fdc.c |    3 +-
 hw/pc.c  |    3 +-
 4 files changed, 52 insertions(+), 38 deletions(-)

diff --git a/block.c b/block.c
index 3f072f6..2c3fefb 100644
--- a/block.c
+++ b/block.c
@@ -1884,58 +1884,60 @@ typedef struct FDFormat {
     uint8_t last_sect;
     uint8_t max_track;
     uint8_t max_head;
+    FDriveRate rate;
 } FDFormat;
 
 static const FDFormat fd_formats[] = {
     /* First entry is default format */
     /* 1.44 MB 3"1/2 floppy disks */
-    { FDRIVE_DRV_144, 18, 80, 1, },
-    { FDRIVE_DRV_144, 20, 80, 1, },
-    { FDRIVE_DRV_144, 21, 80, 1, },
-    { FDRIVE_DRV_144, 21, 82, 1, },
-    { FDRIVE_DRV_144, 21, 83, 1, },
-    { FDRIVE_DRV_144, 22, 80, 1, },
-    { FDRIVE_DRV_144, 23, 80, 1, },
-    { FDRIVE_DRV_144, 24, 80, 1, },
+    { FDRIVE_DRV_144, 18, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 21, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 21, 82, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 21, 83, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 22, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 23, 80, 1, FDRIVE_RATE_500K },
+    { FDRIVE_DRV_144, 24, 80, 1, FDRIVE_RATE_500K, },
     /* 2.88 MB 3"1/2 floppy disks */
-    { FDRIVE_DRV_288, 36, 80, 1, },
-    { FDRIVE_DRV_288, 39, 80, 1, },
-    { FDRIVE_DRV_288, 40, 80, 1, },
-    { FDRIVE_DRV_288, 44, 80, 1, },
-    { FDRIVE_DRV_288, 48, 80, 1, },
+    { FDRIVE_DRV_288, 36, 80, 1, FDRIVE_RATE_1M, },
+    { FDRIVE_DRV_288, 39, 80, 1, FDRIVE_RATE_1M, },
+    { FDRIVE_DRV_288, 40, 80, 1, FDRIVE_RATE_1M, },
+    { FDRIVE_DRV_288, 44, 80, 1, FDRIVE_RATE_1M, },
+    { FDRIVE_DRV_288, 48, 80, 1, FDRIVE_RATE_1M, },
     /* 720 kB 3"1/2 floppy disks */
-    { FDRIVE_DRV_144,  9, 80, 1, },
-    { FDRIVE_DRV_144, 10, 80, 1, },
-    { FDRIVE_DRV_144, 10, 82, 1, },
-    { FDRIVE_DRV_144, 10, 83, 1, },
-    { FDRIVE_DRV_144, 13, 80, 1, },
-    { FDRIVE_DRV_144, 14, 80, 1, },
+    { FDRIVE_DRV_144,  9, 80, 1, FDRIVE_RATE_250K, },
+    { FDRIVE_DRV_144, 10, 80, 1, FDRIVE_RATE_250K, },
+    { FDRIVE_DRV_144, 10, 82, 1, FDRIVE_RATE_250K, },
+    { FDRIVE_DRV_144, 10, 83, 1, FDRIVE_RATE_250K, },
+    { FDRIVE_DRV_144, 13, 80, 1, FDRIVE_RATE_250K, },
+    { FDRIVE_DRV_144, 14, 80, 1, FDRIVE_RATE_250K, },
     /* 1.2 MB 5"1/4 floppy disks */
-    { FDRIVE_DRV_120, 15, 80, 1, },
-    { FDRIVE_DRV_120, 18, 80, 1, },
-    { FDRIVE_DRV_120, 18, 82, 1, },
-    { FDRIVE_DRV_120, 18, 83, 1, },
-    { FDRIVE_DRV_120, 20, 80, 1, },
+    { FDRIVE_DRV_120, 15, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_120, 18, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_120, 18, 82, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_120, 18, 83, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_120, 20, 80, 1, FDRIVE_RATE_500K, },
     /* 720 kB 5"1/4 floppy disks */
-    { FDRIVE_DRV_120,  9, 80, 1, },
-    { FDRIVE_DRV_120, 11, 80, 1, },
+    { FDRIVE_DRV_120,  9, 80, 1, FDRIVE_RATE_250K, },
+    { FDRIVE_DRV_120, 11, 80, 1, FDRIVE_RATE_250K, },
     /* 360 kB 5"1/4 floppy disks */
-    { FDRIVE_DRV_120,  9, 40, 1, },
-    { FDRIVE_DRV_120,  9, 40, 0, },
-    { FDRIVE_DRV_120, 10, 41, 1, },
-    { FDRIVE_DRV_120, 10, 42, 1, },
+    { FDRIVE_DRV_120,  9, 40, 1, FDRIVE_RATE_300K, },
+    { FDRIVE_DRV_120,  9, 40, 0, FDRIVE_RATE_300K, },
+    { FDRIVE_DRV_120, 10, 41, 1, FDRIVE_RATE_300K, },
+    { FDRIVE_DRV_120, 10, 42, 1, FDRIVE_RATE_300K, },
     /* 320 kB 5"1/4 floppy disks */
-    { FDRIVE_DRV_120,  8, 40, 1, },
-    { FDRIVE_DRV_120,  8, 40, 0, },
+    { FDRIVE_DRV_120,  8, 40, 1, FDRIVE_RATE_250K, },
+    { FDRIVE_DRV_120,  8, 40, 0, FDRIVE_RATE_250K, },
     /* 360 kB must match 5"1/4 better than 3"1/2... */
-    { FDRIVE_DRV_144,  9, 80, 0, },
+    { FDRIVE_DRV_144,  9, 80, 0, FDRIVE_RATE_250K, },
     /* end */
-    { FDRIVE_DRV_NONE, -1, -1, 0, },
+    { FDRIVE_DRV_NONE, -1, -1, 0, 0, },
 };
 
 void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads,
                                    int *max_track, int *last_sect,
-                                   FDriveType drive_in, FDriveType *drive)
+                                   FDriveType drive_in, FDriveType *drive,
+                                   FDriveRate *rate)
 {
     const FDFormat *parse;
     uint64_t nb_sectors, size;
@@ -1944,6 +1946,7 @@ void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads,
     bdrv_get_geometry_hint(bs, nb_heads, max_track, last_sect);
     if (*nb_heads != 0 && *max_track != 0 && *last_sect != 0) {
         /* User defined disk */
+        *rate = FDRIVE_RATE_500K;
     } else {
         bdrv_get_geometry(bs, &nb_sectors);
         match = -1;
@@ -1978,6 +1981,7 @@ void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads,
         *max_track = parse->max_track;
         *last_sect = parse->last_sect;
         *drive = parse->drive;
+        *rate = parse->rate;
     }
 }
 
diff --git a/block.h b/block.h
index 3bd4398..27cc966 100644
--- a/block.h
+++ b/block.h
@@ -240,9 +240,17 @@ typedef enum FDriveType {
     FDRIVE_DRV_NONE = 0x03,   /* No drive connected     */
 } FDriveType;
 
+typedef enum FDriveRate {
+    FDRIVE_RATE_500K = 0x00,  /* 500 Kbps */
+    FDRIVE_RATE_300K = 0x01,  /* 300 Kbps */
+    FDRIVE_RATE_250K = 0x02,  /* 250 Kbps */
+    FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
+} FDriveRate;
+
 void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads,
                                    int *max_track, int *last_sect,
-                                   FDriveType drive_in, FDriveType *drive);
+                                   FDriveType drive_in, FDriveType *drive,
+                                   FDriveRate *rate);
 int bdrv_get_translation_hint(BlockDriverState *bs);
 void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error,
                        BlockErrorAction on_write_error);
diff --git a/hw/fdc.c b/hw/fdc.c
index 6726450..629408f 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -172,12 +172,13 @@ static void fd_revalidate(FDrive *drv)
 {
     int nb_heads, max_track, last_sect, ro;
     FDriveType drive;
+    FDriveRate rate;
 
     FLOPPY_DPRINTF("revalidate\n");
     if (drv->bs != NULL && bdrv_is_inserted(drv->bs)) {
         ro = bdrv_is_read_only(drv->bs);
         bdrv_get_floppy_geometry_hint(drv->bs, &nb_heads, &max_track,
-                                      &last_sect, drv->drive, &drive);
+                                      &last_sect, drv->drive, &drive, &rate);
         if (nb_heads != 0 && max_track != 0 && last_sect != 0) {
             FLOPPY_DPRINTF("User defined disk (%d %d %d)",
                            nb_heads - 1, max_track, last_sect);
diff --git a/hw/pc.c b/hw/pc.c
index 85304cf..090e923 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -336,6 +336,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
 {
     int val, nb, nb_heads, max_track, last_sect, i;
     FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
+    FDriveRate rate;
     BlockDriverState *fd[MAX_FD];
     static pc_cmos_init_late_arg arg;
 
@@ -384,7 +385,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
             if (fd[i] && bdrv_is_inserted(fd[i])) {
                 bdrv_get_floppy_geometry_hint(fd[i], &nb_heads, &max_track,
                                               &last_sect, FDRIVE_DRV_NONE,
-                                              &fd_type[i]);
+                                              &fd_type[i], &rate);
             }
         }
     }
-- 
1.7.7.3

Re: [Qemu-devel] [PATCH v2 07/10] block: add a transfer rate for floppy types

[Kevin Wolf]

Am 15.01.2012 08:51, schrieb Hervé Poussineau:
> Floppies must be read at a specific transfer rate, depending of its own format.
> Update floppy description table to include required transfer rate.
> 
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> ---
>  block.c  |   74 ++++++++++++++++++++++++++++++++-----------------------------
>  block.h  |   10 +++++++-
>  hw/fdc.c |    3 +-
>  hw/pc.c  |    3 +-

Meh. Having any floppy-specific logic in the block layer is wrong. We
need to finally get this moved into fdc.c.

Kevin

Re: [Qemu-devel] [PATCH v2 07/10] block: add a transfer rate for floppy types

[Blue Swirl]

On Mon, Jan 16, 2012 at 10:18, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 15.01.2012 08:51, schrieb Hervé Poussineau:
>> Floppies must be read at a specific transfer rate, depending of its own format.
>> Update floppy description table to include required transfer rate.
>>
>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>> ---
>>  block.c  |   74 ++++++++++++++++++++++++++++++++-----------------------------
>>  block.h  |   10 +++++++-
>>  hw/fdc.c |    3 +-
>>  hw/pc.c  |    3 +-
>
> Meh. Having any floppy-specific logic in the block layer is wrong. We
> need to finally get this moved into fdc.c.

Well, actually this code was moved recently from fdc.c to block.c
(5bbdbb4676d17e782ae83055bac58e0751b25e4b). The other geometry
guessing functions (ATA CHS) are also there. If we supported native
floppy (or ATA) pass trough, the geometry would have to be read from
the host device, so I think it's logical to keep that in block level
instead of all devices. Maybe we could also split block.c into
block-fdc.c, block-ata.c etc.

Re: [Qemu-devel] [PATCH v2 07/10] block: add a transfer rate for floppy types

[Kevin Wolf]

Am 21.01.2012 20:02, schrieb Blue Swirl:
> On Mon, Jan 16, 2012 at 10:18, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 15.01.2012 08:51, schrieb Hervé Poussineau:
>>> Floppies must be read at a specific transfer rate, depending of its own format.
>>> Update floppy description table to include required transfer rate.
>>>
>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>> ---
>>>  block.c  |   74 ++++++++++++++++++++++++++++++++-----------------------------
>>>  block.h  |   10 +++++++-
>>>  hw/fdc.c |    3 +-
>>>  hw/pc.c  |    3 +-
>>
>> Meh. Having any floppy-specific logic in the block layer is wrong. We
>> need to finally get this moved into fdc.c.
> 
> Well, actually this code was moved recently from fdc.c to block.c
> (5bbdbb4676d17e782ae83055bac58e0751b25e4b). The other geometry
> guessing functions (ATA CHS) are also there. If we supported native
> floppy (or ATA) pass trough, the geometry would have to be read from
> the host device, so I think it's logical to keep that in block level
> instead of all devices. Maybe we could also split block.c into
> block-fdc.c, block-ata.c etc.

The geometry is guest state, so it shouldn't be in the block layer,
which deals with host state. Maybe we could need some hw/block.c that
deals with guest state concepts that are shared between multiple device.
Images or passthrough backends could provide defaults.

I'll admit that not having an obvious place for media (we only have it
for images and guest devices) doesn't make the design decisions easier.

Kevin

Re: [Qemu-devel] [PATCH v2 07/10] block: add a transfer rate for floppy types

[Blue Swirl]

On Mon, Jan 23, 2012 at 08:51, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 21.01.2012 20:02, schrieb Blue Swirl:
>> On Mon, Jan 16, 2012 at 10:18, Kevin Wolf <kwolf@redhat.com> wrote:
>>> Am 15.01.2012 08:51, schrieb Hervé Poussineau:
>>>> Floppies must be read at a specific transfer rate, depending of its own format.
>>>> Update floppy description table to include required transfer rate.
>>>>
>>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>>> ---
>>>>  block.c  |   74 ++++++++++++++++++++++++++++++++-----------------------------
>>>>  block.h  |   10 +++++++-
>>>>  hw/fdc.c |    3 +-
>>>>  hw/pc.c  |    3 +-
>>>
>>> Meh. Having any floppy-specific logic in the block layer is wrong. We
>>> need to finally get this moved into fdc.c.
>>
>> Well, actually this code was moved recently from fdc.c to block.c
>> (5bbdbb4676d17e782ae83055bac58e0751b25e4b). The other geometry
>> guessing functions (ATA CHS) are also there. If we supported native
>> floppy (or ATA) pass trough, the geometry would have to be read from
>> the host device, so I think it's logical to keep that in block level
>> instead of all devices. Maybe we could also split block.c into
>> block-fdc.c, block-ata.c etc.
>
> The geometry is guest state, so it shouldn't be in the block layer,
> which deals with host state. Maybe we could need some hw/block.c that
> deals with guest state concepts that are shared between multiple device.
> Images or passthrough backends could provide defaults.

Yes, that's even better.

> I'll admit that not having an obvious place for media (we only have it
> for images and guest devices) doesn't make the design decisions easier.

Right. Maybe the layering is not correct.

> Kevin

Re: [Qemu-devel] [PATCH v2 07/10] block: add a transfer rate for floppy types

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 21.01.2012 20:02, schrieb Blue Swirl:
>> On Mon, Jan 16, 2012 at 10:18, Kevin Wolf <kwolf@redhat.com> wrote:
>>> Am 15.01.2012 08:51, schrieb Hervé Poussineau:
>>>> Floppies must be read at a specific transfer rate, depending of its own format.
>>>> Update floppy description table to include required transfer rate.
>>>>
>>>> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
>>>> ---
>>>>  block.c  |   74 ++++++++++++++++++++++++++++++++-----------------------------
>>>>  block.h  |   10 +++++++-
>>>>  hw/fdc.c |    3 +-
>>>>  hw/pc.c  |    3 +-
>>>
>>> Meh. Having any floppy-specific logic in the block layer is wrong. We
>>> need to finally get this moved into fdc.c.
>> 
>> Well, actually this code was moved recently from fdc.c to block.c
>> (5bbdbb4676d17e782ae83055bac58e0751b25e4b). The other geometry
>> guessing functions (ATA CHS) are also there. If we supported native
>> floppy (or ATA) pass trough, the geometry would have to be read from
>> the host device, so I think it's logical to keep that in block level
>> instead of all devices. Maybe we could also split block.c into
>> block-fdc.c, block-ata.c etc.
>
> The geometry is guest state, so it shouldn't be in the block layer,
> which deals with host state.

Fully agree.

I tried to tackle geometry in my quest to untangle block device host and
guest state, but didn't get far.

>                              Maybe we could need some hw/block.c that
> deals with guest state concepts that are shared between multiple device.
> Images or passthrough backends could provide defaults.
>
> I'll admit that not having an obvious place for media (we only have it
> for images and guest devices) doesn't make the design decisions easier.

Yup.

[Qemu-devel] [PATCH v2 08/10] fdc: check if media rate is correct before doing any transfer

[Hervé Poussineau]

The programmed rate has to be the same as the required rate for the
floppy format ; if that's not the case, the transfer should abort.

Revalidate floppy after migration, so media_rate field doesn't have
to be saved/restored.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/fdc.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 629408f..685ea88 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -83,6 +83,7 @@ typedef struct FDrive {
     uint16_t bps;             /* Bytes per sector       */
     uint8_t ro;               /* Is read-only           */
     uint8_t media_changed;    /* Is media changed       */
+    uint8_t media_rate;       /* Data rate of medium    */
 } FDrive;
 
 static void fd_init(FDrive *drv)
@@ -195,6 +196,7 @@ static void fd_revalidate(FDrive *drv)
         drv->last_sect = last_sect;
         drv->ro = ro;
         drv->drive = drive;
+        drv->media_rate = rate;
     } else {
         FLOPPY_DPRINTF("No disk in drive\n");
         drv->last_sect = 0;
@@ -303,6 +305,7 @@ enum {
 };
 
 enum {
+    FD_SR1_MA       = 0x01, /* Missing address mark */
     FD_SR1_NW       = 0x02, /* Not writable */
     FD_SR1_EC       = 0x80, /* End of cylinder */
 };
@@ -582,6 +585,7 @@ static int fdc_post_load(void *opaque, int version_id)
 
     SET_CUR_DRV(s, s->dor_vmstate & FD_DOR_SELMASK);
     s->dor = s->dor_vmstate & ~FD_DOR_SELMASK;
+    fd_revalidate(get_cur_drv(s));
     return 0;
 }
 
@@ -1077,6 +1081,18 @@ static void fdctrl_start_transfer(FDCtrl *fdctrl, int direction)
         break;
     }
 
+    /* Check the data rate. If the programmed data rate does not match
+     * the currently inserted medium, the operation has to fail. */
+    if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
+        FLOPPY_DPRINTF("data rate mismatch (fdc=%d, media=%d)\n",
+                       fdctrl->dsr & FD_DSR_DRATEMASK, cur_drv->media_rate);
+        fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
+        fdctrl->fifo[3] = kt;
+        fdctrl->fifo[4] = kh;
+        fdctrl->fifo[5] = ks;
+        return;
+    }
+
     /* Set the FIFO state */
     fdctrl->data_dir = direction;
     fdctrl->data_pos = 0;
@@ -1799,7 +1815,14 @@ static void fdctrl_result_timer(void *opaque)
     if (cur_drv->last_sect != 0) {
         cur_drv->sect = (cur_drv->sect % cur_drv->last_sect) + 1;
     }
-    fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
+    /* READ_ID can't automatically succeed! */
+    if ((fdctrl->dsr & FD_DSR_DRATEMASK) != cur_drv->media_rate) {
+        FLOPPY_DPRINTF("read id rate mismatch (fdc=%d, media=%d)\n",
+                       fdctrl->dsr & FD_DSR_DRATEMASK, cur_drv->media_rate);
+        fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM, FD_SR1_MA, 0x00);
+    } else {
+        fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
+    }
 }
 
 static void fdctrl_change_cb(void *opaque, bool load)
-- 
1.7.7.3

[Qemu-devel] [PATCH v2 09/10] fdc: fix seek command, which shouldn't check tracks

[Hervé Poussineau]

The seek command just sends step pulses to the drive and doesn't care if
there is a medium inserted of if it is banging the head against the drive.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/fdc.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 685ea88..61d70eb 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -1599,13 +1599,12 @@ static void fdctrl_handle_seek(FDCtrl *fdctrl, int direction)
     SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
     cur_drv = get_cur_drv(fdctrl);
     fdctrl_reset_fifo(fdctrl);
-    if (fdctrl->fifo[2] > cur_drv->max_track) {
-        fdctrl_raise_irq(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK);
-    } else {
-        cur_drv->track = fdctrl->fifo[2];
-        /* Raise Interrupt */
-        fdctrl_raise_irq(fdctrl, FD_SR0_SEEK);
-    }
+    /* The seek command just sends step pulses to the drive and doesn't care if
+     * there is a medium inserted of if it's banging the head against the drive.
+     */
+    cur_drv->track = fdctrl->fifo[2];
+    /* Raise Interrupt */
+    fdctrl_raise_irq(fdctrl, FD_SR0_SEEK);
 }
 
 static void fdctrl_handle_perpendicular_mode(FDCtrl *fdctrl, int direction)
-- 
1.7.7.3

Re: [Qemu-devel] [PATCH v2 09/10] fdc: fix seek command, which shouldn't check tracks

[Kevin Wolf]

Am 15.01.2012 08:51, schrieb Hervé Poussineau:
> The seek command just sends step pulses to the drive and doesn't care if
> there is a medium inserted of if it is banging the head against the drive.
> 
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> ---
>  hw/fdc.c |   13 ++++++-------
>  1 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/fdc.c b/hw/fdc.c
> index 685ea88..61d70eb 100644
> --- a/hw/fdc.c
> +++ b/hw/fdc.c
> @@ -1599,13 +1599,12 @@ static void fdctrl_handle_seek(FDCtrl *fdctrl, int direction)
>      SET_CUR_DRV(fdctrl, fdctrl->fifo[1] & FD_DOR_SELMASK);
>      cur_drv = get_cur_drv(fdctrl);
>      fdctrl_reset_fifo(fdctrl);
> -    if (fdctrl->fifo[2] > cur_drv->max_track) {
> -        fdctrl_raise_irq(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK);
> -    } else {
> -        cur_drv->track = fdctrl->fifo[2];
> -        /* Raise Interrupt */
> -        fdctrl_raise_irq(fdctrl, FD_SR0_SEEK);
> -    }
> +    /* The seek command just sends step pulses to the drive and doesn't care if
> +     * there is a medium inserted of if it's banging the head against the drive.
> +     */
> +    cur_drv->track = fdctrl->fifo[2];

Do you really want to update this with an invalid value? This value is
used in other places without being checked again against max_track.

> +    /* Raise Interrupt */
> +    fdctrl_raise_irq(fdctrl, FD_SR0_SEEK);
>  }
>  
>  static void fdctrl_handle_perpendicular_mode(FDCtrl *fdctrl, int direction)

Kevin

[Qemu-devel] [PATCH v2 10/10] fdc: DIR (Digital Input Register) should return status of current drive...

[Hervé Poussineau]

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 hw/fdc.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 61d70eb..e68a1cb 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -215,6 +215,7 @@ static void fdctrl_reset_fifo(FDCtrl *fdctrl);
 static int fdctrl_transfer_handler (void *opaque, int nchan,
                                     int dma_pos, int dma_len);
 static void fdctrl_raise_irq(FDCtrl *fdctrl, uint8_t status0);
+static FDrive *get_cur_drv(FDCtrl *fdctrl);
 
 static uint32_t fdctrl_read_statusA(FDCtrl *fdctrl);
 static uint32_t fdctrl_read_statusB(FDCtrl *fdctrl);
@@ -934,14 +935,9 @@ static uint32_t fdctrl_read_dir(FDCtrl *fdctrl)
 {
     uint32_t retval = 0;
 
-    if (fdctrl_media_changed(drv0(fdctrl))
-     || fdctrl_media_changed(drv1(fdctrl))
-#if MAX_FD == 4
-     || fdctrl_media_changed(drv2(fdctrl))
-     || fdctrl_media_changed(drv3(fdctrl))
-#endif
-        )
+    if (fdctrl_media_changed(get_cur_drv(fdctrl))) {
         retval |= FD_DIR_DSKCHG;
+    }
     if (retval != 0) {
         FLOPPY_DPRINTF("Floppy digital input register: 0x%02x\n", retval);
     }
-- 
1.7.7.3