Skip to contentSkip to conversation

Misc fixes for floppy emulation

PATCH Superseded Hervé Poussineau 2 participants 22 messages Jan 23, 2012 – Jan 31, 2012

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

v3 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 v2->v3:
- removed patch changing controller stepping to 0
- fixed out of bound access after bad seek command

Changes v1->v2:
- updated commit messages
- added missing 'break' and braces

Hervé Poussineau (9):
fdc: take side count into account
fdc: set busy bit when starting a command
fdc: most control commands do not generate interrupts
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 |  111 ++++++++++++++++++++++++++++++++++++++++++++++++--------------
 hw/pc.c  |    3 +-
 4 files changed, 136 insertions(+), 62 deletions(-)
Signature · 1 line
1.7.7.3
#1 lore raw

PATCH v3 1/9 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

Re: [Qemu-devel] [PATCH v3 1/9] fdc: take side count into account

Quoted 2 lines · Hervé Poussineau

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

Impact? Are single sided floppies broken before the patch? How?

#3 lore raw

Re: [Qemu-devel] [PATCH v3 1/9] fdc: take side count into account

Quoted 6 lines · Markus Armbruster
Quoted 2 lines · Hervé Poussineau

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

Impact?

This repairs single-sided floppies

Quoted 1 line

Are single sided floppies broken before the patch? How?

Yes. For head > 0, wrong sector number is calculated, so data is read/written at wrong place on underlying block device.

Fortunately, mostly nobody is still using single-sided floppies (only some 360 kB floppies are single-sided), so bug has been neglected since a long time.

Hervé

#4 lore raw

Re: [Qemu-devel] [PATCH v3 1/9] fdc: take side count into account

Quoted 10 lines · Hervé Poussineau
Quoted 6 lines · Markus Armbruster
Quoted 2 lines · Hervé Poussineau

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

Impact?

This repairs single-sided floppies

Quoted 8 lines
Quoted 1 line

Are single sided floppies broken before the patch? How?

Yes. For head > 0, wrong sector number is calculated, so data is read/written at wrong place on underlying block device.

Fortunately, mostly nobody is still using single-sided floppies (only some 360 kB floppies are single-sided), so bug has been neglected since a long time.

Nice info, thanks. Best put it in the commit message.

#5 lore raw

Re: [Qemu-devel] [PATCH v3 1/9] fdc: take side count into account

Hi,

Quoted 9 lines · Markus Armbruster
Quoted 7 lines
Quoted 1 line

Are single sided floppies broken before the patch? How?

Yes. For head > 0, wrong sector number is calculated, so data is read/written at wrong place on underlying block device.

Fortunately, mostly nobody is still using single-sided floppies (only some 360 kB floppies are single-sided), so bug has been neglected since a long time.

Nice info, thanks. Best put it in the commit message.

Will do when respinning the serie

Hervé

#6 lore raw

PATCH v3 2/9 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
#7 lore raw

PATCH v3 3/9 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
#8 lore raw

PATCH v3 4/9 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 bedaeca..4a363bd 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 v3 5/9 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 4a363bd..4875291 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

Re: [Qemu-devel] [PATCH v3 5/9] fdc: add CCR (Configuration Control Register) write register

Quoted 2 lines · Hervé Poussineau

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

Looks like guest writes to CCR are silently ignored before this patch. Is that correct? Impact?

#11 lore raw

Re: [Qemu-devel] [PATCH v3 5/9] fdc: add CCR (Configuration Control Register) write register

Quoted 7 lines · Markus Armbruster
Quoted 2 lines · Hervé Poussineau

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

Looks like guest writes to CCR are silently ignored before this patch. Is that correct?

Yes.

Quoted 1 line

Impact?

CCR is only used to program media rate, which is not checked without this patchset.

So, none for this patch.

#12 lore raw

Re: [Qemu-devel] [PATCH v3 5/9] fdc: add CCR (Configuration Control Register) write register

Quoted 9 lines · Hervé Poussineau
Quoted 7 lines · Markus Armbruster
Quoted 2 lines · Hervé Poussineau

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

Looks like guest writes to CCR are silently ignored before this patch. Is that correct?

Quoted 8 lines

Yes.

Quoted 1 line

Impact?

CCR is only used to program media rate, which is not checked without this patchset.

So, none for this patch.

Aha, it's just a prerequisite for the rest of the series. Makes sense.

Explain in commit message?

#13 lore raw

Re: [Qemu-devel] [PATCH v3 5/9] fdc: add CCR (Configuration Control Register) write register

Quoted 12 lines · Markus Armbruster
Quoted 10 lines · Hervé Poussineau
Quoted 6 lines · Markus Armbruster
Quoted 2 lines · Hervé Poussineau

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

Looks like guest writes to CCR are silently ignored before this patch. Is that correct?

Yes.

Quoted 1 line

Impact?

Quoted 9 lines
Quoted 4 lines

CCR is only used to program media rate, which is not checked without this patchset.

So, none for this patch.

Aha, it's just a prerequisite for the rest of the series. Makes sense.

Explain in commit message?

Will do when respinning the serie

Hervé

#14 lore raw

PATCH v3 6/9 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 4875291..60acd60 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
#15 lore raw

PATCH v3 7/9 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 60acd60..8c200fb 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
#16 lore raw

Re: [Qemu-devel] [PATCH v3 7/9] fdc: check if media rate is correct before doing any transfer

Quoted 5 lines · 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.

I'm not at all sure that's a good idea.

The rate is guest state. It's value depends on the media. We guess the media to use for a particular image. Same thing as for the geometry (heads, tracks, sectors per track). We do migrate the geometry, see vmstate_fdrive.

Why is it a good idea to treat rate differently?

Doesn't your fd_revalidate() in fdc_post_load() clobber the migrated geometry?

#17 lore raw

PATCH v3 8/9 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 |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)
hw/fdc.c +6 -3
index 8c200fb..3fa7704 100644
@@ -1599,13 +1599,16 @@ 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);
+    /* 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.
+     */
     if (fdctrl->fifo[2] > cur_drv->max_track) {
-        fdctrl_raise_irq(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK);
+        cur_drv->track = cur_drv->max_track;
     } else {
         cur_drv->track = fdctrl->fifo[2];
-        /* Raise Interrupt */
-        fdctrl_raise_irq(fdctrl, FD_SR0_SEEK);
     }
+    /* 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

PATCH v3 9/9 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 3fa7704..79a7190 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
#19 lore raw

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

Impact? What exactly is broken and fixed?