Skip to contentSkip to conversation

Misc fixes for floppy emulation

PATCH Superseded Hervé Poussineau 4 participants 20 messages Jan 15, 2012 – Jan 27, 2012

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

v2 cover Misc fixes for floppy emulation

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(-)
Signature · 1 line
1.7.7.3
#1 lore raw

PATCH v2 1/10 fdc: take side count into account

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

---
 hw/fdc.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)
hw/fdc.c +11 -6
index 70aa5c7..c1898a6 100644
@@ -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 */
Signature · 1 line
1.7.7.3
#2 lore raw

PATCH v2 2/10 fdc: set busy bit when starting a command

In reply to Hervé Poussineau

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

---
 hw/fdc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
hw/fdc.c +1 -1
index c1898a6..1b9f303 100644
@@ -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);
Signature · 1 line
1.7.7.3
#3 lore raw

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

Am 15.01.2012 08:51, schrieb Hervé Poussineau:

Quoted 3 lines

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

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

#4 lore raw

PATCH v2 3/10 fdc: most control commands do not generate interrupts

In reply to Hervé Poussineau

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

---
 hw/fdc.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
hw/fdc.c +6 -6
index 1b9f303..bedaeca 100644
@@ -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);
     }
 }
 
Signature · 1 line
1.7.7.3
#5 lore raw

PATCH v2 4/10 fdc: emulate stepping 0

In reply to Hervé Poussineau

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

---
 hw/fdc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
hw/fdc.c +1 -1
index bedaeca..0e167f8 100644
@@ -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);
 }
 
Signature · 1 line
1.7.7.3
#6 lore raw

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

Am 15.01.2012 08:51, schrieb Hervé Poussineau:

Quoted 12 lines

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

---
 hw/fdc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
hw/fdc.c +0 -0
index bedaeca..0e167f8 100644

hw/fdc.c static void fdctrl_handle_version(FDCtrl *fdctrl, int direction) · line 1391

Quoted 8 lines
hw/fdc.c +1 -1
@@ -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

#7 lore raw

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

Hi,

Quoted 7 lines · Kevin Wolf

Am 15.01.2012 08:51, schrieb Hervé Poussineau:

Quoted 2 lines

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

Quoted 1 line

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é

#8 lore raw

PATCH v2 5/10 fdc: handle read-only floppies (abort early on write commands)

In reply to Hervé Poussineau

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

---
 hw/fdc.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)
hw/fdc.c +11 -0
index 0e167f8..078ff0c 100644
@@ -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),
Signature · 1 line
1.7.7.3
#9 lore raw

PATCH v2 6/10 fdc: add CCR (Configuration Control Register) write register

In reply to Hervé Poussineau

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

---
 hw/fdc.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)
hw/fdc.c +22 -0
index 078ff0c..6726450 100644
@@ -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;
Signature · 1 line
1.7.7.3
#10 lore raw

PATCH v2 7/10 block: add a transfer rate for floppy types

In reply to 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.

---
 block.c  |   74 ++++++++++++++++++++++++++++++++-----------------------------
 block.h  |   10 +++++++-
 hw/fdc.c |    3 +-
 hw/pc.c  |    3 +-
 4 files changed, 52 insertions(+), 38 deletions(-)
block.c +39 -35
index 3f072f6..2c3fefb 100644
@@ -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;
     }
 }
 
block.h +9 -1
index 3bd4398..27cc966 100644
@@ -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);
hw/fdc.c +2 -1
index 6726450..629408f 100644
@@ -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);
hw/pc.c +2 -1
index 85304cf..090e923 100644
@@ -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);
             }
         }
     }
Signature · 1 line
1.7.7.3
#11 lore raw

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

Am 15.01.2012 08:51, schrieb Hervé Poussineau:

Quoted 9 lines

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

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

#12 lore raw

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

Quoted 11 lines · Kevin Wolf

Am 15.01.2012 08:51, schrieb Hervé Poussineau:

Quoted 9 lines

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

---
 block.c  |   74 ++++++++++++++++++++++++++++++++----------------------------- block.h  |   10 +++++++-
 hw/fdc.c |    3 +-
 hw/pc.c  |    3 +-

Quoted 2 lines

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.

#13 lore raw

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

Am 21.01.2012 20:02, schrieb Blue Swirl:

Quoted 15 lines · Kevin Wolf
Quoted 11 lines

Am 15.01.2012 08:51, schrieb Hervé Poussineau:

Quoted 9 lines

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

---
 block.c  |   74 ++++++++++++++++++++++++++++++++-----------------------------
 block.h  |   10 +++++++-
 hw/fdc.c |    3 +-
 hw/pc.c  |    3 +-
Quoted 2 lines

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

Quoted 7 lines

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

#14 lore raw

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

Quoted 24 lines · Kevin Wolf

Am 21.01.2012 20:02, schrieb Blue Swirl:

Quoted 15 lines · Kevin Wolf
Quoted 11 lines

Am 15.01.2012 08:51, schrieb Hervé Poussineau:

Quoted 9 lines

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

---
 block.c  |   74 ++++++++++++++++++++++++++++++++----------------------------- block.h  |   10 +++++++-
 hw/fdc.c |    3 +-
 hw/pc.c  |    3 +-

Quoted 2 lines

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

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 1 line

Kevin

#15 lore raw

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

In reply to Kevin Wolf
Quoted 24 lines · Kevin Wolf

Am 21.01.2012 20:02, schrieb Blue Swirl:

Quoted 15 lines · Kevin Wolf
Quoted 11 lines

Am 15.01.2012 08:51, schrieb Hervé Poussineau:

Quoted 9 lines

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

---
 block.c  |   74 ++++++++++++++++++++++++++++++++-----------------------------
 block.h  |   10 +++++++-
 hw/fdc.c |    3 +-
 hw/pc.c  |    3 +-
Quoted 2 lines

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

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 6 lines

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.

#16 lore raw

PATCH v2 8/10 fdc: check if media rate is correct before doing any transfer

In reply to 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.

---
 hw/fdc.c |   25 ++++++++++++++++++++++++-
 1 files changed, 24 insertions(+), 1 deletions(-)
hw/fdc.c +24 -1
index 629408f..685ea88 100644
@@ -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)
Signature · 1 line
1.7.7.3
#17 lore raw

PATCH v2 9/10 fdc: fix seek command, which shouldn't check tracks

In reply to 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.

---
 hw/fdc.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)
hw/fdc.c +6 -7
index 685ea88..61d70eb 100644
@@ -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)
Signature · 1 line
1.7.7.3
#18 lore raw

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

Am 15.01.2012 08:51, schrieb Hervé Poussineau:

Quoted 17 lines

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.

---
 hw/fdc.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)
hw/fdc.c +0 -1
index 685ea88..61d70eb 100644
@@ -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) {

hw/fdc.c static void fdctrl_handle_seek(FDCtrl *fdctrl, int direction) · line 1605

Quoted 10 lines
hw/fdc.c +4 -6
-        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.

hw/fdc.c static void fdctrl_handle_seek(FDCtrl *fdctrl, int direction) · line 1610

Quoted 5 lines
hw/fdc.c +2 -0
+    /* Raise Interrupt */
+    fdctrl_raise_irq(fdctrl, FD_SR0_SEEK);
 }
 
 static void fdctrl_handle_perpendicular_mode(FDCtrl *fdctrl, int direction)

Kevin

#19 lore raw

PATCH v2 10/10 fdc: DIR (Digital Input Register) should return status of current drive...

In reply to Hervé Poussineau
---
 hw/fdc.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)
hw/fdc.c +3 -7
index 61d70eb..e68a1cb 100644
@@ -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);
     }
Signature · 1 line
1.7.7.3