[Qemu-devel] [RFC 00/10] fix 2.88mb floppy diskette support

[Qemu-devel] [RFC 00/10] fix 2.88mb floppy diskette support

[John Snow]

Yep, it's been broken for 10 years too.
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 it based
on the diskette you insert at boot.

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

I want to change the default to a 2.88MB type and users to switch
back and forth between 2.88 and 1.44MB diskettes.

Shuffle things around a bit to make this magic happen.

This is just an RFC, for many reasons, mostly the 2.4 timeframe,
my unfamiliarity with the qdev property system, uncertainty over
where/how I added this properties, uncertainty over whether or not
READ_ID is really expected to work without a floppy disk or not
(which impacts if fdc-test is broken or not!) and other sloppy
design issues.

John Snow (10):
  fdc: Make default FDrive type explicit
  fdc: add default drive type option
  fdc: respect default drive type
  fdc: move pick_geometry
  fdc: refactor pick_geometry
  fdc: add physical disk sizes
  fdc: add disk field
  fdc: refactor pick_geometry
  qtest/fdc: Support for 2.88MB drives
  fdc: change default drive to 288

 hw/block/fdc.c               | 189 ++++++++++++++++++++++++++++++-------------
 hw/core/qdev-properties.c    |  12 +++
 include/hw/block/fdc.h       |   7 +-
 include/hw/qdev-properties.h |   1 +
 qapi/block.json              |  15 ++++
 tests/fdc-test.c             |   2 +-
 6 files changed, 161 insertions(+), 65 deletions(-)

-- 
2.1.0

[Qemu-devel] [RFC 01/10] fdc: Make default FDrive type explicit

[John Snow]

What happens currently is if a drive is not inserted, we
won't match any of the drive types (None) or the geometries,
so we'll wind up picking the very first drive (1.44MB type)
as a default.

This patch makes the default picking a lot more explicit.

If a floppy image is inserted, QEMU will continue as it has
in the past to choose the drive type based on the image provided.

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

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 5e1b67e..cdf9e09 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -123,6 +123,13 @@ static void pick_geometry(BlockBackend *blk, int *nb_heads,
     uint64_t nb_sectors, size;
     int i, first_match, match;
 
+    /* Pick a default drive type if there's no media inserted AND we have
+     * not yet announced our drive type to the CMOS. */
+    if (!blk_is_inserted(blk) && drive_in == FDRIVE_DRV_NONE) {
+        parse = &fd_formats[0];
+        goto out;
+    }
+
     blk_get_geometry(blk, &nb_sectors);
     match = -1;
     first_match = -1;
@@ -152,6 +159,8 @@ static void pick_geometry(BlockBackend *blk, int *nb_heads,
         }
         parse = &fd_formats[match];
     }
+
+ out:
     *nb_heads = parse->max_head + 1;
     *max_track = parse->max_track;
     *last_sect = parse->last_sect;
-- 
2.1.0

[Qemu-devel] [RFC 02/10] fdc: add default drive type option

[John Snow]

We want to change the current default drive type,
but to be kind, we need to allow users to specify
the old drive type somehow.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c               | 13 +++++++++++++
 hw/core/qdev-properties.c    | 12 ++++++++++++
 include/hw/block/fdc.h       |  7 +------
 include/hw/qdev-properties.h |  1 +
 qapi/block.json              | 15 +++++++++++++++
 5 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index cdf9e09..1023a01 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -67,6 +67,8 @@ typedef struct FDFormat {
     FDriveRate rate;
 } FDFormat;
 
+#define FDRIVE_DEFAULT FDRIVE_DRV_144
+
 static const FDFormat fd_formats[] = {
     /* First entry is default format */
     /* 1.44 MB 3"1/2 floppy disks */
@@ -578,6 +580,9 @@ struct FDCtrl {
     /* Timers state */
     uint8_t timer0;
     uint8_t timer1;
+
+    FDriveType defaultA;
+    FDriveType defaultB;
 };
 
 #define TYPE_SYSBUS_FDC "base-sysbus-fdc"
@@ -2423,6 +2428,10 @@ 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("defaultA", FDCtrlISABus, state.defaultA,
+                        FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType),
+    DEFINE_PROP_DEFAULT("defaultB", FDCtrlISABus, state.defaultB,
+                        FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2471,6 +2480,10 @@ 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("defaultA", FDCtrlSysBus, state.defaultA,
+                        FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType),
+    DEFINE_PROP_DEFAULT("defaultB", FDCtrlSysBus, state.defaultB,
+                        FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 47c1e8f..7ff8030 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -7,6 +7,7 @@
 #include "net/hub.h"
 #include "qapi/visitor.h"
 #include "sysemu/char.h"
+#include "hw/block/fdc.h"
 
 void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
                                   Error **errp)
@@ -543,6 +544,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, "
+                   "none/120/144/288",
+    .enum_table = FDRIVE_DRV_lookup,
+    .get = get_enum,
+    .set = set_enum
+};
+
 /* --- pci address --- */
 
 /*
diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
index d48b2f8..f727027 100644
--- a/include/hw/block/fdc.h
+++ b/include/hw/block/fdc.h
@@ -6,12 +6,7 @@
 /* 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;
+typedef FDRIVE_DRV FDriveType;
 
 #define TYPE_ISA_FDC "isa-fdc"
 
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 0cfff1c..0872b41 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;
diff --git a/qapi/block.json b/qapi/block.json
index aad645c..0c747a1 100644
--- a/qapi/block.json
+++ b/qapi/block.json
@@ -40,6 +40,21 @@
   'data': ['auto', 'none', 'lba', 'large', 'rechs']}
 
 ##
+# FDRIVE_DRV:
+#
+# 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.5MB 5.25" drive
+# @none: No drive connected
+#
+# Since: 2.4
+##
+{ 'enum': 'FDRIVE_DRV',
+  'data': ['144', '288', '120', 'none']}
+
+##
 # @BlockdevSnapshotInternal
 #
 # @device: the name of the device to generate the snapshot from
-- 
2.1.0

[Qemu-devel] [RFC 03/10] fdc: respect default drive type

[John Snow]

Respect the default drive type as proffered via the CLI.

This patch overloads the "drive out" parameter of pick_geometry
to be used as a "default hint" which is offered by fd_revalidate.

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

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 1023a01..87fd770 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -128,7 +128,13 @@ static void pick_geometry(BlockBackend *blk, int *nb_heads,
     /* Pick a default drive type if there's no media inserted AND we have
      * not yet announced our drive type to the CMOS. */
     if (!blk_is_inserted(blk) && drive_in == FDRIVE_DRV_NONE) {
-        parse = &fd_formats[0];
+        for (i = 0; ; i++) {
+            parse = &fd_formats[i];
+            if ((parse->drive == FDRIVE_DRV_NONE) ||
+                (parse->drive == *drive)) {
+                break;
+            }
+        }
         goto out;
     }
 
@@ -295,11 +301,13 @@ static void fd_recalibrate(FDrive *drv)
     fd_seek(drv, 0, 0, 1, 1);
 }
 
+static FDriveType get_default_drive_type(FDrive *drv);
+
 /* Revalidate a disk drive after a disk change */
 static void fd_revalidate(FDrive *drv)
 {
     int nb_heads, max_track, last_sect, ro;
-    FDriveType drive;
+    FDriveType drive = get_default_drive_type(drv);
     FDriveRate rate;
 
     FLOPPY_DPRINTF("revalidate\n");
@@ -609,6 +617,21 @@ typedef struct FDCtrlISABus {
     int32_t bootindexB;
 } FDCtrlISABus;
 
+static FDriveType get_default_drive_type(FDrive *drv)
+{
+    FDCtrl *fdctrl = drv->fdctrl;
+
+    if (0 < MAX_FD && (&fdctrl->drives[0] == drv)) {
+        return fdctrl->defaultA;
+    }
+
+    if (1 < MAX_FD && (&fdctrl->drives[1] == drv)) {
+        return fdctrl->defaultB;
+    }
+
+    return FDRIVE_DEFAULT;
+}
+
 static uint32_t fdctrl_read (void *opaque, uint32_t reg)
 {
     FDCtrl *fdctrl = opaque;
-- 
2.1.0

[Qemu-devel] [RFC 04/10] fdc: move pick_geometry

[John Snow]

Code motion: I want to refactor pick_geometry to work with
the FDrive type directly, so shuffle it below that definition.

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

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 87fd770..be6bf25 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -116,66 +116,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;
-
-    /* Pick a default drive type if there's no media inserted AND we have
-     * not yet announced our drive type to the CMOS. */
-    if (!blk_is_inserted(blk) && drive_in == FDRIVE_DRV_NONE) {
-        for (i = 0; ; i++) {
-            parse = &fd_formats[i];
-            if ((parse->drive == FDRIVE_DRV_NONE) ||
-                (parse->drive == *drive)) {
-                break;
-            }
-        }
-        goto out;
-    }
-
-    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];
-    }
-
- out:
-    *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))
 
@@ -303,6 +243,66 @@ static void fd_recalibrate(FDrive *drv)
 
 static FDriveType get_default_drive_type(FDrive *drv);
 
+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;
+
+    /* Pick a default drive type if there's no media inserted AND we have
+     * not yet announced our drive type to the CMOS. */
+    if (!blk_is_inserted(blk) && drive_in == FDRIVE_DRV_NONE) {
+        for (i = 0; ; i++) {
+            parse = &fd_formats[i];
+            if ((parse->drive == FDRIVE_DRV_NONE) ||
+                (parse->drive == *drive)) {
+                break;
+            }
+        }
+        goto out;
+    }
+
+    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];
+    }
+
+ out:
+    *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.1.0

[Qemu-devel] [RFC 05/10] fdc: refactor pick_geometry

[John Snow]

Lessen the number of parameters it takes.

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

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index be6bf25..4004b98 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -243,22 +243,20 @@ static void fd_recalibrate(FDrive *drv)
 
 static FDriveType get_default_drive_type(FDrive *drv);
 
-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, int *nb_heads)
 {
+    BlockBackend *blk = drv->blk;
     const FDFormat *parse;
     uint64_t nb_sectors, size;
     int i, first_match, match;
 
     /* Pick a default drive type if there's no media inserted AND we have
      * not yet announced our drive type to the CMOS. */
-    if (!blk_is_inserted(blk) && drive_in == FDRIVE_DRV_NONE) {
+    if (!blk_is_inserted(blk) && drv->drive == FDRIVE_DRV_NONE) {
         for (i = 0; ; i++) {
             parse = &fd_formats[i];
             if ((parse->drive == FDRIVE_DRV_NONE) ||
-                (parse->drive == *drive)) {
+                (parse->drive == get_default_drive_type(drv))) {
                 break;
             }
         }
@@ -273,8 +271,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) {
@@ -297,40 +295,33 @@ static void pick_geometry(BlockBackend *blk, int *nb_heads,
 
  out:
     *nb_heads = parse->max_head + 1;
-    *max_track = parse->max_track;
-    *last_sect = parse->last_sect;
-    *drive = parse->drive;
-    *rate = parse->rate;
+    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 = get_default_drive_type(drv);
-    FDriveRate rate;
+    int nb_heads = -1;
 
     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, &nb_heads);
         if (!blk_is_inserted(drv->blk)) {
             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");
+                           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.1.0

[Qemu-devel] [RFC 06/10] 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.

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

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 4004b98..6e5f87b 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -59,6 +59,11 @@ typedef enum FDriveRate {
     FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
 } FDriveRate;
 
+typedef enum FDriveSize {
+    FDRIVE_SIZE_350,
+    FDRIVE_SIZE_525
+} FDriveSize;
+
 typedef struct FDFormat {
     FDriveType drive;
     uint8_t last_sect;
@@ -95,15 +100,15 @@ static const FDFormat fd_formats[] = {
     { FDRIVE_DRV_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, 80, 1, FDRIVE_RATE_500K, }, /* conflicts w/ #0 */
     { 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, },
+    { FDRIVE_DRV_120, 20, 80, 1, FDRIVE_RATE_500K, }, /* conflicts w/ #1 */
     /* 720 kB 5"1/4 floppy disks */
-    { FDRIVE_DRV_120,  9, 80, 1, FDRIVE_RATE_250K, },
+    { FDRIVE_DRV_120,  9, 80, 1, FDRIVE_RATE_250K, }, /* conflicts w/ #13 */
     { FDRIVE_DRV_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, 1, FDRIVE_RATE_300K, }, /* conflicts w/ #32 */
     { 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, },
@@ -116,6 +121,20 @@ static const FDFormat fd_formats[] = {
     { FDRIVE_DRV_NONE, -1, -1, 0, 0, },
 };
 
+__attribute__((__unused__))
+static FDriveSize drive_size(FDriveType drive)
+{
+    switch (drive) {
+    case FDRIVE_DRV_120:
+        return FDRIVE_SIZE_525;
+    case FDRIVE_DRV_144:
+    case FDRIVE_DRV_288:
+        return FDRIVE_SIZE_350;
+    default:
+        return -1;
+    }
+}
+
 #define GET_CUR_DRV(fdctrl) ((fdctrl)->cur_drv)
 #define SET_CUR_DRV(fdctrl, drive) ((fdctrl)->cur_drv = (drive))
 
-- 
2.1.0

[Qemu-devel] [RFC 07/10] fdc: add disk field

[John Snow]

Allows us to distinguish between the current DISK type and the current
DRIVE type. The DRIVE is what's reported to CMOS, the DISK is whatever
the revalidate function suspects has been inserted into the drive.

The 'drive' field maintains the same meaning as it did previously, however,
the revalidate function will no longer update the "drive" field once it has
been changed away from FDRIVE_DRV_NONE, opting to only update the "disk" field
instead.

This should have no impact on the current pick_geometry algorithm, because
it is already incapable of choosing a new drive type after initial setup.
Thus, as of this patch, disk and drive will always be the same.

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.

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

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 6e5f87b..92c81eb 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -154,7 +154,8 @@ typedef struct FDrive {
     FDCtrl *fdctrl;
     BlockBackend *blk;
     /* Drive status */
-    FDriveType drive;
+    FDriveType drive;         /* CMOS drive type */
+    FDriveType disk;          /* Current disk type */
     uint8_t perpendicular;    /* 2.88 MB access mode    */
     /* Position */
     uint8_t head;
@@ -174,6 +175,7 @@ static void fd_init(FDrive *drv)
 {
     /* Drive */
     drv->drive = FDRIVE_DRV_NONE;
+    drv->disk = FDRIVE_DRV_NONE;
     drv->perpendicular = 0;
     /* Disk */
     drv->last_sect = 0;
@@ -316,7 +318,10 @@ static void pick_geometry(FDrive *drv, int *nb_heads)
     *nb_heads = parse->max_head + 1;
     drv->max_track = parse->max_track;
     drv->last_sect = parse->last_sect;
-    drv->drive = parse->drive;
+    if (drv->drive == FDRIVE_DRV_NONE) {
+        drv->drive = parse->drive;
+    }
+    drv->disk = parse->drive;
     drv->media_rate = parse->rate;
 }
 
-- 
2.1.0

[Qemu-devel] [RFC 08/10] fdc: refactor 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 heurstic.

This patch attempts to leave all previously working use cases working
identically to how they did before. However, because we can now change
diskette types at runtime, there is now an early return pathway if there
is no diskette to prevent revalidation. We opt instead to leave all
previously known values the same as they were.

This will cause changes in behavior for previous cases where a user with
a 1.44MB floppy drive removed a 20 sector diskette. The old behavior will
revert the CHS to 80/18/2 instead of the previous 80/20/2. This should
hopefully not perturb any guests.

This was changed for this usage case: A 2.88MB drive has a 1.44MB diskette
inserted. The diskette is ejected. Revalidate occurs and changes the CHS
values *and the rate* back to the 2.88MB default. Commands like READ_ID
will now fail unless the user re-programs the rate.

So this patch skips re-alidation for drives with no diskettes to preserve
expected behavior in e.g. fdc-test.

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

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 92c81eb..7d4206d 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -121,7 +121,6 @@ static const FDFormat fd_formats[] = {
     { FDRIVE_DRV_NONE, -1, -1, 0, 0, },
 };
 
-__attribute__((__unused__))
 static FDriveSize drive_size(FDriveType drive)
 {
     switch (drive) {
@@ -269,58 +268,70 @@ static void pick_geometry(FDrive *drv, int *nb_heads)
     BlockBackend *blk = drv->blk;
     const FDFormat *parse;
     uint64_t nb_sectors, size;
-    int i, first_match, match;
+    int i;
+    int match;
+    int size_match;
+    int type_match;
 
     /* Pick a default drive type if there's no media inserted AND we have
-     * not yet announced our drive type to the CMOS. */
+     * not yet announced our drive type to the CMOS. Otherwise, we will
+     * follow the legacy QEMU behavior of choosing a drive type based on
+     * the inserted media. */
     if (!blk_is_inserted(blk) && drv->drive == FDRIVE_DRV_NONE) {
-        for (i = 0; ; i++) {
-            parse = &fd_formats[i];
-            if ((parse->drive == FDRIVE_DRV_NONE) ||
-                (parse->drive == get_default_drive_type(drv))) {
-                break;
-            }
-        }
-        goto out;
+        /* Cement our drive type... */
+        drv->drive = get_default_drive_type(drv);
+        /* Let the loop below pick some suitable defaults */
+    } else if (!blk_is_inserted(blk)) {
+        /* Don't wipe out the previous values. */
+        return;
     }
 
     blk_get_geometry(blk, &nb_sectors);
-    match = -1;
-    first_match = -1;
+    match = size_match = type_match = -1;
+
+    /* Bear with me:
+     * 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 number of sectors,
+     * (4) 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.
+     */
     for (i = 0; ; i++) {
         parse = &fd_formats[i];
         if (parse->drive == FDRIVE_DRV_NONE) {
             break;
         }
-        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) {
-                match = i;
-                break;
+        size = (parse->max_head + 1) * parse->max_track * parse->last_sect;
+        if (nb_sectors == size) {
+            if (parse->drive == drv->drive) {
+                /* Perfect! */
+                goto out;
+            } else if (drive_size(parse->drive) == drive_size(drv->drive)) {
+                /* Probably good! */
+                match = (match == -1) ? i : match;
+            } else {
+                /* Starting to not look so great. */
+                size_match = (size_match == -1) ? i : size_match;
             }
-            if (first_match == -1) {
-                first_match = i;
+        } else if ((parse->drive == drv->drive) && type_match == -1) {
+            /* Not great, but matches legacy QEMU behavior. */
+            type_match = i;
             }
         }
-    }
+
     if (match == -1) {
-        if (first_match == -1) {
-            match = 1;
-        } else {
-            match = first_match;
+        match = (size_match != -1) ? size_match : type_match;
         }
-        parse = &fd_formats[match];
-    }
+    parse = &(fd_formats[match]);
 
  out:
     *nb_heads = parse->max_head + 1;
     drv->max_track = parse->max_track;
     drv->last_sect = parse->last_sect;
-    if (drv->drive == FDRIVE_DRV_NONE) {
-        drv->drive = parse->drive;
-    }
     drv->disk = parse->drive;
     drv->media_rate = parse->rate;
 }
@@ -336,11 +347,13 @@ static void fd_revalidate(FDrive *drv)
         pick_geometry(drv, &nb_heads);
         if (!blk_is_inserted(drv->blk)) {
             FLOPPY_DPRINTF("No disk in drive\n");
+            return;
         } else {
             FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n", nb_heads,
                            drv->max_track, drv->last_sect,
                            drv->ro ? "ro" : "rw");
         }
+        g_assert_cmpint(nb_heads, !=, -1);
         if (nb_heads == 1) {
             drv->flags &= ~FDISK_DBL_SIDES;
         } else {
-- 
2.1.0

[Qemu-devel] [RFC 09/10] 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.

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 416394f..7c47385 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.1.0

[Qemu-devel] [RFC 10/10] fdc: change default drive 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.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 7d4206d..b176aab 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -72,7 +72,7 @@ typedef struct FDFormat {
     FDriveRate rate;
 } FDFormat;
 
-#define FDRIVE_DEFAULT FDRIVE_DRV_144
+#define FDRIVE_DEFAULT FDRIVE_DRV_288
 
 static const FDFormat fd_formats[] = {
     /* First entry is default format */
-- 
2.1.0

Re: [Qemu-devel] [RFC 02/10] fdc: add default drive type option

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> We want to change the current default drive type,
> but to be kind, we need to allow users to specify
> the old drive type somehow.

Uh, what is *this* commit about?  as far as I can tell, it adds drive
type properties (not a "default drive type option"), but doesn't put
them to use, yet.

>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/block/fdc.c               | 13 +++++++++++++
>  hw/core/qdev-properties.c    | 12 ++++++++++++
>  include/hw/block/fdc.h       |  7 +------
>  include/hw/qdev-properties.h |  1 +
>  qapi/block.json              | 15 +++++++++++++++
>  5 files changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index cdf9e09..1023a01 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -67,6 +67,8 @@ typedef struct FDFormat {
>      FDriveRate rate;
>  } FDFormat;
>  
> +#define FDRIVE_DEFAULT FDRIVE_DRV_144
> +
>  static const FDFormat fd_formats[] = {
>      /* First entry is default format */
>      /* 1.44 MB 3"1/2 floppy disks */
> @@ -578,6 +580,9 @@ struct FDCtrl {
>      /* Timers state */
>      uint8_t timer0;
>      uint8_t timer1;
> +
> +    FDriveType defaultA;
> +    FDriveType defaultB;
>  };
>  
>  #define TYPE_SYSBUS_FDC "base-sysbus-fdc"
> @@ -2423,6 +2428,10 @@ 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("defaultA", FDCtrlISABus, state.defaultA,
> +                        FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType),
> +    DEFINE_PROP_DEFAULT("defaultB", FDCtrlISABus, state.defaultB,
> +                        FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -2471,6 +2480,10 @@ 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("defaultA", FDCtrlSysBus, state.defaultA,
> +                        FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType),
> +    DEFINE_PROP_DEFAULT("defaultB", FDCtrlSysBus, state.defaultB,
> +                        FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  

"defaultA" is not exactly a self-documenting name for a property
selecting the drive type.  Even equally laconic "typeA" feels better.

> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 47c1e8f..7ff8030 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -7,6 +7,7 @@
>  #include "net/hub.h"
>  #include "qapi/visitor.h"
>  #include "sysemu/char.h"
> +#include "hw/block/fdc.h"

Why do you need to add the include?

>  
>  void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
>                                    Error **errp)
> @@ -543,6 +544,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, "
> +                   "none/120/144/288",
> +    .enum_table = FDRIVE_DRV_lookup,
> +    .get = get_enum,
> +    .set = set_enum
> +};
> +
>  /* --- pci address --- */
>  
>  /*
> diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
> index d48b2f8..f727027 100644
> --- a/include/hw/block/fdc.h
> +++ b/include/hw/block/fdc.h
> @@ -6,12 +6,7 @@
>  /* 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;
> +typedef FDRIVE_DRV FDriveType;

See below.

>  
>  #define TYPE_ISA_FDC "isa-fdc"
>  
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 0cfff1c..0872b41 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;
> diff --git a/qapi/block.json b/qapi/block.json
> index aad645c..0c747a1 100644
> --- a/qapi/block.json
> +++ b/qapi/block.json
> @@ -40,6 +40,21 @@
>    'data': ['auto', 'none', 'lba', 'large', 'rechs']}
>  
>  ##
> +# FDRIVE_DRV:

Stick in the usual @, please, just for consistency.

FDRIVE_DRV is an unusual name for a QAPI type.  I guess you chose it so
only the type name changes, but the enumeration constants stay the same.
You then hide away the type name change with a typedef in fdc.h.

Clever, but I think in the longer run, I'd rather take a more
conventional type name.

> +#
> +# 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.5MB 5.25" drive
> +# @none: No drive connected
> +#
> +# Since: 2.4
> +##
> +{ 'enum': 'FDRIVE_DRV',
> +  'data': ['144', '288', '120', 'none']}
> +
> +##
>  # @BlockdevSnapshotInternal
>  #
>  # @device: the name of the device to generate the snapshot from

Re: [Qemu-devel] [RFC 02/10] fdc: add default drive type option

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> We want to change the current default drive type,
> but to be kind, we need to allow users to specify
> the old drive type somehow.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/block/fdc.c               | 13 +++++++++++++
>  hw/core/qdev-properties.c    | 12 ++++++++++++
>  include/hw/block/fdc.h       |  7 +------
>  include/hw/qdev-properties.h |  1 +
>  qapi/block.json              | 15 +++++++++++++++
>  5 files changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index cdf9e09..1023a01 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -67,6 +67,8 @@ typedef struct FDFormat {
>      FDriveRate rate;
>  } FDFormat;
>  
> +#define FDRIVE_DEFAULT FDRIVE_DRV_144
> +
>  static const FDFormat fd_formats[] = {
>      /* First entry is default format */
>      /* 1.44 MB 3"1/2 floppy disks */
> @@ -578,6 +580,9 @@ struct FDCtrl {
>      /* Timers state */
>      uint8_t timer0;
>      uint8_t timer1;
> +
> +    FDriveType defaultA;
> +    FDriveType defaultB;
>  };
>  
>  #define TYPE_SYSBUS_FDC "base-sysbus-fdc"
> @@ -2423,6 +2428,10 @@ 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("defaultA", FDCtrlISABus, state.defaultA,
> +                        FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType),
> +    DEFINE_PROP_DEFAULT("defaultB", FDCtrlISABus, state.defaultB,
> +                        FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -2471,6 +2480,10 @@ 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("defaultA", FDCtrlSysBus, state.defaultA,
> +                        FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType),
> +    DEFINE_PROP_DEFAULT("defaultB", FDCtrlSysBus, state.defaultB,
> +                        FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  

You missed sun4m_fdc_properties[].

[...]

Re: [Qemu-devel] [RFC 03/10] fdc: respect default drive type

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Respect the default drive type as proffered via the CLI.
>
> This patch overloads the "drive out" parameter of pick_geometry
> to be used as a "default hint" which is offered by fd_revalidate.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/block/fdc.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 1023a01..87fd770 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -128,7 +128,13 @@ static void pick_geometry(BlockBackend *blk, int *nb_heads,
>      /* Pick a default drive type if there's no media inserted AND we have
>       * not yet announced our drive type to the CMOS. */
>      if (!blk_is_inserted(blk) && drive_in == FDRIVE_DRV_NONE) {
> -        parse = &fd_formats[0];
> +        for (i = 0; ; i++) {
> +            parse = &fd_formats[i];
> +            if ((parse->drive == FDRIVE_DRV_NONE) ||
> +                (parse->drive == *drive)) {
> +                break;
> +            }
> +        }
>          goto out;
>      }
>  

Before the patch: if we have no medium, and drv->drive (a.k.a. drive_in)
is still FDRIVE_DRV_NONE, pick format #0 (standard 3.5" 1.44MiB).

Afterwards: pick first one matching the value of
get_default_drive_type(), i.e. the value of the new property.

So the property is really a default type, which applies only when we
start without a medium in the drive.

Is that what we want?

When I specifically ask for a 5.25" 1.2MiB drive, I'd be rather
surprised when it silently morphs into a 3.5" 2.88MiB drive just because
I've forced a funny medium in before startup.

The obvious way to do drive types is selecting one with a property,
defaulting to the most common type, i.e. standard 3.5" 1.44Mib.

To preserve backward compatibility, we need a way to say "pick one for
the medium, else pick standard, and we need to make it the default.  At
least for old machine types.

Opinions?

> @@ -295,11 +301,13 @@ static void fd_recalibrate(FDrive *drv)
>      fd_seek(drv, 0, 0, 1, 1);
>  }
>  
> +static FDriveType get_default_drive_type(FDrive *drv);
> +
>  /* Revalidate a disk drive after a disk change */
>  static void fd_revalidate(FDrive *drv)
>  {
>      int nb_heads, max_track, last_sect, ro;
> -    FDriveType drive;
> +    FDriveType drive = get_default_drive_type(drv);
>      FDriveRate rate;
>  
>      FLOPPY_DPRINTF("revalidate\n");
> @@ -609,6 +617,21 @@ typedef struct FDCtrlISABus {
>      int32_t bootindexB;
>  } FDCtrlISABus;
>  
> +static FDriveType get_default_drive_type(FDrive *drv)
> +{
> +    FDCtrl *fdctrl = drv->fdctrl;
> +
> +    if (0 < MAX_FD && (&fdctrl->drives[0] == drv)) {
> +        return fdctrl->defaultA;
> +    }
> +
> +    if (1 < MAX_FD && (&fdctrl->drives[1] == drv)) {
> +        return fdctrl->defaultB;
> +    }
> +
> +    return FDRIVE_DEFAULT;
> +}
> +
>  static uint32_t fdctrl_read (void *opaque, uint32_t reg)
>  {
>      FDCtrl *fdctrl = opaque;

Why do you need to guard with MAX_FD?  If MAX_FD < 2, surely the
properties don't exist, and fdctrl->drives[i] still has its initial
value FDRIVE_DEFAULT, doesn't it?

Re: [Qemu-devel] [RFC 05/10] fdc: refactor pick_geometry

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Lessen the number of parameters it takes.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

Haven't reviewed in detail, but: YES, please!

Re: [Qemu-devel] [RFC 06/10] fdc: add physical disk sizes

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/block/fdc.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 4004b98..6e5f87b 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -59,6 +59,11 @@ typedef enum FDriveRate {
>      FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
>  } FDriveRate;
>  
> +typedef enum FDriveSize {
> +    FDRIVE_SIZE_350,
> +    FDRIVE_SIZE_525
> +} FDriveSize;
> +
>  typedef struct FDFormat {
>      FDriveType drive;
>      uint8_t last_sect;
> @@ -95,15 +100,15 @@ static const FDFormat fd_formats[] = {
>      { FDRIVE_DRV_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, 80, 1, FDRIVE_RATE_500K, }, /* conflicts w/ #0 */
>      { 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, },
> +    { FDRIVE_DRV_120, 20, 80, 1, FDRIVE_RATE_500K, }, /* conflicts w/ #1 */
>      /* 720 kB 5"1/4 floppy disks */
> -    { FDRIVE_DRV_120,  9, 80, 1, FDRIVE_RATE_250K, },
> +    { FDRIVE_DRV_120,  9, 80, 1, FDRIVE_RATE_250K, }, /* conflicts w/ #13 */
>      { FDRIVE_DRV_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, 1, FDRIVE_RATE_300K, }, /* conflicts w/ #32 */
>      { 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, },

Can you elaborate on what "conflicts w/" means here?

> @@ -116,6 +121,20 @@ static const FDFormat fd_formats[] = {
>      { FDRIVE_DRV_NONE, -1, -1, 0, 0, },
>  };
>  
> +__attribute__((__unused__))
> +static FDriveSize drive_size(FDriveType drive)
> +{
> +    switch (drive) {
> +    case FDRIVE_DRV_120:
> +        return FDRIVE_SIZE_525;
> +    case FDRIVE_DRV_144:
> +    case FDRIVE_DRV_288:
> +        return FDRIVE_SIZE_350;
> +    default:
> +        return -1;

Works, but isn't so nice.  When I see a function returning an enum type,
I expect the return value to be one of its enum constants.

You can either return int, or add the error value to the enum type.

> +    }
> +}
> +
>  #define GET_CUR_DRV(fdctrl) ((fdctrl)->cur_drv)
>  #define SET_CUR_DRV(fdctrl, drive) ((fdctrl)->cur_drv = (drive))

Re: [Qemu-devel] [RFC 08/10] fdc: refactor pick_geometry

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This one is the crazy one.

I'm afraid I don't have the mental capacity to properly review this one
right now.

>From what I've understood of your series so far, it's a strict decrease
of fdc craziness.  Thanks!

Re: [Qemu-devel] [RFC 02/10] fdc: add default drive type option

[John Snow]

On 07/03/2015 09:18 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> We want to change the current default drive type,
>> but to be kind, we need to allow users to specify
>> the old drive type somehow.
> 
> Uh, what is *this* commit about?  as far as I can tell, it adds drive
> type properties (not a "default drive type option"), but doesn't put
> them to use, yet.
> 

Lost the refactor lottery and didn't update all of my initial commit
messages. *ahem* Sorry.

>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  hw/block/fdc.c               | 13 +++++++++++++
>>  hw/core/qdev-properties.c    | 12 ++++++++++++
>>  include/hw/block/fdc.h       |  7 +------
>>  include/hw/qdev-properties.h |  1 +
>>  qapi/block.json              | 15 +++++++++++++++
>>  5 files changed, 42 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index cdf9e09..1023a01 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -67,6 +67,8 @@ typedef struct FDFormat {
>>      FDriveRate rate;
>>  } FDFormat;
>>  
>> +#define FDRIVE_DEFAULT FDRIVE_DRV_144
>> +
>>  static const FDFormat fd_formats[] = {
>>      /* First entry is default format */
>>      /* 1.44 MB 3"1/2 floppy disks */
>> @@ -578,6 +580,9 @@ struct FDCtrl {
>>      /* Timers state */
>>      uint8_t timer0;
>>      uint8_t timer1;
>> +
>> +    FDriveType defaultA;
>> +    FDriveType defaultB;
>>  };
>>  
>>  #define TYPE_SYSBUS_FDC "base-sysbus-fdc"
>> @@ -2423,6 +2428,10 @@ 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("defaultA", FDCtrlISABus, state.defaultA,
>> +                        FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType),
>> +    DEFINE_PROP_DEFAULT("defaultB", FDCtrlISABus, state.defaultB,
>> +                        FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>  
>> @@ -2471,6 +2480,10 @@ 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("defaultA", FDCtrlSysBus, state.defaultA,
>> +                        FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType),
>> +    DEFINE_PROP_DEFAULT("defaultB", FDCtrlSysBus, state.defaultB,
>> +                        FDRIVE_DEFAULT, qdev_prop_fdc_drive_type, FDriveType),
>>      DEFINE_PROP_END_OF_LIST(),
>>  };
>>  
> 
> "defaultA" is not exactly a self-documenting name for a property
> selecting the drive type.  Even equally laconic "typeA" feels better.
> 
>> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
>> index 47c1e8f..7ff8030 100644
>> --- a/hw/core/qdev-properties.c
>> +++ b/hw/core/qdev-properties.c
>> @@ -7,6 +7,7 @@
>>  #include "net/hub.h"
>>  #include "qapi/visitor.h"
>>  #include "sysemu/char.h"
>> +#include "hw/block/fdc.h"
> 
> Why do you need to add the include?
> 

... Ghosts from iterations past.

>>  
>>  void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
>>                                    Error **errp)
>> @@ -543,6 +544,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, "
>> +                   "none/120/144/288",
>> +    .enum_table = FDRIVE_DRV_lookup,
>> +    .get = get_enum,
>> +    .set = set_enum
>> +};
>> +
>>  /* --- pci address --- */
>>  
>>  /*
>> diff --git a/include/hw/block/fdc.h b/include/hw/block/fdc.h
>> index d48b2f8..f727027 100644
>> --- a/include/hw/block/fdc.h
>> +++ b/include/hw/block/fdc.h
>> @@ -6,12 +6,7 @@
>>  /* 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;
>> +typedef FDRIVE_DRV FDriveType;
> 
> See below.
> 
>>  
>>  #define TYPE_ISA_FDC "isa-fdc"
>>  
>> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
>> index 0cfff1c..0872b41 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;
>> diff --git a/qapi/block.json b/qapi/block.json
>> index aad645c..0c747a1 100644
>> --- a/qapi/block.json
>> +++ b/qapi/block.json
>> @@ -40,6 +40,21 @@
>>    'data': ['auto', 'none', 'lba', 'large', 'rechs']}
>>  
>>  ##
>> +# FDRIVE_DRV:
> 
> Stick in the usual @, please, just for consistency.
> 
> FDRIVE_DRV is an unusual name for a QAPI type.  I guess you chose it so
> only the type name changes, but the enumeration constants stay the same.
> You then hide away the type name change with a typedef in fdc.h.
> 
> Clever, but I think in the longer run, I'd rather take a more
> conventional type name.
> 

If the rest of the series can be polished, I'll add a more traditional
enum. This was more or less a quick PoC hack to avoid a lot of churn.

>> +#
>> +# 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.5MB 5.25" drive
>> +# @none: No drive connected
>> +#
>> +# Since: 2.4
>> +##
>> +{ 'enum': 'FDRIVE_DRV',
>> +  'data': ['144', '288', '120', 'none']}
>> +
>> +##
>>  # @BlockdevSnapshotInternal
>>  #
>>  # @device: the name of the device to generate the snapshot from

Re: [Qemu-devel] [RFC 06/10] fdc: add physical disk sizes

[John Snow]

On 07/03/2015 09:38 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> 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.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  hw/block/fdc.c | 27 +++++++++++++++++++++++----
>>  1 file changed, 23 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index 4004b98..6e5f87b 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -59,6 +59,11 @@ typedef enum FDriveRate {
>>      FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
>>  } FDriveRate;
>>  
>> +typedef enum FDriveSize {
>> +    FDRIVE_SIZE_350,
>> +    FDRIVE_SIZE_525
>> +} FDriveSize;
>> +
>>  typedef struct FDFormat {
>>      FDriveType drive;
>>      uint8_t last_sect;
>> @@ -95,15 +100,15 @@ static const FDFormat fd_formats[] = {
>>      { FDRIVE_DRV_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, 80, 1, FDRIVE_RATE_500K, }, /* conflicts w/ #0 */
>>      { 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, },
>> +    { FDRIVE_DRV_120, 20, 80, 1, FDRIVE_RATE_500K, }, /* conflicts w/ #1 */
>>      /* 720 kB 5"1/4 floppy disks */
>> -    { FDRIVE_DRV_120,  9, 80, 1, FDRIVE_RATE_250K, },
>> +    { FDRIVE_DRV_120,  9, 80, 1, FDRIVE_RATE_250K, }, /* conflicts w/ #13 */
>>      { FDRIVE_DRV_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, 1, FDRIVE_RATE_300K, }, /* conflicts w/ #32 */
>>      { 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, },
> 
> Can you elaborate on what "conflicts w/" means here?
> 

Bad documentation, yes.

The "conflict" here is that the number of sectors described by this
format collides with another. It's not clear by glancing at the table
which ones conflict.

The physical media size can be used to disambiguate these cases.

>> @@ -116,6 +121,20 @@ static const FDFormat fd_formats[] = {
>>      { FDRIVE_DRV_NONE, -1, -1, 0, 0, },
>>  };
>>  
>> +__attribute__((__unused__))
>> +static FDriveSize drive_size(FDriveType drive)
>> +{
>> +    switch (drive) {
>> +    case FDRIVE_DRV_120:
>> +        return FDRIVE_SIZE_525;
>> +    case FDRIVE_DRV_144:
>> +    case FDRIVE_DRV_288:
>> +        return FDRIVE_SIZE_350;
>> +    default:
>> +        return -1;
> 
> Works, but isn't so nice.  When I see a function returning an enum type,
> I expect the return value to be one of its enum constants.
> 
> You can either return int, or add the error value to the enum type.
> 

I think I'll add an error case to the enum. FDRIVE_SIZE_INVALID, etc.

>> +    }
>> +}
>> +
>>  #define GET_CUR_DRV(fdctrl) ((fdctrl)->cur_drv)
>>  #define SET_CUR_DRV(fdctrl, drive) ((fdctrl)->cur_drv = (drive))

Thanks!

Re: [Qemu-devel] [RFC 08/10] fdc: refactor pick_geometry

[John Snow]

On 07/03/2015 09:45 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> This one is the crazy one.
> 
> I'm afraid I don't have the mental capacity to properly review this one
> right now.
> 
> From what I've understood of your series so far, it's a strict decrease
> of fdc craziness.  Thanks!
> 

Yup, this is the one that definitely changes the behavior a little, and
the hard part in reviewing this is (I hope) understanding how the old
code works.

Hopefully the new version is a little more explicit in how it guesses.

Thanks again for looking.

--js

Re: [Qemu-devel] [RFC 03/10] fdc: respect default drive type

[John Snow]

On 07/03/2015 09:34 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Respect the default drive type as proffered via the CLI.
>>
>> This patch overloads the "drive out" parameter of pick_geometry
>> to be used as a "default hint" which is offered by fd_revalidate.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  hw/block/fdc.c | 27 +++++++++++++++++++++++++--
>>  1 file changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index 1023a01..87fd770 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -128,7 +128,13 @@ static void pick_geometry(BlockBackend *blk, int *nb_heads,
>>      /* Pick a default drive type if there's no media inserted AND we have
>>       * not yet announced our drive type to the CMOS. */
>>      if (!blk_is_inserted(blk) && drive_in == FDRIVE_DRV_NONE) {
>> -        parse = &fd_formats[0];
>> +        for (i = 0; ; i++) {
>> +            parse = &fd_formats[i];
>> +            if ((parse->drive == FDRIVE_DRV_NONE) ||
>> +                (parse->drive == *drive)) {
>> +                break;
>> +            }
>> +        }
>>          goto out;
>>      }
>>  
> 
> Before the patch: if we have no medium, and drv->drive (a.k.a. drive_in)
> is still FDRIVE_DRV_NONE, pick format #0 (standard 3.5" 1.44MiB).
> 

My commit message is actually wrong, the code actually picks format #1,
but that's only useful for choosing the drive type while there's no
floppy. As soon as one is inserted it will re-validate to the closest
1.44MB type.

> Afterwards: pick first one matching the value of
> get_default_drive_type(), i.e. the value of the new property.
> 
> So the property is really a default type, which applies only when we
> start without a medium in the drive.
> 
> Is that what we want?
> 

It is a "minimal" change that just allows you to configure what it
defaults back to. It's a 'weak' setting.

Was that a good call? hmm...

> When I specifically ask for a 5.25" 1.2MiB drive, I'd be rather
> surprised when it silently morphs into a 3.5" 2.88MiB drive just because
> I've forced a funny medium in before startup.
> 

Ah, here it is. I can just add "typeA" and "typeB" properties instead of
defaultA/B, and force the drive into that role.

> The obvious way to do drive types is selecting one with a property,
> defaulting to the most common type, i.e. standard 3.5" 1.44Mib.
> 

Hm?

Not sure I follow, but the goal here is to use the 2.88MB type, because
it can accept both kinds of diskettes, so it has the greatest
compatibility for floppy images (including the virtio driver diskette
which is a 2.88MB type.)

The problem is the 1.44MB drive type and the current inflexibility of
the revalidate+pick_geometry algorithm that doesn't allow you to change
from 1.44 to 2.88 or vice versa.

> To preserve backward compatibility, we need a way to say "pick one for
> the medium, else pick standard, and we need to make it the default.  At
> least for old machine types.
> 
> Opinions?
> 

Machines prior to (let's say 2.5 here) should use something close to the
old behavior: "Choose 1.44MB if no diskette, pick the best match (by
sector count) otherwise."

New machines apply nearly the same behavior, except they opt for 2.88MB
as the default.

New properties allow users to override the default-selection behavior
and/or just force a drive type.

>> @@ -295,11 +301,13 @@ static void fd_recalibrate(FDrive *drv)
>>      fd_seek(drv, 0, 0, 1, 1);
>>  }
>>  
>> +static FDriveType get_default_drive_type(FDrive *drv);
>> +
>>  /* Revalidate a disk drive after a disk change */
>>  static void fd_revalidate(FDrive *drv)
>>  {
>>      int nb_heads, max_track, last_sect, ro;
>> -    FDriveType drive;
>> +    FDriveType drive = get_default_drive_type(drv);
>>      FDriveRate rate;
>>  
>>      FLOPPY_DPRINTF("revalidate\n");
>> @@ -609,6 +617,21 @@ typedef struct FDCtrlISABus {
>>      int32_t bootindexB;
>>  } FDCtrlISABus;
>>  
>> +static FDriveType get_default_drive_type(FDrive *drv)
>> +{
>> +    FDCtrl *fdctrl = drv->fdctrl;
>> +
>> +    if (0 < MAX_FD && (&fdctrl->drives[0] == drv)) {
>> +        return fdctrl->defaultA;
>> +    }
>> +
>> +    if (1 < MAX_FD && (&fdctrl->drives[1] == drv)) {
>> +        return fdctrl->defaultB;
>> +    }
>> +
>> +    return FDRIVE_DEFAULT;
>> +}
>> +
>>  static uint32_t fdctrl_read (void *opaque, uint32_t reg)
>>  {
>>      FDCtrl *fdctrl = opaque;
> 
> Why do you need to guard with MAX_FD?  If MAX_FD < 2, surely the
> properties don't exist, and fdctrl->drives[i] still has its initial
> value FDRIVE_DEFAULT, doesn't it?
> 

Why would the properties not exist?

Re: [Qemu-devel] [RFC 03/10] fdc: respect default drive type

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 07/03/2015 09:34 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> Respect the default drive type as proffered via the CLI.
>>>
>>> This patch overloads the "drive out" parameter of pick_geometry
>>> to be used as a "default hint" which is offered by fd_revalidate.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  hw/block/fdc.c | 27 +++++++++++++++++++++++++--
>>>  1 file changed, 25 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>> index 1023a01..87fd770 100644
>>> --- a/hw/block/fdc.c
>>> +++ b/hw/block/fdc.c
>>> @@ -128,7 +128,13 @@ static void pick_geometry(BlockBackend *blk, int *nb_heads,
>>>      /* Pick a default drive type if there's no media inserted AND we have
>>>       * not yet announced our drive type to the CMOS. */
>>>      if (!blk_is_inserted(blk) && drive_in == FDRIVE_DRV_NONE) {
>>> -        parse = &fd_formats[0];
>>> +        for (i = 0; ; i++) {
>>> +            parse = &fd_formats[i];
>>> +            if ((parse->drive == FDRIVE_DRV_NONE) ||
>>> +                (parse->drive == *drive)) {
>>> +                break;
>>> +            }
>>> +        }
>>>          goto out;
>>>      }
>>>  
>> 
>> Before the patch: if we have no medium, and drv->drive (a.k.a. drive_in)
>> is still FDRIVE_DRV_NONE, pick format #0 (standard 3.5" 1.44MiB).
>> 
>
> My commit message is actually wrong, the code actually picks format #1,

Now I'm confused: &fd_formats[0] looks like format #0 to me.

> but that's only useful for choosing the drive type while there's no
> floppy. As soon as one is inserted it will re-validate to the closest
> 1.44MB type.
>
>> Afterwards: pick first one matching the value of
>> get_default_drive_type(), i.e. the value of the new property.
>> 
>> So the property is really a default type, which applies only when we
>> start without a medium in the drive.
>> 
>> Is that what we want?
>> 
>
> It is a "minimal" change that just allows you to configure what it
> defaults back to. It's a 'weak' setting.
>
> Was that a good call? hmm...
>
>> When I specifically ask for a 5.25" 1.2MiB drive, I'd be rather
>> surprised when it silently morphs into a 3.5" 2.88MiB drive just because
>> I've forced a funny medium in before startup.
>> 
>
> Ah, here it is. I can just add "typeA" and "typeB" properties instead of
> defaultA/B, and force the drive into that role.
>
>> The obvious way to do drive types is selecting one with a property,
>> defaulting to the most common type, i.e. standard 3.5" 1.44Mib.
>> 
>
> Hm?
>
> Not sure I follow, but the goal here is to use the 2.88MB type, because
> it can accept both kinds of diskettes, so it has the greatest
> compatibility for floppy images (including the virtio driver diskette
> which is a 2.88MB type.)

The 2.88 type may well be a more useful default, because it takes a
strict superset of media.  On the other hand, it puts a different value
into the CMOS floppy byte, which could conceivably confuse guests that
haven't been updated for this kind of high-tech gadget.  I'm not telling
you what default to pick, only what the tradeoffs are.

> The problem is the 1.44MB drive type and the current inflexibility of
> the revalidate+pick_geometry algorithm that doesn't allow you to change
> from 1.44 to 2.88 or vice versa.

Well, you can't change physical drives from 1.44 to 2.88 or vice versa,
either.

>> To preserve backward compatibility, we need a way to say "pick one for
>> the medium, else pick standard, and we need to make it the default.  At
>> least for old machine types.
>> 
>> Opinions?
>> 
>
> Machines prior to (let's say 2.5 here) should use something close to the
> old behavior: "Choose 1.44MB if no diskette, pick the best match (by
> sector count) otherwise."

If you want the new, saner behavior with and old machine type, pick the
appropriate type, e.g. say type=288.

> New machines apply nearly the same behavior, except they opt for 2.88MB
> as the default.
>
> New properties allow users to override the default-selection behavior
> and/or just force a drive type.

Possible alternative for new machines: default to 2.88 type, done.

If you want the old behavior with a new machine type then, you get to
specify type=magic or something.

Matter of UI taste.

>>> @@ -295,11 +301,13 @@ static void fd_recalibrate(FDrive *drv)
>>>      fd_seek(drv, 0, 0, 1, 1);
>>>  }
>>>  
>>> +static FDriveType get_default_drive_type(FDrive *drv);
>>> +
>>>  /* Revalidate a disk drive after a disk change */
>>>  static void fd_revalidate(FDrive *drv)
>>>  {
>>>      int nb_heads, max_track, last_sect, ro;
>>> -    FDriveType drive;
>>> +    FDriveType drive = get_default_drive_type(drv);
>>>      FDriveRate rate;
>>>  
>>>      FLOPPY_DPRINTF("revalidate\n");
>>> @@ -609,6 +617,21 @@ typedef struct FDCtrlISABus {
>>>      int32_t bootindexB;
>>>  } FDCtrlISABus;
>>>  
>>> +static FDriveType get_default_drive_type(FDrive *drv)
>>> +{
>>> +    FDCtrl *fdctrl = drv->fdctrl;
>>> +
>>> +    if (0 < MAX_FD && (&fdctrl->drives[0] == drv)) {
>>> +        return fdctrl->defaultA;
>>> +    }
>>> +
>>> +    if (1 < MAX_FD && (&fdctrl->drives[1] == drv)) {
>>> +        return fdctrl->defaultB;
>>> +    }
>>> +
>>> +    return FDRIVE_DEFAULT;
>>> +}
>>> +
>>>  static uint32_t fdctrl_read (void *opaque, uint32_t reg)
>>>  {
>>>      FDCtrl *fdctrl = opaque;
>> 
>> Why do you need to guard with MAX_FD?  If MAX_FD < 2, surely the
>> properties don't exist, and fdctrl->drives[i] still has its initial
>> value FDRIVE_DEFAULT, doesn't it?
>> 
>
> Why would the properties not exist?

Before I answer your question: currently, MAX_FD is always 2, so the
question is moot.  The #ifdeffery in fdc.c suggests the code is also
prepared for MAX_FD 4, but no other values.

Now your question.  MAX_FD limits the number of drives the controller
supports.  Surely the controller's QOM/qdev interface shouldn't expose
more drives than you can actually connect.

"isa-fdc" and "sysbus-fdc" support two, "driveA" and "driveB", mapping
to state.drives[0..1].  "SUNW,fdtwo" supports one "drive", mapping to
state.drives[0].

Note that state.drives[] has MAX_FD elements.  If we changed MAX_FD to
one, we'd have to drop "isa-fdc"'s and "sysbus-fdc"'s "driveB", or else
they'd overrun state.drives[].

Re: [Qemu-devel] [RFC 02/10] fdc: add default drive type option

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 07/03/2015 09:18 AM, Markus Armbruster wrote:
[...]
>>> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
>>> index 0cfff1c..0872b41 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;
>>> diff --git a/qapi/block.json b/qapi/block.json
>>> index aad645c..0c747a1 100644
>>> --- a/qapi/block.json
>>> +++ b/qapi/block.json
>>> @@ -40,6 +40,21 @@
>>>    'data': ['auto', 'none', 'lba', 'large', 'rechs']}
>>>  
>>>  ##
>>> +# FDRIVE_DRV:
>> 
>> Stick in the usual @, please, just for consistency.
>> 
>> FDRIVE_DRV is an unusual name for a QAPI type.  I guess you chose it so
>> only the type name changes, but the enumeration constants stay the same.
>> You then hide away the type name change with a typedef in fdc.h.
>> 
>> Clever, but I think in the longer run, I'd rather take a more
>> conventional type name.
>> 
>
> If the rest of the series can be polished, I'll add a more traditional
> enum. This was more or less a quick PoC hack to avoid a lot of churn.

Makes sense, actually :)

[...]

Re: [Qemu-devel] [RFC 10/10] fdc: change default drive to 288

[Kevin O'Connor]

On Tue, Jun 30, 2015 at 09:20:40PM -0400, John Snow wrote:
> 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.

On real machines, 1.44MB floppy drives were very common.  It was
exceptionally rare to see 2.88MB floppy drives though.  There is a
risk that changing the default to an exotic piece of hardware will
expose quirks in guest Operating Systems.

-Kevin

Re: [Qemu-devel] [RFC 10/10] fdc: change default drive to 288

[John Snow]

On 07/05/2015 10:53 AM, Kevin O'Connor wrote:
> On Tue, Jun 30, 2015 at 09:20:40PM -0400, John Snow wrote:
>> 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.
> 
> On real machines, 1.44MB floppy drives were very common.  It was
> exceptionally rare to see 2.88MB floppy drives though.  There is a
> risk that changing the default to an exotic piece of hardware will
> expose quirks in guest Operating Systems.
> 
> -Kevin
> 

Definitely.

Which is why I do want to add some properties to allow users to help
guide the type of floppy drive they get. 2.88MB might well be a saner
default for when no diskette is inserted (It allows for, I believe, the
widest choice of likely types to be inserted later.)

Windows (MSDOS through Windows8) and Linux both seem completely fine,
which I think represents the lion's share of likely guests.

For other guests, you can always tweak the CLI to provide a drive type
the guest can use.

Re: [Qemu-devel] [RFC 03/10] fdc: respect default drive type

[John Snow]

On 07/04/2015 02:47 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 07/03/2015 09:34 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> Respect the default drive type as proffered via the CLI.
>>>>
>>>> This patch overloads the "drive out" parameter of pick_geometry
>>>> to be used as a "default hint" which is offered by fd_revalidate.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>>  hw/block/fdc.c | 27 +++++++++++++++++++++++++--
>>>>  1 file changed, 25 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>>> index 1023a01..87fd770 100644
>>>> --- a/hw/block/fdc.c
>>>> +++ b/hw/block/fdc.c
>>>> @@ -128,7 +128,13 @@ static void pick_geometry(BlockBackend *blk, int *nb_heads,
>>>>      /* Pick a default drive type if there's no media inserted AND we have
>>>>       * not yet announced our drive type to the CMOS. */
>>>>      if (!blk_is_inserted(blk) && drive_in == FDRIVE_DRV_NONE) {
>>>> -        parse = &fd_formats[0];
>>>> +        for (i = 0; ; i++) {
>>>> +            parse = &fd_formats[i];
>>>> +            if ((parse->drive == FDRIVE_DRV_NONE) ||
>>>> +                (parse->drive == *drive)) {
>>>> +                break;
>>>> +            }
>>>> +        }
>>>>          goto out;
>>>>      }
>>>>  
>>>
>>> Before the patch: if we have no medium, and drv->drive (a.k.a. drive_in)
>>> is still FDRIVE_DRV_NONE, pick format #0 (standard 3.5" 1.44MiB).
>>>
>>
>> My commit message is actually wrong, the code actually picks format #1,
> 
> Now I'm confused: &fd_formats[0] looks like format #0 to me.
> 

It's this bit here (prior to this RFC):

    if (match == -1) {
        if (first_match == -1) {
            match = 1;

If we found absolutely nothing, we choose the "first" entry, literally
&fd_formats[1]. But it doesn't really matter, 1.44MB is 1.44MB as far as
CMOS cares.

>> but that's only useful for choosing the drive type while there's no
>> floppy. As soon as one is inserted it will re-validate to the closest
>> 1.44MB type.
>>
>>> Afterwards: pick first one matching the value of
>>> get_default_drive_type(), i.e. the value of the new property.
>>>
>>> So the property is really a default type, which applies only when we
>>> start without a medium in the drive.
>>>
>>> Is that what we want?
>>>
>>
>> It is a "minimal" change that just allows you to configure what it
>> defaults back to. It's a 'weak' setting.
>>
>> Was that a good call? hmm...
>>
>>> When I specifically ask for a 5.25" 1.2MiB drive, I'd be rather
>>> surprised when it silently morphs into a 3.5" 2.88MiB drive just because
>>> I've forced a funny medium in before startup.
>>>
>>
>> Ah, here it is. I can just add "typeA" and "typeB" properties instead of
>> defaultA/B, and force the drive into that role.
>>
>>> The obvious way to do drive types is selecting one with a property,
>>> defaulting to the most common type, i.e. standard 3.5" 1.44Mib.
>>>
>>
>> Hm?
>>
>> Not sure I follow, but the goal here is to use the 2.88MB type, because
>> it can accept both kinds of diskettes, so it has the greatest
>> compatibility for floppy images (including the virtio driver diskette
>> which is a 2.88MB type.)
> 
> The 2.88 type may well be a more useful default, because it takes a
> strict superset of media.  On the other hand, it puts a different value
> into the CMOS floppy byte, which could conceivably confuse guests that
> haven't been updated for this kind of high-tech gadget.  I'm not telling
> you what default to pick, only what the tradeoffs are.
> 

Partly why I wanted to preserve the "Pick a drive type based on the
media" approach to maximize compatibility where possible. I may well end
up hurting some compatibility in some places when you boot without a
diskette, but MSDOS, Windows 7, 8, and Fedora all seemed happy, so I was
happy.

>> The problem is the 1.44MB drive type and the current inflexibility of
>> the revalidate+pick_geometry algorithm that doesn't allow you to change
>> from 1.44 to 2.88 or vice versa.
> 
> Well, you can't change physical drives from 1.44 to 2.88 or vice versa,
> either.
> 

You can change diskettes, though. The code confuses drive types with
diskette types. The static data table we have describes both.

Initial validation controls what nibble we put in CMOS. secondary
validations actually only change the CHS and rotation rate variables. So
the first validation controls /drive/ type and secondary validations
control /diskette/ type.

The way it works now is that once you choose your *drive* type, you
cannot change the *diskette*, but yes in the real world you can put a
1.44MB into a 2.88MB capable drive (I think? The emulator seems happy
with it...!)

>>> To preserve backward compatibility, we need a way to say "pick one for
>>> the medium, else pick standard, and we need to make it the default.  At
>>> least for old machine types.
>>>
>>> Opinions?
>>>
>>
>> Machines prior to (let's say 2.5 here) should use something close to the
>> old behavior: "Choose 1.44MB if no diskette, pick the best match (by
>> sector count) otherwise."
> 
> If you want the new, saner behavior with and old machine type, pick the
> appropriate type, e.g. say type=288.
> 

Yup.

>> New machines apply nearly the same behavior, except they opt for 2.88MB
>> as the default.
>>
>> New properties allow users to override the default-selection behavior
>> and/or just force a drive type.
> 
> Possible alternative for new machines: default to 2.88 type, done.
> 

Are you suggesting we ditch the diskette heuristic for picking drive
type altogether in new machine types?

I wouldn't mind, per se, since it's more mindful and explicit, but I
think I have the capacity to break a lot of user scripts with that.

> If you want the old behavior with a new machine type then, you get to
> specify type=magic or something.
> 
> Matter of UI taste.
> 

Hm.

>>>> @@ -295,11 +301,13 @@ static void fd_recalibrate(FDrive *drv)
>>>>      fd_seek(drv, 0, 0, 1, 1);
>>>>  }
>>>>  
>>>> +static FDriveType get_default_drive_type(FDrive *drv);
>>>> +
>>>>  /* Revalidate a disk drive after a disk change */
>>>>  static void fd_revalidate(FDrive *drv)
>>>>  {
>>>>      int nb_heads, max_track, last_sect, ro;
>>>> -    FDriveType drive;
>>>> +    FDriveType drive = get_default_drive_type(drv);
>>>>      FDriveRate rate;
>>>>  
>>>>      FLOPPY_DPRINTF("revalidate\n");
>>>> @@ -609,6 +617,21 @@ typedef struct FDCtrlISABus {
>>>>      int32_t bootindexB;
>>>>  } FDCtrlISABus;
>>>>  
>>>> +static FDriveType get_default_drive_type(FDrive *drv)
>>>> +{
>>>> +    FDCtrl *fdctrl = drv->fdctrl;
>>>> +
>>>> +    if (0 < MAX_FD && (&fdctrl->drives[0] == drv)) {
>>>> +        return fdctrl->defaultA;
>>>> +    }
>>>> +
>>>> +    if (1 < MAX_FD && (&fdctrl->drives[1] == drv)) {
>>>> +        return fdctrl->defaultB;
>>>> +    }
>>>> +
>>>> +    return FDRIVE_DEFAULT;
>>>> +}
>>>> +
>>>>  static uint32_t fdctrl_read (void *opaque, uint32_t reg)
>>>>  {
>>>>      FDCtrl *fdctrl = opaque;
>>>
>>> Why do you need to guard with MAX_FD?  If MAX_FD < 2, surely the
>>> properties don't exist, and fdctrl->drives[i] still has its initial
>>> value FDRIVE_DEFAULT, doesn't it?
>>>
>>
>> Why would the properties not exist?
> 
> Before I answer your question: currently, MAX_FD is always 2, so the
> question is moot.  The #ifdeffery in fdc.c suggests the code is also
> prepared for MAX_FD 4, but no other values.
> 
> Now your question.  MAX_FD limits the number of drives the controller
> supports.  Surely the controller's QOM/qdev interface shouldn't expose
> more drives than you can actually connect.
> 
> "isa-fdc" and "sysbus-fdc" support two, "driveA" and "driveB", mapping
> to state.drives[0..1].  "SUNW,fdtwo" supports one "drive", mapping to
> state.drives[0].
> 
> Note that state.drives[] has MAX_FD elements.  If we changed MAX_FD to
> one, we'd have to drop "isa-fdc"'s and "sysbus-fdc"'s "driveB", or else
> they'd overrun state.drives[].
> 

OK, you mean "Surely if MAX_FD is ever not two, then we will also remove
e.g. driveB" -- I was thinking there was some kind of magic.

Anyway, yes. I'll replace the weird logic with just an assertion. If
anyone does change MAX_FD they'll get a "helpful reminder" that this
function might also need to change.

In summary, your recommendations are:
- Try using 'typeA' and 'typeB' to force drive type instead of
defaultA/defaultB to set preferences for the default magic type
- Use a type of 'magic' to perform autodetection if that behavior is
desired.

Yeah?
--js

Re: [Qemu-devel] [RFC 03/10] fdc: respect default drive type

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 07/04/2015 02:47 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> On 07/03/2015 09:34 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> Respect the default drive type as proffered via the CLI.
>>>>>
>>>>> This patch overloads the "drive out" parameter of pick_geometry
>>>>> to be used as a "default hint" which is offered by fd_revalidate.
>>>>>
>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>> ---
>>>>>  hw/block/fdc.c | 27 +++++++++++++++++++++++++--
>>>>>  1 file changed, 25 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>>>>> index 1023a01..87fd770 100644
>>>>> --- a/hw/block/fdc.c
>>>>> +++ b/hw/block/fdc.c
>>>>> @@ -128,7 +128,13 @@ static void pick_geometry(BlockBackend *blk, int *nb_heads,
>>>>>      /* Pick a default drive type if there's no media inserted AND we have
>>>>>       * not yet announced our drive type to the CMOS. */
>>>>>      if (!blk_is_inserted(blk) && drive_in == FDRIVE_DRV_NONE) {
>>>>> -        parse = &fd_formats[0];
>>>>> +        for (i = 0; ; i++) {
>>>>> +            parse = &fd_formats[i];
>>>>> +            if ((parse->drive == FDRIVE_DRV_NONE) ||
>>>>> +                (parse->drive == *drive)) {
>>>>> +                break;
>>>>> +            }
>>>>> +        }
>>>>>          goto out;
>>>>>      }
>>>>>  
>>>>
>>>> Before the patch: if we have no medium, and drv->drive (a.k.a. drive_in)
>>>> is still FDRIVE_DRV_NONE, pick format #0 (standard 3.5" 1.44MiB).
>>>>
>>>
>>> My commit message is actually wrong, the code actually picks format #1,
>> 
>> Now I'm confused: &fd_formats[0] looks like format #0 to me.
>> 
>
> It's this bit here (prior to this RFC):
>
>     if (match == -1) {
>         if (first_match == -1) {
>             match = 1;
>
> If we found absolutely nothing, we choose the "first" entry, literally
> &fd_formats[1].

Sneaky!

>                 But it doesn't really matter, 1.44MB is 1.44MB as far as
> CMOS cares.

Okay.

>>> but that's only useful for choosing the drive type while there's no
>>> floppy. As soon as one is inserted it will re-validate to the closest
>>> 1.44MB type.
>>>
>>>> Afterwards: pick first one matching the value of
>>>> get_default_drive_type(), i.e. the value of the new property.
>>>>
>>>> So the property is really a default type, which applies only when we
>>>> start without a medium in the drive.
>>>>
>>>> Is that what we want?
>>>>
>>>
>>> It is a "minimal" change that just allows you to configure what it
>>> defaults back to. It's a 'weak' setting.
>>>
>>> Was that a good call? hmm...
>>>
>>>> When I specifically ask for a 5.25" 1.2MiB drive, I'd be rather
>>>> surprised when it silently morphs into a 3.5" 2.88MiB drive just because
>>>> I've forced a funny medium in before startup.
>>>>
>>>
>>> Ah, here it is. I can just add "typeA" and "typeB" properties instead of
>>> defaultA/B, and force the drive into that role.
>>>
>>>> The obvious way to do drive types is selecting one with a property,
>>>> defaulting to the most common type, i.e. standard 3.5" 1.44Mib.
>>>>
>>>
>>> Hm?
>>>
>>> Not sure I follow, but the goal here is to use the 2.88MB type, because
>>> it can accept both kinds of diskettes, so it has the greatest
>>> compatibility for floppy images (including the virtio driver diskette
>>> which is a 2.88MB type.)
>> 
>> The 2.88 type may well be a more useful default, because it takes a
>> strict superset of media.  On the other hand, it puts a different value
>> into the CMOS floppy byte, which could conceivably confuse guests that
>> haven't been updated for this kind of high-tech gadget.  I'm not telling
>> you what default to pick, only what the tradeoffs are.
>> 
>
> Partly why I wanted to preserve the "Pick a drive type based on the
> media" approach to maximize compatibility where possible. I may well end
> up hurting some compatibility in some places when you boot without a
> diskette, but MSDOS, Windows 7, 8, and Fedora all seemed happy, so I was
> happy.
>
>>> The problem is the 1.44MB drive type and the current inflexibility of
>>> the revalidate+pick_geometry algorithm that doesn't allow you to change
>>> from 1.44 to 2.88 or vice versa.
>> 
>> Well, you can't change physical drives from 1.44 to 2.88 or vice versa,
>> either.
>> 
>
> You can change diskettes, though. The code confuses drive types with
> diskette types. The static data table we have describes both.
>
> Initial validation controls what nibble we put in CMOS. secondary
> validations actually only change the CHS and rotation rate variables. So
> the first validation controls /drive/ type and secondary validations
> control /diskette/ type.
>
> The way it works now is that once you choose your *drive* type, you
> cannot change the *diskette*, but yes in the real world you can put a
> 1.44MB into a 2.88MB capable drive (I think? The emulator seems happy
> with it...!)

I figure the following could make the most sense:

* Initial validation: if the user didn't ask for a specific drive type,
  pick one.

* Any validation, including initial: pick one of the diskette types this
  drive type can do.

>>>> To preserve backward compatibility, we need a way to say "pick one for
>>>> the medium, else pick standard, and we need to make it the default.  At
>>>> least for old machine types.
>>>>
>>>> Opinions?
>>>>
>>>
>>> Machines prior to (let's say 2.5 here) should use something close to the
>>> old behavior: "Choose 1.44MB if no diskette, pick the best match (by
>>> sector count) otherwise."
>> 
>> If you want the new, saner behavior with and old machine type, pick the
>> appropriate type, e.g. say type=288.
>> 
>
> Yup.
>
>>> New machines apply nearly the same behavior, except they opt for 2.88MB
>>> as the default.
>>>
>>> New properties allow users to override the default-selection behavior
>>> and/or just force a drive type.
>> 
>> Possible alternative for new machines: default to 2.88 type, done.
>> 
>
> Are you suggesting we ditch the diskette heuristic for picking drive
> type altogether in new machine types?
>
> I wouldn't mind, per se, since it's more mindful and explicit, but I
> think I have the capacity to break a lot of user scripts with that.

Current behavior: pick drive type and diskette type to fit image.

New default behavior: drive type is fixed, pick diskette type to fit
image.  Breaks only when

* no diskette type fits this image, or

* the one we pick presents different medium contents to the guest than
  we got before, or

* the guest chokes on the different disk or diskette type even though
  the actual contents is the same.

Whether it breaks "a lot of user scripts" depends on what default drive
type we pick, and on what images users use.  I suspect 1.44MiB is most
common.  I could be wrong.

>> If you want the old behavior with a new machine type then, you get to
>> specify type=magic or something.
>> 
>> Matter of UI taste.
>> 
>
> Hm.

If you feel the time for getting our floppy user interface right has
long passed, you have a point.  Use your judgement.

>>>>> @@ -295,11 +301,13 @@ static void fd_recalibrate(FDrive *drv)
>>>>>      fd_seek(drv, 0, 0, 1, 1);
>>>>>  }
>>>>>  
>>>>> +static FDriveType get_default_drive_type(FDrive *drv);
>>>>> +
>>>>>  /* Revalidate a disk drive after a disk change */
>>>>>  static void fd_revalidate(FDrive *drv)
>>>>>  {
>>>>>      int nb_heads, max_track, last_sect, ro;
>>>>> -    FDriveType drive;
>>>>> +    FDriveType drive = get_default_drive_type(drv);
>>>>>      FDriveRate rate;
>>>>>  
>>>>>      FLOPPY_DPRINTF("revalidate\n");
>>>>> @@ -609,6 +617,21 @@ typedef struct FDCtrlISABus {
>>>>>      int32_t bootindexB;
>>>>>  } FDCtrlISABus;
>>>>>  
>>>>> +static FDriveType get_default_drive_type(FDrive *drv)
>>>>> +{
>>>>> +    FDCtrl *fdctrl = drv->fdctrl;
>>>>> +
>>>>> +    if (0 < MAX_FD && (&fdctrl->drives[0] == drv)) {
>>>>> +        return fdctrl->defaultA;
>>>>> +    }
>>>>> +
>>>>> +    if (1 < MAX_FD && (&fdctrl->drives[1] == drv)) {
>>>>> +        return fdctrl->defaultB;
>>>>> +    }
>>>>> +
>>>>> +    return FDRIVE_DEFAULT;
>>>>> +}
>>>>> +
>>>>>  static uint32_t fdctrl_read (void *opaque, uint32_t reg)
>>>>>  {
>>>>>      FDCtrl *fdctrl = opaque;
>>>>
>>>> Why do you need to guard with MAX_FD?  If MAX_FD < 2, surely the
>>>> properties don't exist, and fdctrl->drives[i] still has its initial
>>>> value FDRIVE_DEFAULT, doesn't it?
>>>>
>>>
>>> Why would the properties not exist?
>> 
>> Before I answer your question: currently, MAX_FD is always 2, so the
>> question is moot.  The #ifdeffery in fdc.c suggests the code is also
>> prepared for MAX_FD 4, but no other values.
>> 
>> Now your question.  MAX_FD limits the number of drives the controller
>> supports.  Surely the controller's QOM/qdev interface shouldn't expose
>> more drives than you can actually connect.
>> 
>> "isa-fdc" and "sysbus-fdc" support two, "driveA" and "driveB", mapping
>> to state.drives[0..1].  "SUNW,fdtwo" supports one "drive", mapping to
>> state.drives[0].
>> 
>> Note that state.drives[] has MAX_FD elements.  If we changed MAX_FD to
>> one, we'd have to drop "isa-fdc"'s and "sysbus-fdc"'s "driveB", or else
>> they'd overrun state.drives[].
>> 
>
> OK, you mean "Surely if MAX_FD is ever not two, then we will also remove
> e.g. driveB" -- I was thinking there was some kind of magic.
>
> Anyway, yes. I'll replace the weird logic with just an assertion. If
> anyone does change MAX_FD they'll get a "helpful reminder" that this
> function might also need to change.
>
> In summary, your recommendations are:
> - Try using 'typeA' and 'typeB' to force drive type instead of
> defaultA/defaultB to set preferences for the default magic type
> - Use a type of 'magic' to perform autodetection if that behavior is
> desired.
>
> Yeah?

Yeah, and

- Pick the default for typeA and typeB you think is best.