[Qemu-devel] [PATCH v3 0/9] Misc fixes for floppy emulation

[Qemu-devel] [PATCH v3 0/9] 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 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(-)

-- 
1.7.7.3

[Qemu-devel] [PATCH v3 1/9] 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

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

[Markus Armbruster]

Hervé Poussineau <hpoussin@reactos.org> writes:

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

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

[Hervé Poussineau]

Markus Armbruster a écrit :
> Hervé Poussineau <hpoussin@reactos.org> writes:
> 
>> 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

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

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

[Markus Armbruster]

Hervé Poussineau <hpoussin@reactos.org> writes:

> Markus Armbruster a écrit :
>> Hervé Poussineau <hpoussin@reactos.org> writes:
>>
>>> 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
>
>> 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.

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

[Hervé Poussineau]

Hi,

Markus Armbruster a écrit :
>>> 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é

[Qemu-devel] [PATCH v3 2/9] 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

[Qemu-devel] [PATCH v3 3/9] 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 v3 4/9] 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 bedaeca..4a363bd 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 v3 5/9] 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 4a363bd..4875291 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

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

[Markus Armbruster]

Hervé Poussineau <hpoussin@reactos.org> writes:

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

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

[Hervé Poussineau]

Markus Armbruster a écrit :
> Hervé Poussineau <hpoussin@reactos.org> writes:
> 
>> 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.

> Impact?

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

So, none for this patch.

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

[Markus Armbruster]

Hervé Poussineau <hpoussin@reactos.org> writes:

> Markus Armbruster a écrit :
>> Hervé Poussineau <hpoussin@reactos.org> writes:
>>
>>> 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.
>
>> 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?

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

[Hervé Poussineau]

Markus Armbruster a écrit :
> Hervé Poussineau <hpoussin@reactos.org> writes:
> 
>> Markus Armbruster a écrit :
>>> Hervé Poussineau <hpoussin@reactos.org> writes:
>>>
>>>> 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.
>>
>>> 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?
> 

Will do when respinning the serie

Hervé

[Qemu-devel] [PATCH v3 6/9] 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 4875291..60acd60 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

[Qemu-devel] [PATCH v3 7/9] 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 60acd60..8c200fb 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

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

[Markus Armbruster]

Hervé Poussineau <hpoussin@reactos.org> writes:

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

[Qemu-devel] [PATCH v3 8/9] 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 |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index 8c200fb..3fa7704 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -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)
-- 
1.7.7.3

[Qemu-devel] [PATCH v3 9/9] 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 3fa7704..79a7190 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

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

[Markus Armbruster]

Impact?  What exactly is broken and fixed?

Re: [Qemu-devel] [PATCH v3 0/9] Misc fixes for floppy emulation

[Markus Armbruster]

Looks sane, except for the migration of media rate.  See my reply to
7/9.

Re: [Qemu-devel] [PATCH v3 0/9] Misc fixes for floppy emulation

[Hervé Poussineau]

Hi,

Markus Armbruster a écrit :
> Looks sane, except for the migration of media rate.  See my reply to
> 7/9.
> 

I'm not very fluent with requirements for store/save/migration 
compatibility things, so can someone knowing them can look at the 
migration of media rate problem? Paolo?

Markus reply is available at:
https://lists.gnu.org/archive/html/qemu-devel/2012-01/msg03601.html

Thanks in advance

Regards,

Hervé