* [Qemu-devel] [PATCH v3 00/11] fdc: fix 2.88mb floppy diskette support
@ 2015-12-16 22:16 John Snow
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 01/11] fdc: move pick_geometry John Snow
` (10 more replies)
0 siblings, 11 replies; 29+ messages in thread
From: John Snow @ 2015-12-16 22:16 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, John Snow, armbru, qemu-devel
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.
===
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-v3:
https://github.com/jnsnow/qemu/releases/tag/fdc-default-v3
John Snow (11):
fdc: move pick_geometry
fdc: refactor pick_geometry
fdc: add disk field
fdc: add default drive type option
fdc: Add fallback 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 for ISA FDC 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/compat.h | 6 +-
include/hw/qdev-properties.h | 1 +
qapi/block.json | 16 +++
tests/fdc-test.c | 2 +-
8 files changed, 260 insertions(+), 119 deletions(-)
--
2.4.3
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v3 01/11] fdc: move pick_geometry
2015-12-16 22:16 [Qemu-devel] [PATCH v3 00/11] fdc: fix 2.88mb floppy diskette support John Snow
@ 2015-12-16 22:16 ` John Snow
2015-12-18 16:15 ` Eric Blake
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 02/11] fdc: refactor pick_geometry John Snow
` (9 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2015-12-16 22:16 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] 29+ messages in thread
* [Qemu-devel] [PATCH v3 02/11] fdc: refactor pick_geometry
2015-12-16 22:16 [Qemu-devel] [PATCH v3 00/11] fdc: fix 2.88mb floppy diskette support John Snow
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 01/11] fdc: move pick_geometry John Snow
@ 2015-12-16 22:16 ` John Snow
2015-12-17 7:53 ` Markus Armbruster
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 03/11] fdc: add disk field John Snow
` (8 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2015-12-16 22:16 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] 29+ messages in thread
* [Qemu-devel] [PATCH v3 03/11] fdc: add disk field
2015-12-16 22:16 [Qemu-devel] [PATCH v3 00/11] fdc: fix 2.88mb floppy diskette support John Snow
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 01/11] fdc: move pick_geometry John Snow
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 02/11] fdc: refactor pick_geometry John Snow
@ 2015-12-16 22:16 ` John Snow
2015-12-17 8:30 ` Markus Armbruster
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 04/11] fdc: add default drive type option John Snow
` (7 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2015-12-16 22:16 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] 29+ messages in thread
* [Qemu-devel] [PATCH v3 04/11] fdc: add default drive type option
2015-12-16 22:16 [Qemu-devel] [PATCH v3 00/11] fdc: fix 2.88mb floppy diskette support John Snow
` (2 preceding siblings ...)
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 03/11] fdc: add disk field John Snow
@ 2015-12-16 22:16 ` John Snow
2015-12-16 23:03 ` Eric Blake
2015-12-18 15:54 ` Markus Armbruster
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 05/11] fdc: Add fallback option John Snow
` (6 subsequent siblings)
10 siblings, 2 replies; 29+ messages in thread
From: John Snow @ 2015-12-16 22:16 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 | 107 ++++++++++++++++++++++++++-----------------
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, 102 insertions(+), 59 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 13fef23..ad0e052 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -60,58 +60,60 @@ 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;
+#define FDRIVE_DEFAULT FLOPPY_DRIVE_TYPE_AUTO
+
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 +135,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 +158,10 @@ typedef struct FDrive {
static void fd_init(FDrive *drv)
{
/* Drive */
- drv->drive = FDRIVE_DRV_NONE;
+ drv->drive = FLOPPY_DRIVE_TYPE_NONE; /* FIXME: Obey CLI properties */
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 +258,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 +291,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 +569,7 @@ struct FDCtrl {
/* Timers state */
uint8_t timer0;
uint8_t timer1;
+
};
#define TYPE_SYSBUS_FDC "base-sysbus-fdc"
@@ -2224,6 +2228,8 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl, Error **errp)
if (drive->blk) {
blk_set_dev_ops(drive->blk, &fdctrl_block_ops, drive);
drive->media_inserted = blk_is_inserted(drive->blk);
+ } else {
+ drive->drive = FLOPPY_DRIVE_TYPE_NONE;
}
}
}
@@ -2396,7 +2402,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 +2427,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,
+ FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
+ FloppyDriveType),
+ DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlISABus, state.drives[1].drive,
+ FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
+ FloppyDriveType),
DEFINE_PROP_END_OF_LIST(),
};
@@ -2469,6 +2481,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,
+ FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
+ FloppyDriveType),
+ DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlSysBus, state.drives[1].drive,
+ FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
+ FloppyDriveType),
DEFINE_PROP_END_OF_LIST(),
};
@@ -2489,6 +2507,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,
+ 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 72d9b9c..02bfc74 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] 29+ messages in thread
* [Qemu-devel] [PATCH v3 05/11] fdc: Add fallback option
2015-12-16 22:16 [Qemu-devel] [PATCH v3 00/11] fdc: fix 2.88mb floppy diskette support John Snow
` (3 preceding siblings ...)
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 04/11] fdc: add default drive type option John Snow
@ 2015-12-16 22:16 ` John Snow
2015-12-18 15:57 ` Markus Armbruster
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 06/11] fdc: do not call revalidate on eject John Snow
` (5 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2015-12-16 22:16 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, John Snow, armbru, qemu-devel
Add the fallback drive type as an option so we can control
the behavior as a function of the QEMU machine version.
Signed-off-by: John Snow <jsnow@redhat.com>
---
hw/block/fdc.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index ad0e052..b587de8 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -155,6 +155,9 @@ typedef struct FDrive {
bool media_inserted; /* Is there a medium in the tray */
} FDrive;
+
+static FloppyDriveType get_fallback_drive_type(FDrive *drv);
+
static void fd_init(FDrive *drv)
{
/* Drive */
@@ -570,8 +573,15 @@ struct FDCtrl {
uint8_t timer0;
uint8_t timer1;
+ FloppyDriveType fallback;
};
+__attribute__((__unused__))
+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)
@@ -2302,6 +2312,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;
@@ -2433,6 +2447,9 @@ static Property isa_fdc_properties[] = {
DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlISABus, state.drives[1].drive,
FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
FloppyDriveType),
+ DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
+ FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
+ FloppyDriveType),
DEFINE_PROP_END_OF_LIST(),
};
@@ -2487,6 +2504,9 @@ static Property sysbus_fdc_properties[] = {
DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlSysBus, state.drives[1].drive,
FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
FloppyDriveType),
+ DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
+ FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
+ FloppyDriveType),
DEFINE_PROP_END_OF_LIST(),
};
@@ -2510,6 +2530,9 @@ static Property sun4m_fdc_properties[] = {
DEFINE_PROP_DEFAULT("fdtype", FDCtrlSysBus, state.drives[0].drive,
FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
FloppyDriveType),
+ DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
+ FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
+ FloppyDriveType),
DEFINE_PROP_END_OF_LIST(),
};
--
2.4.3
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v3 06/11] fdc: do not call revalidate on eject
2015-12-16 22:16 [Qemu-devel] [PATCH v3 00/11] fdc: fix 2.88mb floppy diskette support John Snow
` (4 preceding siblings ...)
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 05/11] fdc: Add fallback option John Snow
@ 2015-12-16 22:16 ` John Snow
2015-12-18 16:11 ` Markus Armbruster
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 07/11] fdc: implement new drive type property John Snow
` (4 subsequent siblings)
10 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2015-12-16 22:16 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 b587de8..e752758 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -167,6 +167,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)
@@ -249,13 +250,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;
@@ -293,8 +302,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) {
@@ -303,6 +311,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 */
@@ -311,15 +327,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;
}
}
@@ -2196,9 +2215,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)
@@ -2234,13 +2255,14 @@ 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);
} else {
drive->drive = FLOPPY_DRIVE_TYPE_NONE;
}
+ fdctrl_change_cb(drive, drive->media_inserted);
}
}
--
2.4.3
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v3 07/11] fdc: implement new drive type property
2015-12-16 22:16 [Qemu-devel] [PATCH v3 00/11] fdc: fix 2.88mb floppy diskette support John Snow
` (5 preceding siblings ...)
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 06/11] fdc: do not call revalidate on eject John Snow
@ 2015-12-16 22:16 ` John Snow
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 08/11] fdc: add physical disk sizes John Snow
` (3 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2015-12-16 22:16 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 | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index e752758..f44472c 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -155,13 +155,11 @@ typedef struct FDrive {
bool media_inserted; /* Is there a medium in the tray */
} FDrive;
-
static FloppyDriveType get_fallback_drive_type(FDrive *drv);
static void fd_init(FDrive *drv)
{
/* Drive */
- drv->drive = FLOPPY_DRIVE_TYPE_NONE; /* FIXME: Obey CLI properties */
drv->perpendicular = 0;
/* Disk */
drv->disk = FLOPPY_DRIVE_TYPE_NONE;
@@ -274,7 +272,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) {
@@ -288,7 +286,10 @@ 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;
}
@@ -316,9 +317,19 @@ 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 = get_fallback_drive_type(drv);
+ } else {
+ if (pick_geometry(drv)) {
+ drv->drive = drv->disk;
+ }
+ }
+
+ g_assert(drv->drive != FLOPPY_DRIVE_TYPE_AUTO);
}
/* Revalidate a disk drive after a disk change */
--
2.4.3
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v3 08/11] fdc: add physical disk sizes
2015-12-16 22:16 [Qemu-devel] [PATCH v3 00/11] fdc: fix 2.88mb floppy diskette support John Snow
` (6 preceding siblings ...)
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 07/11] fdc: implement new drive type property John Snow
@ 2015-12-16 22:16 ` John Snow
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 09/11] fdc: rework pick_geometry John Snow
` (2 subsequent siblings)
10 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2015-12-16 22:16 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 f44472c..1d0c594 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;
@@ -69,11 +75,15 @@ typedef struct FDFormat {
#define FDRIVE_DEFAULT FLOPPY_DRIVE_TYPE_AUTO
+/* 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, },
@@ -87,7 +97,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, },
@@ -95,15 +105,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, },
@@ -111,11 +121,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] 29+ messages in thread
* [Qemu-devel] [PATCH v3 09/11] fdc: rework pick_geometry
2015-12-16 22:16 [Qemu-devel] [PATCH v3 00/11] fdc: fix 2.88mb floppy diskette support John Snow
` (7 preceding siblings ...)
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 08/11] fdc: add physical disk sizes John Snow
@ 2015-12-16 22:16 ` John Snow
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 10/11] qtest/fdc: Support for 2.88MB drives John Snow
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 11/11] fdc: change auto fallback drive for ISA FDC to 288 John Snow
10 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2015-12-16 22:16 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, 41 insertions(+), 22 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 1d0c594..b9b9bf7 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -126,7 +126,6 @@ static const FDFormat fd_formats[] = {
{ FLOPPY_DRIVE_TYPE_NONE, -1, -1, 0, 0, },
};
-__attribute__((__unused__))
static FDriveSize drive_size(FloppyDriveType drive)
{
switch (drive) {
@@ -280,46 +279,66 @@ 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;
+ 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) {
- /* 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) {
drv->flags &= ~FDISK_DBL_SIDES;
} else {
--
2.4.3
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v3 10/11] qtest/fdc: Support for 2.88MB drives
2015-12-16 22:16 [Qemu-devel] [PATCH v3 00/11] fdc: fix 2.88mb floppy diskette support John Snow
` (8 preceding siblings ...)
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 09/11] fdc: rework pick_geometry John Snow
@ 2015-12-16 22:16 ` John Snow
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 11/11] fdc: change auto fallback drive for ISA FDC to 288 John Snow
10 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2015-12-16 22:16 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] 29+ messages in thread
* [Qemu-devel] [PATCH v3 11/11] fdc: change auto fallback drive for ISA FDC to 288
2015-12-16 22:16 [Qemu-devel] [PATCH v3 00/11] fdc: fix 2.88mb floppy diskette support John Snow
` (9 preceding siblings ...)
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 10/11] qtest/fdc: Support for 2.88MB drives John Snow
@ 2015-12-16 22:16 ` John Snow
2015-12-16 22:30 ` John Snow
10 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2015-12-16 22:16 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.
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 | 6 +++++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index b9b9bf7..745b284 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -2524,7 +2524,7 @@ static Property isa_fdc_properties[] = {
FDRIVE_DEFAULT, 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 24dd2c0..5a53f3c 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -2,7 +2,11 @@
#define HW_COMPAT_H
#define HW_COMPAT_2_5 \
- /* empty */
+ {\
+ .driver = "fdc",\
+ .property = "fallback",\
+ .value = "144",\
+ },
#define HW_COMPAT_2_4 \
{\
--
2.4.3
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v3 11/11] fdc: change auto fallback drive for ISA FDC to 288
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 11/11] fdc: change auto fallback drive for ISA FDC to 288 John Snow
@ 2015-12-16 22:30 ` John Snow
0 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2015-12-16 22:30 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, armbru, qemu-devel
On 12/16/2015 05:16 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 | 6 +++++-
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index b9b9bf7..745b284 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -2524,7 +2524,7 @@ static Property isa_fdc_properties[] = {
> FDRIVE_DEFAULT, 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 24dd2c0..5a53f3c 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -2,7 +2,11 @@
> #define HW_COMPAT_H
>
> #define HW_COMPAT_2_5 \
> - /* empty */
> + {\
> + .driver = "fdc",\
This should be "isa-fdc". >:[
> + .property = "fallback",\
> + .value = "144",\
> + },
>
> #define HW_COMPAT_2_4 \
> {\
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v3 04/11] fdc: add default drive type option
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 04/11] fdc: add default drive type option John Snow
@ 2015-12-16 23:03 ` Eric Blake
2015-12-18 15:54 ` Markus Armbruster
1 sibling, 0 replies; 29+ messages in thread
From: Eric Blake @ 2015-12-16 23:03 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: kwolf, armbru, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1365 bytes --]
On 12/16/2015 03:16 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
> 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>
> ---
> @@ -566,6 +569,7 @@ struct FDCtrl {
> /* Timers state */
> uint8_t timer0;
> uint8_t timer1;
> +
> };
>
Spurious blank line?
Otherwise, looks good;
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] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v3 02/11] fdc: refactor pick_geometry
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 02/11] fdc: refactor pick_geometry John Snow
@ 2015-12-17 7:53 ` Markus Armbruster
2015-12-17 17:50 ` John Snow
0 siblings, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2015-12-17 7:53 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, qemu-devel, qemu-block
John Snow <jsnow@redhat.com> writes:
> 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.
For now, there's just one.
Diffstat suggests it's an overall simplification.
> 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");
> + }
One half of the debug print moved from...
> }
>
> /* 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");
> }
... here. Recommend to move both or none. If you add more callers,
both might make more sense.
> - 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;
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v3 03/11] fdc: add disk field
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 03/11] fdc: add disk field John Snow
@ 2015-12-17 8:30 ` Markus Armbruster
2015-12-17 16:59 ` John Snow
0 siblings, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2015-12-17 8:30 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, qemu-devel, qemu-block
John Snow <jsnow@redhat.com> writes:
> 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 {
typedef struct FDrive {
> FDCtrl *fdctrl;
> BlockBackend *blk;
> /* Drive status */
> - FDriveType drive;
> + FDriveType drive; /* CMOS drive type */
> + FDriveType disk; /* Current disk type */
where
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;
FDriveType makes obvious sense as drive type.
> uint8_t perpendicular; /* 2.88 MB access mode */
If I understand @perpendicular correctly, it's just an extra hoop a
driver needs to jump through to actually access a 2.88M disk.
> /* Position */
> uint8_t head;
uint8_t track;
uint8_t sect;
/* Media */
Shouldn't @disk go here?
FDiskFlags flags;
uint8_t last_sect; /* Nb sector per track */
uint8_t max_track; /* Nb of tracks */
uint16_t bps; /* Bytes per sector */
uint8_t ro; /* Is read-only */
uint8_t media_changed; /* Is media changed */
uint8_t media_rate; /* Data rate of medium */
bool media_inserted; /* Is there a medium in the tray */
} FDrive;
A disk / medium is characterised by it's form factor, density /
FDriveRate, #sides, #tracks, #sectors per track, and a read-only bit
(ignoring a few details that don't interest us). Obviously, the drive
type restricts possible disk types.
What does @disk add over the media description we already have?
Aside: @bps appears to be write-only, and the value written looks odd.
> @@ -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) {
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v3 03/11] fdc: add disk field
2015-12-17 8:30 ` Markus Armbruster
@ 2015-12-17 16:59 ` John Snow
2015-12-17 18:15 ` Markus Armbruster
0 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2015-12-17 16:59 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel, qemu-block
On 12/17/2015 03:30 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> 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 {
> typedef struct FDrive {
>> FDCtrl *fdctrl;
>> BlockBackend *blk;
>> /* Drive status */
>> - FDriveType drive;
>> + FDriveType drive; /* CMOS drive type */
>> + FDriveType disk; /* Current disk type */
>
> where
>
> 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;
>
> FDriveType makes obvious sense as drive type.
>
Sadly yes, because we have very thoroughly intermixed the concept of
media and drive, so DriveType still makes sense for the Diskette.
>> uint8_t perpendicular; /* 2.88 MB access mode */
>
> If I understand @perpendicular correctly, it's just an extra hoop a
> driver needs to jump through to actually access a 2.88M disk.
>
>> /* Position */
>> uint8_t head;
> uint8_t track;
> uint8_t sect;
> /* Media */
>
> Shouldn't @disk go here?
>
Oh, yes.
> FDiskFlags flags;
> uint8_t last_sect; /* Nb sector per track */
> uint8_t max_track; /* Nb of tracks */
> uint16_t bps; /* Bytes per sector */
> uint8_t ro; /* Is read-only */
> uint8_t media_changed; /* Is media changed */
> uint8_t media_rate; /* Data rate of medium */
>
> bool media_inserted; /* Is there a medium in the tray */
> } FDrive;
>
> A disk / medium is characterised by it's form factor, density /
> FDriveRate, #sides, #tracks, #sectors per track, and a read-only bit
> (ignoring a few details that don't interest us). Obviously, the drive
> type restricts possible disk types.
>
> What does @disk add over the media description we already have?
>
It's mostly a semantic game to allow the pick_geometry function to
never, ever, ever write to the "drive" field -- it will operate
exclusively on the "disk" field. Callers can decide what to do with that
information.
The idea in the long haul is to make @drive a "write once or never" kind
of ordeal, and I introduced the new field to help enforce that.
It's purely sugar.
Maybe in the future if I work on the FDC some more it will become useful
for other purposes, but for now it's almost exclusively to inform the
'drive' type when drive is set to AUTO.
> Aside: @bps appears to be write-only, and the value written looks odd.
>
>> @@ -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) {
--
—js
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v3 02/11] fdc: refactor pick_geometry
2015-12-17 7:53 ` Markus Armbruster
@ 2015-12-17 17:50 ` John Snow
2015-12-17 19:09 ` Markus Armbruster
0 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2015-12-17 17:50 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel, qemu-block
On 12/17/2015 02:53 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> 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.
>
> For now, there's just one.
>
> Diffstat suggests it's an overall simplification.
>
>> 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");
>> + }
>
> One half of the debug print moved from...
>
>> }
>>
>> /* 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");
>> }
>
> ... here. Recommend to move both or none. If you add more callers,
> both might make more sense.
>
Later in the series, pick_geometry cannot be called if there's no
diskette, so the "no disk!" message stayed outside.
... I'll just wiggle this printf back into fd_revalidate, so it looks sane.
I encourage you to take a few shots of something stiff and press further
into the series. It doesn't get truly wild until patch 9. If you
eyeballed everything except patch 9, I'd be willing to just do a very
early dev window pull and call it a day on this.
--js
>> - 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;
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v3 03/11] fdc: add disk field
2015-12-17 16:59 ` John Snow
@ 2015-12-17 18:15 ` Markus Armbruster
2015-12-17 18:55 ` John Snow
0 siblings, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2015-12-17 18:15 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, qemu-devel, qemu-block
John Snow <jsnow@redhat.com> writes:
> On 12/17/2015 03:30 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> 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 {
>> typedef struct FDrive {
>>> FDCtrl *fdctrl;
>>> BlockBackend *blk;
>>> /* Drive status */
>>> - FDriveType drive;
>>> + FDriveType drive; /* CMOS drive type */
>>> + FDriveType disk; /* Current disk type */
>>
>> where
>>
>> 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;
>>
>> FDriveType makes obvious sense as drive type.
>>
>
> Sadly yes, because we have very thoroughly intermixed the concept of
> media and drive, so DriveType still makes sense for the Diskette.
>
>>> uint8_t perpendicular; /* 2.88 MB access mode */
>>
>> If I understand @perpendicular correctly, it's just an extra hoop a
>> driver needs to jump through to actually access a 2.88M disk.
>>
>>> /* Position */
>>> uint8_t head;
>> uint8_t track;
>> uint8_t sect;
>> /* Media */
>>
>> Shouldn't @disk go here?
>>
>
> Oh, yes.
>
>> FDiskFlags flags;
>> uint8_t last_sect; /* Nb sector per track */
>> uint8_t max_track; /* Nb of tracks */
>> uint16_t bps; /* Bytes per sector */
>> uint8_t ro; /* Is read-only */
>> uint8_t media_changed; /* Is media changed */
>> uint8_t media_rate; /* Data rate of medium */
>>
>> bool media_inserted; /* Is there a medium in the tray */
>> } FDrive;
>>
>> A disk / medium is characterised by it's form factor, density /
>> FDriveRate, #sides, #tracks, #sectors per track, and a read-only bit
>> (ignoring a few details that don't interest us). Obviously, the drive
>> type restricts possible disk types.
>>
>> What does @disk add over the media description we already have?
>>
>
> It's mostly a semantic game to allow the pick_geometry function to
> never, ever, ever write to the "drive" field -- it will operate
> exclusively on the "disk" field. Callers can decide what to do with that
> information.
>
> The idea in the long haul is to make @drive a "write once or never" kind
> of ordeal, and I introduced the new field to help enforce that.
>
> It's purely sugar.
>
> Maybe in the future if I work on the FDC some more it will become useful
> for other purposes, but for now it's almost exclusively to inform the
> 'drive' type when drive is set to AUTO.
Could the following work?
@drive is the connected drive's type, if any. Can't change without a
power cycle.
@flags, @last_sect, ... together describe the medium (a.k.a. disk), if
any. @drive constrains the possible values.
*Except* we can have a special "Schrödinger's drive", for backward
compatibility. It's type is indeterminate until something looks at it.
Then its wave function collapses, and an ordinary drive emerges.
[...]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v3 03/11] fdc: add disk field
2015-12-17 18:15 ` Markus Armbruster
@ 2015-12-17 18:55 ` John Snow
2015-12-17 19:04 ` Markus Armbruster
0 siblings, 1 reply; 29+ messages in thread
From: John Snow @ 2015-12-17 18:55 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel, qemu-block
On 12/17/2015 01:15 PM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> On 12/17/2015 03:30 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> 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 {
>>> typedef struct FDrive {
>>>> FDCtrl *fdctrl;
>>>> BlockBackend *blk;
>>>> /* Drive status */
>>>> - FDriveType drive;
>>>> + FDriveType drive; /* CMOS drive type */
>>>> + FDriveType disk; /* Current disk type */
>>>
>>> where
>>>
>>> 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;
>>>
>>> FDriveType makes obvious sense as drive type.
>>>
>>
>> Sadly yes, because we have very thoroughly intermixed the concept of
>> media and drive, so DriveType still makes sense for the Diskette.
>>
>>>> uint8_t perpendicular; /* 2.88 MB access mode */
>>>
>>> If I understand @perpendicular correctly, it's just an extra hoop a
>>> driver needs to jump through to actually access a 2.88M disk.
>>>
>>>> /* Position */
>>>> uint8_t head;
>>> uint8_t track;
>>> uint8_t sect;
>>> /* Media */
>>>
>>> Shouldn't @disk go here?
>>>
>>
>> Oh, yes.
>>
>>> FDiskFlags flags;
>>> uint8_t last_sect; /* Nb sector per track */
>>> uint8_t max_track; /* Nb of tracks */
>>> uint16_t bps; /* Bytes per sector */
>>> uint8_t ro; /* Is read-only */
>>> uint8_t media_changed; /* Is media changed */
>>> uint8_t media_rate; /* Data rate of medium */
>>>
>>> bool media_inserted; /* Is there a medium in the tray */
>>> } FDrive;
>>>
>>> A disk / medium is characterised by it's form factor, density /
>>> FDriveRate, #sides, #tracks, #sectors per track, and a read-only bit
>>> (ignoring a few details that don't interest us). Obviously, the drive
>>> type restricts possible disk types.
>>>
>>> What does @disk add over the media description we already have?
>>>
>>
>> It's mostly a semantic game to allow the pick_geometry function to
>> never, ever, ever write to the "drive" field -- it will operate
>> exclusively on the "disk" field. Callers can decide what to do with that
>> information.
>>
>> The idea in the long haul is to make @drive a "write once or never" kind
>> of ordeal, and I introduced the new field to help enforce that.
>>
>> It's purely sugar.
>>
>> Maybe in the future if I work on the FDC some more it will become useful
>> for other purposes, but for now it's almost exclusively to inform the
>> 'drive' type when drive is set to AUTO.
>
> Could the following work?
>
> @drive is the connected drive's type, if any. Can't change without a
> power cycle.
>
> @flags, @last_sect, ... together describe the medium (a.k.a. disk), if
> any. @drive constrains the possible values.
>
> *Except* we can have a special "Schrödinger's drive", for backward
> compatibility. It's type is indeterminate until something looks at it.
> Then its wave function collapses, and an ordinary drive emerges.
>
> [...]
>
That is essentially what's going on now.
By the end of this series, the drive type is initialized to 120, 144,
288, auto or none. (default auto.)
Three of those are static and then never change. None reverts you back
to having "no drive" permanently. Auto is the Schrodinger's Drive type.
During initialization, we collapse the waveform via pick_drive. After
this series, there is no code that runs post-init that sets the drive
type anywhere.
Though you are arguing that I could do it all without @disk, by
investigating other metadata. This is true, but I thought it was nice to
have it cached. Not strictly necessary but I just felt like it was the
right thing to do to save it.
--js
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v3 03/11] fdc: add disk field
2015-12-17 18:55 ` John Snow
@ 2015-12-17 19:04 ` Markus Armbruster
0 siblings, 0 replies; 29+ messages in thread
From: Markus Armbruster @ 2015-12-17 19:04 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, qemu-devel, qemu-block
John Snow <jsnow@redhat.com> writes:
> On 12/17/2015 01:15 PM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> On 12/17/2015 03:30 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> 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 {
>>>> typedef struct FDrive {
>>>>> FDCtrl *fdctrl;
>>>>> BlockBackend *blk;
>>>>> /* Drive status */
>>>>> - FDriveType drive;
>>>>> + FDriveType drive; /* CMOS drive type */
>>>>> + FDriveType disk; /* Current disk type */
>>>>
>>>> where
>>>>
>>>> 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;
>>>>
>>>> FDriveType makes obvious sense as drive type.
>>>>
>>>
>>> Sadly yes, because we have very thoroughly intermixed the concept of
>>> media and drive, so DriveType still makes sense for the Diskette.
>>>
>>>>> uint8_t perpendicular; /* 2.88 MB access mode */
>>>>
>>>> If I understand @perpendicular correctly, it's just an extra hoop a
>>>> driver needs to jump through to actually access a 2.88M disk.
>>>>
>>>>> /* Position */
>>>>> uint8_t head;
>>>> uint8_t track;
>>>> uint8_t sect;
>>>> /* Media */
>>>>
>>>> Shouldn't @disk go here?
>>>>
>>>
>>> Oh, yes.
>>>
>>>> FDiskFlags flags;
>>>> uint8_t last_sect; /* Nb sector per track */
>>>> uint8_t max_track; /* Nb of tracks */
>>>> uint16_t bps; /* Bytes per sector */
>>>> uint8_t ro; /* Is read-only */
>>>> uint8_t media_changed; /* Is media changed */
>>>> uint8_t media_rate; /* Data rate of medium */
>>>>
>>>> bool media_inserted; /* Is there a medium in the tray */
>>>> } FDrive;
>>>>
>>>> A disk / medium is characterised by it's form factor, density /
>>>> FDriveRate, #sides, #tracks, #sectors per track, and a read-only bit
>>>> (ignoring a few details that don't interest us). Obviously, the drive
>>>> type restricts possible disk types.
>>>>
>>>> What does @disk add over the media description we already have?
>>>>
>>>
>>> It's mostly a semantic game to allow the pick_geometry function to
>>> never, ever, ever write to the "drive" field -- it will operate
>>> exclusively on the "disk" field. Callers can decide what to do with that
>>> information.
>>>
>>> The idea in the long haul is to make @drive a "write once or never" kind
>>> of ordeal, and I introduced the new field to help enforce that.
>>>
>>> It's purely sugar.
>>>
>>> Maybe in the future if I work on the FDC some more it will become useful
>>> for other purposes, but for now it's almost exclusively to inform the
>>> 'drive' type when drive is set to AUTO.
>>
>> Could the following work?
>>
>> @drive is the connected drive's type, if any. Can't change without a
>> power cycle.
>>
>> @flags, @last_sect, ... together describe the medium (a.k.a. disk), if
>> any. @drive constrains the possible values.
>>
>> *Except* we can have a special "Schrödinger's drive", for backward
>> compatibility. It's type is indeterminate until something looks at it.
>> Then its wave function collapses, and an ordinary drive emerges.
>>
>> [...]
>>
>
> That is essentially what's going on now.
Quite possible, but the two FDriveType confuse me :)
> By the end of this series, the drive type is initialized to 120, 144,
> 288, auto or none. (default auto.)
>
> Three of those are static and then never change. None reverts you back
> to having "no drive" permanently. Auto is the Schrodinger's Drive type.
>
> During initialization, we collapse the waveform via pick_drive. After
> this series, there is no code that runs post-init that sets the drive
> type anywhere.
>
> Though you are arguing that I could do it all without @disk, by
> investigating other metadata. This is true, but I thought it was nice to
> have it cached. Not strictly necessary but I just felt like it was the
> right thing to do to save it.
I'm not saying it isn't the right thing, only that it's locally
unobvious to me. Perhaps it gets obvious later on. Perhaps a few more
words in comments or the commit message could help.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v3 02/11] fdc: refactor pick_geometry
2015-12-17 17:50 ` John Snow
@ 2015-12-17 19:09 ` Markus Armbruster
0 siblings, 0 replies; 29+ messages in thread
From: Markus Armbruster @ 2015-12-17 19:09 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, qemu-devel, qemu-block
John Snow <jsnow@redhat.com> writes:
> On 12/17/2015 02:53 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> 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.
>>
>> For now, there's just one.
>>
>> Diffstat suggests it's an overall simplification.
>>
>>> 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");
>>> + }
>>
>> One half of the debug print moved from...
>>
>>> }
>>>
>>> /* 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");
>>> }
>>
>> ... here. Recommend to move both or none. If you add more callers,
>> both might make more sense.
>>
>
> Later in the series, pick_geometry cannot be called if there's no
> diskette, so the "no disk!" message stayed outside.
>
> ... I'll just wiggle this printf back into fd_revalidate, so it looks sane.
>
> I encourage you to take a few shots of something stiff and press further
> into the series. It doesn't get truly wild until patch 9. If you
> eyeballed everything except patch 9, I'd be willing to just do a very
> early dev window pull and call it a day on this.
Just one day left before I vanish for a long Christas break. I'll see
what I can do.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v3 04/11] fdc: add default drive type option
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 04/11] fdc: add default drive type option John Snow
2015-12-16 23:03 ` Eric Blake
@ 2015-12-18 15:54 ` Markus Armbruster
2015-12-18 17:20 ` John Snow
1 sibling, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2015-12-18 15:54 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, qemu-devel, qemu-block
John Snow <jsnow@redhat.com> writes:
> 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).
Err, will we have two different kinds of auto then?
> In order to allow users to obtain the old behaviors, though, a mechanism
> for specifying the exact type of drive we want is needed.
I'd expect users who understand drive types sufficiently to pick one not
to pick the old behavior, because it's *nuts*.
> 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 | 107 ++++++++++++++++++++++++++-----------------
> 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, 102 insertions(+), 59 deletions(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 13fef23..ad0e052 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -60,58 +60,60 @@ 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;
>
> +#define FDRIVE_DEFAULT FLOPPY_DRIVE_TYPE_AUTO
> +
> 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 +135,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 +158,10 @@ typedef struct FDrive {
> static void fd_init(FDrive *drv)
> {
> /* Drive */
> - drv->drive = FDRIVE_DRV_NONE;
> + drv->drive = FLOPPY_DRIVE_TYPE_NONE; /* FIXME: Obey CLI properties */
> 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 +258,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 +291,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 +569,7 @@ struct FDCtrl {
> /* Timers state */
> uint8_t timer0;
> uint8_t timer1;
> +
> };
>
> #define TYPE_SYSBUS_FDC "base-sysbus-fdc"
> @@ -2224,6 +2228,8 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl, Error **errp)
> if (drive->blk) {
> blk_set_dev_ops(drive->blk, &fdctrl_block_ops, drive);
> drive->media_inserted = blk_is_inserted(drive->blk);
> + } else {
> + drive->drive = FLOPPY_DRIVE_TYPE_NONE;
> }
> }
> }
> @@ -2396,7 +2402,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 +2427,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,
> + FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
> + FloppyDriveType),
> + DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlISABus, state.drives[1].drive,
> + FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
> + FloppyDriveType),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -2469,6 +2481,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,
> + FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
> + FloppyDriveType),
> + DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlSysBus, state.drives[1].drive,
> + FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
> + FloppyDriveType),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -2489,6 +2507,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,
> + 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 72d9b9c..02bfc74 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']}
We have to permit members names starting with a digit, but that can't
make me like them.
> +
> +##
> # @BlockdevSnapshotInternal
> #
> # @device: the name of the device to generate the snapshot from
Massive churn because you chose to rename FDriveType and its members.
Could be avoided if anyone cares.
However, the churn makes the other changes in this patch hard to see.
Can you split it into a pure rename (including the new QAPI type) and
the rest?
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v3 05/11] fdc: Add fallback option
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 05/11] fdc: Add fallback option John Snow
@ 2015-12-18 15:57 ` Markus Armbruster
2015-12-18 17:22 ` John Snow
0 siblings, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2015-12-18 15:57 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, qemu-devel, qemu-block
John Snow <jsnow@redhat.com> writes:
> Add the fallback drive type as an option so we can control
> the behavior as a function of the QEMU machine version.
What's a "fallback drive type", and what does (or will) it do?
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> hw/block/fdc.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index ad0e052..b587de8 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -155,6 +155,9 @@ typedef struct FDrive {
> bool media_inserted; /* Is there a medium in the tray */
> } FDrive;
>
> +
> +static FloppyDriveType get_fallback_drive_type(FDrive *drv);
> +
> static void fd_init(FDrive *drv)
> {
> /* Drive */
> @@ -570,8 +573,15 @@ struct FDCtrl {
> uint8_t timer0;
> uint8_t timer1;
>
> + FloppyDriveType fallback;
> };
>
> +__attribute__((__unused__))
> +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)
>
> @@ -2302,6 +2312,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;
> @@ -2433,6 +2447,9 @@ static Property isa_fdc_properties[] = {
> DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlISABus, state.drives[1].drive,
> FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
> FloppyDriveType),
> + DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
> + FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
> + FloppyDriveType),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -2487,6 +2504,9 @@ static Property sysbus_fdc_properties[] = {
> DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlSysBus, state.drives[1].drive,
> FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
> FloppyDriveType),
> + DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
> + FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
> + FloppyDriveType),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> @@ -2510,6 +2530,9 @@ static Property sun4m_fdc_properties[] = {
> DEFINE_PROP_DEFAULT("fdtype", FDCtrlSysBus, state.drives[0].drive,
> FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
> FloppyDriveType),
> + DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
> + FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
> + FloppyDriveType),
> DEFINE_PROP_END_OF_LIST(),
> };
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v3 06/11] fdc: do not call revalidate on eject
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 06/11] fdc: do not call revalidate on eject John Snow
@ 2015-12-18 16:11 ` Markus Armbruster
2015-12-18 20:13 ` John Snow
0 siblings, 1 reply; 29+ messages in thread
From: Markus Armbruster @ 2015-12-18 16:11 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, qemu-devel, qemu-block
John Snow <jsnow@redhat.com> writes:
> 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 b587de8..e752758 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -167,6 +167,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)
> @@ -249,13 +250,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)
Meaning of return value?
> {
> 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;
> @@ -293,8 +302,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) {
> @@ -303,6 +311,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 */
> @@ -311,15 +327,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;
The left hand side is the "current disk type" (@disk's comment says so).
The right hand side is "no drive connected" drive type (the enum's
comment says so).
Thus, we set the current disk type to the "no drive connected" drive
type. Sounds nuts, doesn't it? :)
Perhaps drv->disk isn't a disk type, but really into what an auto drive
type should be morphed. Correct?
> + } 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;
> }
> }
>
> @@ -2196,9 +2215,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);
> + }
Hmm. Looks like drv->last_sect, ->max_track and ->flags now remain
after an eject. If yes, why is that a good idea?
> }
>
> static bool fdctrl_is_tray_open(void *opaque)
> @@ -2234,13 +2255,14 @@ 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);
> } else {
> drive->drive = FLOPPY_DRIVE_TYPE_NONE;
> }
> + fdctrl_change_cb(drive, drive->media_inserted);
> }
> }
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v3 01/11] fdc: move pick_geometry
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 01/11] fdc: move pick_geometry John Snow
@ 2015-12-18 16:15 ` Eric Blake
0 siblings, 0 replies; 29+ messages in thread
From: Eric Blake @ 2015-12-18 16:15 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: kwolf, armbru, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 518 bytes --]
On 12/16/2015 03:16 PM, John Snow wrote:
> 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(-)
>
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] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v3 04/11] fdc: add default drive type option
2015-12-18 15:54 ` Markus Armbruster
@ 2015-12-18 17:20 ` John Snow
0 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2015-12-18 17:20 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel, qemu-block
On 12/18/2015 10:54 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> 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).
>
> Err, will we have two different kinds of auto then?
>
"No," the auto behavior will follow the same rules, but the fallback
type is configurable. Not that it's sane to do it, but it lets me
configure the new behavior in a way that's pretty solidly backwards
compatible using HW_COMPAT.
It's four steps:
1) Codify the old behavior as type=auto,fallback=144
2) Add drive sizes
3) Adjust pick_geometry to allow 1.44MB diskettes to be used in 2.88MB
drives
4) Set new behavior for 2.6+ to type=auto,fallback=288
"Why not just set the new default to type=288?"
Concerns and lack of testing that I will break esoteric targets. This
change is the safest, most minimal change I can make that accomplishes
my goal (2.88MB drives in most cases on x86 platforms to allow virtio
driver diskettes to be inserted post-boot.)
>> In order to allow users to obtain the old behaviors, though, a mechanism
>> for specifying the exact type of drive we want is needed.
>
> I'd expect users who understand drive types sufficiently to pick one not
> to pick the old behavior, because it's *nuts*.
>
Yes, I meant mostly if they wanted to force a 1.44MB drive type. libvirt
installations can just have you choose 120, 144 or 288 and be done with
it and always explicitly state which it wants. Easy.
Manual executions of QEMU still get the magic. The magic isn't *so* bad
now, honestly.
>> 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 | 107 ++++++++++++++++++++++++++-----------------
>> 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, 102 insertions(+), 59 deletions(-)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index 13fef23..ad0e052 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -60,58 +60,60 @@ 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;
>>
>> +#define FDRIVE_DEFAULT FLOPPY_DRIVE_TYPE_AUTO
>> +
>> 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 +135,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 +158,10 @@ typedef struct FDrive {
>> static void fd_init(FDrive *drv)
>> {
>> /* Drive */
>> - drv->drive = FDRIVE_DRV_NONE;
>> + drv->drive = FLOPPY_DRIVE_TYPE_NONE; /* FIXME: Obey CLI properties */
>> 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 +258,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 +291,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 +569,7 @@ struct FDCtrl {
>> /* Timers state */
>> uint8_t timer0;
>> uint8_t timer1;
>> +
>> };
>>
>> #define TYPE_SYSBUS_FDC "base-sysbus-fdc"
>> @@ -2224,6 +2228,8 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl, Error **errp)
>> if (drive->blk) {
>> blk_set_dev_ops(drive->blk, &fdctrl_block_ops, drive);
>> drive->media_inserted = blk_is_inserted(drive->blk);
>> + } else {
>> + drive->drive = FLOPPY_DRIVE_TYPE_NONE;
>> }
>> }
>> }
>> @@ -2396,7 +2402,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 +2427,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,
>> + FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
>> + FloppyDriveType),
>> + DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlISABus, state.drives[1].drive,
>> + FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
>> + FloppyDriveType),
>> DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> @@ -2469,6 +2481,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,
>> + FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
>> + FloppyDriveType),
>> + DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlSysBus, state.drives[1].drive,
>> + FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
>> + FloppyDriveType),
>> DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> @@ -2489,6 +2507,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,
>> + 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 72d9b9c..02bfc74 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']}
>
> We have to permit members names starting with a digit, but that can't
> make me like them.
>
I wasn't aware they were such a problem. FLOPPY_DRIVE_TYPE_144 doesn't
look too bad and neither does type=144. Does it get weird somewhere else
that I am unaware of?
>> +
>> +##
>> # @BlockdevSnapshotInternal
>> #
>> # @device: the name of the device to generate the snapshot from
>
> Massive churn because you chose to rename FDriveType and its members.
> Could be avoided if anyone cares.
>
> However, the churn makes the other changes in this patch hard to see.
> Can you split it into a pure rename (including the new QAPI type) and
> the rest?
>
Yes, no problem. Apologies and thank you.
--js
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v3 05/11] fdc: Add fallback option
2015-12-18 15:57 ` Markus Armbruster
@ 2015-12-18 17:22 ` John Snow
0 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2015-12-18 17:22 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel, qemu-block
On 12/18/2015 10:57 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> Add the fallback drive type as an option so we can control
>> the behavior as a function of the QEMU machine version.
>
> What's a "fallback drive type", and what does (or will) it do?
>
I assume you mean "Make your commit messages better."
The fallback type accompanies the "auto" drive type as the fallback
drive type that gets selected if there is an issue auto-guessing from
the diskette.
It comes into play in two places:
(1) There's simply no diskette, or
(2) We couldn't figure out what kind of diskette it was.
The legacy behavior is implicitly type=auto,fallback=144. It is now
explicitly so, and the new behavior (at patch 11) is type=auto,fallback=288.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> hw/block/fdc.c | 23 +++++++++++++++++++++++
>> 1 file changed, 23 insertions(+)
>>
>> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
>> index ad0e052..b587de8 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -155,6 +155,9 @@ typedef struct FDrive {
>> bool media_inserted; /* Is there a medium in the tray */
>> } FDrive;
>>
>> +
>> +static FloppyDriveType get_fallback_drive_type(FDrive *drv);
>> +
>> static void fd_init(FDrive *drv)
>> {
>> /* Drive */
>> @@ -570,8 +573,15 @@ struct FDCtrl {
>> uint8_t timer0;
>> uint8_t timer1;
>>
>> + FloppyDriveType fallback;
>> };
>>
>> +__attribute__((__unused__))
>> +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)
>>
>> @@ -2302,6 +2312,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;
>> @@ -2433,6 +2447,9 @@ static Property isa_fdc_properties[] = {
>> DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlISABus, state.drives[1].drive,
>> FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
>> FloppyDriveType),
>> + DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
>> + FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
>> + FloppyDriveType),
>> DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> @@ -2487,6 +2504,9 @@ static Property sysbus_fdc_properties[] = {
>> DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlSysBus, state.drives[1].drive,
>> FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
>> FloppyDriveType),
>> + DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
>> + FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
>> + FloppyDriveType),
>> DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> @@ -2510,6 +2530,9 @@ static Property sun4m_fdc_properties[] = {
>> DEFINE_PROP_DEFAULT("fdtype", FDCtrlSysBus, state.drives[0].drive,
>> FDRIVE_DEFAULT, qdev_prop_fdc_drive_type,
>> FloppyDriveType),
>> + DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
>> + FLOPPY_DRIVE_TYPE_144, qdev_prop_fdc_drive_type,
>> + FloppyDriveType),
>> DEFINE_PROP_END_OF_LIST(),
>> };
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v3 06/11] fdc: do not call revalidate on eject
2015-12-18 16:11 ` Markus Armbruster
@ 2015-12-18 20:13 ` John Snow
0 siblings, 0 replies; 29+ messages in thread
From: John Snow @ 2015-12-18 20:13 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, qemu-devel, qemu-block
On 12/18/2015 11:11 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
>
>> 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 b587de8..e752758 100644
>> --- a/hw/block/fdc.c
>> +++ b/hw/block/fdc.c
>> @@ -167,6 +167,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)
>> @@ -249,13 +250,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)
>
> Meaning of return value?
>
1: "Modified diskette geometry values."
Will amend with documentation.
>> {
>> 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;
>> @@ -293,8 +302,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) {
>> @@ -303,6 +311,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 */
>> @@ -311,15 +327,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;
>
> The left hand side is the "current disk type" (@disk's comment says so).
>
> The right hand side is "no drive connected" drive type (the enum's
> comment says so).
>
> Thus, we set the current disk type to the "no drive connected" drive
> type. Sounds nuts, doesn't it? :)
>
> Perhaps drv->disk isn't a disk type, but really into what an auto drive
> type should be morphed. Correct?
>
... yyyyes, it certainly informs us, if our type is auto, what we should
be setting the drive type to.
I'm definitely cheating, since there really should be two separate
enumerations, honestly:
DriveType: {120, 144, 288, auto, none}
DiskType: {120, 144, 288, none}
Though I concede the disk field isn't strictly needed, I was just
desperate to not have the one field mean two things simultaneously.
>> + } 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;
>> }
>> }
>>
>> @@ -2196,9 +2215,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);
>> + }
>
> Hmm. Looks like drv->last_sect, ->max_track and ->flags now remain
> after an eject. If yes, why is that a good idea?
>
It probably isn't. Overlooked. I can allow the call to propagate as far
as the revalidate function, since it won't call pick_geometry for empty
drives anymore anyway.
>> }
>>
>> static bool fdctrl_is_tray_open(void *opaque)
>> @@ -2234,13 +2255,14 @@ 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);
>> } else {
>> drive->drive = FLOPPY_DRIVE_TYPE_NONE;
>> }
>> + fdctrl_change_cb(drive, drive->media_inserted);
>> }
>> }
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2015-12-18 20:13 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-16 22:16 [Qemu-devel] [PATCH v3 00/11] fdc: fix 2.88mb floppy diskette support John Snow
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 01/11] fdc: move pick_geometry John Snow
2015-12-18 16:15 ` Eric Blake
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 02/11] fdc: refactor pick_geometry John Snow
2015-12-17 7:53 ` Markus Armbruster
2015-12-17 17:50 ` John Snow
2015-12-17 19:09 ` Markus Armbruster
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 03/11] fdc: add disk field John Snow
2015-12-17 8:30 ` Markus Armbruster
2015-12-17 16:59 ` John Snow
2015-12-17 18:15 ` Markus Armbruster
2015-12-17 18:55 ` John Snow
2015-12-17 19:04 ` Markus Armbruster
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 04/11] fdc: add default drive type option John Snow
2015-12-16 23:03 ` Eric Blake
2015-12-18 15:54 ` Markus Armbruster
2015-12-18 17:20 ` John Snow
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 05/11] fdc: Add fallback option John Snow
2015-12-18 15:57 ` Markus Armbruster
2015-12-18 17:22 ` John Snow
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 06/11] fdc: do not call revalidate on eject John Snow
2015-12-18 16:11 ` Markus Armbruster
2015-12-18 20:13 ` John Snow
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 07/11] fdc: implement new drive type property John Snow
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 08/11] fdc: add physical disk sizes John Snow
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 09/11] fdc: rework pick_geometry John Snow
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 10/11] qtest/fdc: Support for 2.88MB drives John Snow
2015-12-16 22:16 ` [Qemu-devel] [PATCH v3 11/11] fdc: change auto fallback drive for ISA FDC to 288 John Snow
2015-12-16 22:30 ` 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.