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

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

[John Snow]

requires: 1448895398-13465-1-git-send-email-ehabkost@redhat.com
          pc: Add pc-*-2.6 machine classes

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.

===
v4:
===

Hopefully a more logical patch order with smaller changes.

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

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

001/11:[----] [--] 'fdc: move pick_geometry'
002/11:[----] [--] 'fdc: refactor pick_geometry'
003/11:[----] [--] 'fdc: add disk field'
004/11:[0037] [FC] 'fdc: add default drive type option'
005/11:[down] 'fdc: Add fallback option'
006/11:[----] [-C] 'fdc: do not call revalidate on eject'
007/11:[0030] [FC] 'fdc: implement new drive type property'
008/11:[----] [-C] 'fdc: add physical disk sizes'
009/11:[0018] [FC] 'fdc: rework pick_geometry'
010/11:[----] [--] 'qtest/fdc: Support for 2.88MB drives'
011/11:[down] 'fdc: change auto fallback drive for ISA FDC to 288'

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-v4:
https://github.com/jnsnow/qemu/releases/tag/fdc-default-v4

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               | 315 +++++++++++++++++++++++++++++--------------
 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, 259 insertions(+), 116 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v4 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 858f5f7..1e24524 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -114,51 +114,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))
 
@@ -286,6 +241,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 v4 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.

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 1e24524..f1438e3 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -241,11 +241,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;
@@ -258,8 +256,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) {
@@ -279,41 +277,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 v4 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.

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 f1438e3..041f1aa 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -60,7 +60,7 @@ typedef enum FDriveRate {
 } FDriveRate;
 
 typedef struct FDFormat {
-    FDriveType drive;
+    FloppyDriveType drive;
     uint8_t last_sect;
     uint8_t max_track;
     uint8_t max_head;
@@ -70,48 +70,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)
@@ -133,7 +133,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;
@@ -154,7 +154,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;
@@ -253,11 +253,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) {
@@ -2396,7 +2396,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 9e37186..e9c397a 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..36d4bac 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.5MB 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 v4 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.

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 041f1aa..e72a906 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -140,6 +140,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           */
@@ -157,6 +158,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;
 }
@@ -286,6 +288,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 v4 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.

If drive type is NONE, it considers all drive types in the candidate
geometry table to be a match, and saves the first such one as
"first_match." If drive_type is not NONE, first_match is set to the first
candidate geometry in the table that matched our specific drive type.

This means:

- 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 gets fussier and attempts to
  only pick a geometry if it matches our drive type. In this case,
  first_match must always be set because all known drive types have
  candidate geometries listed in the table.

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

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 e72a906..370ece1 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -273,7 +273,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 v4 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.

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 370ece1..4b7f755 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -150,6 +150,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)
@@ -161,6 +162,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)
@@ -243,13 +246,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;
@@ -289,31 +303,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;
     }
 }
 
@@ -2184,6 +2218,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);
 }
 
@@ -2220,11 +2255,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 v4 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.

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 4b7f755..e9551e5 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -153,6 +153,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 */
@@ -313,8 +316,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);
     }
 }
 
@@ -597,11 +599,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)
 
@@ -2337,6 +2345,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;
@@ -2462,6 +2474,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(),
 };
 
@@ -2510,6 +2525,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(),
 };
 
@@ -2530,6 +2548,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 v4 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.)

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 e9551e5..f475add 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -159,7 +159,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;
@@ -263,7 +262,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;
     }
 
@@ -276,7 +275,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) {
@@ -313,11 +312,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 */
@@ -2474,6 +2479,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),
@@ -2525,6 +2536,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),
@@ -2548,6 +2565,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 v4 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.

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 f475add..8a9747c 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -59,6 +59,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;
@@ -67,11 +73,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, },
@@ -85,7 +95,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" 1400 */
     { 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, },
@@ -93,15 +103,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, },
@@ -109,11 +119,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 v4 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 | 63 ++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 42 insertions(+), 21 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 8a9747c..f8f7e6d 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -124,7 +124,6 @@ static const FDFormat fd_formats[] = {
     { FLOPPY_DRIVE_TYPE_NONE, -1, -1, 0, 0, },
 };
 
-__attribute__((__unused__))
 static FDriveSize drive_size(FloppyDriveType drive)
 {
     switch (drive) {
@@ -283,45 +282,67 @@ 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 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.
+     */
     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 */
+                goto out;
+            } else if (drive_size(parse->drive) == drive_size(drv->drive)) {
+                /* (2) physical size match */
+                match = (match == -1) ? i : match;
+            } else {
+                /* (3) nsectors match only */
+                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)))) {
+                /* (4) type matches, or type matches the autodetect default if
+                 *     we are using the autodetect mechanism. */
+                type_match = i;
             }
         }
     }
+
     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;
-        }
-        parse = &fd_formats[match];
+        match = (size_match != -1) ? size_match : type_match;
+    }
+
+    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 v4 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.

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

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 f8f7e6d..fb0786d 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2531,7 +2531,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] [Qemu-block] [PATCH v4 00/12] fdc: fix 2.88mb floppy diskette support

[Denis V. Lunev]

On 01/20/2016 09:51 AM, John Snow wrote:
> requires: 1448895398-13465-1-git-send-email-ehabkost@redhat.com
>            pc: Add pc-*-2.6 machine classes
>
> 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.
>
> ===
> v4:
> ===
>
> Hopefully a more logical patch order with smaller changes.
>
> 001/12:[----] [--] 'fdc: move pick_geometry'
> 002/12:[down] 'fdc: reduce number of pick_geometry arguments'
> 003/12:[down] 'fdc: add drive type qapi enum'
> 004/12:[0008] [FC] 'fdc: add disk field'
> 005/12:[down] 'fdc: Throw an assertion on misconfigured fd_formats table'
> 006/12:[down] 'fdc: add pick_drive'
> 007/12:[0018] [FC] 'fdc: Add fallback option'
> 008/12:[down] 'fdc: add drive type option'
> 009/12:[----] [-C] 'fdc: add physical disk sizes'
> 010/12:[0014] [FC] 'fdc: rework pick_geometry'
> 011/12:[----] [--] 'qtest/fdc: Support for 2.88MB drives'
> 012/12:[0010] [FC] 'fdc: change auto fallback drive for ISA FDC to 288'
>
> 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:
> ===
>
> 001/11:[----] [--] 'fdc: move pick_geometry'
> 002/11:[----] [--] 'fdc: refactor pick_geometry'
> 003/11:[----] [--] 'fdc: add disk field'
> 004/11:[0037] [FC] 'fdc: add default drive type option'
> 005/11:[down] 'fdc: Add fallback option'
> 006/11:[----] [-C] 'fdc: do not call revalidate on eject'
> 007/11:[0030] [FC] 'fdc: implement new drive type property'
> 008/11:[----] [-C] 'fdc: add physical disk sizes'
> 009/11:[0018] [FC] 'fdc: rework pick_geometry'
> 010/11:[----] [--] 'qtest/fdc: Support for 2.88MB drives'
> 011/11:[down] 'fdc: change auto fallback drive for ISA FDC to 288'
>
> 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-v4:
> https://github.com/jnsnow/qemu/releases/tag/fdc-default-v4
>
> 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               | 315 +++++++++++++++++++++++++++++--------------
>   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, 259 insertions(+), 116 deletions(-)
>
should we recreate ACPI tables after geometry switch?
This would be especially interesting for the case of
Win2k12 (or Win8.1 if you prefer) under OVMF.

Den

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

[John Snow]

On 01/20/2016 02:55 AM, Denis V. Lunev wrote:
> On 01/20/2016 09:51 AM, John Snow wrote:
>> requires: 1448895398-13465-1-git-send-email-ehabkost@redhat.com
>>            pc: Add pc-*-2.6 machine classes
>>
>> 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.
>>
>> ===
>> v4:
>> ===
>>
>> Hopefully a more logical patch order with smaller changes.
>>
>> 001/12:[----] [--] 'fdc: move pick_geometry'
>> 002/12:[down] 'fdc: reduce number of pick_geometry arguments'
>> 003/12:[down] 'fdc: add drive type qapi enum'
>> 004/12:[0008] [FC] 'fdc: add disk field'
>> 005/12:[down] 'fdc: Throw an assertion on misconfigured fd_formats table'
>> 006/12:[down] 'fdc: add pick_drive'
>> 007/12:[0018] [FC] 'fdc: Add fallback option'
>> 008/12:[down] 'fdc: add drive type option'
>> 009/12:[----] [-C] 'fdc: add physical disk sizes'
>> 010/12:[0014] [FC] 'fdc: rework pick_geometry'
>> 011/12:[----] [--] 'qtest/fdc: Support for 2.88MB drives'
>> 012/12:[0010] [FC] 'fdc: change auto fallback drive for ISA FDC to 288'
>>
>> 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:
>> ===
>>
>> 001/11:[----] [--] 'fdc: move pick_geometry'
>> 002/11:[----] [--] 'fdc: refactor pick_geometry'
>> 003/11:[----] [--] 'fdc: add disk field'
>> 004/11:[0037] [FC] 'fdc: add default drive type option'
>> 005/11:[down] 'fdc: Add fallback option'
>> 006/11:[----] [-C] 'fdc: do not call revalidate on eject'
>> 007/11:[0030] [FC] 'fdc: implement new drive type property'
>> 008/11:[----] [-C] 'fdc: add physical disk sizes'
>> 009/11:[0018] [FC] 'fdc: rework pick_geometry'
>> 010/11:[----] [--] 'qtest/fdc: Support for 2.88MB drives'
>> 011/11:[down] 'fdc: change auto fallback drive for ISA FDC to 288'
>>
>> 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-v4:
>> https://github.com/jnsnow/qemu/releases/tag/fdc-default-v4
>>
>> 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               | 315
>> +++++++++++++++++++++++++++++--------------
>>   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, 259 insertions(+), 116 deletions(-)
>>
> should we recreate ACPI tables after geometry switch?
> This would be especially interesting for the case of
> Win2k12 (or Win8.1 if you prefer) under OVMF.
> 
> Den

This series doesn't really alter the concept that disk geometry can
change at runtime -- Not knowing much about the ACPI reverse engineering
that happened to make Windows 8/10 happy, does it work currently? Can
you change to different density floppies and have it work out alright?

If not, you can submit a patch against master as it is today -- this
series only does two things:

(1) Alters the heuristics for which type of floppy drive is chosen at
boot time (No change to ACPI table generation should be needed.)

(2) Allows 1.44MB diskettes to be recognized by 2.88MB drive types. This
might require some changes, but check out pick_geometry both before and
after this patchset -- there's a whole table of different geometries
that we already allow users to switch between at runtime. If the
geometry needs to update there, too, then it's already broken before
this patchset.

It should be easy enough to slide a geometry update in fd_revalidate()
if needed.

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

[Denis V. Lunev]

On 01/20/2016 10:40 PM, John Snow wrote:
>
> On 01/20/2016 02:55 AM, Denis V. Lunev wrote:
>> On 01/20/2016 09:51 AM, John Snow wrote:
>>> requires: 1448895398-13465-1-git-send-email-ehabkost@redhat.com
>>>             pc: Add pc-*-2.6 machine classes
>>>
>>> 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.
>>>
>>> ===
>>> v4:
>>> ===
>>>
>>> Hopefully a more logical patch order with smaller changes.
>>>
>>> 001/12:[----] [--] 'fdc: move pick_geometry'
>>> 002/12:[down] 'fdc: reduce number of pick_geometry arguments'
>>> 003/12:[down] 'fdc: add drive type qapi enum'
>>> 004/12:[0008] [FC] 'fdc: add disk field'
>>> 005/12:[down] 'fdc: Throw an assertion on misconfigured fd_formats table'
>>> 006/12:[down] 'fdc: add pick_drive'
>>> 007/12:[0018] [FC] 'fdc: Add fallback option'
>>> 008/12:[down] 'fdc: add drive type option'
>>> 009/12:[----] [-C] 'fdc: add physical disk sizes'
>>> 010/12:[0014] [FC] 'fdc: rework pick_geometry'
>>> 011/12:[----] [--] 'qtest/fdc: Support for 2.88MB drives'
>>> 012/12:[0010] [FC] 'fdc: change auto fallback drive for ISA FDC to 288'
>>>
>>> 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:
>>> ===
>>>
>>> 001/11:[----] [--] 'fdc: move pick_geometry'
>>> 002/11:[----] [--] 'fdc: refactor pick_geometry'
>>> 003/11:[----] [--] 'fdc: add disk field'
>>> 004/11:[0037] [FC] 'fdc: add default drive type option'
>>> 005/11:[down] 'fdc: Add fallback option'
>>> 006/11:[----] [-C] 'fdc: do not call revalidate on eject'
>>> 007/11:[0030] [FC] 'fdc: implement new drive type property'
>>> 008/11:[----] [-C] 'fdc: add physical disk sizes'
>>> 009/11:[0018] [FC] 'fdc: rework pick_geometry'
>>> 010/11:[----] [--] 'qtest/fdc: Support for 2.88MB drives'
>>> 011/11:[down] 'fdc: change auto fallback drive for ISA FDC to 288'
>>>
>>> 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-v4:
>>> https://github.com/jnsnow/qemu/releases/tag/fdc-default-v4
>>>
>>> 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               | 315
>>> +++++++++++++++++++++++++++++--------------
>>>    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, 259 insertions(+), 116 deletions(-)
>>>
>> should we recreate ACPI tables after geometry switch?
>> This would be especially interesting for the case of
>> Win2k12 (or Win8.1 if you prefer) under OVMF.
>>
>> Den
> This series doesn't really alter the concept that disk geometry can
> change at runtime -- Not knowing much about the ACPI reverse engineering
> that happened to make Windows 8/10 happy, does it work currently? Can
> you change to different density floppies and have it work out alright?
>
> If not, you can submit a patch against master as it is today -- this
> series only does two things:
>
> (1) Alters the heuristics for which type of floppy drive is chosen at
> boot time (No change to ACPI table generation should be needed.)
>
> (2) Allows 1.44MB diskettes to be recognized by 2.88MB drive types. This
> might require some changes, but check out pick_geometry both before and
> after this patchset -- there's a whole table of different geometries
> that we already allow users to switch between at runtime. If the
> geometry needs to update there, too, then it's already broken before
> this patchset.
>
> It should be easy enough to slide a geometry update in fd_revalidate()
> if needed.
may be. I have not personally checked, may be it just
works. I think that OVMF should be taken into the
equation...

So, in two words. We care about the floppy too :) Though
we have static config that works for our case.

Den

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

[Eric Blake]

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

On 01/19/2016 11:51 PM, John Snow wrote:
> 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.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/block/fdc.c | 50 ++++++++++++++++++++------------------------------
>  1 file changed, 20 insertions(+), 30 deletions(-)
> 

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

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

[Eric Blake]

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

On 01/19/2016 11:51 PM, John Snow wrote:
> 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.
> 
> 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(-)
> 

> +++ 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.5MB 5.25" drive

Umm, how does 120 match with 1.5M?  Elsewhere the code comments say:

     /* 1.2 MB 5"1/4 floppy disks */

With the typo fixed,
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 --]

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

[Eric Blake]

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

On 01/19/2016 11:51 PM, John Snow wrote:
> 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.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/block/fdc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 

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

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

[John Snow]

On 01/20/2016 03:33 PM, Eric Blake wrote:
> On 01/19/2016 11:51 PM, John Snow wrote:
>> 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.
>>
>> 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(-)
>>
> 
>> +++ 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.5MB 5.25" drive
> 
> Umm, how does 120 match with 1.5M?  Elsewhere the code comments say:
> 
>      /* 1.2 MB 5"1/4 floppy disks */
> 
> With the typo fixed,
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

!? just a typo I never noticed... I must have had the "5" buffered
because of the 5.25" physical size...

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

[John Snow]

On 01/20/2016 03:35 PM, Eric Blake wrote:
> On 01/19/2016 11:51 PM, John Snow wrote:
>> 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.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  hw/block/fdc.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Grazie.

--js

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

[Eric Blake]

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

On 01/19/2016 11:51 PM, John Snow wrote:
> 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.
> 
> If drive type is NONE, it considers all drive types in the candidate
> geometry table to be a match, and saves the first such one as
> "first_match." If drive_type is not NONE, first_match is set to the first
> candidate geometry in the table that matched our specific drive type.

That seems subtly different than how I read the code (it is possible to
exit the for loop with match == 0 but first_match == -1, if nb_sectors
is right for the very first entry; but your statement implies that
"first_match" will always be non-negative after the loop). Maybe a
better wording would be:

The code starts iterating over all entries in the table, 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:

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 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 gets fussier and attempts to
>   only pick a geometry if it matches our drive type. In this case,
>   first_match must always be set because all known drive types have
>   candidate geometries listed in the table.

- If drive type is not NONE, pick_geometry() was fussier and only looked
at rows that match 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.

But this part is right.

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

The code change itself is correct, so with an improved commit message,
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 --]

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

[John Snow]

On 01/20/2016 04:23 PM, Eric Blake wrote:
> On 01/19/2016 11:51 PM, John Snow wrote:
>> 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.
>>
>> If drive type is NONE, it considers all drive types in the candidate
>> geometry table to be a match, and saves the first such one as
>> "first_match." If drive_type is not NONE, first_match is set to the first
>> candidate geometry in the table that matched our specific drive type.
> 
> That seems subtly different than how I read the code (it is possible to
> exit the for loop with match == 0 but first_match == -1, if nb_sectors
> is right for the very first entry; but your statement implies that
> "first_match" will always be non-negative after the loop). Maybe a
> better wording would be:
> 

Yeah, I was oversimplifying in retrospect. In any case where we bother
to read first_match, it must always be set. We don't bother when we get
a real, exact match.

> The code starts iterating over all entries in the table, 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 is literally the worst function in QEMU. It is so wrong, describing
why it is wrong is itself difficult.

>>
>> This means:
> 
> 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 NONE, the default is truly fd_formats[0], a 1.44MB
> type, because "first_match" will always get set to the first item.
> 

Just adding quotes, OK.

>>
>> - If drive type is not NONE, pick_geometry gets fussier and attempts to
>>   only pick a geometry if it matches our drive type. In this case,
>>   first_match must always be set because all known drive types have
>>   candidate geometries listed in the table.
> 
> - If drive type is not NONE, pick_geometry() was fussier and only looked
> at rows that match our drive type.  However, since all possible drive
> types are represented in the table, we still know that "first_match" was
> set.
> 

gets, was, is. I can use your wording, anyway.

>>
>> - 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.
> 
> But this part is right.
> 
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  hw/block/fdc.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> The code change itself is correct, so with an improved commit message,
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks, I'll revise the message and tentatively stick your R-B on it.

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

[Eric Blake]

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

On 01/19/2016 11:51 PM, John Snow wrote:
> 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.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/block/fdc.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 46 insertions(+), 10 deletions(-)
> 

> +++ b/hw/block/fdc.c
> @@ -150,6 +150,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)
> @@ -161,6 +162,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;

Not necessarily in this patch, but should media_changed be swapped to
'bool'?

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

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

[Eric Blake]

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

On 01/19/2016 11:51 PM, John Snow wrote:
> 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.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---

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

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

[Eric Blake]

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

On 01/19/2016 11:51 PM, John Snow wrote:
> 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

I might have written '"auto".' (the '.' is not part of the behavior
name).  But while grammar guides are starting to concede this style,
they still admit that for a long time English used to always put the
trailing '.' of a sentence inside the "", regardless of whether the
quoted material originally included a period at that point or whether it
was just the speaker ending their sentence on quoted material.

> 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.)
> 
> 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 e9551e5..f475add 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -159,7 +159,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;
> @@ -263,7 +262,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)) {

I might have dropped the two inner () pairs.  But that's cosmetic.

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

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

[Eric Blake]

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

On 01/19/2016 11:51 PM, John Snow wrote:
> 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.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/block/fdc.c | 40 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 8 deletions(-)
> 

> +/* 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, },
> @@ -85,7 +95,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" 1400 */

Typo? I think you meant 1440.

> +__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;
> +    }
> +}

With that fixed,
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 --]

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

[John Snow]

On 01/20/2016 05:43 PM, Eric Blake wrote:
> On 01/19/2016 11:51 PM, John Snow wrote:
>> 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
> 
> I might have written '"auto".' (the '.' is not part of the behavior
> name).  But while grammar guides are starting to concede this style,
> they still admit that for a long time English used to always put the
> trailing '.' of a sentence inside the "", regardless of whether the
> quoted material originally included a period at that point or whether it
> was just the speaker ending their sentence on quoted material.
> 

"Starting to concede" -- I think Strunk & White disagrees with you on
the concept that this is a cool new thing that young people do!

I will start putting my punctuation outside the quotation marks as soon
as everyone else agrees to stop using two spaces after the full stop.

I am taking no prisoners.

>> 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.)
>>
>> 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 e9551e5..f475add 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -159,7 +159,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;
>> @@ -263,7 +262,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)) {
> 
> I might have dropped the two inner () pairs.  But that's cosmetic.
> 

This nit, however, I'll edit.

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

--js

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

[John Snow]

On 01/20/2016 05:48 PM, Eric Blake wrote:
> On 01/19/2016 11:51 PM, John Snow wrote:
>> 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.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  hw/block/fdc.c | 40 ++++++++++++++++++++++++++++++++--------
>>  1 file changed, 32 insertions(+), 8 deletions(-)
>>
> 
>> +/* 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, },
>> @@ -85,7 +95,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" 1400 */
> 
> Typo? I think you meant 1440.
> 

Good catch.

>> +__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;
>> +    }
>> +}
> 
> With that fixed,
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks,
--js

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

[Eric Blake]

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

On 01/19/2016 11: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.

This is the one I worry about; details below.

>  - Fall back to the first geometry that matched our type.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---

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

Yay - comments!  Makes it somewhat easier to follow.

> +     */
>      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) {

To make sure I understand this:

The first three conditions are reached only when visiting a row where
nb_sectors matches.

> +            if (magic || parse->drive == drv->drive) {
> +                /* (1) perfect match */
> +                goto out;

The conditional means we get here only if the user specified 'magic'
(aka the new "auto" behavior of picking the first drive that works) or
an actual size (144/288/120).  Since the sectors match, we are set; and
this matches the old behavior (either drive type and sectors match, or
drive type was unspecified and sectors match).

> +            } else if (drive_size(parse->drive) == drive_size(drv->drive)) {
> +                /* (2) physical size match */
> +                match = (match == -1) ? i : match;

Here, drive_size(parse->drive) will never be FDRIVE_SIZE_UNKNOWN, but
drive_size(drv->drive) depends on what the user told us.  If they didn't
tell us what disk size to target, including if they asked for "auto",
this rule will never match.  If they DID tell us a disk size
(144/288/120), then this favors their disk size.  More concretely, if
they asked for 2880 sectors and a 288 disk, the old algorithm would fail
(no 288 entry has 2880 sectors), but now succeeds (the 144 entry with
2880 sectors has the same 3.5" disk size).  This is a good bug fix.

> +            } else {
> +                /* (3) nsectors match only */

nb_sectors?

> +                size_match = (size_match == -1) ? i : size_match;

Here, we don't have a drive match.  Either the user did not specify a
drive type [backwards-compatible behavior - the old loop found the first
entry with a matching size among every row of the table], or they
specified 144/288 while we are visiting entries for 120, or they
specified 120 while we are visiting entries for 144/288) [new behavior].

Should I worry about the new behavior?  If the size is one of the four
ambiguous rows (2880, 3200, 1440, 720), then we are okay - even though
we set 'size_match' on this iteration, another iteration will also set
'match' on the counterpart row, and 'match' takes priority over
'size_match'.

BUT what if the user asked for 3360 sectors and a 120 disk?  The old
code would have set 'first_match' to the 120 15,80,1 row (first row that
matches the drive type, since the search is limited to just 120 rows and
there was no size match); but your code looks for 3360 across ALL rows,
and ends up setting 'size_match' to the 144 21,80,1 row.  And since your
code never sets 'match' (none of the 120 rows match in size), you end up
_changing_ the disk format compared to the old code.

Thus, I think you still have a bug. :(

>              }
> -            if (first_match == -1) {
> -                first_match = i;
> +        } else if (type_match == -1) {

Here, we are visiting a row where sectors doesn't match.

> +            if ((parse->drive == drv->drive) ||
> +                (magic && (parse->drive == get_fallback_drive_type(drv)))) {
> +                /* (4) type matches, or type matches the autodetect default if
> +                 *     we are using the autodetect mechanism. */
> +                type_match = i;

In which case, if the user told us a size, or the default for the "auto"
size matches, we set 'type_match' (in effect, picking the first
144/288/120 row that matches their explicit or default type).

>              }
>          }
>      }
> +
>      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;
> -        }
> -        parse = &fd_formats[match];
> +        match = (size_match != -1) ? size_match : type_match;

Only reached for (2), (3), and (4).  If (2) fired, 'match' is set and we
take that.  Otherwise if (3) fired, 'size_match' is set and we take that
(but see my claim that (3) can be buggy).  Otherwise, (4) better have
fired, because if not...

> +    }
> +
> +    if (match == -1) {
> +        error_setg(&error_abort, "No candidate geometries present in table "
> +                   " for floppy drive type '%s'",
> +                   FloppyDriveType_lookup[drv->drive]);

...we abort.  The abort looks sane.

>      }
>  
> +    parse = &(fd_formats[match]);
> +
> + out:

And the label is what lets us handle (1) as higher priority than
anything else.

>      if (parse->max_head == 0) {
>          drv->flags &= ~FDISK_DBL_SIDES;
>      } else {
> 

I think you can salvage the patch, though.  The condition for (3) is
currently 'else [if (1)]'.  But what if it is instead 'else if
(drive_size(drv->drive) == FDRIVE_SIZE_UKNOWN)'?  Then we are only
looking for a sector match on rows where we don't know what drive the
user wants, and leaving 'size_match' unchanged on rows that differ in
disk size from an explicit user's request.

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

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

[Roman Kagan]

On Wed, Jan 20, 2016 at 02:40:14PM -0500, John Snow wrote:
> On 01/20/2016 02:55 AM, Denis V. Lunev wrote:
> > should we recreate ACPI tables after geometry switch?
> > This would be especially interesting for the case of
> > Win2k12 (or Win8.1 if you prefer) under OVMF.
> > 
> > Den
> 
> This series doesn't really alter the concept that disk geometry can
> change at runtime -- Not knowing much about the ACPI reverse engineering
> that happened to make Windows 8/10 happy, does it work currently? Can
> you change to different density floppies and have it work out alright?

No, exactly because the geometry is determined once startup.

> If not, you can submit a patch against master as it is today -- this
> series only does two things:
> 
> (1) Alters the heuristics for which type of floppy drive is chosen at
> boot time (No change to ACPI table generation should be needed.)
> 
> (2) Allows 1.44MB diskettes to be recognized by 2.88MB drive types. This
> might require some changes, but check out pick_geometry both before and
> after this patchset -- there's a whole table of different geometries
> that we already allow users to switch between at runtime. If the
> geometry needs to update there, too, then it's already broken before
> this patchset.

Right.

This series conflicts slightly with the patches to generate ACPI objects
for floppies (which haven't made it into the mainstream qemu yet)
because of the adjustments in the floppy API.  Not a big deal.

> It should be easy enough to slide a geometry update in fd_revalidate()
> if needed.

Now that is a bit trickier: the currently submitted code queries the
floppy properties at SSDT build time, and sticks static objects into
AML; if that really needs updating at runtime it'll require certain
refactoring.

That said I'm not certain what exactly has to be done here.  Physical
machines do not have their floppy drives changable at runtime, do they?
So the OSes should be fine assuming that the drive stays the same, and
it's only the diskette that can change.  I'd guess that the OS driver
should do the necessary revalidation on its own, without ACPI
assistance; I'll give it a try when I have some time.

But again, as you said, people are mainly interested in floppies to
bootstrap a Windows installation on virtio disks, so support of floppy
geometry update at runtime is non-critical for most users.

Thanks,
Roman.

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

[John Snow]

On 01/21/2016 05:53 AM, Roman Kagan wrote:
> On Wed, Jan 20, 2016 at 02:40:14PM -0500, John Snow wrote:
>> On 01/20/2016 02:55 AM, Denis V. Lunev wrote:
>>> should we recreate ACPI tables after geometry switch?
>>> This would be especially interesting for the case of
>>> Win2k12 (or Win8.1 if you prefer) under OVMF.
>>>
>>> Den
>>
>> This series doesn't really alter the concept that disk geometry can
>> change at runtime -- Not knowing much about the ACPI reverse engineering
>> that happened to make Windows 8/10 happy, does it work currently? Can
>> you change to different density floppies and have it work out alright?
> 
> No, exactly because the geometry is determined once startup.
> 
>> If not, you can submit a patch against master as it is today -- this
>> series only does two things:
>>
>> (1) Alters the heuristics for which type of floppy drive is chosen at
>> boot time (No change to ACPI table generation should be needed.)
>>
>> (2) Allows 1.44MB diskettes to be recognized by 2.88MB drive types. This
>> might require some changes, but check out pick_geometry both before and
>> after this patchset -- there's a whole table of different geometries
>> that we already allow users to switch between at runtime. If the
>> geometry needs to update there, too, then it's already broken before
>> this patchset.
> 
> Right.
> 
> This series conflicts slightly with the patches to generate ACPI objects
> for floppies (which haven't made it into the mainstream qemu yet)
> because of the adjustments in the floppy API.  Not a big deal.
> 
>> It should be easy enough to slide a geometry update in fd_revalidate()
>> if needed.
> 
> Now that is a bit trickier: the currently submitted code queries the
> floppy properties at SSDT build time, and sticks static objects into
> AML; if that really needs updating at runtime it'll require certain
> refactoring.
> 
> That said I'm not certain what exactly has to be done here.  Physical
> machines do not have their floppy drives changable at runtime, do they?
> So the OSes should be fine assuming that the drive stays the same, and
> it's only the diskette that can change.  I'd guess that the OS driver
> should do the necessary revalidation on its own, without ACPI
> assistance; I'll give it a try when I have some time.
> 
> But again, as you said, people are mainly interested in floppies to
> bootstrap a Windows installation on virtio disks, so support of floppy
> geometry update at runtime is non-critical for most users.
> 
> Thanks,
> Roman.
> 

I'm a little confused here. I am not proposing the ability to change a
floppy drive type at runtime, just the ability to insert different kinds
of diskettes, which does happen in the real world.

e.g. a 2.88MB capable floppy drive that can read either 1.44MB or 2.88MB
types.

--js

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

[Roman Kagan]

On Thu, Jan 21, 2016 at 09:59:09AM -0500, John Snow wrote:
> > Now that is a bit trickier: the currently submitted code queries the
> > floppy properties at SSDT build time, and sticks static objects into
> > AML; if that really needs updating at runtime it'll require certain
> > refactoring.
> > 
> > That said I'm not certain what exactly has to be done here.  Physical
> > machines do not have their floppy drives changable at runtime, do they?
> > So the OSes should be fine assuming that the drive stays the same, and
> > it's only the diskette that can change.  I'd guess that the OS driver
> > should do the necessary revalidation on its own, without ACPI
> > assistance; I'll give it a try when I have some time.
> > 
> > But again, as you said, people are mainly interested in floppies to
> > bootstrap a Windows installation on virtio disks, so support of floppy
> > geometry update at runtime is non-critical for most users.
> 
> I'm a little confused here. I am not proposing the ability to change a
> floppy drive type at runtime, just the ability to insert different kinds
> of diskettes, which does happen in the real world.
> 
> e.g. a 2.88MB capable floppy drive that can read either 1.44MB or 2.88MB
> types.

That's right; the question WRT ACPI is whether the Windows driver
expects the drive parameters or the currently inserted diskette
parameters in the ACPI object; I guess the former, so we should be OK
with that data generated once at startup.

Roman.

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

[Eric Blake]

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

On 01/19/2016 11:51 PM, John Snow wrote:
> 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(-)

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

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

[Eric Blake]

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

On 01/19/2016 11:51 PM, 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.
> 
> 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.
> 
> 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(-)
> 

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

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

[John Snow]

On 01/20/2016 06:45 PM, Eric Blake wrote:
> On 01/19/2016 11: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.
>>

For sake of review, we'll call these choices (A) and (B).

>> 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.
> 
> This is the one I worry about; details below.
> 
>>  - Fall back to the first geometry that matched our type.
>>

And these choices will be (1) through (4) as they are annotated in the
comment below in this way as well.

(1) is identical to (A), and (4) is identical to (B).
That leaves (2) and (3) are new behaviors.

There is strong rationale for (2).
(3) was invented as a last-ditch effort before (4), see below!

>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
> 
>> -    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 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.
> 
> Yay - comments!  Makes it somewhat easier to follow.
> 
>> +     */
>>      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) {
> 
> To make sure I understand this:
> 
> The first three conditions are reached only when visiting a row where
> nb_sectors matches.
> 

Yes.

>> +            if (magic || parse->drive == drv->drive) {
>> +                /* (1) perfect match */
>> +                goto out;
> 
> The conditional means we get here only if the user specified 'magic'
> (aka the new "auto" behavior of picking the first drive that works) or
> an actual size (144/288/120).  Since the sectors match, we are set; and
> this matches the old behavior (either drive type and sectors match, or
> drive type was unspecified and sectors match).
> 

Yes. This is the old (A), identical to what is now documented as (1).

>> +            } else if (drive_size(parse->drive) == drive_size(drv->drive)) {
>> +                /* (2) physical size match */
>> +                match = (match == -1) ? i : match;
> 
> Here, drive_size(parse->drive) will never be FDRIVE_SIZE_UNKNOWN, but
> drive_size(drv->drive) depends on what the user told us.  If they didn't
> tell us what disk size to target, including if they asked for "auto",
> this rule will never match.  If they DID tell us a disk size
> (144/288/120), then this favors their disk size.  More concretely, if
> they asked for 2880 sectors and a 288 disk, the old algorithm would fail
> (no 288 entry has 2880 sectors), but now succeeds (the 144 entry with
> 2880 sectors has the same 3.5" disk size).  This is a good bug fix.
> 

Yes. This is new behavior, documented as (2).

>> +            } else {
>> +                /* (3) nsectors match only */
> 
> nb_sectors?
> 

Yes, I can update the comment to be less fuzzy.

>> +                size_match = (size_match == -1) ? i : size_match;
> 
> Here, we don't have a drive match.  Either the user did not specify a
> drive type [backwards-compatible behavior - the old loop found the first
> entry with a matching size among every row of the table], or they

The old behavior you are referencing is (A), but that's already taken
care of under (1) above because of the (magic || parse->drive ==
drv->drive) conditional.

If magic is set, the types (or physical sizes) do not need to match,
just the number of sectors -- and the match is so strong, we exit
immediately.

> specified 144/288 while we are visiting entries for 120, or they
> specified 120 while we are visiting entries for 144/288) [new behavior].
> 

New indeed.

> Should I worry about the new behavior?  If the size is one of the four
> ambiguous rows (2880, 3200, 1440, 720), then we are okay - even though
> we set 'size_match' on this iteration, another iteration will also set
> 'match' on the counterpart row, and 'match' takes priority over
> 'size_match'.
> 

You are right to worry. Rationale below.

> BUT what if the user asked for 3360 sectors and a 120 disk?  The old
> code would have set 'first_match' to the 120 15,80,1 row (first row that
> matches the drive type, since the search is limited to just 120 rows and
> there was no size match); but your code looks for 3360 across ALL rows,
> and ends up setting 'size_match' to the 144 21,80,1 row.  And since your
> code never sets 'match' (none of the 120 rows match in size), you end up
> _changing_ the disk format compared to the old code.
> 
> Thus, I think you still have a bug. :(
> 

It's definitely new behavior. In either case, I think we can agree that
QEMU is choosing a garbage format as a last-ditch effort to see if
something works.

The old behavior of "Choose a wrong geometry of the right type as a
last-chance effort" was something I thought that maybe I could "improve"
by doing:

"Choose a wrong geometry of the right type as a last-chance effort,
unless we found something that is the correct number of sectors, in
which case maybe use that as a backup strategy because it might work
better."

It is decidedly "new" behavior, but it is similarly to the old strategy
a recourse action when faced with the absence of a more precise match
(right size and type (1) or right size and correct physical size (2))

The thought was basically: "I bet the floppy drive code would cope
better by setting a geometry that is at the very least the right number
of sectors over one that's clearly wrong."

>>              }
>> -            if (first_match == -1) {
>> -                first_match = i;
>> +        } else if (type_match == -1) {
> 
> Here, we are visiting a row where sectors doesn't match.
> 

Yes, the old "hail mary" behavior present from 2003-2016, what I
annotated as type (B) in my reply above. Now documented as behavior (4).

>> +            if ((parse->drive == drv->drive) ||
>> +                (magic && (parse->drive == get_fallback_drive_type(drv)))) {
>> +                /* (4) type matches, or type matches the autodetect default if
>> +                 *     we are using the autodetect mechanism. */
>> +                type_match = i;
> 
> In which case, if the user told us a size, or the default for the "auto"
> size matches, we set 'type_match' (in effect, picking the first
> 144/288/120 row that matches their explicit or default type).
> 

Right, this is the old behavior. "We couldn't find the right geometry,
so we're just going to pick one that is of the type you specified and
hope you know what you're doing."

The nugget of new behavior here is that if the user did NOT specify a
type, and we STILL managed to not find a precise matching geometry,
we'll choose the first geometry that belongs to the fallback type.

>>              }
>>          }
>>      }
>> +
>>      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;
>> -        }
>> -        parse = &fd_formats[match];
>> +        match = (size_match != -1) ? size_match : type_match;
> 
> Only reached for (2), (3), and (4).  If (2) fired, 'match' is set and we
> take that.  Otherwise if (3) fired, 'size_match' is set and we take that
> (but see my claim that (3) can be buggy).  Otherwise, (4) better have
> fired, because if not...
> 

Should be no way for it not to!

>> +    }
>> +
>> +    if (match == -1) {
>> +        error_setg(&error_abort, "No candidate geometries present in table "
>> +                   " for floppy drive type '%s'",
>> +                   FloppyDriveType_lookup[drv->drive]);
> 
> ...we abort.  The abort looks sane.
> 
>>      }
>>  
>> +    parse = &(fd_formats[match]);
>> +
>> + out:
> 
> And the label is what lets us handle (1) as higher priority than
> anything else.
> 
>>      if (parse->max_head == 0) {
>>          drv->flags &= ~FDISK_DBL_SIDES;
>>      } else {
>>
> 
> I think you can salvage the patch, though.  The condition for (3) is
> currently 'else [if (1)]'.  But what if it is instead 'else if
> (drive_size(drv->drive) == FDRIVE_SIZE_UKNOWN)'?  Then we are only
> looking for a sector match on rows where we don't know what drive the
> user wants, and leaving 'size_match' unchanged on rows that differ in
> disk size from an explicit user's request.
> 

Should be unnecessary -- (1) should handle nb_sectors match adequately
when the user did not specify a drive type.

If (3) is undesired, it can be scrapped outright.

--js

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

[Eric Blake]

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

On 01/21/2016 01:14 PM, John Snow wrote:
> 
> 
> On 01/20/2016 06:45 PM, Eric Blake wrote:
>> On 01/19/2016 11: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.
>>>
> 
> For sake of review, we'll call these choices (A) and (B).
> 
>>> 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.
>>
>> This is the one I worry about; details below.
>>
>>>  - Fall back to the first geometry that matched our type.
>>>
> 
> And these choices will be (1) through (4) as they are annotated in the
> comment below in this way as well.
> 
> (1) is identical to (A), and (4) is identical to (B).

Agreed.

> That leaves (2) and (3) are new behaviors.
> 
> There is strong rationale for (2).
> (3) was invented as a last-ditch effort before (4), see below!
> 

>>
>> Thus, I think you still have a bug. :(
>>
> 
> It's definitely new behavior. In either case, I think we can agree that
> QEMU is choosing a garbage format as a last-ditch effort to see if
> something works.
> 
> The old behavior of "Choose a wrong geometry of the right type as a
> last-chance effort" was something I thought that maybe I could "improve"
> by doing:
> 
> "Choose a wrong geometry of the right type as a last-chance effort,
> unless we found something that is the correct number of sectors, in
> which case maybe use that as a backup strategy because it might work
> better."

I don't think the geometry for a 120 row is going to work right for a
guest expecting 144/288 behavior.

> 
> It is decidedly "new" behavior, but it is similarly to the old strategy
> a recourse action when faced with the absence of a more precise match
> (right size and type (1) or right size and correct physical size (2))
> 
> The thought was basically: "I bet the floppy drive code would cope
> better by setting a geometry that is at the very least the right number
> of sectors over one that's clearly wrong."

I think the point of this function is:

If the user told us a disk type and gave us an image with an exact
number of sectors, use the geometry associated with that size.  Behavior
(2) refines this to further pick sizes that are compatible with the
current disk type (with a 288 drive, the exact geometry for 2880 sectors
visiting a 144 disk is probably better than the behavior (B) choice for
the first 288 disk).

If the user did not tell us a disk type, favor a geometry that matches
the number of sectors.  If the size can occur from more than one disk
type, we used to favor the disk type that occurred first in the list
(2880 sectors maps to 3.5", not 5.25"; while 720 sectors favors 5.25");
due to behavior (2) the new code may instead favor the disk size that
matches the default fallback of "auto" (if auto falls back to 144 or
288, 720 sectors will now select that size rather than 5.25").

If the user told us a disk type but we don't have a sector match, then
pick the first geometry associated with that disk type (hopefully the
host image is smaller than that choice, and we pad out the host file to
pretend that it matches the most common use of that disk type from the
guest's view).

Finally, the user told us nothing and we have no size heuristic to fall
back on, so pick a (hopefully sane) default of the first table entry
(old behavior) or the "auto" fallback (new behavior).

> 
>>>              }
>>> -            if (first_match == -1) {
>>> -                first_match = i;
>>> +        } else if (type_match == -1) {
>>
>> Here, we are visiting a row where sectors doesn't match.
>>
> 
> Yes, the old "hail mary" behavior present from 2003-2016, what I
> annotated as type (B) in my reply above. Now documented as behavior (4).
> 
>>> +            if ((parse->drive == drv->drive) ||
>>> +                (magic && (parse->drive == get_fallback_drive_type(drv)))) {
>>> +                /* (4) type matches, or type matches the autodetect default if
>>> +                 *     we are using the autodetect mechanism. */
>>> +                type_match = i;
>>
>> In which case, if the user told us a size, or the default for the "auto"
>> size matches, we set 'type_match' (in effect, picking the first
>> 144/288/120 row that matches their explicit or default type).
>>
> 
> Right, this is the old behavior. "We couldn't find the right geometry,
> so we're just going to pick one that is of the type you specified and
> hope you know what you're doing."
> 
> The nugget of new behavior here is that if the user did NOT specify a
> type, and we STILL managed to not find a precise matching geometry,
> we'll choose the first geometry that belongs to the fallback type.

Which seems like a good change (if you don't tell us a disk type, then
we'll default to giving you a 144/288 drive UNLESS hueristics prove you
were probably trying to open a 120 format based on sector size).

>> I think you can salvage the patch, though.  The condition for (3) is
>> currently 'else [if (1)]'.  But what if it is instead 'else if

I meant that as 'if (true)', not 'if (condition identical to (1) in the
discussion above)'.

>> (drive_size(drv->drive) == FDRIVE_SIZE_UKNOWN)'?  Then we are only
>> looking for a sector match on rows where we don't know what drive the
>> user wants, and leaving 'size_match' unchanged on rows that differ in
>> disk size from an explicit user's request.
>>
> 
> Should be unnecessary -- (1) should handle nb_sectors match adequately
> when the user did not specify a drive type.

After (re-)reading the use of 'magic', I concur.

> 
> If (3) is undesired, it can be scrapped outright.

I think that's the best approach then - drop condition (3) outright, and
only do an early exit (1), set 'match' (2), or set 'type_match' (4).

Tricky patch, but I think with that change, I'll be on board for your v5
patch.

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