[Qemu-devel] [PATCH v5 00/12] fdc: fix 2.88mb floppy diskette support

[Qemu-devel] [PATCH v5 00/12] fdc: fix 2.88mb floppy diskette support

[John Snow]

Yes, it's been broken for ten years.
No, it's not a CVE.

The problem is that QEMU doesn't have a configuration option for the type
of floppy drive you want. It determines that based on the type of
diskette inserted at boot time.

If you don't insert one, it always chooses a 1.44MB type.

If you want to insert a 2.88MB floppy after boot, you simply cannot.

"Wow, who cares?"

Good question -- Unfortunately, the virtio-win floppy disk images that
Red Hat/Fedora ship require a 2.88MB drive, so if you forgot to insert
them at boot, you'd have to change your VM configuration and try again.

For a one-shot operation, that's kind of obnoxious -- it'd be nice to
allow one to just insert the diskette on-demand.

"OK, What are you changing in this decades-old device?"

(1) Add a new property to allow users to specify what kind of drive they
    want without relying on magical guessing behavior.
    Choices are: 120, 144, 288, auto, and none.

    120, 144 and 288 refer to 1.20MB, 1.44MB, and 2.88MB drives.
    auto refers to the auto-detect behavior QEMU currently has.
    none ... hides the drive. You probably don't want to use this,
    but it's there if you feel like creating a drive you can't use.

(2) Add a new "fallback" property for use with the "auto" drive type
    that allows us to specify the backup behavior, too. In most cases
    this property won't be needed, but it is provided for allowing
    QEMU to be fully backwards compatible.

(3) Add the concept of physical diskette size to QEMU, classifying
    120-style diskettes as fundamentally different from 144 and 288 ones.

(4) Revamp the automatic guessing heuristic to understand that
    2.88MB style drives can accept 1.44MB diskettes.

(5) Change the automatic fallback type for the automatic guessing
    heuristic from 1.44MB to 2.88MB for 2.6 machines and beyond,
    leaving 2.5- machines set to default to auto/144.

(6) A lot of code cleanup in general.

"Won't this break everything, you madman?"

No: I tested this in MS-DOS 6.22, Fedora 23 and Windows 8.1. All
seemed perfectly happy with 2.88MB drives as the default for 1.44
or 2.88MB floppy diskette images.

And: Older machine types will happily still default to the 1.44
     type just like they used to, so really nothing should change
     at all for most guests.

If there ARE any guests affected in 2.6+ machine types, you are
urged to use an explicit drive type that matches your application
if the automatic behavior is unsuitable.

===
v5:
===

Nearly identical to v4.

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/12:[----] [--] 'fdc: move pick_geometry'
002/12:[----] [--] 'fdc: reduce number of pick_geometry arguments'
003/12:[0002] [FC] 'fdc: add drive type qapi enum'
004/12:[----] [--] 'fdc: add disk field'
005/12:[----] [--] 'fdc: Throw an assertion on misconfigured fd_formats table'
006/12:[----] [--] 'fdc: add pick_drive'
007/12:[----] [--] 'fdc: Add fallback option'
008/12:[0002] [FC] 'fdc: add drive type option'
009/12:[0002] [FC] 'fdc: add physical disk sizes'
010/12:[0031] [FC] 'fdc: rework pick_geometry'
011/12:[----] [--] 'qtest/fdc: Support for 2.88MB drives'
012/12:[----] [--] 'fdc: change auto fallback drive for ISA FDC to 288'

03: Fixed 1.5MB typo to say 1.2MB.
08: Removed extraneous parentheses.
09: Fixed sector count comment to be 1440 instead of 1400.
10: Removed what was match type #3 and replaced it with a warning.
    Updated some comments.

===
v4:
===

Hopefully a more logical patch order with smaller changes.

02: Kept both debug printfs in fd_revalidate.
03: New patch, QAPI enumeration change only.
04: Re-ordered FDrive fields
    Fallout from 03.
05: New patch.
06: Almost completely re-done.
07: Added media_validated property
    Fallout from patch re-ordering.
08: Re-ordered.
10: Changed return type of pick_geometry to int.
    Changed one error pathway to abort, as it's not a run-time problem.
12: Rebased on top of current master.

===
v3:
===

04: Remove typeA/typeB members of FDCtrl. Store e.g. -fdtypeA options
           directly into FDCtrl.drives[x].drive instead.
05: Add a new fallback= option that controls fdtype{A,B}=auto behavior.
07: replace get_default_drive_type which is no longer needed
    add get_fallback_drive_type.
09: Reworked the auto/fallback section of pick_geometry.

________________________________________________________________________________

For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch fdc-default
https://github.com/jnsnow/qemu/tree/fdc-default

This version is tagged fdc-default-v5:
https://github.com/jnsnow/qemu/releases/tag/fdc-default-v5

John Snow (12):
  fdc: move pick_geometry
  fdc: reduce number of pick_geometry arguments
  fdc: add drive type qapi enum
  fdc: add disk field
  fdc: Throw an assertion on misconfigured fd_formats table
  fdc: add pick_drive
  fdc: Add fallback option
  fdc: add drive type option
  fdc: add physical disk sizes
  fdc: rework pick_geometry
  qtest/fdc: Support for 2.88MB drives
  fdc: change auto fallback drive for ISA FDC to 288

 hw/block/fdc.c               | 326 ++++++++++++++++++++++++++++++-------------
 hw/core/qdev-properties.c    |  11 ++
 hw/i386/pc.c                 |  17 +--
 include/hw/block/fdc.h       |   9 +-
 include/hw/compat.h          |   4 +
 include/hw/qdev-properties.h |   1 +
 qapi/block.json              |  16 +++
 tests/fdc-test.c             |   2 +-
 8 files changed, 270 insertions(+), 116 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v5 01/12] fdc: move pick_geometry

[John Snow]

Code motion: I want to refactor this function to work with FDrive
directly, so shuffle it below that definition.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c | 90 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 45 insertions(+), 45 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 6711c6a..71d931e 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -115,51 +115,6 @@ static const FDFormat fd_formats[] = {
     { FDRIVE_DRV_NONE, -1, -1, 0, 0, },
 };
 
-static void pick_geometry(BlockBackend *blk, int *nb_heads,
-                          int *max_track, int *last_sect,
-                          FDriveType drive_in, FDriveType *drive,
-                          FDriveRate *rate)
-{
-    const FDFormat *parse;
-    uint64_t nb_sectors, size;
-    int i, first_match, match;
-
-    blk_get_geometry(blk, &nb_sectors);
-    match = -1;
-    first_match = -1;
-    for (i = 0; ; i++) {
-        parse = &fd_formats[i];
-        if (parse->drive == FDRIVE_DRV_NONE) {
-            break;
-        }
-        if (drive_in == parse->drive ||
-            drive_in == FDRIVE_DRV_NONE) {
-            size = (parse->max_head + 1) * parse->max_track *
-                parse->last_sect;
-            if (nb_sectors == size) {
-                match = i;
-                break;
-            }
-            if (first_match == -1) {
-                first_match = i;
-            }
-        }
-    }
-    if (match == -1) {
-        if (first_match == -1) {
-            match = 1;
-        } else {
-            match = first_match;
-        }
-        parse = &fd_formats[match];
-    }
-    *nb_heads = parse->max_head + 1;
-    *max_track = parse->max_track;
-    *last_sect = parse->last_sect;
-    *drive = parse->drive;
-    *rate = parse->rate;
-}
-
 #define GET_CUR_DRV(fdctrl) ((fdctrl)->cur_drv)
 #define SET_CUR_DRV(fdctrl, drive) ((fdctrl)->cur_drv = (drive))
 
@@ -287,6 +242,51 @@ static void fd_recalibrate(FDrive *drv)
     fd_seek(drv, 0, 0, 1, 1);
 }
 
+static void pick_geometry(BlockBackend *blk, int *nb_heads,
+                          int *max_track, int *last_sect,
+                          FDriveType drive_in, FDriveType *drive,
+                          FDriveRate *rate)
+{
+    const FDFormat *parse;
+    uint64_t nb_sectors, size;
+    int i, first_match, match;
+
+    blk_get_geometry(blk, &nb_sectors);
+    match = -1;
+    first_match = -1;
+    for (i = 0; ; i++) {
+        parse = &fd_formats[i];
+        if (parse->drive == FDRIVE_DRV_NONE) {
+            break;
+        }
+        if (drive_in == parse->drive ||
+            drive_in == FDRIVE_DRV_NONE) {
+            size = (parse->max_head + 1) * parse->max_track *
+                parse->last_sect;
+            if (nb_sectors == size) {
+                match = i;
+                break;
+            }
+            if (first_match == -1) {
+                first_match = i;
+            }
+        }
+    }
+    if (match == -1) {
+        if (first_match == -1) {
+            match = 1;
+        } else {
+            match = first_match;
+        }
+        parse = &fd_formats[match];
+    }
+    *nb_heads = parse->max_head + 1;
+    *max_track = parse->max_track;
+    *last_sect = parse->last_sect;
+    *drive = parse->drive;
+    *rate = parse->rate;
+}
+
 /* Revalidate a disk drive after a disk change */
 static void fd_revalidate(FDrive *drv)
 {
-- 
2.4.3

[Qemu-devel] [PATCH v5 02/12] fdc: reduce number of pick_geometry arguments

[John Snow]

Modify this function to operate directly on FDrive objects instead of
unpacking and passing all of those parameters manually. Reduces the
complexity in the caller and reduces the number of args to just one.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c | 50 ++++++++++++++++++++------------------------------
 1 file changed, 20 insertions(+), 30 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 71d931e..5505219 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -242,11 +242,9 @@ static void fd_recalibrate(FDrive *drv)
     fd_seek(drv, 0, 0, 1, 1);
 }
 
-static void pick_geometry(BlockBackend *blk, int *nb_heads,
-                          int *max_track, int *last_sect,
-                          FDriveType drive_in, FDriveType *drive,
-                          FDriveRate *rate)
+static void pick_geometry(FDrive *drv)
 {
+    BlockBackend *blk = drv->blk;
     const FDFormat *parse;
     uint64_t nb_sectors, size;
     int i, first_match, match;
@@ -259,8 +257,8 @@ static void pick_geometry(BlockBackend *blk, int *nb_heads,
         if (parse->drive == FDRIVE_DRV_NONE) {
             break;
         }
-        if (drive_in == parse->drive ||
-            drive_in == FDRIVE_DRV_NONE) {
+        if (drv->drive == parse->drive ||
+            drv->drive == FDRIVE_DRV_NONE) {
             size = (parse->max_head + 1) * parse->max_track *
                 parse->last_sect;
             if (nb_sectors == size) {
@@ -280,41 +278,33 @@ static void pick_geometry(BlockBackend *blk, int *nb_heads,
         }
         parse = &fd_formats[match];
     }
-    *nb_heads = parse->max_head + 1;
-    *max_track = parse->max_track;
-    *last_sect = parse->last_sect;
-    *drive = parse->drive;
-    *rate = parse->rate;
+
+    if (parse->max_head == 0) {
+        drv->flags &= ~FDISK_DBL_SIDES;
+    } else {
+        drv->flags |= FDISK_DBL_SIDES;
+    }
+    drv->max_track = parse->max_track;
+    drv->last_sect = parse->last_sect;
+    drv->drive = parse->drive;
+    drv->media_rate = parse->rate;
 }
 
 /* Revalidate a disk drive after a disk change */
 static void fd_revalidate(FDrive *drv)
 {
-    int nb_heads, max_track, last_sect, ro;
-    FDriveType drive;
-    FDriveRate rate;
-
     FLOPPY_DPRINTF("revalidate\n");
     if (drv->blk != NULL) {
-        ro = blk_is_read_only(drv->blk);
-        pick_geometry(drv->blk, &nb_heads, &max_track,
-                      &last_sect, drv->drive, &drive, &rate);
+        drv->ro = blk_is_read_only(drv->blk);
+        pick_geometry(drv);
         if (!drv->media_inserted) {
             FLOPPY_DPRINTF("No disk in drive\n");
         } else {
-            FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n", nb_heads,
-                           max_track, last_sect, ro ? "ro" : "rw");
+            FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n",
+                           (drv->flags & FDISK_DBL_SIDES) ? 2 : 1,
+                           drv->max_track, drv->last_sect,
+                           drv->ro ? "ro" : "rw");
         }
-        if (nb_heads == 1) {
-            drv->flags &= ~FDISK_DBL_SIDES;
-        } else {
-            drv->flags |= FDISK_DBL_SIDES;
-        }
-        drv->max_track = max_track;
-        drv->last_sect = last_sect;
-        drv->ro = ro;
-        drv->drive = drive;
-        drv->media_rate = rate;
     } else {
         FLOPPY_DPRINTF("No drive connected\n");
         drv->last_sect = 0;
-- 
2.4.3

[Qemu-devel] [PATCH v5 03/12] fdc: add drive type qapi enum

[John Snow]

Change the floppy drive type to a QAPI enum type, to allow us to
specify the floppy drive type from the CLI in a forthcoming patch.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c         | 80 +++++++++++++++++++++++++-------------------------
 hw/i386/pc.c           | 17 ++++++-----
 include/hw/block/fdc.h |  9 +-----
 qapi/block.json        | 16 ++++++++++
 4 files changed, 66 insertions(+), 56 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 5505219..e37934d 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -61,7 +61,7 @@ typedef enum FDriveRate {
 } FDriveRate;
 
 typedef struct FDFormat {
-    FDriveType drive;
+    FloppyDriveType drive;
     uint8_t last_sect;
     uint8_t max_track;
     uint8_t max_head;
@@ -71,48 +71,48 @@ typedef struct 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_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, },
+    { FLOPPY_DRIVE_TYPE_144, 18, 80, 1, FDRIVE_RATE_500K, },
+    { FLOPPY_DRIVE_TYPE_144, 20, 80, 1, FDRIVE_RATE_500K, },
+    { FLOPPY_DRIVE_TYPE_144, 21, 80, 1, FDRIVE_RATE_500K, },
+    { FLOPPY_DRIVE_TYPE_144, 21, 82, 1, FDRIVE_RATE_500K, },
+    { FLOPPY_DRIVE_TYPE_144, 21, 83, 1, FDRIVE_RATE_500K, },
+    { FLOPPY_DRIVE_TYPE_144, 22, 80, 1, FDRIVE_RATE_500K, },
+    { FLOPPY_DRIVE_TYPE_144, 23, 80, 1, FDRIVE_RATE_500K, },
+    { FLOPPY_DRIVE_TYPE_144, 24, 80, 1, FDRIVE_RATE_500K, },
     /* 2.88 MB 3"1/2 floppy disks */
-    { 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, },
+    { FLOPPY_DRIVE_TYPE_288, 36, 80, 1, FDRIVE_RATE_1M, },
+    { FLOPPY_DRIVE_TYPE_288, 39, 80, 1, FDRIVE_RATE_1M, },
+    { FLOPPY_DRIVE_TYPE_288, 40, 80, 1, FDRIVE_RATE_1M, },
+    { FLOPPY_DRIVE_TYPE_288, 44, 80, 1, FDRIVE_RATE_1M, },
+    { FLOPPY_DRIVE_TYPE_288, 48, 80, 1, FDRIVE_RATE_1M, },
     /* 720 kB 3"1/2 floppy disks */
-    { 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, },
+    { FLOPPY_DRIVE_TYPE_144,  9, 80, 1, FDRIVE_RATE_250K, },
+    { FLOPPY_DRIVE_TYPE_144, 10, 80, 1, FDRIVE_RATE_250K, },
+    { FLOPPY_DRIVE_TYPE_144, 10, 82, 1, FDRIVE_RATE_250K, },
+    { FLOPPY_DRIVE_TYPE_144, 10, 83, 1, FDRIVE_RATE_250K, },
+    { FLOPPY_DRIVE_TYPE_144, 13, 80, 1, FDRIVE_RATE_250K, },
+    { FLOPPY_DRIVE_TYPE_144, 14, 80, 1, FDRIVE_RATE_250K, },
     /* 1.2 MB 5"1/4 floppy disks */
-    { 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, },
+    { FLOPPY_DRIVE_TYPE_120, 15, 80, 1, FDRIVE_RATE_500K, },
+    { FLOPPY_DRIVE_TYPE_120, 18, 80, 1, FDRIVE_RATE_500K, },
+    { FLOPPY_DRIVE_TYPE_120, 18, 82, 1, FDRIVE_RATE_500K, },
+    { FLOPPY_DRIVE_TYPE_120, 18, 83, 1, FDRIVE_RATE_500K, },
+    { FLOPPY_DRIVE_TYPE_120, 20, 80, 1, FDRIVE_RATE_500K, },
     /* 720 kB 5"1/4 floppy disks */
-    { FDRIVE_DRV_120,  9, 80, 1, FDRIVE_RATE_250K, },
-    { FDRIVE_DRV_120, 11, 80, 1, FDRIVE_RATE_250K, },
+    { FLOPPY_DRIVE_TYPE_120,  9, 80, 1, FDRIVE_RATE_250K, },
+    { FLOPPY_DRIVE_TYPE_120, 11, 80, 1, FDRIVE_RATE_250K, },
     /* 360 kB 5"1/4 floppy disks */
-    { 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, },
+    { FLOPPY_DRIVE_TYPE_120,  9, 40, 1, FDRIVE_RATE_300K, },
+    { FLOPPY_DRIVE_TYPE_120,  9, 40, 0, FDRIVE_RATE_300K, },
+    { FLOPPY_DRIVE_TYPE_120, 10, 41, 1, FDRIVE_RATE_300K, },
+    { FLOPPY_DRIVE_TYPE_120, 10, 42, 1, FDRIVE_RATE_300K, },
     /* 320 kB 5"1/4 floppy disks */
-    { FDRIVE_DRV_120,  8, 40, 1, FDRIVE_RATE_250K, },
-    { FDRIVE_DRV_120,  8, 40, 0, FDRIVE_RATE_250K, },
+    { FLOPPY_DRIVE_TYPE_120,  8, 40, 1, FDRIVE_RATE_250K, },
+    { FLOPPY_DRIVE_TYPE_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_RATE_250K, },
+    { FLOPPY_DRIVE_TYPE_144,  9, 80, 0, FDRIVE_RATE_250K, },
     /* end */
-    { FDRIVE_DRV_NONE, -1, -1, 0, 0, },
+    { FLOPPY_DRIVE_TYPE_NONE, -1, -1, 0, 0, },
 };
 
 #define GET_CUR_DRV(fdctrl) ((fdctrl)->cur_drv)
@@ -134,7 +134,7 @@ typedef struct FDrive {
     FDCtrl *fdctrl;
     BlockBackend *blk;
     /* Drive status */
-    FDriveType drive;
+    FloppyDriveType drive;    /* CMOS drive type        */
     uint8_t perpendicular;    /* 2.88 MB access mode    */
     /* Position */
     uint8_t head;
@@ -155,7 +155,7 @@ typedef struct FDrive {
 static void fd_init(FDrive *drv)
 {
     /* Drive */
-    drv->drive = FDRIVE_DRV_NONE;
+    drv->drive = FLOPPY_DRIVE_TYPE_NONE;
     drv->perpendicular = 0;
     /* Disk */
     drv->last_sect = 0;
@@ -254,11 +254,11 @@ static void pick_geometry(FDrive *drv)
     first_match = -1;
     for (i = 0; ; i++) {
         parse = &fd_formats[i];
-        if (parse->drive == FDRIVE_DRV_NONE) {
+        if (parse->drive == FLOPPY_DRIVE_TYPE_NONE) {
             break;
         }
         if (drv->drive == parse->drive ||
-            drv->drive == FDRIVE_DRV_NONE) {
+            drv->drive == FLOPPY_DRIVE_TYPE_NONE) {
             size = (parse->max_head + 1) * parse->max_track *
                 parse->last_sect;
             if (nb_sectors == size) {
@@ -2397,7 +2397,7 @@ static void sysbus_fdc_common_realize(DeviceState *dev, Error **errp)
     fdctrl_realize_common(fdctrl, errp);
 }
 
-FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
+FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
 {
     FDCtrlISABus *isa = ISA_FDC(fdc);
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 838636c..78cf8fa 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -199,24 +199,24 @@ static void pic_irq_request(void *opaque, int irq, int level)
 
 #define REG_EQUIPMENT_BYTE          0x14
 
-static int cmos_get_fd_drive_type(FDriveType fd0)
+static int cmos_get_fd_drive_type(FloppyDriveType fd0)
 {
     int val;
 
     switch (fd0) {
-    case FDRIVE_DRV_144:
+    case FLOPPY_DRIVE_TYPE_144:
         /* 1.44 Mb 3"5 drive */
         val = 4;
         break;
-    case FDRIVE_DRV_288:
+    case FLOPPY_DRIVE_TYPE_288:
         /* 2.88 Mb 3"5 drive */
         val = 5;
         break;
-    case FDRIVE_DRV_120:
+    case FLOPPY_DRIVE_TYPE_120:
         /* 1.2 Mb 5"5 drive */
         val = 2;
         break;
-    case FDRIVE_DRV_NONE:
+    case FLOPPY_DRIVE_TYPE_NONE:
     default:
         val = 0;
         break;
@@ -287,7 +287,8 @@ static void pc_boot_set(void *opaque, const char *boot_device, Error **errp)
 static void pc_cmos_init_floppy(ISADevice *rtc_state, ISADevice *floppy)
 {
     int val, nb, i;
-    FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
+    FloppyDriveType fd_type[2] = { FLOPPY_DRIVE_TYPE_NONE,
+                                   FLOPPY_DRIVE_TYPE_NONE };
 
     /* floppy type */
     if (floppy) {
@@ -301,10 +302,10 @@ static void pc_cmos_init_floppy(ISADevice *rtc_state, ISADevice *floppy)
 
     val = rtc_get_memory(rtc_state, REG_EQUIPMENT_BYTE);
     nb = 0;
-    if (fd_type[0] < FDRIVE_DRV_NONE) {
+    if (fd_type[0] != FLOPPY_DRIVE_TYPE_NONE) {
         nb++;
     }
-    if (fd_type[1] < FDRIVE_DRV_NONE) {
+    if (fd_type[1] != FLOPPY_DRIVE_TYPE_NONE) {
         nb++;
     }
     switch (nb) {
diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
index d48b2f8..adce14f 100644
--- a/include/hw/block/fdc.h
+++ b/include/hw/block/fdc.h
@@ -6,13 +6,6 @@
 /* fdc.c */
 #define MAX_FD 2
 
-typedef enum FDriveType {
-    FDRIVE_DRV_144  = 0x00,   /* 1.44 MB 3"5 drive      */
-    FDRIVE_DRV_288  = 0x01,   /* 2.88 MB 3"5 drive      */
-    FDRIVE_DRV_120  = 0x02,   /* 1.2  MB 5"25 drive     */
-    FDRIVE_DRV_NONE = 0x03,   /* No drive connected     */
-} FDriveType;
-
 #define TYPE_ISA_FDC "isa-fdc"
 
 ISADevice *fdctrl_init_isa(ISABus *bus, DriveInfo **fds);
@@ -21,6 +14,6 @@ void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
 void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
                        DriveInfo **fds, qemu_irq *fdc_tc);
 
-FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i);
+FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i);
 
 #endif
diff --git a/qapi/block.json b/qapi/block.json
index 84022f1..ed61f82 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -40,6 +40,22 @@
   'data': ['auto', 'none', 'lba', 'large', 'rechs']}
 
 ##
+# @FloppyDriveType
+#
+# Type of Floppy drive to be emulated by the Floppy Disk Controller.
+#
+# @144:  1.44MB 3.5" drive
+# @288:  2.88MB 3.5" drive
+# @120:  1.2MB 5.25" drive
+# @none: No drive connected
+# @auto: Automatically determined by inserted media at boot
+#
+# Since: 2.6
+##
+{ 'enum': 'FloppyDriveType',
+  'data': ['144', '288', '120', 'none', 'auto']}
+
+##
 # @BlockdevSnapshotInternal
 #
 # @device: the name of the device to generate the snapshot from
-- 
2.4.3

[Qemu-devel] [PATCH v5 04/12] fdc: add disk field

[John Snow]

Currently, 'drive' is used both to represent the current diskette
type as well as the current drive type.

This patch adds a 'disk' field that is updated explicitly to match
the type of the disk.

As of this patch, disk and drive are always the same, but forthcoming
patches to change the behavior of pick_geometry will invalidate this
assumption.

disk does not need to be migrated because it is not user-visible state
nor is it currently used for any calculations. It is purely informative,
and will be rebuilt automatically via fd_revalidate on the new host.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index e37934d..18e363b 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -141,6 +141,7 @@ typedef struct FDrive {
     uint8_t track;
     uint8_t sect;
     /* Media */
+    FloppyDriveType disk;     /* Current disk type      */
     FDiskFlags flags;
     uint8_t last_sect;        /* Nb sector per track    */
     uint8_t max_track;        /* Nb of tracks           */
@@ -158,6 +159,7 @@ static void fd_init(FDrive *drv)
     drv->drive = FLOPPY_DRIVE_TYPE_NONE;
     drv->perpendicular = 0;
     /* Disk */
+    drv->disk = FLOPPY_DRIVE_TYPE_NONE;
     drv->last_sect = 0;
     drv->max_track = 0;
 }
@@ -287,6 +289,7 @@ static void pick_geometry(FDrive *drv)
     drv->max_track = parse->max_track;
     drv->last_sect = parse->last_sect;
     drv->drive = parse->drive;
+    drv->disk = drv->media_inserted ? parse->drive : FLOPPY_DRIVE_TYPE_NONE;
     drv->media_rate = parse->rate;
 }
 
-- 
2.4.3

[Qemu-devel] [PATCH v5 05/12] fdc: Throw an assertion on misconfigured fd_formats table

[John Snow]

pick_geometry is a convoluted function that makes it difficult to tell
at a glance what QEMU's current behavior for choosing a floppy drive
type is when it can't quite identify the diskette.

The code iterates over all entries in the candidate geometry table
("fd_formats") and if our specific drive type matches a row in the table,
then either "match" is set to that entry (an exact match) and the loop
exits, or "first_match" will be non-negative (the first such entry that
shares the same drive type), and the loop continues. If our specific
drive type is NONE, then all drive types in the candidate geometry table
are considered. After iteration, if "match" was not set, we fall back to
"first match".

This means that either "match" was set, or we exited the loop without an
exact match, in which case:

- If drive type is NONE, the default is truly fd_formats[0], a 1.44MB
  type, because "first_match" will always get set to the first item.

- If drive type is not NONE, pick_geometry's iteration was fussier and
  only looked at rows that matched our drive type. However, since all
  possible drive types are represented in the table, we still know that
  "first match" was set.

- If drive type is not NONE and the fd_formats table lists no options for
  our drive type, we choose fd_formats[1], an incomprehensibly bizarre
  choice that can never happen anyway.

Correct this: If first_match is -1, it can ONLY mean we didn't edit our
fd_formats table correctly. Throw an assertion instead.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 18e363b..a8f0cf2 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -274,7 +274,9 @@ static void pick_geometry(FDrive *drv)
     }
     if (match == -1) {
         if (first_match == -1) {
-            match = 1;
+            error_setg(&error_abort, "No candidate geometries present in table "
+                       " for floppy drive type '%s'",
+                       FloppyDriveType_lookup[drv->drive]);
         } else {
             match = first_match;
         }
-- 
2.4.3

[Qemu-devel] [PATCH v5 06/12] fdc: add pick_drive

[John Snow]

Split apart pick_geometry by creating a pick_drive routine that will only
ever called during device bring-up instead of relying on pick_geometry to
be used in both cases.

With this change, the drive field is changed to be 'write once'. It is
not altered after the initialization routines exit.

media_validated does not need to be migrated. The target VM
will just revalidate the media on post_load anyway.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 10 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index a8f0cf2..f8e070e 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -151,6 +151,7 @@ typedef struct FDrive {
     uint8_t media_rate;       /* Data rate of medium    */
 
     bool media_inserted;      /* Is there a medium in the tray */
+    bool media_validated;     /* Have we validated the media? */
 } FDrive;
 
 static void fd_init(FDrive *drv)
@@ -162,6 +163,8 @@ static void fd_init(FDrive *drv)
     drv->disk = FLOPPY_DRIVE_TYPE_NONE;
     drv->last_sect = 0;
     drv->max_track = 0;
+    drv->ro = true;
+    drv->media_changed = 1;
 }
 
 #define NUM_SIDES(drv) ((drv)->flags & FDISK_DBL_SIDES ? 2 : 1)
@@ -244,13 +247,24 @@ static void fd_recalibrate(FDrive *drv)
     fd_seek(drv, 0, 0, 1, 1);
 }
 
-static void pick_geometry(FDrive *drv)
+/**
+ * Determine geometry based on inserted diskette.
+ * Will not operate on an empty drive.
+ *
+ * @return: 0 on success, -1 if the drive is empty.
+ */
+static int pick_geometry(FDrive *drv)
 {
     BlockBackend *blk = drv->blk;
     const FDFormat *parse;
     uint64_t nb_sectors, size;
     int i, first_match, match;
 
+    /* We can only pick a geometry if we have a diskette. */
+    if (!drv->media_inserted) {
+        return -1;
+    }
+
     blk_get_geometry(blk, &nb_sectors);
     match = -1;
     first_match = -1;
@@ -290,31 +304,51 @@ static void pick_geometry(FDrive *drv)
     }
     drv->max_track = parse->max_track;
     drv->last_sect = parse->last_sect;
-    drv->drive = parse->drive;
-    drv->disk = drv->media_inserted ? parse->drive : FLOPPY_DRIVE_TYPE_NONE;
+    drv->disk = parse->drive;
     drv->media_rate = parse->rate;
+    return 0;
+}
+
+static void pick_drive_type(FDrive *drv)
+{
+    if (pick_geometry(drv) == 0) {
+        drv->drive = drv->disk;
+    } else {
+        /* Legacy behavior: default to 1.44MB floppy */
+        drv->drive = FLOPPY_DRIVE_TYPE_144;
+    }
 }
 
 /* Revalidate a disk drive after a disk change */
 static void fd_revalidate(FDrive *drv)
 {
+    int rc;
+
     FLOPPY_DPRINTF("revalidate\n");
     if (drv->blk != NULL) {
         drv->ro = blk_is_read_only(drv->blk);
-        pick_geometry(drv);
         if (!drv->media_inserted) {
             FLOPPY_DPRINTF("No disk in drive\n");
-        } else {
-            FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n",
-                           (drv->flags & FDISK_DBL_SIDES) ? 2 : 1,
-                           drv->max_track, drv->last_sect,
-                           drv->ro ? "ro" : "rw");
+            drv->disk = FLOPPY_DRIVE_TYPE_NONE;
+        } else if (!drv->media_validated) {
+            rc = pick_geometry(drv);
+            if (rc) {
+                FLOPPY_DPRINTF("Could not validate floppy drive media");
+            } else {
+                drv->media_validated = true;
+                FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n",
+                               (drv->flags & FDISK_DBL_SIDES) ? 2 : 1,
+                               drv->max_track, drv->last_sect,
+                               drv->ro ? "ro" : "rw");
+            }
         }
     } else {
         FLOPPY_DPRINTF("No drive connected\n");
         drv->last_sect = 0;
         drv->max_track = 0;
         drv->flags &= ~FDISK_DBL_SIDES;
+        drv->drive = FLOPPY_DRIVE_TYPE_NONE;
+        drv->disk = FLOPPY_DRIVE_TYPE_NONE;
     }
 }
 
@@ -2185,6 +2219,7 @@ static void fdctrl_change_cb(void *opaque, bool load)
     drive->media_inserted = load && drive->blk && blk_is_inserted(drive->blk);
 
     drive->media_changed = 1;
+    drive->media_validated = false;
     fd_revalidate(drive);
 }
 
@@ -2221,11 +2256,12 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl, Error **errp)
         }
 
         fd_init(drive);
-        fdctrl_change_cb(drive, 0);
         if (drive->blk) {
             blk_set_dev_ops(drive->blk, &fdctrl_block_ops, drive);
             drive->media_inserted = blk_is_inserted(drive->blk);
+            pick_drive_type(drive);
         }
+        fd_revalidate(drive);
     }
 }
 
-- 
2.4.3

[Qemu-devel] [PATCH v5 07/12] fdc: Add fallback option

[John Snow]

Currently, QEMU chooses a drive type automatically based on the inserted
media. If there is no disk inserted, it chooses a 1.44MB drive type.

Change this behavior to be configurable, but leave it defaulted to 1.44.

This is not earnestly intended to be used by a user or a management
library, but rather exists so that pre-2.6 board types can configure it
to be a legacy value.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c               | 25 +++++++++++++++++++++++--
 hw/core/qdev-properties.c    | 11 +++++++++++
 include/hw/qdev-properties.h |  1 +
 3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index f8e070e..4caed9b 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -154,6 +154,9 @@ typedef struct FDrive {
     bool media_validated;     /* Have we validated the media? */
 } FDrive;
 
+
+static FloppyDriveType get_fallback_drive_type(FDrive *drv);
+
 static void fd_init(FDrive *drv)
 {
     /* Drive */
@@ -314,8 +317,7 @@ static void pick_drive_type(FDrive *drv)
     if (pick_geometry(drv) == 0) {
         drv->drive = drv->disk;
     } else {
-        /* Legacy behavior: default to 1.44MB floppy */
-        drv->drive = FLOPPY_DRIVE_TYPE_144;
+        drv->drive = get_fallback_drive_type(drv);
     }
 }
 
@@ -598,11 +600,17 @@ struct FDCtrl {
     FDrive drives[MAX_FD];
     int reset_sensei;
     uint32_t check_media_rate;
+    FloppyDriveType fallback; /* type=auto failure fallback */
     /* Timers state */
     uint8_t timer0;
     uint8_t timer1;
 };
 
+static FloppyDriveType get_fallback_drive_type(FDrive *drv)
+{
+    return drv->fdctrl->fallback;
+}
+
 #define TYPE_SYSBUS_FDC "base-sysbus-fdc"
 #define SYSBUS_FDC(obj) OBJECT_CHECK(FDCtrlSysBus, (obj), TYPE_SYSBUS_FDC)
 
@@ -2338,6 +2346,10 @@ static void fdctrl_realize_common(FDCtrl *fdctrl, Error **errp)
     int i, j;
     static int command_tables_inited = 0;
 
+    if (fdctrl->fallback == FLOPPY_DRIVE_TYPE_AUTO) {
+        error_setg(errp, "Cannot choose a fallback FDrive type of 'auto'");
+    }
+
     /* Fill 'command_to_handler' lookup table */
     if (!command_tables_inited) {
         command_tables_inited = 1;
@@ -2463,6 +2475,9 @@ static Property isa_fdc_properties[] = {
     DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.drives[1].blk),
     DEFINE_PROP_BIT("check_media_rate", FDCtrlISABus, state.check_media_rate,
                     0, true),
+    DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
+                        FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
+                        FloppyDriveType),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2511,6 +2526,9 @@ static const VMStateDescription vmstate_sysbus_fdc ={
 static Property sysbus_fdc_properties[] = {
     DEFINE_PROP_DRIVE("driveA", FDCtrlSysBus, state.drives[0].blk),
     DEFINE_PROP_DRIVE("driveB", FDCtrlSysBus, state.drives[1].blk),
+    DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
+                        FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
+                        FloppyDriveType),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2531,6 +2549,9 @@ static const TypeInfo sysbus_fdc_info = {
 
 static Property sun4m_fdc_properties[] = {
     DEFINE_PROP_DRIVE("drive", FDCtrlSysBus, state.drives[0].blk),
+    DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
+                        FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
+                        FloppyDriveType),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 3572810..aacad66 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -541,6 +541,17 @@ PropertyInfo qdev_prop_bios_chs_trans = {
     .set = set_enum,
 };
 
+/* --- FDC default drive types */
+
+PropertyInfo qdev_prop_fdc_drive_type = {
+    .name = "FdcDriveType",
+    .description = "FDC drive type, "
+                   "144/288/120/none/auto",
+    .enum_table = FloppyDriveType_lookup,
+    .get = get_enum,
+    .set = set_enum
+};
+
 /* --- pci address --- */
 
 /*
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 254afd8..03a1b91 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -20,6 +20,7 @@ extern PropertyInfo qdev_prop_ptr;
 extern PropertyInfo qdev_prop_macaddr;
 extern PropertyInfo qdev_prop_losttickpolicy;
 extern PropertyInfo qdev_prop_bios_chs_trans;
+extern PropertyInfo qdev_prop_fdc_drive_type;
 extern PropertyInfo qdev_prop_drive;
 extern PropertyInfo qdev_prop_netdev;
 extern PropertyInfo qdev_prop_vlan;
-- 
2.4.3

[Qemu-devel] [PATCH v5 08/12] fdc: add drive type option

[John Snow]

This patch adds a new explicit Floppy Drive Type option. The existing
behavior in QEMU is to automatically guess a drive type based on the
media inserted, or if a diskette is not present, arbitrarily assign one.

This behavior can be described as "auto." This patch adds the option
to pick an explicit behavior: 120, 144, 288 or none. The new "auto"
option is intended to mimic current behavior, while the other types
pick one explicitly.

Set the type given by the CLI during fd_init. If the type remains the
default (auto), we'll attempt to scan an inserted diskette if present
to determine a type. If auto is selected but no diskette is present,
we fall back to a predetermined default (currently 1.44MB to match
legacy QEMU behavior.)

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 4caed9b..e89ce2d 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -160,7 +160,6 @@ static FloppyDriveType get_fallback_drive_type(FDrive *drv);
 static void fd_init(FDrive *drv)
 {
     /* Drive */
-    drv->drive = FLOPPY_DRIVE_TYPE_NONE;
     drv->perpendicular = 0;
     /* Disk */
     drv->disk = FLOPPY_DRIVE_TYPE_NONE;
@@ -264,7 +263,7 @@ static int pick_geometry(FDrive *drv)
     int i, first_match, match;
 
     /* We can only pick a geometry if we have a diskette. */
-    if (!drv->media_inserted) {
+    if (!drv->media_inserted || drv->drive == FLOPPY_DRIVE_TYPE_NONE) {
         return -1;
     }
 
@@ -277,7 +276,7 @@ static int pick_geometry(FDrive *drv)
             break;
         }
         if (drv->drive == parse->drive ||
-            drv->drive == FLOPPY_DRIVE_TYPE_NONE) {
+            drv->drive == FLOPPY_DRIVE_TYPE_AUTO) {
             size = (parse->max_head + 1) * parse->max_track *
                 parse->last_sect;
             if (nb_sectors == size) {
@@ -314,11 +313,17 @@ static int pick_geometry(FDrive *drv)
 
 static void pick_drive_type(FDrive *drv)
 {
+    if (drv->drive != FLOPPY_DRIVE_TYPE_AUTO) {
+        return;
+    }
+
     if (pick_geometry(drv) == 0) {
         drv->drive = drv->disk;
     } else {
         drv->drive = get_fallback_drive_type(drv);
     }
+
+    g_assert(drv->drive != FLOPPY_DRIVE_TYPE_AUTO);
 }
 
 /* Revalidate a disk drive after a disk change */
@@ -2475,6 +2480,12 @@ static Property isa_fdc_properties[] = {
     DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.drives[1].blk),
     DEFINE_PROP_BIT("check_media_rate", FDCtrlISABus, state.check_media_rate,
                     0, true),
+    DEFINE_PROP_DEFAULT("fdtypeA", FDCtrlISABus, state.drives[0].drive,
+                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
+                        FloppyDriveType),
+    DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlISABus, state.drives[1].drive,
+                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
+                        FloppyDriveType),
     DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
                         FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
                         FloppyDriveType),
@@ -2526,6 +2537,12 @@ static const VMStateDescription vmstate_sysbus_fdc ={
 static Property sysbus_fdc_properties[] = {
     DEFINE_PROP_DRIVE("driveA", FDCtrlSysBus, state.drives[0].blk),
     DEFINE_PROP_DRIVE("driveB", FDCtrlSysBus, state.drives[1].blk),
+    DEFINE_PROP_DEFAULT("fdtypeA", FDCtrlSysBus, state.drives[0].drive,
+                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
+                        FloppyDriveType),
+    DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlSysBus, state.drives[1].drive,
+                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
+                        FloppyDriveType),
     DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
                         FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
                         FloppyDriveType),
@@ -2549,6 +2566,9 @@ static const TypeInfo sysbus_fdc_info = {
 
 static Property sun4m_fdc_properties[] = {
     DEFINE_PROP_DRIVE("drive", FDCtrlSysBus, state.drives[0].blk),
+    DEFINE_PROP_DEFAULT("fdtype", FDCtrlSysBus, state.drives[0].drive,
+                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
+                        FloppyDriveType),
     DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
                         FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
                         FloppyDriveType),
-- 
2.4.3

[Qemu-devel] [PATCH v5 09/12] fdc: add physical disk sizes

[John Snow]

2.88MB capable drives can accept 1.44MB floppies,
for instance. To rework the pick_geometry function,
we need to know if our current drive can even accept
the type of disks we're considering.

NB: This allows us to distinguish between all of the
"total sectors" collisions between 1.20MB and 1.44MB
diskette types, by using the physical drive size as a
differentiator.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c | 40 ++++++++++++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index e89ce2d..e51154b 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -60,6 +60,12 @@ typedef enum FDriveRate {
     FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
 } FDriveRate;
 
+typedef enum FDriveSize {
+    FDRIVE_SIZE_UNKNOWN,
+    FDRIVE_SIZE_350,
+    FDRIVE_SIZE_525,
+} FDriveSize;
+
 typedef struct FDFormat {
     FloppyDriveType drive;
     uint8_t last_sect;
@@ -68,11 +74,15 @@ typedef struct FDFormat {
     FDriveRate rate;
 } FDFormat;
 
+/* In many cases, the total sector size of a format is enough to uniquely
+ * identify it. However, there are some total sector collisions between
+ * formats of different physical size, and these are noted below by
+ * highlighting the total sector size for entries with collisions. */
 static const FDFormat fd_formats[] = {
     /* First entry is default format */
     /* 1.44 MB 3"1/2 floppy disks */
-    { FLOPPY_DRIVE_TYPE_144, 18, 80, 1, FDRIVE_RATE_500K, },
-    { FLOPPY_DRIVE_TYPE_144, 20, 80, 1, FDRIVE_RATE_500K, },
+    { FLOPPY_DRIVE_TYPE_144, 18, 80, 1, FDRIVE_RATE_500K, }, /* 3.5" 2880 */
+    { FLOPPY_DRIVE_TYPE_144, 20, 80, 1, FDRIVE_RATE_500K, }, /* 3.5" 3200 */
     { FLOPPY_DRIVE_TYPE_144, 21, 80, 1, FDRIVE_RATE_500K, },
     { FLOPPY_DRIVE_TYPE_144, 21, 82, 1, FDRIVE_RATE_500K, },
     { FLOPPY_DRIVE_TYPE_144, 21, 83, 1, FDRIVE_RATE_500K, },
@@ -86,7 +96,7 @@ static const FDFormat fd_formats[] = {
     { FLOPPY_DRIVE_TYPE_288, 44, 80, 1, FDRIVE_RATE_1M, },
     { FLOPPY_DRIVE_TYPE_288, 48, 80, 1, FDRIVE_RATE_1M, },
     /* 720 kB 3"1/2 floppy disks */
-    { FLOPPY_DRIVE_TYPE_144,  9, 80, 1, FDRIVE_RATE_250K, },
+    { FLOPPY_DRIVE_TYPE_144,  9, 80, 1, FDRIVE_RATE_250K, }, /* 3.5" 1440 */
     { FLOPPY_DRIVE_TYPE_144, 10, 80, 1, FDRIVE_RATE_250K, },
     { FLOPPY_DRIVE_TYPE_144, 10, 82, 1, FDRIVE_RATE_250K, },
     { FLOPPY_DRIVE_TYPE_144, 10, 83, 1, FDRIVE_RATE_250K, },
@@ -94,15 +104,15 @@ static const FDFormat fd_formats[] = {
     { FLOPPY_DRIVE_TYPE_144, 14, 80, 1, FDRIVE_RATE_250K, },
     /* 1.2 MB 5"1/4 floppy disks */
     { FLOPPY_DRIVE_TYPE_120, 15, 80, 1, FDRIVE_RATE_500K, },
-    { FLOPPY_DRIVE_TYPE_120, 18, 80, 1, FDRIVE_RATE_500K, },
+    { FLOPPY_DRIVE_TYPE_120, 18, 80, 1, FDRIVE_RATE_500K, }, /* 5.25" 2880 */
     { FLOPPY_DRIVE_TYPE_120, 18, 82, 1, FDRIVE_RATE_500K, },
     { FLOPPY_DRIVE_TYPE_120, 18, 83, 1, FDRIVE_RATE_500K, },
-    { FLOPPY_DRIVE_TYPE_120, 20, 80, 1, FDRIVE_RATE_500K, },
+    { FLOPPY_DRIVE_TYPE_120, 20, 80, 1, FDRIVE_RATE_500K, }, /* 5.25" 3200 */
     /* 720 kB 5"1/4 floppy disks */
-    { FLOPPY_DRIVE_TYPE_120,  9, 80, 1, FDRIVE_RATE_250K, },
+    { FLOPPY_DRIVE_TYPE_120,  9, 80, 1, FDRIVE_RATE_250K, }, /* 5.25" 1440 */
     { FLOPPY_DRIVE_TYPE_120, 11, 80, 1, FDRIVE_RATE_250K, },
     /* 360 kB 5"1/4 floppy disks */
-    { FLOPPY_DRIVE_TYPE_120,  9, 40, 1, FDRIVE_RATE_300K, },
+    { FLOPPY_DRIVE_TYPE_120,  9, 40, 1, FDRIVE_RATE_300K, }, /* 5.25" 720 */
     { FLOPPY_DRIVE_TYPE_120,  9, 40, 0, FDRIVE_RATE_300K, },
     { FLOPPY_DRIVE_TYPE_120, 10, 41, 1, FDRIVE_RATE_300K, },
     { FLOPPY_DRIVE_TYPE_120, 10, 42, 1, FDRIVE_RATE_300K, },
@@ -110,11 +120,25 @@ static const FDFormat fd_formats[] = {
     { FLOPPY_DRIVE_TYPE_120,  8, 40, 1, FDRIVE_RATE_250K, },
     { FLOPPY_DRIVE_TYPE_120,  8, 40, 0, FDRIVE_RATE_250K, },
     /* 360 kB must match 5"1/4 better than 3"1/2... */
-    { FLOPPY_DRIVE_TYPE_144,  9, 80, 0, FDRIVE_RATE_250K, },
+    { FLOPPY_DRIVE_TYPE_144,  9, 80, 0, FDRIVE_RATE_250K, }, /* 3.5" 720 */
     /* end */
     { FLOPPY_DRIVE_TYPE_NONE, -1, -1, 0, 0, },
 };
 
+__attribute__((__unused__))
+static FDriveSize drive_size(FloppyDriveType drive)
+{
+    switch (drive) {
+    case FLOPPY_DRIVE_TYPE_120:
+        return FDRIVE_SIZE_525;
+    case FLOPPY_DRIVE_TYPE_144:
+    case FLOPPY_DRIVE_TYPE_288:
+        return FDRIVE_SIZE_350;
+    default:
+        return FDRIVE_SIZE_UNKNOWN;
+    }
+}
+
 #define GET_CUR_DRV(fdctrl) ((fdctrl)->cur_drv)
 #define SET_CUR_DRV(fdctrl, drive) ((fdctrl)->cur_drv = (drive))
 
-- 
2.4.3

[Qemu-devel] [PATCH v5 10/12] fdc: rework pick_geometry

[John Snow]

This one is the crazy one.

fd_revalidate currently uses pick_geometry to tell if the diskette
geometry has changed upon an eject/insert event, but it won't allow us
to insert a 1.44MB diskette into a 2.88MB drive. This is inflexible.

The new algorithm applies a new heuristic to guessing disk geometries
that allows us to switch diskette types as long as the physical size
matches before falling back to the old heuristic.

The old one is roughly:
 - If the size (sectors) and type matches, choose it.
 - Fall back to the first geometry that matched our type.

The new one is:
 - If the size (sectors) and type matches, choose it.
 - If the size (sectors) and physical size match, choose it.
 - If the size (sectors) matches at all, choose it begrudgingly.
 - Fall back to the first geometry that matched our type.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c | 72 ++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 20 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index e51154b..6e0c5fc 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -125,7 +125,6 @@ static const FDFormat fd_formats[] = {
     { FLOPPY_DRIVE_TYPE_NONE, -1, -1, 0, 0, },
 };
 
-__attribute__((__unused__))
 static FDriveSize drive_size(FloppyDriveType drive)
 {
     switch (drive) {
@@ -284,45 +283,78 @@ static int pick_geometry(FDrive *drv)
     BlockBackend *blk = drv->blk;
     const FDFormat *parse;
     uint64_t nb_sectors, size;
-    int i, first_match, match;
+    int i;
+    int match, size_match, type_match;
+    bool magic = drv->drive == FLOPPY_DRIVE_TYPE_AUTO;
 
     /* We can only pick a geometry if we have a diskette. */
     if (!drv->media_inserted || drv->drive == FLOPPY_DRIVE_TYPE_NONE) {
         return -1;
     }
 
+    /* We need to determine the likely geometry of the inserted medium.
+     * In order of preference, we look for:
+     * (1) The same drive type and number of sectors,
+     * (2) The same diskette size and number of sectors,
+     * (3) The same drive type.
+     *
+     * In all cases, matches that occur higher in the drive table will take
+     * precedence over matches that occur later in the table.
+     */
     blk_get_geometry(blk, &nb_sectors);
-    match = -1;
-    first_match = -1;
+    match = size_match = type_match = -1;
     for (i = 0; ; i++) {
         parse = &fd_formats[i];
         if (parse->drive == FLOPPY_DRIVE_TYPE_NONE) {
             break;
         }
-        if (drv->drive == parse->drive ||
-            drv->drive == FLOPPY_DRIVE_TYPE_AUTO) {
-            size = (parse->max_head + 1) * parse->max_track *
-                parse->last_sect;
-            if (nb_sectors == size) {
-                match = i;
-                break;
+        size = (parse->max_head + 1) * parse->max_track * parse->last_sect;
+        if (nb_sectors == size) {
+            if (magic || parse->drive == drv->drive) {
+                /* (1) perfect match -- nb_sectors and drive type */
+                goto out;
+            } else if (drive_size(parse->drive) == drive_size(drv->drive)) {
+                /* (2) size match -- nb_sectors and physical medium size */
+                match = (match == -1) ? i : match;
+            } else {
+                /* This is suspicious -- Did the user misconfigure? */
+                size_match = (size_match == -1) ? i : size_match;
             }
-            if (first_match == -1) {
-                first_match = i;
+        } else if (type_match == -1) {
+            if ((parse->drive == drv->drive) ||
+                (magic && (parse->drive == get_fallback_drive_type(drv)))) {
+                /* (3) type match -- nb_sectors mismatch, but matches the type
+                 *     specified explicitly by the user, or matches the fallback
+                 *     default type when using the drive autodetect mechanism */
+                type_match = i;
             }
         }
     }
+
+    /* No exact match found */
     if (match == -1) {
-        if (first_match == -1) {
-            error_setg(&error_abort, "No candidate geometries present in table "
-                       " for floppy drive type '%s'",
-                       FloppyDriveType_lookup[drv->drive]);
-        } else {
-            match = first_match;
+        if (size_match != -1) {
+            parse = &fd_formats[size_match];
+            FLOPPY_DPRINTF("User requested floppy drive type '%s', "
+                           "but inserted medium appears to be a "
+                           "%d sector '%s' type\n",
+                           FloppyDriveType_lookup[drv->drive],
+                           nb_sectors,
+                           FloppyDriveType_lookup[parse->drive]);
         }
-        parse = &fd_formats[match];
+        match = type_match;
     }
 
+    /* No match of any kind found -- fd_format is misconfigured, abort. */
+    if (match == -1) {
+        error_setg(&error_abort, "No candidate geometries present in table "
+                   " for floppy drive type '%s'",
+                   FloppyDriveType_lookup[drv->drive]);
+    }
+
+    parse = &(fd_formats[match]);
+
+ out:
     if (parse->max_head == 0) {
         drv->flags &= ~FDISK_DBL_SIDES;
     } else {
-- 
2.4.3

[Qemu-devel] [PATCH v5 11/12] qtest/fdc: Support for 2.88MB drives

[John Snow]

The old test assumes a 1.44MB drive.
Assert that the QEMU default drive is now either 1.44 or 2.88.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/fdc-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index b5a4696..526d459 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -267,7 +267,7 @@ static void test_cmos(void)
     uint8_t cmos;
 
     cmos = cmos_read(CMOS_FLOPPY);
-    g_assert(cmos == 0x40);
+    g_assert(cmos == 0x40 || cmos == 0x50);
 }
 
 static void test_no_media_on_start(void)
-- 
2.4.3

[Qemu-devel] [PATCH v5 12/12] fdc: change auto fallback drive for ISA FDC to 288

[John Snow]

The 2.88 drive is more suitable as a default because
it can still read 1.44 images correctly, but the reverse
is not true.

Since there exist virtio-win drivers that are shipped on
2.88 floppy images, this patch will allow VMs booted without
a floppy disk inserted to later insert a 2.88MB floppy and
have that work.

This patch has been tested with msdos, freedos, fedora,
windows 8 and windows 10 without issue: if problems do
arise for certain guests being unable to cope with 2.88MB
drives as the default, they are in the minority and can use
type=144 as needed (or insert a proper boot medium and omit
type=144/288 or use type=auto) to obtain different drive types.

As icing, the default will remain auto/144 for any pre-2.6
machine types, hopefully minimizing the impact of this change
in legacy hw to basically zero.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c      | 2 +-
 include/hw/compat.h | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 6e0c5fc..e3b0e1e 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2543,7 +2543,7 @@ static Property isa_fdc_properties[] = {
                         FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
                         FloppyDriveType),
     DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
-                        FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
+                        FLOPPY_DRIVE_TYPE_288, qdev_prop_fdc_drive_type,
                         FloppyDriveType),
     DEFINE_PROP_END_OF_LIST(),
 };
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 491884b..2ebe739 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -3,6 +3,10 @@
 
 #define HW_COMPAT_2_5 \
     {\
+        .driver   = "isa-fdc",\
+        .property = "fallback",\
+        .value    = "144",\
+    },{\
         .driver   = "pvscsi",\
         .property = "x-old-pci-configuration",\
         .value    = "on",\
-- 
2.4.3

Re: [Qemu-devel] [PATCH v5 10/12] fdc: rework pick_geometry

[John Snow]

On 01/22/2016 03:51 PM, John Snow wrote:
> This one is the crazy one.
> 
> fd_revalidate currently uses pick_geometry to tell if the diskette
> geometry has changed upon an eject/insert event, but it won't allow us
> to insert a 1.44MB diskette into a 2.88MB drive. This is inflexible.
> 
> The new algorithm applies a new heuristic to guessing disk geometries
> that allows us to switch diskette types as long as the physical size
> matches before falling back to the old heuristic.
> 
> The old one is roughly:
>  - If the size (sectors) and type matches, choose it.
>  - Fall back to the first geometry that matched our type.
> 
> The new one is:
>  - If the size (sectors) and type matches, choose it.
>  - If the size (sectors) and physical size match, choose it.
>  - If the size (sectors) matches at all, choose it begrudgingly.
>  - Fall back to the first geometry that matched our type.
> 

Goofed and didn't update commit. Will change on PULL to omit the third
line if the patch is otherwise OK.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/block/fdc.c | 72 ++++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 52 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index e51154b..6e0c5fc 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -125,7 +125,6 @@ static const FDFormat fd_formats[] = {
>      { FLOPPY_DRIVE_TYPE_NONE, -1, -1, 0, 0, },
>  };
>  
> -__attribute__((__unused__))
>  static FDriveSize drive_size(FloppyDriveType drive)
>  {
>      switch (drive) {
> @@ -284,45 +283,78 @@ static int pick_geometry(FDrive *drv)
>      BlockBackend *blk = drv->blk;
>      const FDFormat *parse;
>      uint64_t nb_sectors, size;
> -    int i, first_match, match;
> +    int i;
> +    int match, size_match, type_match;
> +    bool magic = drv->drive == FLOPPY_DRIVE_TYPE_AUTO;
>  
>      /* We can only pick a geometry if we have a diskette. */
>      if (!drv->media_inserted || drv->drive == FLOPPY_DRIVE_TYPE_NONE) {
>          return -1;
>      }
>  
> +    /* We need to determine the likely geometry of the inserted medium.
> +     * In order of preference, we look for:
> +     * (1) The same drive type and number of sectors,
> +     * (2) The same diskette size and number of sectors,
> +     * (3) The same drive type.
> +     *
> +     * In all cases, matches that occur higher in the drive table will take
> +     * precedence over matches that occur later in the table.
> +     */
>      blk_get_geometry(blk, &nb_sectors);
> -    match = -1;
> -    first_match = -1;
> +    match = size_match = type_match = -1;
>      for (i = 0; ; i++) {
>          parse = &fd_formats[i];
>          if (parse->drive == FLOPPY_DRIVE_TYPE_NONE) {
>              break;
>          }
> -        if (drv->drive == parse->drive ||
> -            drv->drive == FLOPPY_DRIVE_TYPE_AUTO) {
> -            size = (parse->max_head + 1) * parse->max_track *
> -                parse->last_sect;
> -            if (nb_sectors == size) {
> -                match = i;
> -                break;
> +        size = (parse->max_head + 1) * parse->max_track * parse->last_sect;
> +        if (nb_sectors == size) {
> +            if (magic || parse->drive == drv->drive) {
> +                /* (1) perfect match -- nb_sectors and drive type */
> +                goto out;
> +            } else if (drive_size(parse->drive) == drive_size(drv->drive)) {
> +                /* (2) size match -- nb_sectors and physical medium size */
> +                match = (match == -1) ? i : match;
> +            } else {
> +                /* This is suspicious -- Did the user misconfigure? */
> +                size_match = (size_match == -1) ? i : size_match;
>              }
> -            if (first_match == -1) {
> -                first_match = i;
> +        } else if (type_match == -1) {
> +            if ((parse->drive == drv->drive) ||
> +                (magic && (parse->drive == get_fallback_drive_type(drv)))) {
> +                /* (3) type match -- nb_sectors mismatch, but matches the type
> +                 *     specified explicitly by the user, or matches the fallback
> +                 *     default type when using the drive autodetect mechanism */
> +                type_match = i;
>              }
>          }
>      }
> +
> +    /* No exact match found */
>      if (match == -1) {
> -        if (first_match == -1) {
> -            error_setg(&error_abort, "No candidate geometries present in table "
> -                       " for floppy drive type '%s'",
> -                       FloppyDriveType_lookup[drv->drive]);
> -        } else {
> -            match = first_match;
> +        if (size_match != -1) {
> +            parse = &fd_formats[size_match];
> +            FLOPPY_DPRINTF("User requested floppy drive type '%s', "
> +                           "but inserted medium appears to be a "
> +                           "%d sector '%s' type\n",
> +                           FloppyDriveType_lookup[drv->drive],
> +                           nb_sectors,
> +                           FloppyDriveType_lookup[parse->drive]);
>          }
> -        parse = &fd_formats[match];
> +        match = type_match;
>      }
>  
> +    /* No match of any kind found -- fd_format is misconfigured, abort. */
> +    if (match == -1) {
> +        error_setg(&error_abort, "No candidate geometries present in table "
> +                   " for floppy drive type '%s'",
> +                   FloppyDriveType_lookup[drv->drive]);
> +    }
> +
> +    parse = &(fd_formats[match]);
> +
> + out:
>      if (parse->max_head == 0) {
>          drv->flags &= ~FDISK_DBL_SIDES;
>      } else {
> 

-- 
—js

Re: [Qemu-devel] [PATCH v5 10/12] fdc: rework pick_geometry

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1302 bytes --]

On 01/22/2016 01:59 PM, John Snow wrote:
> 
> 
> On 01/22/2016 03:51 PM, John Snow wrote:
>> This one is the crazy one.
>>
>> fd_revalidate currently uses pick_geometry to tell if the diskette
>> geometry has changed upon an eject/insert event, but it won't allow us
>> to insert a 1.44MB diskette into a 2.88MB drive. This is inflexible.
>>
>> The new algorithm applies a new heuristic to guessing disk geometries
>> that allows us to switch diskette types as long as the physical size
>> matches before falling back to the old heuristic.
>>
>> The old one is roughly:
>>  - If the size (sectors) and type matches, choose it.
>>  - Fall back to the first geometry that matched our type.
>>
>> The new one is:
>>  - If the size (sectors) and type matches, choose it.
>>  - If the size (sectors) and physical size match, choose it.
>>  - If the size (sectors) matches at all, choose it begrudgingly.
>>  - Fall back to the first geometry that matched our type.
>>
> 
> Goofed and didn't update commit. Will change on PULL to omit the third
> line if the patch is otherwise OK.

Yep, that's the only thing I noticed.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]