[Qemu-devel] [PATCH 0/3] fdc: Use separate qdev device for drives

[Qemu-devel] [PATCH 0/3] fdc: Use separate qdev device for drives

[Kevin Wolf]

We have been complaining for a long time about how the floppy controller and
floppy drives are combined in a single qdev device and how this makes the
device awkward to work with because it behaves different from all other block
devices.

The latest reason to complain was when I noticed that using qdev device names
in QMP commands (e.g. for media change) doesn't really work when only the
controller is a qdev device, but the drives aren't.

So I decided to have a go at it, and this is the result.

It doesn't actually change any of the inner workings of the floppy controller,
but it wires things up differently on the qdev layer so that a floppy
controller now exposes a bus on which the floppy drives sit. This results in a
structure that is similar to IDE where the actual drive state is still in the
controller and the qdev device basically just contains the qdev properties -
not pretty, but quite workable.

The commit message of patch 3 explains how to use it. In short, there is a
'-device floppy' now and it does what you would expect if you ever used ide-cd.

The other problem is old command lines, especially those using things like
'-global isa-fdc,driveA=...'. In order to keep them working, we need to forward
the property to an internally created floppy drive device. This is a bit like
usb-storage, which we know is ugly, but works well enough in practice. The good
news here is that in contrast to usb-storage, the floppy controller only does
the forwarding for legacy configurations; as soon as you start using '-device
floppy', it doesn't happen any more.

So as you may have expected, this conversion doesn't result in a perfect
device, but I think it's definitely an improvement over the old state. I hope
you like it despite the warts. :-)

Kevin Wolf (3):
  fdc: Add a floppy qbus
  fdc: Add a floppy drive qdev
  fdc: Move qdev properties to FloppyDrive

 hw/block/fdc.c | 255 +++++++++++++++++++++++++++++++++++++++++++++++----------
 vl.c           |   1 +
 2 files changed, 211 insertions(+), 45 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] fdc: Add a floppy qbus

[Kevin Wolf]

This adds a qbus to the floppy controller that should contain the floppy
drives eventually. At the moment it just exists and is empty.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/block/fdc.c | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index f73af7d..4164b31 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -52,6 +52,33 @@
         }                                                       \
     } while (0)
 
+
+/********************************************************/
+/* qdev floppy bus                                      */
+
+#define TYPE_FLOPPY_BUS "Floppy"
+#define FLOPPY_BUS(obj) OBJECT_CHECK(FloppyBus, (obj), TYPE_FLOPPY_BUS)
+
+typedef struct FDCtrl FDCtrl;
+
+typedef struct FloppyBus {
+    BusState bus;
+    FDCtrl *fdc;
+} FloppyBus;
+
+static const TypeInfo floppy_bus_info = {
+    .name = TYPE_FLOPPY_BUS,
+    .parent = TYPE_BUS,
+    .instance_size = sizeof(FloppyBus),
+};
+
+static void floppy_bus_create(FDCtrl *fdc, FloppyBus *bus, DeviceState *dev)
+{
+    qbus_create_inplace(bus, sizeof(FloppyBus), TYPE_FLOPPY_BUS, dev, NULL);
+    bus->fdc = fdc;
+}
+
+
 /********************************************************/
 /* Floppy drive emulation                               */
 
@@ -148,8 +175,6 @@ static FDriveSize drive_size(FloppyDriveType drive)
 #define FD_SECTOR_SC           2   /* Sector size code */
 #define FD_RESET_SENSEI_COUNT  4   /* Number of sense interrupts on RESET */
 
-typedef struct FDCtrl FDCtrl;
-
 /* Floppy disk drive emulation */
 typedef enum FDiskFlags {
     FDISK_DBL_SIDES  = 0x01,
@@ -684,6 +709,7 @@ struct FDCtrl {
     /* Power down config (also with status regB access mode */
     uint8_t pwrd;
     /* Floppy drives */
+    FloppyBus bus;
     uint8_t num_floppies;
     FDrive drives[MAX_FD];
     int reset_sensei;
@@ -2441,7 +2467,8 @@ void sun4m_fdctrl_init(qemu_irq irq, hwaddr io_base,
     *fdc_tc = qdev_get_gpio_in(dev, 0);
 }
 
-static void fdctrl_realize_common(FDCtrl *fdctrl, Error **errp)
+static void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl,
+                                  Error **errp)
 {
     int i, j;
     static int command_tables_inited = 0;
@@ -2479,6 +2506,8 @@ static void fdctrl_realize_common(FDCtrl *fdctrl, Error **errp)
         k->register_channel(fdctrl->dma, fdctrl->dma_chann,
                             &fdctrl_transfer_handler, fdctrl);
     }
+
+    floppy_bus_create(fdctrl, &fdctrl->bus, dev);
     fdctrl_connect_drives(fdctrl, errp);
 }
 
@@ -2506,7 +2535,7 @@ static void isabus_fdc_realize(DeviceState *dev, Error **errp)
     }
 
     qdev_set_legacy_instance_id(dev, isa->iobase, 2);
-    fdctrl_realize_common(fdctrl, &err);
+    fdctrl_realize_common(dev, fdctrl, &err);
     if (err != NULL) {
         error_propagate(errp, err);
         return;
@@ -2557,7 +2586,7 @@ static void sysbus_fdc_common_realize(DeviceState *dev, Error **errp)
     FDCtrlSysBus *sys = SYSBUS_FDC(dev);
     FDCtrl *fdctrl = &sys->state;
 
-    fdctrl_realize_common(fdctrl, errp);
+    fdctrl_realize_common(dev, fdctrl, errp);
 }
 
 FloppyDriveType isa_fdc_get_drive_type(ISADevice *fdc, int i)
@@ -2742,6 +2771,7 @@ static void fdc_register_types(void)
     type_register_static(&sysbus_fdc_type_info);
     type_register_static(&sysbus_fdc_info);
     type_register_static(&sun4m_fdc_info);
+    type_register_static(&floppy_bus_info);
 }
 
 type_init(fdc_register_types)
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/3] fdc: Add a floppy drive qdev

[Kevin Wolf]

Floppy controllers automatically create two floppy drive devices in qdev
now. (They always created two drives, but managed them only internally.)

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/block/fdc.c | 152 +++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 115 insertions(+), 37 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 4164b31..d1e2339 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -60,6 +60,8 @@
 #define FLOPPY_BUS(obj) OBJECT_CHECK(FloppyBus, (obj), TYPE_FLOPPY_BUS)
 
 typedef struct FDCtrl FDCtrl;
+typedef struct FDrive FDrive;
+static FDrive *get_drv(FDCtrl *fdctrl, int unit);
 
 typedef struct FloppyBus {
     BusState bus;
@@ -180,7 +182,7 @@ typedef enum FDiskFlags {
     FDISK_DBL_SIDES  = 0x01,
 } FDiskFlags;
 
-typedef struct FDrive {
+struct FDrive {
     FDCtrl *fdctrl;
     BlockBackend *blk;
     /* Drive status */
@@ -201,7 +203,7 @@ typedef struct FDrive {
     uint8_t media_rate;       /* Data rate of medium    */
 
     bool media_validated;     /* Have we validated the media? */
-} FDrive;
+};
 
 
 static FloppyDriveType get_fallback_drive_type(FDrive *drv);
@@ -466,6 +468,101 @@ static void fd_revalidate(FDrive *drv)
     }
 }
 
+static void fd_change_cb(void *opaque, bool load)
+{
+    FDrive *drive = opaque;
+
+    drive->media_changed = 1;
+    drive->media_validated = false;
+    fd_revalidate(drive);
+}
+
+static const BlockDevOps fd_block_ops = {
+    .change_media_cb = fd_change_cb,
+};
+
+
+#define TYPE_FLOPPY_DRIVE "floppy"
+#define FLOPPY_DRIVE(obj) \
+     OBJECT_CHECK(FloppyDrive, (obj), TYPE_FLOPPY_DRIVE)
+
+typedef struct FloppyDrive {
+    DeviceState qdev;
+    uint32_t    unit;
+} FloppyDrive;
+
+static Property floppy_drive_properties[] = {
+    DEFINE_PROP_UINT32("unit", FloppyDrive, unit, -1),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static int floppy_drive_init(DeviceState *qdev)
+{
+    FloppyDrive *dev = FLOPPY_DRIVE(qdev);
+    FloppyBus *bus = DO_UPCAST(FloppyBus, bus, dev->qdev.parent_bus);
+    FDrive *drive;
+
+    if (dev->unit == -1) {
+        for (dev->unit = 0; dev->unit < MAX_FD; dev->unit++) {
+            drive = get_drv(bus->fdc, dev->unit);
+            if (!drive->blk) {
+                break;
+            }
+        }
+    }
+
+    if (dev->unit >= MAX_FD) {
+        error_report("Can't create floppy unit %d, bus supports only %d units",
+                     dev->unit, MAX_FD);
+        return -1;
+    }
+
+    /* TODO Check whether unit is in use */
+
+    drive = get_drv(bus->fdc, dev->unit);
+    drive->fdctrl = bus->fdc;
+
+    if (drive->blk) {
+        if (blk_get_on_error(drive->blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC) {
+            error_report("fdc doesn't support drive option werror");
+            return -1;
+        }
+        if (blk_get_on_error(drive->blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
+            error_report("fdc doesn't support drive option rerror");
+            return -1;
+        }
+    } else {
+        /* Anonymous BlockBackend for an empty drive */
+        drive->blk = blk_new();
+    }
+
+    fd_init(drive);
+    if (drive->blk) {
+        blk_set_dev_ops(drive->blk, &fd_block_ops, drive);
+        pick_drive_type(drive);
+    }
+    fd_revalidate(drive);
+
+    return 0;
+}
+
+static void floppy_drive_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *k = DEVICE_CLASS(klass);
+    k->init = floppy_drive_init;
+    set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
+    k->bus_type = TYPE_FLOPPY_BUS;
+    k->props = floppy_drive_properties;
+    k->desc = "virtual floppy drive";
+}
+
+static const TypeInfo floppy_drive_info = {
+    .name = TYPE_FLOPPY_DRIVE,
+    .parent = TYPE_DEVICE,
+    .instance_size = sizeof(FloppyDrive),
+    .class_init = floppy_drive_class_init,
+};
+
 /********************************************************/
 /* Intel 82078 floppy disk controller emulation          */
 
@@ -1184,9 +1281,9 @@ static inline FDrive *drv3(FDCtrl *fdctrl)
 }
 #endif
 
-static FDrive *get_cur_drv(FDCtrl *fdctrl)
+static FDrive *get_drv(FDCtrl *fdctrl, int unit)
 {
-    switch (fdctrl->cur_drv) {
+    switch (unit) {
         case 0: return drv0(fdctrl);
         case 1: return drv1(fdctrl);
 #if MAX_FD == 4
@@ -1197,6 +1294,11 @@ static FDrive *get_cur_drv(FDCtrl *fdctrl)
     }
 }
 
+static FDrive *get_cur_drv(FDCtrl *fdctrl)
+{
+    return get_drv(fdctrl, fdctrl->cur_drv);
+}
+
 /* Status A register : 0x00 (read-only) */
 static uint32_t fdctrl_read_statusA(FDCtrl *fdctrl)
 {
@@ -2356,46 +2458,21 @@ static void fdctrl_result_timer(void *opaque)
     }
 }
 
-static void fdctrl_change_cb(void *opaque, bool load)
-{
-    FDrive *drive = opaque;
-
-    drive->media_changed = 1;
-    drive->media_validated = false;
-    fd_revalidate(drive);
-}
-
-static const BlockDevOps fdctrl_block_ops = {
-    .change_media_cb = fdctrl_change_cb,
-};
-
 /* Init functions */
 static void fdctrl_connect_drives(FDCtrl *fdctrl, Error **errp)
 {
     unsigned int i;
-    FDrive *drive;
+    DeviceState *dev;
+    Error *local_err = NULL;
 
     for (i = 0; i < MAX_FD; i++) {
-        drive = &fdctrl->drives[i];
-        drive->fdctrl = fdctrl;
-
-        if (drive->blk) {
-            if (blk_get_on_error(drive->blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC) {
-                error_setg(errp, "fdc doesn't support drive option werror");
-                return;
-            }
-            if (blk_get_on_error(drive->blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
-                error_setg(errp, "fdc doesn't support drive option rerror");
-                return;
-            }
-        }
-
-        fd_init(drive);
-        if (drive->blk) {
-            blk_set_dev_ops(drive->blk, &fdctrl_block_ops, drive);
-            pick_drive_type(drive);
+        dev = qdev_create(&fdctrl->bus.bus, "floppy");
+        qdev_prop_set_uint32(dev, "unit", i);
+        object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
         }
-        fd_revalidate(drive);
     }
 }
 
@@ -2772,6 +2849,7 @@ static void fdc_register_types(void)
     type_register_static(&sysbus_fdc_info);
     type_register_static(&sun4m_fdc_info);
     type_register_static(&floppy_bus_info);
+    type_register_static(&floppy_drive_info);
 }
 
 type_init(fdc_register_types)
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/3] fdc: Move qdev properties to FloppyDrive

[Kevin Wolf]

This makes the FloppyDrive qdev object actually useful: Now that it has
all properties that don't belong to the controller, you can actually
use '-device floppy' and get a working result.

Command line semantics is consistent with CD-ROM drives: By default you
get a single empty floppy drive. You can override it with -drive and
using the same index, but if you use -drive to add a floppy to a
different index, you get both of them. However, as soon as you use any
'-device floppy', even to a different slot, the default drive is
disabled.

Using '-device floppy' without specifying the unit will choose the first
free slot on the controller.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/block/fdc.c | 105 ++++++++++++++++++++++++++++++++++++++++++++-------------
 vl.c           |   1 +
 2 files changed, 82 insertions(+), 24 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index d1e2339..949c16a 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -35,6 +35,7 @@
 #include "qemu/timer.h"
 #include "hw/isa/isa.h"
 #include "hw/sysbus.h"
+#include "hw/block/block.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/sysemu.h"
@@ -487,12 +488,18 @@ static const BlockDevOps fd_block_ops = {
      OBJECT_CHECK(FloppyDrive, (obj), TYPE_FLOPPY_DRIVE)
 
 typedef struct FloppyDrive {
-    DeviceState qdev;
-    uint32_t    unit;
+    DeviceState     qdev;
+    uint32_t        unit;
+    BlockConf       conf;
+    FloppyDriveType type;
 } FloppyDrive;
 
 static Property floppy_drive_properties[] = {
     DEFINE_PROP_UINT32("unit", FloppyDrive, unit, -1),
+    DEFINE_BLOCK_PROPERTIES(FloppyDrive, conf),
+    DEFINE_PROP_DEFAULT("drive-type", FloppyDrive, type,
+                        FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
+                        FloppyDriveType),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -501,6 +508,7 @@ static int floppy_drive_init(DeviceState *qdev)
     FloppyDrive *dev = FLOPPY_DRIVE(qdev);
     FloppyBus *bus = DO_UPCAST(FloppyBus, bus, dev->qdev.parent_bus);
     FDrive *drive;
+    int ret;
 
     if (dev->unit == -1) {
         for (dev->unit = 0; dev->unit < MAX_FD; dev->unit++) {
@@ -517,25 +525,49 @@ static int floppy_drive_init(DeviceState *qdev)
         return -1;
     }
 
-    /* TODO Check whether unit is in use */
-
     drive = get_drv(bus->fdc, dev->unit);
-    drive->fdctrl = bus->fdc;
-
     if (drive->blk) {
-        if (blk_get_on_error(drive->blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC) {
-            error_report("fdc doesn't support drive option werror");
-            return -1;
-        }
-        if (blk_get_on_error(drive->blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
-            error_report("fdc doesn't support drive option rerror");
-            return -1;
-        }
-    } else {
+        error_report("Floppy unit %d is in use", dev->unit);
+        return -1;
+    }
+
+    if (!dev->conf.blk) {
         /* Anonymous BlockBackend for an empty drive */
-        drive->blk = blk_new();
+        dev->conf.blk = blk_new();
+        ret = blk_attach_dev(dev->conf.blk, dev);
+        assert(ret == 0);
+    }
+
+    blkconf_blocksizes(&dev->conf);
+    if (dev->conf.logical_block_size != 512 ||
+        dev->conf.physical_block_size != 512)
+    {
+        error_report("Physical and logical block size must be 512 for floppy");
+        return -1;
+    }
+
+    /* rerror/werror aren't supported by fdc and therefore not even registered
+     * with qdev. So set the defaults manually before they are used in
+     * blkconf_apply_backend_options(). */
+    dev->conf.rerror = BLOCKDEV_ON_ERROR_AUTO;
+    dev->conf.werror = BLOCKDEV_ON_ERROR_AUTO;
+    blkconf_apply_backend_options(&dev->conf);
+
+    /* 'enospc' is the default for -drive, 'report' is what blk_new() gives us
+     * for empty drives. */
+    if (blk_get_on_error(dev->conf.blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC &&
+        blk_get_on_error(dev->conf.blk, 0) != BLOCKDEV_ON_ERROR_REPORT) {
+        error_report("fdc doesn't support drive option werror");
+        return -1;
+    }
+    if (blk_get_on_error(dev->conf.blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
+        error_report("fdc doesn't support drive option rerror");
+        return -1;
     }
 
+    drive->blk = dev->conf.blk;
+    drive->fdctrl = bus->fdc;
+
     fd_init(drive);
     if (drive->blk) {
         blk_set_dev_ops(drive->blk, &fd_block_ops, drive);
@@ -809,6 +841,10 @@ struct FDCtrl {
     FloppyBus bus;
     uint8_t num_floppies;
     FDrive drives[MAX_FD];
+    struct {
+        BlockBackend *blk;
+        FloppyDriveType type;
+    } qdev_for_drives[MAX_FD];
     int reset_sensei;
     uint32_t check_media_rate;
     FloppyDriveType fallback; /* type=auto failure fallback */
@@ -2459,15 +2495,36 @@ static void fdctrl_result_timer(void *opaque)
 }
 
 /* Init functions */
-static void fdctrl_connect_drives(FDCtrl *fdctrl, Error **errp)
+static void fdctrl_connect_drives(FDCtrl *fdctrl, Error **errp,
+                                  DeviceState *fdc_dev)
 {
     unsigned int i;
     DeviceState *dev;
     Error *local_err = NULL;
 
     for (i = 0; i < MAX_FD; i++) {
+        BlockBackend *blk = fdctrl->qdev_for_drives[i].blk;
+        if (!blk) {
+            continue;
+        }
+
         dev = qdev_create(&fdctrl->bus.bus, "floppy");
         qdev_prop_set_uint32(dev, "unit", i);
+        qdev_prop_set_enum(dev, "drive-type", fdctrl->qdev_for_drives[i].type);
+
+        if (blk) {
+            blk_ref(blk);
+            blk_detach_dev(blk, fdc_dev);
+            fdctrl->qdev_for_drives[i].blk = NULL;
+            qdev_prop_set_drive(dev, "drive", blk, &local_err);
+            blk_unref(blk);
+
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return;
+            }
+        }
+
         object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
@@ -2585,7 +2642,7 @@ static void fdctrl_realize_common(DeviceState *dev, FDCtrl *fdctrl,
     }
 
     floppy_bus_create(fdctrl, &fdctrl->bus, dev);
-    fdctrl_connect_drives(fdctrl, errp);
+    fdctrl_connect_drives(fdctrl, errp, dev);
 }
 
 static const MemoryRegionPortio fdc_portio_list[] = {
@@ -2710,14 +2767,14 @@ static Property isa_fdc_properties[] = {
     DEFINE_PROP_UINT32("iobase", FDCtrlISABus, iobase, 0x3f0),
     DEFINE_PROP_UINT32("irq", FDCtrlISABus, irq, 6),
     DEFINE_PROP_UINT32("dma", FDCtrlISABus, dma, 2),
-    DEFINE_PROP_DRIVE("driveA", FDCtrlISABus, state.drives[0].blk),
-    DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.drives[1].blk),
+    DEFINE_PROP_DRIVE("driveA", FDCtrlISABus, state.qdev_for_drives[0].blk),
+    DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.qdev_for_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,
+    DEFINE_PROP_DEFAULT("fdtypeA", FDCtrlISABus, state.qdev_for_drives[0].type,
                         FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
                         FloppyDriveType),
-    DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlISABus, state.drives[1].drive,
+    DEFINE_PROP_DEFAULT("fdtypeB", FDCtrlISABus, state.qdev_for_drives[1].type,
                         FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
                         FloppyDriveType),
     DEFINE_PROP_DEFAULT("fallback", FDCtrlISABus, state.fallback,
@@ -2769,8 +2826,8 @@ 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_DRIVE("driveA", FDCtrlSysBus, state.qdev_for_drives[0].blk),
+    DEFINE_PROP_DRIVE("driveB", FDCtrlSysBus, state.qdev_for_drives[1].blk),
     DEFINE_PROP_DEFAULT("fdtypeA", FDCtrlSysBus, state.drives[0].drive,
                         FLOPPY_DRIVE_TYPE_AUTO, qdev_prop_fdc_drive_type,
                         FloppyDriveType),
diff --git a/vl.c b/vl.c
index b3c80d5..f72ded6 100644
--- a/vl.c
+++ b/vl.c
@@ -216,6 +216,7 @@ static struct {
     { .driver = "isa-serial",           .flag = &default_serial    },
     { .driver = "isa-parallel",         .flag = &default_parallel  },
     { .driver = "isa-fdc",              .flag = &default_floppy    },
+    { .driver = "floppy",               .flag = &default_floppy    },
     { .driver = "ide-cd",               .flag = &default_cdrom     },
     { .driver = "ide-hd",               .flag = &default_cdrom     },
     { .driver = "ide-drive",            .flag = &default_cdrom     },
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/3] fdc: Use separate qdev device for drives

[Kevin Wolf]

Am 05.09.2016 um 17:43 hat Kevin Wolf geschrieben:
> We have been complaining for a long time about how the floppy controller and
> floppy drives are combined in a single qdev device and how this makes the
> device awkward to work with because it behaves different from all other block
> devices.
> 
> The latest reason to complain was when I noticed that using qdev device names
> in QMP commands (e.g. for media change) doesn't really work when only the
> controller is a qdev device, but the drives aren't.
> 
> So I decided to have a go at it, and this is the result.
> 
> It doesn't actually change any of the inner workings of the floppy controller,
> but it wires things up differently on the qdev layer so that a floppy
> controller now exposes a bus on which the floppy drives sit. This results in a
> structure that is similar to IDE where the actual drive state is still in the
> controller and the qdev device basically just contains the qdev properties -
> not pretty, but quite workable.
> 
> The commit message of patch 3 explains how to use it. In short, there is a
> '-device floppy' now and it does what you would expect if you ever used ide-cd.
> 
> The other problem is old command lines, especially those using things like
> '-global isa-fdc,driveA=...'. In order to keep them working, we need to forward
> the property to an internally created floppy drive device. This is a bit like
> usb-storage, which we know is ugly, but works well enough in practice. The good
> news here is that in contrast to usb-storage, the floppy controller only does
> the forwarding for legacy configurations; as soon as you start using '-device
> floppy', it doesn't happen any more.
> 
> So as you may have expected, this conversion doesn't result in a perfect
> device, but I think it's definitely an improvement over the old state. I hope
> you like it despite the warts. :-)
> 
> Kevin Wolf (3):
>   fdc: Add a floppy qbus
>   fdc: Add a floppy drive qdev
>   fdc: Move qdev properties to FloppyDrive
> 
>  hw/block/fdc.c | 255 +++++++++++++++++++++++++++++++++++++++++++++++----------
>  vl.c           |   1 +
>  2 files changed, 211 insertions(+), 45 deletions(-)

Ping?

This series really isn't as scary as one may think when looking only at
the subject lines.

Kevin

Re: [Qemu-devel] [PATCH 2/3] fdc: Add a floppy drive qdev

[John Snow]

On 09/05/2016 11:43 AM, Kevin Wolf wrote:
> Floppy controllers automatically create two floppy drive devices in qdev
> now. (They always created two drives, but managed them only internally.)
>

This appears to *actually* create and expose two drives by default, is 
this intentional?

Previously, we created "two drives", but only exposed ones for which 
drive->blk was true -- which was in practice only the first drive by 
default. The CMOS magic we'd expose would be either 0x40 or 0x50 (1.44MB 
and no drive or 2.88MB and no drive) -- now it's 0x55 (two 2.88MB drives.)

As it stands right now, both drives get made as type "AUTO". If 
drive->blk is present, we'll evaluate this to either 128, 144 or 288. 
We'll use a fallback type of 288 if no medium is present or we couldn't 
figure out what was in there.

Otherwise, we'll fall through to fd_revalidate's "No Drive Connected" 
segment which will more or less delete the drive.

This changes fdc-test and acpi-test which will need to be changed if 
this is really what you want.

--js

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/block/fdc.c | 152 +++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 115 insertions(+), 37 deletions(-)
>
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index 4164b31..d1e2339 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -60,6 +60,8 @@
>  #define FLOPPY_BUS(obj) OBJECT_CHECK(FloppyBus, (obj), TYPE_FLOPPY_BUS)
>
>  typedef struct FDCtrl FDCtrl;
> +typedef struct FDrive FDrive;
> +static FDrive *get_drv(FDCtrl *fdctrl, int unit);
>
>  typedef struct FloppyBus {
>      BusState bus;
> @@ -180,7 +182,7 @@ typedef enum FDiskFlags {
>      FDISK_DBL_SIDES  = 0x01,
>  } FDiskFlags;
>
> -typedef struct FDrive {
> +struct FDrive {
>      FDCtrl *fdctrl;
>      BlockBackend *blk;
>      /* Drive status */
> @@ -201,7 +203,7 @@ typedef struct FDrive {
>      uint8_t media_rate;       /* Data rate of medium    */
>
>      bool media_validated;     /* Have we validated the media? */
> -} FDrive;
> +};
>
>
>  static FloppyDriveType get_fallback_drive_type(FDrive *drv);
> @@ -466,6 +468,101 @@ static void fd_revalidate(FDrive *drv)
>      }
>  }
>
> +static void fd_change_cb(void *opaque, bool load)
> +{
> +    FDrive *drive = opaque;
> +
> +    drive->media_changed = 1;
> +    drive->media_validated = false;
> +    fd_revalidate(drive);
> +}
> +
> +static const BlockDevOps fd_block_ops = {
> +    .change_media_cb = fd_change_cb,
> +};
> +
> +
> +#define TYPE_FLOPPY_DRIVE "floppy"
> +#define FLOPPY_DRIVE(obj) \
> +     OBJECT_CHECK(FloppyDrive, (obj), TYPE_FLOPPY_DRIVE)
> +
> +typedef struct FloppyDrive {
> +    DeviceState qdev;
> +    uint32_t    unit;
> +} FloppyDrive;
> +
> +static Property floppy_drive_properties[] = {
> +    DEFINE_PROP_UINT32("unit", FloppyDrive, unit, -1),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static int floppy_drive_init(DeviceState *qdev)
> +{
> +    FloppyDrive *dev = FLOPPY_DRIVE(qdev);
> +    FloppyBus *bus = DO_UPCAST(FloppyBus, bus, dev->qdev.parent_bus);
> +    FDrive *drive;
> +
> +    if (dev->unit == -1) {
> +        for (dev->unit = 0; dev->unit < MAX_FD; dev->unit++) {
> +            drive = get_drv(bus->fdc, dev->unit);
> +            if (!drive->blk) {
> +                break;
> +            }
> +        }
> +    }
> +
> +    if (dev->unit >= MAX_FD) {
> +        error_report("Can't create floppy unit %d, bus supports only %d units",
> +                     dev->unit, MAX_FD);
> +        return -1;
> +    }
> +
> +    /* TODO Check whether unit is in use */
> +
> +    drive = get_drv(bus->fdc, dev->unit);
> +    drive->fdctrl = bus->fdc;
> +
> +    if (drive->blk) {
> +        if (blk_get_on_error(drive->blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC) {
> +            error_report("fdc doesn't support drive option werror");
> +            return -1;
> +        }
> +        if (blk_get_on_error(drive->blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
> +            error_report("fdc doesn't support drive option rerror");
> +            return -1;
> +        }
> +    } else {
> +        /* Anonymous BlockBackend for an empty drive */
> +        drive->blk = blk_new();
> +    }
> +
> +    fd_init(drive);
> +    if (drive->blk) {
> +        blk_set_dev_ops(drive->blk, &fd_block_ops, drive);
> +        pick_drive_type(drive);
> +    }
> +    fd_revalidate(drive);
> +
> +    return 0;
> +}
> +
> +static void floppy_drive_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *k = DEVICE_CLASS(klass);
> +    k->init = floppy_drive_init;
> +    set_bit(DEVICE_CATEGORY_STORAGE, k->categories);
> +    k->bus_type = TYPE_FLOPPY_BUS;
> +    k->props = floppy_drive_properties;
> +    k->desc = "virtual floppy drive";
> +}
> +
> +static const TypeInfo floppy_drive_info = {
> +    .name = TYPE_FLOPPY_DRIVE,
> +    .parent = TYPE_DEVICE,
> +    .instance_size = sizeof(FloppyDrive),
> +    .class_init = floppy_drive_class_init,
> +};
> +
>  /********************************************************/
>  /* Intel 82078 floppy disk controller emulation          */
>
> @@ -1184,9 +1281,9 @@ static inline FDrive *drv3(FDCtrl *fdctrl)
>  }
>  #endif
>
> -static FDrive *get_cur_drv(FDCtrl *fdctrl)
> +static FDrive *get_drv(FDCtrl *fdctrl, int unit)
>  {
> -    switch (fdctrl->cur_drv) {
> +    switch (unit) {
>          case 0: return drv0(fdctrl);
>          case 1: return drv1(fdctrl);
>  #if MAX_FD == 4
> @@ -1197,6 +1294,11 @@ static FDrive *get_cur_drv(FDCtrl *fdctrl)
>      }
>  }
>
> +static FDrive *get_cur_drv(FDCtrl *fdctrl)
> +{
> +    return get_drv(fdctrl, fdctrl->cur_drv);
> +}
> +
>  /* Status A register : 0x00 (read-only) */
>  static uint32_t fdctrl_read_statusA(FDCtrl *fdctrl)
>  {
> @@ -2356,46 +2458,21 @@ static void fdctrl_result_timer(void *opaque)
>      }
>  }
>
> -static void fdctrl_change_cb(void *opaque, bool load)
> -{
> -    FDrive *drive = opaque;
> -
> -    drive->media_changed = 1;
> -    drive->media_validated = false;
> -    fd_revalidate(drive);
> -}
> -
> -static const BlockDevOps fdctrl_block_ops = {
> -    .change_media_cb = fdctrl_change_cb,
> -};
> -
>  /* Init functions */
>  static void fdctrl_connect_drives(FDCtrl *fdctrl, Error **errp)
>  {
>      unsigned int i;
> -    FDrive *drive;
> +    DeviceState *dev;
> +    Error *local_err = NULL;
>
>      for (i = 0; i < MAX_FD; i++) {
> -        drive = &fdctrl->drives[i];
> -        drive->fdctrl = fdctrl;
> -
> -        if (drive->blk) {
> -            if (blk_get_on_error(drive->blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC) {
> -                error_setg(errp, "fdc doesn't support drive option werror");
> -                return;
> -            }
> -            if (blk_get_on_error(drive->blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
> -                error_setg(errp, "fdc doesn't support drive option rerror");
> -                return;
> -            }
> -        }
> -
> -        fd_init(drive);
> -        if (drive->blk) {
> -            blk_set_dev_ops(drive->blk, &fdctrl_block_ops, drive);
> -            pick_drive_type(drive);
> +        dev = qdev_create(&fdctrl->bus.bus, "floppy");
> +        qdev_prop_set_uint32(dev, "unit", i);
> +        object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
>          }
> -        fd_revalidate(drive);
>      }
>  }
>
> @@ -2772,6 +2849,7 @@ static void fdc_register_types(void)
>      type_register_static(&sysbus_fdc_info);
>      type_register_static(&sun4m_fdc_info);
>      type_register_static(&floppy_bus_info);
> +    type_register_static(&floppy_drive_info);
>  }
>
>  type_init(fdc_register_types)
>