[Qemu-devel] [PATCH 0/2] Floppy geometry cleanup

[Qemu-devel] [PATCH 0/2] Floppy geometry cleanup

[Markus Armbruster]

Markus Armbruster (2):
  fdc: Drop broken code for user-defined floppy geometry
  fdc: Move floppy geometry guessing back from block.c

 block.c  |  107 -----------------------------------------------------
 block.h  |   18 ---------
 hw/fdc.c |  125 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------
 hw/fdc.h |   10 ++++-
 hw/pc.c  |   13 +-----
 5 files changed, 124 insertions(+), 149 deletions(-)

-- 
1.7.6.5

[Qemu-devel] [PATCH 1/2] fdc: Drop broken code for user-defined floppy geometry

[Markus Armbruster]

bdrv_get_floppy_geometry_hint() fails to store through its parameter
drive when bs has a geometry hint.  Makes fd_revalidate() assign
random crap to drv->drive.

Has been broken that way for ages.  Harmless, because:

* The only way to set a geometry hint is -drive if=none,cyls=...
  Since commit c219331e, probably unintentional.

* The only use of drv->drive is as argument to another
  bdrv_get_floppy_geometry_hint().  Which doesn't use it, since the
  geometry hint is still there.

Drop the broken code, ignore -drive parameter cyls, heads and secs for
floppies even with if=none, just like before commit c219331e.  Matches
-help, which explains cyls, heads, secs as "hard disk physical
geometry".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c  |   62 ++++++++++++++++++++++++++++----------------------------------
 hw/fdc.c |    3 ---
 2 files changed, 28 insertions(+), 37 deletions(-)

diff --git a/block.c b/block.c
index 0acdcac..f5e7cb6 100644
--- a/block.c
+++ b/block.c
@@ -2308,46 +2308,40 @@ void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads,
     uint64_t nb_sectors, size;
     int i, first_match, match;
 
-    bdrv_get_geometry_hint(bs, nb_heads, max_track, last_sect);
-    if (*nb_heads != 0 && *max_track != 0 && *last_sect != 0) {
-        /* User defined disk */
-        *rate = FDRIVE_RATE_500K;
-    } else {
-        bdrv_get_geometry(bs, &nb_sectors);
-        match = -1;
-        first_match = -1;
-        for (i = 0; ; i++) {
-            parse = &fd_formats[i];
-            if (parse->drive == FDRIVE_DRV_NONE) {
+    bdrv_get_geometry(bs, &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 (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;
+                first_match = i;
             }
-            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 (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;
 }
 
 int bdrv_get_translation_hint(BlockDriverState *bs)
diff --git a/hw/fdc.c b/hw/fdc.c
index 78b4e33..132b1e3 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -189,9 +189,6 @@ static void fd_revalidate(FDrive *drv)
                                       &last_sect, drv->drive, &drive, &rate);
         if (!bdrv_is_inserted(drv->bs)) {
             FLOPPY_DPRINTF("No disk in drive\n");
-        } else if (nb_heads != 0 && max_track != 0 && last_sect != 0) {
-            FLOPPY_DPRINTF("User defined disk (%d %d %d)\n",
-                           nb_heads - 1, max_track, last_sect);
         } else {
             FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n", nb_heads,
                            max_track, last_sect, ro ? "ro" : "rw");
-- 
1.7.6.5

[Qemu-devel] [PATCH 2/2] fdc: Move floppy geometry guessing back from block.c

[Markus Armbruster]

Commit 5bbdbb46 moved it to block.c because "other geometry guessing
functions already reside in block.c".  Device-specific functionality
should be kept in device code, not the block layer.  Move it back.

Disk geometry guessing is still in block.c.  To be moved out in a
later patch series.

Bonus: the floppy type used in pc_cmos_init() now obviously matches
the one in the FDrive.  Before, we relied on
bdrv_get_floppy_geometry_hint() picking the same type both in
fd_revalidate() and in pc_cmos_init().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c  |  101 ---------------------------------------------------
 block.h  |   18 ---------
 hw/fdc.c |  122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/fdc.h |   10 +++++-
 hw/pc.c  |   13 ++-----
 5 files changed, 124 insertions(+), 140 deletions(-)

diff --git a/block.c b/block.c
index f5e7cb6..66789d5 100644
--- a/block.c
+++ b/block.c
@@ -2243,107 +2243,6 @@ void bdrv_set_io_limits(BlockDriverState *bs,
     bs->io_limits_enabled = bdrv_io_limits_enabled(bs);
 }
 
-/* Recognize floppy formats */
-typedef struct FDFormat {
-    FDriveType drive;
-    uint8_t last_sect;
-    uint8_t max_track;
-    uint8_t max_head;
-    FDriveRate rate;
-} FDFormat;
-
-static const FDFormat fd_formats[] = {
-    /* First entry is default format */
-    /* 1.44 MB 3"1/2 floppy disks */
-    { FDRIVE_DRV_144, 18, 80, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_144, 21, 80, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_144, 21, 82, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_144, 21, 83, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_144, 22, 80, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_144, 23, 80, 1, FDRIVE_RATE_500K, },
-    { FDRIVE_DRV_144, 24, 80, 1, FDRIVE_RATE_500K, },
-    /* 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, },
-    /* 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, },
-    /* 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, },
-    /* 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, },
-    /* 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, },
-    /* 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, },
-    /* 360 kB must match 5"1/4 better than 3"1/2... */
-    { FDRIVE_DRV_144,  9, 80, 0, FDRIVE_RATE_250K, },
-    /* end */
-    { FDRIVE_DRV_NONE, -1, -1, 0, 0, },
-};
-
-void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, 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;
-
-    bdrv_get_geometry(bs, &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;
-}
-
 int bdrv_get_translation_hint(BlockDriverState *bs)
 {
     return bs->translation;
diff --git a/block.h b/block.h
index d135652..f4c77a1 100644
--- a/block.h
+++ b/block.h
@@ -266,24 +266,6 @@ void bdrv_set_geometry_hint(BlockDriverState *bs,
 void bdrv_set_translation_hint(BlockDriverState *bs, int translation);
 void bdrv_get_geometry_hint(BlockDriverState *bs,
                             int *pcyls, int *pheads, int *psecs);
-typedef enum FDriveType {
-    FDRIVE_DRV_144  = 0x00,   /* 1.44 MB 3"5 drive      */
-    FDRIVE_DRV_288  = 0x01,   /* 2.88 MB 3"5 drive      */
-    FDRIVE_DRV_120  = 0x02,   /* 1.2  MB 5"25 drive     */
-    FDRIVE_DRV_NONE = 0x03,   /* No drive connected     */
-} FDriveType;
-
-typedef enum FDriveRate {
-    FDRIVE_RATE_500K = 0x00,  /* 500 Kbps */
-    FDRIVE_RATE_300K = 0x01,  /* 300 Kbps */
-    FDRIVE_RATE_250K = 0x02,  /* 250 Kbps */
-    FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
-} FDriveRate;
-
-void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads,
-                                   int *max_track, int *last_sect,
-                                   FDriveType drive_in, FDriveType *drive,
-                                   FDriveRate *rate);
 int bdrv_get_translation_hint(BlockDriverState *bs);
 void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error,
                        BlockErrorAction on_write_error);
diff --git a/hw/fdc.c b/hw/fdc.c
index 132b1e3..4d94e9d 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -54,6 +54,113 @@
 /********************************************************/
 /* Floppy drive emulation                               */
 
+typedef enum FDriveRate {
+    FDRIVE_RATE_500K = 0x00,  /* 500 Kbps */
+    FDRIVE_RATE_300K = 0x01,  /* 300 Kbps */
+    FDRIVE_RATE_250K = 0x02,  /* 250 Kbps */
+    FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
+} FDriveRate;
+
+typedef struct FDFormat {
+    FDriveType drive;
+    uint8_t last_sect;
+    uint8_t max_track;
+    uint8_t max_head;
+    FDriveRate rate;
+} FDFormat;
+
+static const FDFormat fd_formats[] = {
+    /* First entry is default format */
+    /* 1.44 MB 3"1/2 floppy disks */
+    { FDRIVE_DRV_144, 18, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 21, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 21, 82, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 21, 83, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 22, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 23, 80, 1, FDRIVE_RATE_500K, },
+    { FDRIVE_DRV_144, 24, 80, 1, FDRIVE_RATE_500K, },
+    /* 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, },
+    /* 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, },
+    /* 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, },
+    /* 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, },
+    /* 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, },
+    /* 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, },
+    /* 360 kB must match 5"1/4 better than 3"1/2... */
+    { FDRIVE_DRV_144,  9, 80, 0, FDRIVE_RATE_250K, },
+    /* end */
+    { FDRIVE_DRV_NONE, -1, -1, 0, 0, },
+};
+
+static void pick_geometry(BlockDriverState *bs, 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;
+
+    bdrv_get_geometry(bs, &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))
 
@@ -185,8 +292,8 @@ static void fd_revalidate(FDrive *drv)
     FLOPPY_DPRINTF("revalidate\n");
     if (drv->bs != NULL) {
         ro = bdrv_is_read_only(drv->bs);
-        bdrv_get_floppy_geometry_hint(drv->bs, &nb_heads, &max_track,
-                                      &last_sect, drv->drive, &drive, &rate);
+        pick_geometry(drv->bs, &nb_heads, &max_track,
+                      &last_sect, drv->drive, &drive, &rate);
         if (!bdrv_is_inserted(drv->bs)) {
             FLOPPY_DPRINTF("No disk in drive\n");
         } else {
@@ -2039,18 +2146,13 @@ static int sun4m_fdc_init1(SysBusDevice *dev)
     return fdctrl_init_common(fdctrl);
 }
 
-void fdc_get_bs(BlockDriverState *bs[], ISADevice *dev)
+FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
 {
-    FDCtrlISABus *isa = DO_UPCAST(FDCtrlISABus, busdev, dev);
-    FDCtrl *fdctrl = &isa->state;
-    int i;
+    FDCtrlISABus *isa = DO_UPCAST(FDCtrlISABus, busdev, fdc);
 
-    for (i = 0; i < MAX_FD; i++) {
-        bs[i] = fdctrl->drives[i].bs;
-    }
+    return isa->state.drives[i].drive;
 }
 
-
 static const VMStateDescription vmstate_isa_fdc ={
     .name = "fdc",
     .version_id = 2,
diff --git a/hw/fdc.h b/hw/fdc.h
index 1b32b17..b5c9f31 100644
--- a/hw/fdc.h
+++ b/hw/fdc.h
@@ -6,11 +6,19 @@
 /* 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;
+
 ISADevice *fdctrl_init_isa(ISABus *bus, DriveInfo **fds);
 void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
                         target_phys_addr_t mmio_base, DriveInfo **fds);
 void sun4m_fdctrl_init(qemu_irq irq, target_phys_addr_t io_base,
                        DriveInfo **fds, qemu_irq *fdc_tc);
-void fdc_get_bs(BlockDriverState *bs[], ISADevice *dev);
+
+FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i);
 
 #endif
diff --git a/hw/pc.c b/hw/pc.c
index 8368701..2359f51 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -335,10 +335,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
                   ISADevice *floppy, BusState *idebus0, BusState *idebus1,
                   ISADevice *s)
 {
-    int val, nb, nb_heads, max_track, last_sect, i;
-    FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
-    FDriveRate rate;
-    BlockDriverState *fd[MAX_FD];
+    int val, nb, i;
+    FDriveType fd_type[2];
     static pc_cmos_init_late_arg arg;
 
     /* various important CMOS locations needed by PC/Bochs bios */
@@ -381,13 +379,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
 
     /* floppy type */
     if (floppy) {
-        fdc_get_bs(fd, floppy);
         for (i = 0; i < 2; i++) {
-            if (fd[i]) {
-                bdrv_get_floppy_geometry_hint(fd[i], &nb_heads, &max_track,
-                                              &last_sect, FDRIVE_DRV_NONE,
-                                              &fd_type[i], &rate);
-            }
+            fd_type[i] = isa_fdc_get_drive_type(floppy, i);
         }
     }
     val = (cmos_get_fd_drive_type(fd_type[0]) << 4) |
-- 
1.7.6.5

Re: [Qemu-devel] [PATCH 2/2] fdc: Move floppy geometry guessing back from block.c

[Blue Swirl]

On Mon, Jun 18, 2012 at 9:10 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Commit 5bbdbb46 moved it to block.c because "other geometry guessing
> functions already reside in block.c".  Device-specific functionality
> should be kept in device code, not the block layer.  Move it back.

As discussed earlier, this is media specific, not device specific
(except FDriveType). How about media.c?

>
> Disk geometry guessing is still in block.c.  To be moved out in a
> later patch series.
>
> Bonus: the floppy type used in pc_cmos_init() now obviously matches
> the one in the FDrive.  Before, we relied on
> bdrv_get_floppy_geometry_hint() picking the same type both in
> fd_revalidate() and in pc_cmos_init().
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block.c  |  101 ---------------------------------------------------
>  block.h  |   18 ---------
>  hw/fdc.c |  122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  hw/fdc.h |   10 +++++-
>  hw/pc.c  |   13 ++-----
>  5 files changed, 124 insertions(+), 140 deletions(-)
>
> diff --git a/block.c b/block.c
> index f5e7cb6..66789d5 100644
> --- a/block.c
> +++ b/block.c
> @@ -2243,107 +2243,6 @@ void bdrv_set_io_limits(BlockDriverState *bs,
>     bs->io_limits_enabled = bdrv_io_limits_enabled(bs);
>  }
>
> -/* Recognize floppy formats */
> -typedef struct FDFormat {
> -    FDriveType drive;
> -    uint8_t last_sect;
> -    uint8_t max_track;
> -    uint8_t max_head;
> -    FDriveRate rate;
> -} FDFormat;
> -
> -static const FDFormat fd_formats[] = {
> -    /* First entry is default format */
> -    /* 1.44 MB 3"1/2 floppy disks */
> -    { FDRIVE_DRV_144, 18, 80, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_144, 21, 80, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_144, 21, 82, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_144, 21, 83, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_144, 22, 80, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_144, 23, 80, 1, FDRIVE_RATE_500K, },
> -    { FDRIVE_DRV_144, 24, 80, 1, FDRIVE_RATE_500K, },
> -    /* 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, },
> -    /* 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, },
> -    /* 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, },
> -    /* 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, },
> -    /* 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, },
> -    /* 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, },
> -    /* 360 kB must match 5"1/4 better than 3"1/2... */
> -    { FDRIVE_DRV_144,  9, 80, 0, FDRIVE_RATE_250K, },
> -    /* end */
> -    { FDRIVE_DRV_NONE, -1, -1, 0, 0, },
> -};
> -
> -void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, 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;
> -
> -    bdrv_get_geometry(bs, &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;
> -}
> -
>  int bdrv_get_translation_hint(BlockDriverState *bs)
>  {
>     return bs->translation;
> diff --git a/block.h b/block.h
> index d135652..f4c77a1 100644
> --- a/block.h
> +++ b/block.h
> @@ -266,24 +266,6 @@ void bdrv_set_geometry_hint(BlockDriverState *bs,
>  void bdrv_set_translation_hint(BlockDriverState *bs, int translation);
>  void bdrv_get_geometry_hint(BlockDriverState *bs,
>                             int *pcyls, int *pheads, int *psecs);
> -typedef enum FDriveType {
> -    FDRIVE_DRV_144  = 0x00,   /* 1.44 MB 3"5 drive      */
> -    FDRIVE_DRV_288  = 0x01,   /* 2.88 MB 3"5 drive      */
> -    FDRIVE_DRV_120  = 0x02,   /* 1.2  MB 5"25 drive     */
> -    FDRIVE_DRV_NONE = 0x03,   /* No drive connected     */
> -} FDriveType;
> -
> -typedef enum FDriveRate {
> -    FDRIVE_RATE_500K = 0x00,  /* 500 Kbps */
> -    FDRIVE_RATE_300K = 0x01,  /* 300 Kbps */
> -    FDRIVE_RATE_250K = 0x02,  /* 250 Kbps */
> -    FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
> -} FDriveRate;
> -
> -void bdrv_get_floppy_geometry_hint(BlockDriverState *bs, int *nb_heads,
> -                                   int *max_track, int *last_sect,
> -                                   FDriveType drive_in, FDriveType *drive,
> -                                   FDriveRate *rate);
>  int bdrv_get_translation_hint(BlockDriverState *bs);
>  void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error,
>                        BlockErrorAction on_write_error);
> diff --git a/hw/fdc.c b/hw/fdc.c
> index 132b1e3..4d94e9d 100644
> --- a/hw/fdc.c
> +++ b/hw/fdc.c
> @@ -54,6 +54,113 @@
>  /********************************************************/
>  /* Floppy drive emulation                               */
>
> +typedef enum FDriveRate {
> +    FDRIVE_RATE_500K = 0x00,  /* 500 Kbps */
> +    FDRIVE_RATE_300K = 0x01,  /* 300 Kbps */
> +    FDRIVE_RATE_250K = 0x02,  /* 250 Kbps */
> +    FDRIVE_RATE_1M   = 0x03,  /*   1 Mbps */
> +} FDriveRate;
> +
> +typedef struct FDFormat {
> +    FDriveType drive;
> +    uint8_t last_sect;
> +    uint8_t max_track;
> +    uint8_t max_head;
> +    FDriveRate rate;
> +} FDFormat;
> +
> +static const FDFormat fd_formats[] = {
> +    /* First entry is default format */
> +    /* 1.44 MB 3"1/2 floppy disks */
> +    { FDRIVE_DRV_144, 18, 80, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_144, 20, 80, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_144, 21, 80, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_144, 21, 82, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_144, 21, 83, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_144, 22, 80, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_144, 23, 80, 1, FDRIVE_RATE_500K, },
> +    { FDRIVE_DRV_144, 24, 80, 1, FDRIVE_RATE_500K, },
> +    /* 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, },
> +    /* 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, },
> +    /* 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, },
> +    /* 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, },
> +    /* 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, },
> +    /* 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, },
> +    /* 360 kB must match 5"1/4 better than 3"1/2... */
> +    { FDRIVE_DRV_144,  9, 80, 0, FDRIVE_RATE_250K, },
> +    /* end */
> +    { FDRIVE_DRV_NONE, -1, -1, 0, 0, },
> +};
> +
> +static void pick_geometry(BlockDriverState *bs, 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;
> +
> +    bdrv_get_geometry(bs, &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))
>
> @@ -185,8 +292,8 @@ static void fd_revalidate(FDrive *drv)
>     FLOPPY_DPRINTF("revalidate\n");
>     if (drv->bs != NULL) {
>         ro = bdrv_is_read_only(drv->bs);
> -        bdrv_get_floppy_geometry_hint(drv->bs, &nb_heads, &max_track,
> -                                      &last_sect, drv->drive, &drive, &rate);
> +        pick_geometry(drv->bs, &nb_heads, &max_track,
> +                      &last_sect, drv->drive, &drive, &rate);
>         if (!bdrv_is_inserted(drv->bs)) {
>             FLOPPY_DPRINTF("No disk in drive\n");
>         } else {
> @@ -2039,18 +2146,13 @@ static int sun4m_fdc_init1(SysBusDevice *dev)
>     return fdctrl_init_common(fdctrl);
>  }
>
> -void fdc_get_bs(BlockDriverState *bs[], ISADevice *dev)
> +FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
>  {
> -    FDCtrlISABus *isa = DO_UPCAST(FDCtrlISABus, busdev, dev);
> -    FDCtrl *fdctrl = &isa->state;
> -    int i;
> +    FDCtrlISABus *isa = DO_UPCAST(FDCtrlISABus, busdev, fdc);
>
> -    for (i = 0; i < MAX_FD; i++) {
> -        bs[i] = fdctrl->drives[i].bs;
> -    }
> +    return isa->state.drives[i].drive;
>  }
>
> -
>  static const VMStateDescription vmstate_isa_fdc ={
>     .name = "fdc",
>     .version_id = 2,
> diff --git a/hw/fdc.h b/hw/fdc.h
> index 1b32b17..b5c9f31 100644
> --- a/hw/fdc.h
> +++ b/hw/fdc.h
> @@ -6,11 +6,19 @@
>  /* 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;
> +
>  ISADevice *fdctrl_init_isa(ISABus *bus, DriveInfo **fds);
>  void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
>                         target_phys_addr_t mmio_base, DriveInfo **fds);
>  void sun4m_fdctrl_init(qemu_irq irq, target_phys_addr_t io_base,
>                        DriveInfo **fds, qemu_irq *fdc_tc);
> -void fdc_get_bs(BlockDriverState *bs[], ISADevice *dev);
> +
> +FDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i);
>
>  #endif
> diff --git a/hw/pc.c b/hw/pc.c
> index 8368701..2359f51 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -335,10 +335,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>                   ISADevice *floppy, BusState *idebus0, BusState *idebus1,
>                   ISADevice *s)
>  {
> -    int val, nb, nb_heads, max_track, last_sect, i;
> -    FDriveType fd_type[2] = { FDRIVE_DRV_NONE, FDRIVE_DRV_NONE };
> -    FDriveRate rate;
> -    BlockDriverState *fd[MAX_FD];
> +    int val, nb, i;
> +    FDriveType fd_type[2];
>     static pc_cmos_init_late_arg arg;
>
>     /* various important CMOS locations needed by PC/Bochs bios */
> @@ -381,13 +379,8 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>
>     /* floppy type */
>     if (floppy) {
> -        fdc_get_bs(fd, floppy);
>         for (i = 0; i < 2; i++) {
> -            if (fd[i]) {
> -                bdrv_get_floppy_geometry_hint(fd[i], &nb_heads, &max_track,
> -                                              &last_sect, FDRIVE_DRV_NONE,
> -                                              &fd_type[i], &rate);
> -            }
> +            fd_type[i] = isa_fdc_get_drive_type(floppy, i);
>         }
>     }
>     val = (cmos_get_fd_drive_type(fd_type[0]) << 4) |
> --
> 1.7.6.5
>

Re: [Qemu-devel] [PATCH 2/2] fdc: Move floppy geometry guessing back from block.c

[Markus Armbruster]

Blue Swirl <blauwirbel@gmail.com> writes:

> On Mon, Jun 18, 2012 at 9:10 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Commit 5bbdbb46 moved it to block.c because "other geometry guessing
>> functions already reside in block.c".  Device-specific functionality
>> should be kept in device code, not the block layer.  Move it back.
>
> As discussed earlier, this is media specific, not device specific
> (except FDriveType). How about media.c?

It's floppy-(media-)specific, isn't it?

We discussed separating floppy drive emulation (fdd) from floppy
controller emulation.  Right now, they're mixed up in qdevs isa-fdc,
sysbus-fdc and SUNW,fdtwo.  Separating fdd involves splitting up those
qdevs.  I tried, but ran into QOM infrastructure difficulties.  Since
that part of QOM is being improved, I decided to postpone the splitting
work for a bit.

I don't remember discussing a separation of floppy drive and floppy
media emulation.

Related project: moving hard disk geometry out of the block layer.
Can't move into a device model, because we have three of them sporting
geometry: IDE, SCSI and virtio disks.  I guess I'll move them into a new
file in hw/.  media.c doesn't sound right for hard disks.  disk-geo.c?

I could move floppy geometry to the same file.  But there's zero overlap
between hard disk and floppy disk geometry, and the only consumer of
floppy geometry is the floppy disk device.  I don't expect that to
change, and that's why I'd prefer to make floppy geometry an
implementation detail of the floppy disk device, and hide it in fdc.c
now, fdd.c after the split.

But if you insist, I can unhide it.

Comments?

Re: [Qemu-devel] [PATCH 2/2] fdc: Move floppy geometry guessing back from block.c

[Blue Swirl]

On Tue, Jun 19, 2012 at 7:45 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Blue Swirl <blauwirbel@gmail.com> writes:
>
>> On Mon, Jun 18, 2012 at 9:10 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Commit 5bbdbb46 moved it to block.c because "other geometry guessing
>>> functions already reside in block.c".  Device-specific functionality
>>> should be kept in device code, not the block layer.  Move it back.
>>
>> As discussed earlier, this is media specific, not device specific
>> (except FDriveType). How about media.c?
>
> It's floppy-(media-)specific, isn't it?
>
> We discussed separating floppy drive emulation (fdd) from floppy
> controller emulation.  Right now, they're mixed up in qdevs isa-fdc,
> sysbus-fdc and SUNW,fdtwo.  Separating fdd involves splitting up those
> qdevs.  I tried, but ran into QOM infrastructure difficulties.  Since
> that part of QOM is being improved, I decided to postpone the splitting
> work for a bit.
>
> I don't remember discussing a separation of floppy drive and floppy
> media emulation.

OK, maybe I mixed things up. How I see this is that a floppy drive may
support several different media types, like 720k, 1.44M and 2.88M
floppies, while floppy media may still be formatted differently with
various number of sectors. The media part is similar to CHS for hard
disks, while the drive or type parts are not visible outside of the HD
unit.

>
> Related project: moving hard disk geometry out of the block layer.
> Can't move into a device model, because we have three of them sporting
> geometry: IDE, SCSI and virtio disks.  I guess I'll move them into a new
> file in hw/.  media.c doesn't sound right for hard disks.  disk-geo.c?

hd-geometry.c?

>
> I could move floppy geometry to the same file.  But there's zero overlap
> between hard disk and floppy disk geometry, and the only consumer of
> floppy geometry is the floppy disk device.  I don't expect that to
> change, and that's why I'd prefer to make floppy geometry an
> implementation detail of the floppy disk device, and hide it in fdc.c
> now, fdd.c after the split.
>
> But if you insist, I can unhide it.

How about fd-geometry.c and hd-geometry.c? Or chs-translation.c to
combine both, maybe also other transformations of CHS to linear offset
later?

>
> Comments?

Re: [Qemu-devel] [PATCH 2/2] fdc: Move floppy geometry guessing back from block.c

[Markus Armbruster]

Blue Swirl <blauwirbel@gmail.com> writes:

> On Tue, Jun 19, 2012 at 7:45 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Blue Swirl <blauwirbel@gmail.com> writes:
>>
>>> On Mon, Jun 18, 2012 at 9:10 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Commit 5bbdbb46 moved it to block.c because "other geometry guessing
>>>> functions already reside in block.c".  Device-specific functionality
>>>> should be kept in device code, not the block layer.  Move it back.
>>>
>>> As discussed earlier, this is media specific, not device specific
>>> (except FDriveType). How about media.c?
>>
>> It's floppy-(media-)specific, isn't it?
>>
>> We discussed separating floppy drive emulation (fdd) from floppy
>> controller emulation.  Right now, they're mixed up in qdevs isa-fdc,
>> sysbus-fdc and SUNW,fdtwo.  Separating fdd involves splitting up those
>> qdevs.  I tried, but ran into QOM infrastructure difficulties.  Since
>> that part of QOM is being improved, I decided to postpone the splitting
>> work for a bit.
>>
>> I don't remember discussing a separation of floppy drive and floppy
>> media emulation.
>
> OK, maybe I mixed things up. How I see this is that a floppy drive may
> support several different media types, like 720k, 1.44M and 2.88M
> floppies, while floppy media may still be formatted differently with
> various number of sectors.

Yes, there are several types of floppy drives, each capable of dealing
with a certain set of media geometries.

>                            The media part is similar to CHS for hard
> disks, while the drive or type parts are not visible outside of the HD
> unit.

Hard disk geometry is a property of the device, like floppy drive type,
but unlike floppy geometry.

>> Related project: moving hard disk geometry out of the block layer.
>> Can't move into a device model, because we have three of them sporting
>> geometry: IDE, SCSI and virtio disks.  I guess I'll move them into a new
>> file in hw/.  media.c doesn't sound right for hard disks.  disk-geo.c?
>
> hd-geometry.c?

Sold.

>> I could move floppy geometry to the same file.  But there's zero overlap
>> between hard disk and floppy disk geometry, and the only consumer of
>> floppy geometry is the floppy disk device.  I don't expect that to
>> change, and that's why I'd prefer to make floppy geometry an
>> implementation detail of the floppy disk device, and hide it in fdc.c
>> now, fdd.c after the split.
>>
>> But if you insist, I can unhide it.
>
> How about fd-geometry.c and hd-geometry.c? Or chs-translation.c to
> combine both, maybe also other transformations of CHS to linear offset
> later?

Since there's no overlap between hard and floppy disk geometry, I'd
rather not mix them up in the same file.

Can do fd-geometry.c.  While I can't see what putting floppy geometry in
fd-geometry.c rather than next to its only user buys us (apart from
external linkage), I'll comply with your wish.

>> Comments?

Re: [Qemu-devel] [PATCH 2/2] fdc: Move floppy geometry guessing back from block.c

[Blue Swirl]

On Wed, Jun 20, 2012 at 8:21 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Blue Swirl <blauwirbel@gmail.com> writes:
>
>> On Tue, Jun 19, 2012 at 7:45 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>> Blue Swirl <blauwirbel@gmail.com> writes:
>>>
>>>> On Mon, Jun 18, 2012 at 9:10 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>> Commit 5bbdbb46 moved it to block.c because "other geometry guessing
>>>>> functions already reside in block.c".  Device-specific functionality
>>>>> should be kept in device code, not the block layer.  Move it back.
>>>>
>>>> As discussed earlier, this is media specific, not device specific
>>>> (except FDriveType). How about media.c?
>>>
>>> It's floppy-(media-)specific, isn't it?
>>>
>>> We discussed separating floppy drive emulation (fdd) from floppy
>>> controller emulation.  Right now, they're mixed up in qdevs isa-fdc,
>>> sysbus-fdc and SUNW,fdtwo.  Separating fdd involves splitting up those
>>> qdevs.  I tried, but ran into QOM infrastructure difficulties.  Since
>>> that part of QOM is being improved, I decided to postpone the splitting
>>> work for a bit.
>>>
>>> I don't remember discussing a separation of floppy drive and floppy
>>> media emulation.
>>
>> OK, maybe I mixed things up. How I see this is that a floppy drive may
>> support several different media types, like 720k, 1.44M and 2.88M
>> floppies, while floppy media may still be formatted differently with
>> various number of sectors.
>
> Yes, there are several types of floppy drives, each capable of dealing
> with a certain set of media geometries.
>
>>                            The media part is similar to CHS for hard
>> disks, while the drive or type parts are not visible outside of the HD
>> unit.
>
> Hard disk geometry is a property of the device, like floppy drive type,
> but unlike floppy geometry.

I guess the hard disk situation is internally not unlike to floppies
at all, the manufacturer probably uses the same mechanisms and
electronics for several drive models, only the platter capacity (with
the tracks, heads and sectors like floppies) and related configuration
changes. But for the end user, this is not visible and even the CHS
can be fake if the drive can for example remap bad sectors or use some
platter areas for internal use. Of course users are not able to
exchange platters at will.

>
>>> Related project: moving hard disk geometry out of the block layer.
>>> Can't move into a device model, because we have three of them sporting
>>> geometry: IDE, SCSI and virtio disks.  I guess I'll move them into a new
>>> file in hw/.  media.c doesn't sound right for hard disks.  disk-geo.c?
>>
>> hd-geometry.c?
>
> Sold.
>
>>> I could move floppy geometry to the same file.  But there's zero overlap
>>> between hard disk and floppy disk geometry, and the only consumer of
>>> floppy geometry is the floppy disk device.  I don't expect that to
>>> change, and that's why I'd prefer to make floppy geometry an
>>> implementation detail of the floppy disk device, and hide it in fdc.c
>>> now, fdd.c after the split.
>>>
>>> But if you insist, I can unhide it.
>>
>> How about fd-geometry.c and hd-geometry.c? Or chs-translation.c to
>> combine both, maybe also other transformations of CHS to linear offset
>> later?
>
> Since there's no overlap between hard and floppy disk geometry, I'd
> rather not mix them up in the same file.
>
> Can do fd-geometry.c.  While I can't see what putting floppy geometry in
> fd-geometry.c rather than next to its only user buys us (apart from
> external linkage), I'll comply with your wish.

On second thought, fd-geometry.c would not be used for anything else
(until we have Jaz? Zip?), so merging it with fdc.c makes sense. I'm
not sure the same also applies to hd-geometry.c.

>
>>> Comments?

Re: [Qemu-devel] [PATCH 2/2] fdc: Move floppy geometry guessing back from block.c

[Markus Armbruster]

Blue Swirl <blauwirbel@gmail.com> writes:

> On Wed, Jun 20, 2012 at 8:21 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Blue Swirl <blauwirbel@gmail.com> writes:
>>
>>> On Tue, Jun 19, 2012 at 7:45 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Blue Swirl <blauwirbel@gmail.com> writes:
>>>>
>>>>> On Mon, Jun 18, 2012 at 9:10 AM, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>> Commit 5bbdbb46 moved it to block.c because "other geometry guessing
>>>>>> functions already reside in block.c".  Device-specific functionality
>>>>>> should be kept in device code, not the block layer.  Move it back.
>>>>>
>>>>> As discussed earlier, this is media specific, not device specific
>>>>> (except FDriveType). How about media.c?
>>>>
>>>> It's floppy-(media-)specific, isn't it?
>>>>
>>>> We discussed separating floppy drive emulation (fdd) from floppy
>>>> controller emulation.  Right now, they're mixed up in qdevs isa-fdc,
>>>> sysbus-fdc and SUNW,fdtwo.  Separating fdd involves splitting up those
>>>> qdevs.  I tried, but ran into QOM infrastructure difficulties.  Since
>>>> that part of QOM is being improved, I decided to postpone the splitting
>>>> work for a bit.
>>>>
>>>> I don't remember discussing a separation of floppy drive and floppy
>>>> media emulation.
>>>
>>> OK, maybe I mixed things up. How I see this is that a floppy drive may
>>> support several different media types, like 720k, 1.44M and 2.88M
>>> floppies, while floppy media may still be formatted differently with
>>> various number of sectors.
>>
>> Yes, there are several types of floppy drives, each capable of dealing
>> with a certain set of media geometries.
>>
>>>                            The media part is similar to CHS for hard
>>> disks, while the drive or type parts are not visible outside of the HD
>>> unit.
>>
>> Hard disk geometry is a property of the device, like floppy drive type,
>> but unlike floppy geometry.
>
> I guess the hard disk situation is internally not unlike to floppies
> at all, the manufacturer probably uses the same mechanisms and
> electronics for several drive models, only the platter capacity (with
> the tracks, heads and sectors like floppies) and related configuration
> changes. But for the end user, this is not visible and even the CHS
> can be fake if the drive can for example remap bad sectors or use some
> platter areas for internal use. Of course users are not able to
> exchange platters at will.

Makes sense, but we can ignore such hard disk internals in QEMU.

>>>> Related project: moving hard disk geometry out of the block layer.
>>>> Can't move into a device model, because we have three of them sporting
>>>> geometry: IDE, SCSI and virtio disks.  I guess I'll move them into a new
>>>> file in hw/.  media.c doesn't sound right for hard disks.  disk-geo.c?
>>>
>>> hd-geometry.c?
>>
>> Sold.
>>
>>>> I could move floppy geometry to the same file.  But there's zero overlap
>>>> between hard disk and floppy disk geometry, and the only consumer of
>>>> floppy geometry is the floppy disk device.  I don't expect that to
>>>> change, and that's why I'd prefer to make floppy geometry an
>>>> implementation detail of the floppy disk device, and hide it in fdc.c
>>>> now, fdd.c after the split.
>>>>
>>>> But if you insist, I can unhide it.
>>>
>>> How about fd-geometry.c and hd-geometry.c? Or chs-translation.c to
>>> combine both, maybe also other transformations of CHS to linear offset
>>> later?
>>
>> Since there's no overlap between hard and floppy disk geometry, I'd
>> rather not mix them up in the same file.
>>
>> Can do fd-geometry.c.  While I can't see what putting floppy geometry in
>> fd-geometry.c rather than next to its only user buys us (apart from
>> external linkage), I'll comply with your wish.
>
> On second thought, fd-geometry.c would not be used for anything else
> (until we have Jaz? Zip?), so merging it with fdc.c makes sense. I'm
> not sure the same also applies to hd-geometry.c.

We agree, excellent.  I'll put the floppy geometry code next to its only
user, with the understanding that we'll move it to its own file as soon
as a second user shows up.  The hard disk geometry code goes into
hw/hd-geometry.c.