All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH for-2.6 v2 00/10] fdc: fix 2.88mb floppy diskette support
@ 2015-12-07 23:34 John Snow
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 01/10] fdc: move pick_geometry John Snow
                   ` (9 more replies)
  0 siblings, 10 replies; 17+ messages in thread
From: John Snow @ 2015-12-07 23:34 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, John Snow, armbru, qemu-devel

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.

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

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

(4) Change the automatic fallback type for the automatic guessing
    heuristic from 1.44MB to 2.88MB as it is a more diverse drive.

(5) 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.

If any guests are discovered to be unable to cope with this default,
they are free to choose a 1.44MB drive type at boot, or insert an
appropriate diskette. By and large, this appears to improve the
diskette compatibility for most guests.

________________________________________________________________________________

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

John Snow (10):
  fdc: move pick_geometry
  fdc: refactor pick_geometry
  fdc: add disk field
  fdc: add default drive type option
  fdc: do not call revalidate on eject
  fdc: implement new drive type property
  fdc: add physical disk sizes
  fdc: rework pick_geometry
  qtest/fdc: Support for 2.88MB drives
  fdc: change auto fallback drive to 288

 hw/block/fdc.c               | 317 +++++++++++++++++++++++++++++--------------
 hw/core/qdev-properties.c    |  11 ++
 hw/i386/pc.c                 |  17 +--
 include/hw/block/fdc.h       |   9 +-
 include/hw/qdev-properties.h |   1 +
 qapi/block.json              |  16 +++
 tests/fdc-test.c             |   2 +-
 7 files changed, 255 insertions(+), 118 deletions(-)

-- 
2.4.3

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH for-2.6 v2 01/10] fdc: move pick_geometry
  2015-12-07 23:34 [Qemu-devel] [PATCH for-2.6 v2 00/10] fdc: fix 2.88mb floppy diskette support John Snow
@ 2015-12-07 23:34 ` John Snow
  2015-12-15 21:51   ` Hervé Poussineau
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 02/10] fdc: refactor pick_geometry John Snow
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2015-12-07 23:34 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, John Snow, armbru, qemu-devel

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

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 4292ece..246b631 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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH for-2.6 v2 02/10] fdc: refactor pick_geometry
  2015-12-07 23:34 [Qemu-devel] [PATCH for-2.6 v2 00/10] fdc: fix 2.88mb floppy diskette support John Snow
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 01/10] fdc: move pick_geometry John Snow
@ 2015-12-07 23:34 ` John Snow
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 03/10] fdc: add disk field John Snow
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2015-12-07 23:34 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, John Snow, armbru, qemu-devel

Modify this function to operate directly on FDrive objects instead of
unpacking and passing all of those parameters manually.

Helps reduce complexity in each caller, and reduces the number of args.

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

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 246b631..09bb63d 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,35 @@ 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;
+
+    if (drv->media_inserted) {
+        FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n",
+                       parse->max_head + 1,
+                       drv->max_track, drv->last_sect,
+                       drv->ro ? "ro" : "rw");
+    }
 }
 
 /* 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");
         }
-        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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH for-2.6 v2 03/10] fdc: add disk field
  2015-12-07 23:34 [Qemu-devel] [PATCH for-2.6 v2 00/10] fdc: fix 2.88mb floppy diskette support John Snow
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 01/10] fdc: move pick_geometry John Snow
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 02/10] fdc: refactor pick_geometry John Snow
@ 2015-12-07 23:34 ` John Snow
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 04/10] fdc: add default drive type option John Snow
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2015-12-07 23:34 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, John Snow, armbru, qemu-devel

This allows us to distinguish between the current disk type and the
current drive type. The drive is what's reported to CMOS, the disk is
whatever the pick_geometry function suspects has been inserted.

The drive field maintains the exact same meaning as it did previously,
however pick_geometry/fd_revalidate will be refactored to *never* update
the drive field, considering it frozen in-place during an earlier
initialization call.

Before this patch, pick_geometry/fd_revalidate could only change the
drive field when it was FDRIVE_DRV_NONE, which indicated that we had
not yet reported our drive type to CMOS. After we "pick one," even
though pick_geometry/fd_revalidate re-set drv->drive, it should always
be the same as the value going into these calls, so it is effectively
already static.

As of this patch, disk and drive will always be the same, but that may
not be true by the end of this series.

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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 09bb63d..13fef23 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -133,7 +133,8 @@ typedef struct FDrive {
     FDCtrl *fdctrl;
     BlockBackend *blk;
     /* Drive status */
-    FDriveType drive;
+    FDriveType drive;         /* CMOS drive type */
+    FDriveType disk;          /* Current disk type */
     uint8_t perpendicular;    /* 2.88 MB access mode    */
     /* Position */
     uint8_t head;
@@ -157,6 +158,7 @@ static void fd_init(FDrive *drv)
     drv->drive = FDRIVE_DRV_NONE;
     drv->perpendicular = 0;
     /* Disk */
+    drv->disk = FDRIVE_DRV_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 : FDRIVE_DRV_NONE;
     drv->media_rate = parse->rate;
 
     if (drv->media_inserted) {
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH for-2.6 v2 04/10] fdc: add default drive type option
  2015-12-07 23:34 [Qemu-devel] [PATCH for-2.6 v2 00/10] fdc: fix 2.88mb floppy diskette support John Snow
                   ` (2 preceding siblings ...)
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 03/10] fdc: add disk field John Snow
@ 2015-12-07 23:34 ` John Snow
  2015-12-07 23:56   ` Eric Blake
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 05/10] fdc: do not call revalidate on eject John Snow
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2015-12-07 23:34 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, John Snow, armbru, qemu-devel

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 explicit
behaviors: 120, 144, 288, auto, and none. The new "auto" behavior
is intended to mimick current behavior, while the other types pick
one explicitly.

In a future patch, the goal is to change the FDC's default drive type
from auto (falling back to 1.44MB) to auto (falling back to 2.88MB).

In order to allow users to obtain the old behaviors, though, a mechanism
for specifying the exact type of drive we want is needed.

This patch adds the properties, but it is not acted on yet in favor of
making those changes a little more explicitly clear in standalone patches
later in this patch set.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/block/fdc.c               | 108 ++++++++++++++++++++++++++-----------------
 hw/core/qdev-properties.c    |  11 +++++
 hw/i386/pc.c                 |  17 +++----
 include/hw/block/fdc.h       |   9 +---
 include/hw/qdev-properties.h |   1 +
 qapi/block.json              |  16 +++++++
 6 files changed, 103 insertions(+), 59 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 13fef23..498eb9c 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -60,58 +60,66 @@ typedef enum FDriveRate {
 } FDriveRate;
 
 typedef struct FDFormat {
-    FDriveType drive;
+    FloppyDriveType drive;
     uint8_t last_sect;
     uint8_t max_track;
     uint8_t max_head;
     FDriveRate rate;
 } FDFormat;
 
+/**
+ * FDRIVE_DEFAULT: The default drive type if none specified.
+ * FDRIVE_AUTO_FALLBACK: The default drive type to assume if
+ *                       no media is inserted.
+ */
+#define FDRIVE_DEFAULT FLOPPY_DRIVE_TYPE_AUTO
+#define FDRIVE_AUTO_FALLBACK FLOPPY_DRIVE_TYPE_144
+
 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,8 +141,9 @@ typedef struct FDrive {
     FDCtrl *fdctrl;
     BlockBackend *blk;
     /* Drive status */
-    FDriveType drive;         /* CMOS drive type */
-    FDriveType disk;          /* Current disk type */
+    FloppyDriveType drive;         /* CMOS drive type */
+    FloppyDriveType disk;          /* Current disk type */
+
     uint8_t perpendicular;    /* 2.88 MB access mode    */
     /* Position */
     uint8_t head;
@@ -155,10 +164,10 @@ 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->disk = FDRIVE_DRV_NONE;
+    drv->disk = FLOPPY_DRIVE_TYPE_NONE;
     drv->last_sect = 0;
     drv->max_track = 0;
 }
@@ -255,11 +264,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) {
@@ -288,7 +297,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 : FDRIVE_DRV_NONE;
+    drv->disk = drv->media_inserted ? parse->drive : FLOPPY_DRIVE_TYPE_NONE;
     drv->media_rate = parse->rate;
 
     if (drv->media_inserted) {
@@ -566,6 +575,9 @@ struct FDCtrl {
     /* Timers state */
     uint8_t timer0;
     uint8_t timer1;
+
+    FloppyDriveType typeA;
+    FloppyDriveType typeB;
 };
 
 #define TYPE_SYSBUS_FDC "base-sysbus-fdc"
@@ -2396,7 +2408,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);
 
@@ -2421,6 +2433,10 @@ static Property isa_fdc_properties[] = {
     DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.drives[1].blk),
     DEFINE_PROP_BIT("check_media_rate", FDCtrlISABus, state.check_media_rate,
                     0, true),
+    DEFINE_PROP_DEFAULT("fdtypeA", FDCtrlISABus, state.typeA, FDRIVE_DEFAULT,
+                        qdev_prop_fdc_drive_type, FloppyDriveType),
+    DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlISABus, state.typeB, FDRIVE_DEFAULT,
+                        qdev_prop_fdc_drive_type, FloppyDriveType),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2469,6 +2485,10 @@ static const VMStateDescription vmstate_sysbus_fdc ={
 static Property sysbus_fdc_properties[] = {
     DEFINE_PROP_DRIVE("driveA", FDCtrlSysBus, state.drives[0].blk),
     DEFINE_PROP_DRIVE("driveB", FDCtrlSysBus, state.drives[1].blk),
+    DEFINE_PROP_DEFAULT("fdtypeA", FDCtrlSysBus, state.typeA, FDRIVE_DEFAULT,
+                        qdev_prop_fdc_drive_type, FloppyDriveType),
+    DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlSysBus, state.typeB, FDRIVE_DEFAULT,
+                        qdev_prop_fdc_drive_type, FloppyDriveType),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2489,6 +2509,8 @@ 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.typeA, FDRIVE_DEFAULT,
+                        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 33e245e..b43943e 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/hw/i386/pc.c b/hw/i386/pc.c
index 5e20e07..af48af8 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -207,24 +207,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;
@@ -295,7 +295,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) {
@@ -309,10 +310,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/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 77538a8..c26797e 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -20,6 +20,7 @@ extern PropertyInfo qdev_prop_ptr;
 extern PropertyInfo qdev_prop_macaddr;
 extern PropertyInfo qdev_prop_losttickpolicy;
 extern PropertyInfo qdev_prop_bios_chs_trans;
+extern PropertyInfo qdev_prop_fdc_drive_type;
 extern PropertyInfo qdev_prop_drive;
 extern PropertyInfo qdev_prop_netdev;
 extern PropertyInfo qdev_prop_vlan;
diff --git a/qapi/block.json b/qapi/block.json
index 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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH for-2.6 v2 05/10] fdc: do not call revalidate on eject
  2015-12-07 23:34 [Qemu-devel] [PATCH for-2.6 v2 00/10] fdc: fix 2.88mb floppy diskette support John Snow
                   ` (3 preceding siblings ...)
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 04/10] fdc: add default drive type option John Snow
@ 2015-12-07 23:34 ` John Snow
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 06/10] fdc: implement new drive type property John Snow
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2015-12-07 23:34 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, John Snow, armbru, qemu-devel

Currently, fd_revalidate is called in two different places, with two
different expectations of behavior:

(1) On initialization, as a routine to help pick the drive type and
    initial geometries as a side-effect of the pick_geometry routine

(2) On insert/eject, which either sets the geometries to a default value
    or chooses new geometries based on the inserted diskette.

Break this nonsense apart by creating a new function dedicated towards
picking the drive type on initialization.

This has a few results:

(1) fd_revalidate does not get called on boot anymore for drives with no
    diskette.

(2) pick_geometry will actually get called twice if we have a diskette
    inserted, but this is harmless. (Once for the drive type, and once
    as part of the media callback.)

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

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 498eb9c..39e680b 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -170,6 +170,7 @@ static void fd_init(FDrive *drv)
     drv->disk = FLOPPY_DRIVE_TYPE_NONE;
     drv->last_sect = 0;
     drv->max_track = 0;
+    drv->ro = true;
 }
 
 #define NUM_SIDES(drv) ((drv)->flags & FDISK_DBL_SIDES ? 2 : 1)
@@ -252,13 +253,21 @@ 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.
+ */
+static bool 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 false;
+    }
+
     blk_get_geometry(blk, &nb_sectors);
     match = -1;
     first_match = -1;
@@ -296,8 +305,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->disk = parse->drive;
     drv->media_rate = parse->rate;
 
     if (drv->media_inserted) {
@@ -306,6 +314,14 @@ static void pick_geometry(FDrive *drv)
                        drv->max_track, drv->last_sect,
                        drv->ro ? "ro" : "rw");
     }
+    return true;
+}
+
+static void pick_drive_type(FDrive *drv)
+{
+    if (pick_geometry(drv)) {
+        drv->drive = drv->disk;
+    }
 }
 
 /* Revalidate a disk drive after a disk change */
@@ -314,15 +330,18 @@ static void fd_revalidate(FDrive *drv)
     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");
+            drv->disk = FLOPPY_DRIVE_TYPE_NONE;
+        } else {
+            pick_geometry(drv);
         }
     } else {
         FLOPPY_DPRINTF("No drive connected\n");
         drv->last_sect = 0;
         drv->max_track = 0;
         drv->flags &= ~FDISK_DBL_SIDES;
+        drv->disk = FLOPPY_DRIVE_TYPE_NONE;
     }
 }
 
@@ -2194,9 +2213,11 @@ static void fdctrl_change_cb(void *opaque, bool load)
     FDrive *drive = opaque;
 
     drive->media_inserted = load && drive->blk && blk_is_inserted(drive->blk);
-
     drive->media_changed = 1;
-    fd_revalidate(drive);
+
+    if (load) {
+        fd_revalidate(drive);
+    }
 }
 
 static bool fdctrl_is_tray_open(void *opaque)
@@ -2232,11 +2253,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);
         }
+        fdctrl_change_cb(drive, drive->media_inserted);
     }
 }
 
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH for-2.6 v2 06/10] fdc: implement new drive type property
  2015-12-07 23:34 [Qemu-devel] [PATCH for-2.6 v2 00/10] fdc: fix 2.88mb floppy diskette support John Snow
                   ` (4 preceding siblings ...)
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 05/10] fdc: do not call revalidate on eject John Snow
@ 2015-12-07 23:34 ` John Snow
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 07/10] fdc: add physical disk sizes John Snow
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2015-12-07 23:34 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, John Snow, armbru, qemu-devel

Respect the drive type as given via the CLI.

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

The pick_geometry algorithm is modified to only allow matches outside
of the existing drive type for the new auto behavior. If a user specifies
the "none" type, QEMU will not report this drive to the CMOS.

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

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 39e680b..9bb3021 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -161,10 +161,12 @@ typedef struct FDrive {
     bool media_inserted;      /* Is there a medium in the tray */
 } FDrive;
 
+static FloppyDriveType get_default_drive_type(FDrive *drv);
+
 static void fd_init(FDrive *drv)
 {
     /* Drive */
-    drv->drive = FLOPPY_DRIVE_TYPE_NONE;
+    drv->drive = get_default_drive_type(drv);
     drv->perpendicular = 0;
     /* Disk */
     drv->disk = FLOPPY_DRIVE_TYPE_NONE;
@@ -277,7 +279,7 @@ static bool 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) {
@@ -291,13 +293,17 @@ static bool pick_geometry(FDrive *drv)
     }
     if (match == -1) {
         if (first_match == -1) {
-            match = 1;
+            /* No entry found: drive_type was NONE or we neglected to add any
+             * candidate geometries for our drive type into the fd_formats table
+             */
+            match = ARRAY_SIZE(fd_formats) - 1;
         } else {
             match = first_match;
         }
         parse = &fd_formats[match];
     }
 
+ out:
     if (parse->max_head == 0) {
         drv->flags &= ~FDISK_DBL_SIDES;
     } else {
@@ -319,8 +325,16 @@ static bool pick_geometry(FDrive *drv)
 
 static void pick_drive_type(FDrive *drv)
 {
-    if (pick_geometry(drv)) {
-        drv->drive = drv->disk;
+    if (drv->drive != FLOPPY_DRIVE_TYPE_AUTO) {
+        return;
+    }
+
+    if (!drv->media_inserted) {
+        drv->drive = FDRIVE_AUTO_FALLBACK;
+    } else {
+        if (pick_geometry(drv)) {
+            drv->drive = drv->disk;
+        }
     }
 }
 
@@ -623,6 +637,27 @@ typedef struct FDCtrlISABus {
     int32_t bootindexB;
 } FDCtrlISABus;
 
+static FloppyDriveType get_default_drive_type(FDrive *drv)
+{
+    FDCtrl *fdctrl = drv->fdctrl;
+
+    g_assert_cmpint(MAX_FD, ==, 2);
+
+    if (!drv->blk) {
+        return FLOPPY_DRIVE_TYPE_NONE;
+    }
+
+    if (drv == &fdctrl->drives[0]) {
+        return fdctrl->typeA;
+    }
+
+    if (drv == &fdctrl->drives[1]) {
+        return fdctrl->typeB;
+    }
+
+    return FDRIVE_DEFAULT;
+}
+
 static uint32_t fdctrl_read (void *opaque, uint32_t reg)
 {
     FDCtrl *fdctrl = opaque;
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH for-2.6 v2 07/10] fdc: add physical disk sizes
  2015-12-07 23:34 [Qemu-devel] [PATCH for-2.6 v2 00/10] fdc: fix 2.88mb floppy diskette support John Snow
                   ` (5 preceding siblings ...)
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 06/10] fdc: implement new drive type property John Snow
@ 2015-12-07 23:34 ` John Snow
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 08/10] fdc: rework pick_geometry John Snow
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2015-12-07 23:34 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, John Snow, armbru, qemu-devel

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 9bb3021..12a2595 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;
@@ -75,11 +81,15 @@ typedef struct FDFormat {
 #define FDRIVE_DEFAULT FLOPPY_DRIVE_TYPE_AUTO
 #define FDRIVE_AUTO_FALLBACK FLOPPY_DRIVE_TYPE_144
 
+/* 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, },
@@ -93,7 +103,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, },
@@ -101,15 +111,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, },
@@ -117,11 +127,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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH for-2.6 v2 08/10] fdc: rework pick_geometry
  2015-12-07 23:34 [Qemu-devel] [PATCH for-2.6 v2 00/10] fdc: fix 2.88mb floppy diskette support John Snow
                   ` (6 preceding siblings ...)
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 07/10] fdc: add physical disk sizes John Snow
@ 2015-12-07 23:34 ` John Snow
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 09/10] qtest/fdc: Support for 2.88MB drives John Snow
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 10/10] fdc: change auto fallback drive to 288 John Snow
  9 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2015-12-07 23:34 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, John Snow, armbru, qemu-devel

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 and type matches, choose it.
 - Fall back to the first geometry that matched our type.

The new one is:
 - If the size 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, 40 insertions(+), 23 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 12a2595..246bd83 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -132,7 +132,6 @@ static const FDFormat fd_formats[] = {
     { FLOPPY_DRIVE_TYPE_NONE, -1, -1, 0, 0, },
 };
 
-__attribute__((__unused__))
 static FDriveSize drive_size(FloppyDriveType drive)
 {
     switch (drive) {
@@ -287,45 +286,63 @@ static bool 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) {
         return false;
     }
 
+    /* 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;
-            }
-            if (first_match == -1) {
-                first_match = i;
+        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;
             }
+        } else if ((type_match == -1) &&
+                   ((parse->drive == drv->drive) ||
+                    (magic && (parse->drive == FDRIVE_AUTO_FALLBACK)))) {
+            /* (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) {
-            /* No entry found: drive_type was NONE or we neglected to add any
-             * candidate geometries for our drive type into the fd_formats table
-             */
-            match = ARRAY_SIZE(fd_formats) - 1;
-        } else {
-            match = first_match;
-        }
-        parse = &fd_formats[match];
+        match = (size_match != -1) ? size_match : type_match;
+    }
+
+    if (match == -1) {
+        /* No entry found: drive_type was NONE or we neglected to add any
+         * candidate geometries for our drive type into the fd_formats table. */
+        match = ARRAY_SIZE(fd_formats) - 1;
     }
+    parse = &(fd_formats[match]);
 
  out:
     if (parse->max_head == 0) {
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH for-2.6 v2 09/10] qtest/fdc: Support for 2.88MB drives
  2015-12-07 23:34 [Qemu-devel] [PATCH for-2.6 v2 00/10] fdc: fix 2.88mb floppy diskette support John Snow
                   ` (7 preceding siblings ...)
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 08/10] fdc: rework pick_geometry John Snow
@ 2015-12-07 23:34 ` John Snow
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 10/10] fdc: change auto fallback drive to 288 John Snow
  9 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2015-12-07 23:34 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, John Snow, armbru, qemu-devel

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH for-2.6 v2 10/10] fdc: change auto fallback drive to 288
  2015-12-07 23:34 [Qemu-devel] [PATCH for-2.6 v2 00/10] fdc: fix 2.88mb floppy diskette support John Snow
                   ` (8 preceding siblings ...)
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 09/10] qtest/fdc: Support for 2.88MB drives John Snow
@ 2015-12-07 23:34 ` John Snow
  9 siblings, 0 replies; 17+ messages in thread
From: John Snow @ 2015-12-07 23:34 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, John Snow, armbru, qemu-devel

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.

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

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 246bd83..a82ddd0 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -79,7 +79,7 @@ typedef struct FDFormat {
  *                       no media is inserted.
  */
 #define FDRIVE_DEFAULT FLOPPY_DRIVE_TYPE_AUTO
-#define FDRIVE_AUTO_FALLBACK FLOPPY_DRIVE_TYPE_144
+#define FDRIVE_AUTO_FALLBACK FLOPPY_DRIVE_TYPE_288
 
 /* In many cases, the total sector size of a format is enough to uniquely
  * identify it. However, there are some total sector collisions between
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.6 v2 04/10] fdc: add default drive type option
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 04/10] fdc: add default drive type option John Snow
@ 2015-12-07 23:56   ` Eric Blake
  2015-12-14 20:05     ` John Snow
  0 siblings, 1 reply; 17+ messages in thread
From: Eric Blake @ 2015-12-07 23:56 UTC (permalink / raw)
  To: John Snow, qemu-block; +Cc: kwolf, armbru, qemu-devel

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

On 12/07/2015 04:34 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 explicit
> behaviors: 120, 144, 288, auto, and none. The new "auto" behavior
> is intended to mimick current behavior, while the other types pick

s/mimick/mimic/ (one of those weird 'ic' verbs where the 'k' is
necessary in past tense but not present tense)

> one explicitly.
> 
> In a future patch, the goal is to change the FDC's default drive type
> from auto (falling back to 1.44MB) to auto (falling back to 2.88MB).
> 
> In order to allow users to obtain the old behaviors, though, a mechanism
> for specifying the exact type of drive we want is needed.
> 
> This patch adds the properties, but it is not acted on yet in favor of
> making those changes a little more explicitly clear in standalone patches
> later in this patch set.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---

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

Names start with a digit - not the prettiest, but also not the first
instance, so qapi handles it just fine.  And I don't have any
suggestions for a better yet still concise name.

> +# @none: No drive connected
> +# @auto: Automatically determined by inserted media at boot
> +#
> +# Since: 2.6
> +##
> +{ 'enum': 'FloppyDriveType',
> +  'data': ['144', '288', '120', 'none', 'auto']}

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.6 v2 04/10] fdc: add default drive type option
  2015-12-07 23:56   ` Eric Blake
@ 2015-12-14 20:05     ` John Snow
  2015-12-15 16:29       ` Eric Blake
  0 siblings, 1 reply; 17+ messages in thread
From: John Snow @ 2015-12-14 20:05 UTC (permalink / raw)
  To: Eric Blake, qemu-block; +Cc: kwolf, armbru, qemu-devel



On 12/07/2015 06:56 PM, Eric Blake wrote:
> On 12/07/2015 04:34 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 explicit
>> behaviors: 120, 144, 288, auto, and none. The new "auto" behavior
>> is intended to mimick current behavior, while the other types pick
> 
> s/mimick/mimic/ (one of those weird 'ic' verbs where the 'k' is
> necessary in past tense but not present tense)
> 

>> one explicitly.
>>
>> In a future patch, the goal is to change the FDC's default drive type
>> from auto (falling back to 1.44MB) to auto (falling back to 2.88MB).
>>
>> In order to allow users to obtain the old behaviors, though, a mechanism
>> for specifying the exact type of drive we want is needed.
>>
>> This patch adds the properties, but it is not acted on yet in favor of
>> making those changes a little more explicitly clear in standalone patches
>> later in this patch set.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
> 
>> +++ 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
> 
> Names start with a digit - not the prettiest, but also not the first
> instance, so qapi handles it just fine.  And I don't have any
> suggestions for a better yet still concise name.
> 
>> +# @none: No drive connected
>> +# @auto: Automatically determined by inserted media at boot
>> +#
>> +# Since: 2.6
>> +##
>> +{ 'enum': 'FloppyDriveType',
>> +  'data': ['144', '288', '120', 'none', 'auto']}
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

I was actually contemplating re-spinning this for a v3:

Instead of having a "typeA" and "typeB" properties of the FDC, I'll just
spin the properties in such a way that they write directly to
FDCtrl.drives[n].drive, which avoids the need for two new members and a
method designed to "fetch" the default from the controller.

I also want to add a "fallback" member to the FDC as a CLI configurable
parameter such that when using "auto" you can configure directly what
type of drive you'll get.

This way the default behavior will be "auto" (but configurable) and the
fallback if the drive is empty or it cannot make a confident guess will
be whatever the user chose as "fallback" -- presumed "144" before this
series and "288" afterwards.

I believe this also opens up the possibility for having "fallback"
default to "144" for machine types created prior to 2.6 ... I think.
(I'm less familiar with machine compat code as of yet.)

Sound reasonable?

--js

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.6 v2 04/10] fdc: add default drive type option
  2015-12-14 20:05     ` John Snow
@ 2015-12-15 16:29       ` Eric Blake
  0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2015-12-15 16:29 UTC (permalink / raw)
  To: John Snow, qemu-block; +Cc: kwolf, armbru, qemu-devel

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

On 12/14/2015 01:05 PM, John Snow wrote:

> I was actually contemplating re-spinning this for a v3:
> 
> Instead of having a "typeA" and "typeB" properties of the FDC, I'll just
> spin the properties in such a way that they write directly to
> FDCtrl.drives[n].drive, which avoids the need for two new members and a
> method designed to "fetch" the default from the controller.
> 
> I also want to add a "fallback" member to the FDC as a CLI configurable
> parameter such that when using "auto" you can configure directly what
> type of drive you'll get.
> 
> This way the default behavior will be "auto" (but configurable) and the
> fallback if the drive is empty or it cannot make a confident guess will
> be whatever the user chose as "fallback" -- presumed "144" before this
> series and "288" afterwards.
> 
> I believe this also opens up the possibility for having "fallback"
> default to "144" for machine types created prior to 2.6 ... I think.
> (I'm less familiar with machine compat code as of yet.)
> 
> Sound reasonable?

On paper, yes that sounds reasonable. I'm also not familiar enough with
machine types to know if it will let you keep 2.5 and earlier machines
with a fallback of 144, and newer machines with a fallback of 288, but
sounds promising.


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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.6 v2 01/10] fdc: move pick_geometry
  2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 01/10] fdc: move pick_geometry John Snow
@ 2015-12-15 21:51   ` Hervé Poussineau
  2015-12-15 21:56     ` John Snow
  2015-12-16 21:14     ` John Snow
  0 siblings, 2 replies; 17+ messages in thread
From: Hervé Poussineau @ 2015-12-15 21:51 UTC (permalink / raw)
  To: John Snow, qemu-block; +Cc: kwolf, armbru, qemu-devel

Le 08/12/2015 00:34, John Snow a écrit :
> Code motion: I want to refactor this function to work with FDrive
> directly, so shuffle it below that definition.
>
> 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 4292ece..246b631 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)
>   {

I think it would be better to move the FDiskFlags enum, FDrive, FDCtrl, FDCtrlISABus structures to include/hw/block/fdc.h
That way, we can embed the floppy controller into another chip. I'll need it soon for some PIIX4 improvements.

Regards,

Hervé

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.6 v2 01/10] fdc: move pick_geometry
  2015-12-15 21:51   ` Hervé Poussineau
@ 2015-12-15 21:56     ` John Snow
  2015-12-16 21:14     ` John Snow
  1 sibling, 0 replies; 17+ messages in thread
From: John Snow @ 2015-12-15 21:56 UTC (permalink / raw)
  To: Hervé Poussineau, qemu-block; +Cc: kwolf, armbru, qemu-devel



On 12/15/2015 04:51 PM, Hervé Poussineau wrote:
> Le 08/12/2015 00:34, John Snow a écrit :
>> Code motion: I want to refactor this function to work with FDrive
>> directly, so shuffle it below that definition.
>>
>> 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 4292ece..246b631 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)
>>   {
> 
> I think it would be better to move the FDiskFlags enum, FDrive, FDCtrl,
> FDCtrlISABus structures to include/hw/block/fdc.h
> That way, we can embed the floppy controller into another chip. I'll
> need it soon for some PIIX4 improvements.
> 
> Regards,
> 
> Hervé
> 

Deal.

(Why are you embedding a floppy controller? :)

--js

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH for-2.6 v2 01/10] fdc: move pick_geometry
  2015-12-15 21:51   ` Hervé Poussineau
  2015-12-15 21:56     ` John Snow
@ 2015-12-16 21:14     ` John Snow
  1 sibling, 0 replies; 17+ messages in thread
From: John Snow @ 2015-12-16 21:14 UTC (permalink / raw)
  To: Hervé Poussineau, qemu-block; +Cc: kwolf, armbru, qemu-devel



On 12/15/2015 04:51 PM, Hervé Poussineau wrote:
> Le 08/12/2015 00:34, John Snow a écrit :
>> Code motion: I want to refactor this function to work with FDrive
>> directly, so shuffle it below that definition.
>>
>> 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 4292ece..246b631 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)
>>   {
> 
> I think it would be better to move the FDiskFlags enum, FDrive, FDCtrl,
> FDCtrlISABus structures to include/hw/block/fdc.h
> That way, we can embed the floppy controller into another chip. I'll
> need it soon for some PIIX4 improvements.
> 
> Regards,
> 
> Hervé
> 

Actually, I'm going to decline on this -- I don't know precisely what
you need moved, and it's trivial for you to cut the headers out when you
need them. It'll be more work re-shuffling this series than it's worth
for me to then not get it completely right anyway.

Just ping me with a patch to move what you need and I will get that done
for you then, thanks.

--js

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2015-12-16 21:14 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-07 23:34 [Qemu-devel] [PATCH for-2.6 v2 00/10] fdc: fix 2.88mb floppy diskette support John Snow
2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 01/10] fdc: move pick_geometry John Snow
2015-12-15 21:51   ` Hervé Poussineau
2015-12-15 21:56     ` John Snow
2015-12-16 21:14     ` John Snow
2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 02/10] fdc: refactor pick_geometry John Snow
2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 03/10] fdc: add disk field John Snow
2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 04/10] fdc: add default drive type option John Snow
2015-12-07 23:56   ` Eric Blake
2015-12-14 20:05     ` John Snow
2015-12-15 16:29       ` Eric Blake
2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 05/10] fdc: do not call revalidate on eject John Snow
2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 06/10] fdc: implement new drive type property John Snow
2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 07/10] fdc: add physical disk sizes John Snow
2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 08/10] fdc: rework pick_geometry John Snow
2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 09/10] qtest/fdc: Support for 2.88MB drives John Snow
2015-12-07 23:34 ` [Qemu-devel] [PATCH for-2.6 v2 10/10] fdc: change auto fallback drive to 288 John Snow

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.