[Qemu-devel] [RFC PATCH 2/2] Split fdd devices off the floppy controller

[Markus Armbruster]

For historical reasons, and unlike other block devices, our floppy
devices isa-fdc, sysbus-fdc and SUNW,fdtwo integrate the controller
and the drive(s) in a single qdev.  This makes them weird: we need
-global to set up floppy drives, unlike every other optional device.

This patch explores separating them.  It's not a working solution,
yet.  I'm posting it to give us something concrete to discuss.

Separating them involves splitting the per-drive state (struct FDrive)
into a part that belongs to the controller (remains in FDrive), and a
part that belongs to the drive (moves to new struct FDD).  I should
perhaps rename FDrive to reflect that.

An example for state that clearly belongs to the drive is the block
backend.  This patch moves it.  More members of FDrive need moving,
e.g. drive.  To be done in separate commits.  Might impact migration.

I put the fdd objects right into /machine/.  Maybe they should go
elsewhere.  For what it's worth, IDE drives are in
/machine/peripheral/.

Bug: I give all of them the same name "fdd".  QOM happily accepts it.

Instead of definining a full-blown qbus to connect controller and
drives, I link them directly.

There's no use of QOM links in the tree, yet, and the QOM
documentation isn't terribly helpful there.  Please review my
guesswork.

I add one link per fdd the board supports, but I set it only for the
fdds actually present.  With one of two fdds present, qom-fuse shows:

    $ ls -l machine/unattached/device\[18\]/fdd*
    lrwxr-xr-x. 2 1000 1000 4096 Jan  1  1970 
machine/unattached/device[18]/fdd0 -> ../../../machine/fdd
    lrwxr-xr-x. 2 1000 1000 4096 Jan  1  1970 
machine/unattached/device[18]/fdd1 -> ../../..

The second one is weird.

Unfortunately, eliding the qbus means I can't make the floppy disk a
qdev (sub-class of TYPE_DEVICE), because qdevs can only connect to a
qbus.  Anthony tells me that restriction is gone in his latest QOM
series.

Since it's not a qdev, -device fdd does not work.  Pity, because it
defeats the stated purpose of making floppy disk drives work like
other existing optional devices.

There doesn't seem to be a way to create QOM objects via command line
or monitor.

Speaking of monitor: the QOM commands are only implemented in QMP, and
they are entirely undocumented.  Sets a bad example; wonder how it got
past the maintainer ;)

Note: I *break* -global isa-fdc.driveA=...  The properties are simply
gone.  Fixable if we need backwards compatibility there.

The floppy controller device should probably be a child of a super I/O
device, grandchild of a south bridge device, or similar, depending on
the board.  Outside this commit's scope.

Signed-off-by: Markus Armbruster <address@hidden>
---
 hw/fdc.c |  124 +++++++++++++++++++++++++++++++++++--------------------------
 1 files changed, 71 insertions(+), 53 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index d9c4fbf..98ff87a 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -54,6 +54,33 @@
 /********************************************************/
 /* Floppy drive emulation                               */
 
+typedef struct FDD {
+    Object obj;
+    BlockDriverState *bs;
+} FDD;
+
+#define TYPE_FDD "fdd"
+
+static TypeInfo fdd_info = {
+    .name          = TYPE_FDD,
+    .parent        = TYPE_OBJECT,
+    .instance_size = sizeof(FDD),
+};
+
+static void fdd_create(Object *fdc, const char *link_name,
+                       BlockDriverState *bs)
+{
+    Object *obj = object_new(TYPE_FDD);
+    FDD *fdd = OBJECT_CHECK(FDD, obj, TYPE_FDD);
+
+    fdd->bs = bs;
+    object_property_add_child(qdev_get_machine(), "fdd", obj, NULL);
+    object_property_set_link(fdc, obj, link_name, NULL);
+}
+
+/********************************************************/
+/* Intel 82078 floppy disk controller emulation          */
+
 #define GET_CUR_DRV(fdctrl) ((fdctrl)->cur_drv)
 #define SET_CUR_DRV(fdctrl, drive) ((fdctrl)->cur_drv = (drive))
 
@@ -71,7 +98,7 @@ typedef enum FDiskFlags {
 
 typedef struct FDrive {
     FDCtrl *fdctrl;
-    BlockDriverState *bs;
+    FDD *fdd;
     /* Drive status */
     FDriveType drive;
     uint8_t perpendicular;    /* 2.88 MB access mode    */
@@ -179,9 +206,9 @@ static void fd_revalidate(FDrive *drv)
     FDriveRate rate;
 
     FLOPPY_DPRINTF("revalidate\n");
-    if (drv->bs != NULL && bdrv_is_inserted(drv->bs)) {
-        ro = bdrv_is_read_only(drv->bs);
-        bdrv_get_floppy_geometry_hint(drv->bs, &nb_heads, &max_track,
+    if (drv->fdd != NULL && bdrv_is_inserted(drv->fdd->bs)) {
+        ro = bdrv_is_read_only(drv->fdd->bs);
+        bdrv_get_floppy_geometry_hint(drv->fdd->bs, &nb_heads, &max_track,
                                       &last_sect, drv->drive, &drive, &rate);
         if (nb_heads != 0 && max_track != 0 && last_sect != 0) {
             FLOPPY_DPRINTF("User defined disk (%d %d %d)",
@@ -208,9 +235,6 @@ static void fd_revalidate(FDrive *drv)
     }
 }
 
-/********************************************************/
-/* Intel 82078 floppy disk controller emulation          */
-
 static void fdctrl_reset(FDCtrl *fdctrl, int do_irq);
 static void fdctrl_reset_fifo(FDCtrl *fdctrl);
 static int fdctrl_transfer_handler (void *opaque, int nchan,
@@ -543,7 +567,7 @@ static bool fdrive_media_changed_needed(void *opaque)
 {
     FDrive *drive = opaque;
 
-    return (drive->bs != NULL && drive->media_changed != 1);
+    return (drive->fdd != NULL && drive->media_changed != 1);
 }
 
 static const VMStateDescription vmstate_fdrive_media_changed = {
@@ -729,7 +753,7 @@ static void fdctrl_reset(FDCtrl *fdctrl, int do_irq)
     /* Initialise controller */
     fdctrl->sra = 0;
     fdctrl->srb = 0xc0;
-    if (!fdctrl->drives[1].bs)
+    if (!fdctrl->drives[1].fdd)
         fdctrl->sra |= FD_SRA_nDRV2;
     fdctrl->cur_drv = 0;
     fdctrl->dor = FD_DOR_nRESET;
@@ -1197,7 +1221,7 @@ static int fdctrl_transfer_handler (void *opaque, int 
nchan,
         status2 = FD_SR2_SNS;
     if (dma_len > fdctrl->data_len)
         dma_len = fdctrl->data_len;
-    if (cur_drv->bs == NULL) {
+    if (!cur_drv->fdd) {
         if (fdctrl->data_dir == FD_DIR_WRITE)
             fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 
0x00);
         else
@@ -1218,7 +1242,7 @@ static int fdctrl_transfer_handler (void *opaque, int 
nchan,
         if (fdctrl->data_dir != FD_DIR_WRITE ||
             len < FD_SECTOR_LEN || rel_pos != 0) {
             /* READ & SCAN commands and realign to a sector for WRITE */
-            if (bdrv_read(cur_drv->bs, fd_sector(cur_drv),
+            if (bdrv_read(cur_drv->fdd->bs, fd_sector(cur_drv),
                           fdctrl->fifo, 1) < 0) {
                 FLOPPY_DPRINTF("Floppy: error getting sector %d\n",
                                fd_sector(cur_drv));
@@ -1246,7 +1270,7 @@ static int fdctrl_transfer_handler (void *opaque, int 
nchan,
 
             DMA_read_memory (nchan, fdctrl->fifo + rel_pos,
                              fdctrl->data_pos, len);
-            if (bdrv_write(cur_drv->bs, fd_sector(cur_drv),
+            if (bdrv_write(cur_drv->fdd->bs, fd_sector(cur_drv),
                            fdctrl->fifo, 1) < 0) {
                 FLOPPY_ERROR("writing sector %d\n", fd_sector(cur_drv));
                 fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 
0x00, 0x00);
@@ -1320,7 +1344,8 @@ static uint32_t fdctrl_read_data(FDCtrl *fdctrl)
                                    fd_sector(cur_drv));
                     return 0;
                 }
-            if (bdrv_read(cur_drv->bs, fd_sector(cur_drv), fdctrl->fifo, 1) < 
0) {
+            if (bdrv_read(cur_drv->fdd->bs,
+                          fd_sector(cur_drv), fdctrl->fifo, 1) < 0) {
                 FLOPPY_DPRINTF("error getting sector %d\n",
                                fd_sector(cur_drv));
                 /* Sure, image size is too small... */
@@ -1389,8 +1414,8 @@ static void fdctrl_format_sector(FDCtrl *fdctrl)
         break;
     }
     memset(fdctrl->fifo, 0, FD_SECTOR_LEN);
-    if (cur_drv->bs == NULL ||
-        bdrv_write(cur_drv->bs, fd_sector(cur_drv), fdctrl->fifo, 1) < 0) {
+    if (!cur_drv->fdd ||
+        bdrv_write(cur_drv->fdd->bs, fd_sector(cur_drv), fdctrl->fifo, 1) < 0) 
{
         FLOPPY_ERROR("formatting sector %d\n", fd_sector(cur_drv));
         fdctrl_stop_transfer(fdctrl, FD_SR0_ABNTERM | FD_SR0_SEEK, 0x00, 0x00);
     } else {
@@ -1779,7 +1804,8 @@ static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t 
value)
         if (pos == FD_SECTOR_LEN - 1 ||
             fdctrl->data_pos == fdctrl->data_len) {
             cur_drv = get_cur_drv(fdctrl);
-            if (bdrv_write(cur_drv->bs, fd_sector(cur_drv), fdctrl->fifo, 1) < 
0) {
+            if (bdrv_write(cur_drv->fdd->bs,
+                           fd_sector(cur_drv), fdctrl->fifo, 1) < 0) {
                 FLOPPY_ERROR("writing sector %d\n", fd_sector(cur_drv));
                 return;
             }
@@ -1866,12 +1892,12 @@ static int fdctrl_connect_drives(FDCtrl *fdctrl)
         drive = &fdctrl->drives[i];
         drive->fdctrl = fdctrl;
 
-        if (drive->bs) {
-            if (bdrv_get_on_error(drive->bs, 0) != BLOCK_ERR_STOP_ENOSPC) {
+        if (drive->fdd) {
+            if (bdrv_get_on_error(drive->fdd->bs, 0) != BLOCK_ERR_STOP_ENOSPC) 
{
                 error_report("fdc doesn't support drive option werror");
                 return -1;
             }
-            if (bdrv_get_on_error(drive->bs, 1) != BLOCK_ERR_REPORT) {
+            if (bdrv_get_on_error(drive->fdd->bs, 1) != BLOCK_ERR_REPORT) {
                 error_report("fdc doesn't support drive option rerror");
                 return -1;
             }
@@ -1879,28 +1905,41 @@ static int fdctrl_connect_drives(FDCtrl *fdctrl)
 
         fd_init(drive);
         fd_revalidate(drive);
-        if (drive->bs) {
-            bdrv_set_dev_ops(drive->bs, &fdctrl_block_ops, drive);
+        if (drive->fdd) {
+            bdrv_set_dev_ops(drive->fdd->bs, &fdctrl_block_ops, drive);
         }
     }
     return 0;
 }
 
+static void fdctrl_create_fdd(Object *fdc, FDrive drives[],
+                              DriveInfo *dinfo[], int n)
+{
+    char name[5] = "fdd#";
+    int i;
+
+    for (i = 0; i < n; i++) {
+        name[3] = '0' + i;
+        object_property_add_link(fdc, name, TYPE_FDD,
+                                 (Object **)&drives[i].fdd, NULL);
+        if (dinfo[i]) {
+            fdd_create(fdc, name, dinfo[i]->bdrv);
+        }
+    }
+}
+
 ISADevice *fdctrl_init_isa(ISABus *bus, DriveInfo **fds)
 {
     ISADevice *dev;
+    FDCtrlISABus *isa;
 
     dev = isa_try_create(bus, "isa-fdc");
     if (!dev) {
         return NULL;
     }
 
-    if (fds[0]) {
-        qdev_prop_set_drive_nofail(&dev->qdev, "driveA", fds[0]->bdrv);
-    }
-    if (fds[1]) {
-        qdev_prop_set_drive_nofail(&dev->qdev, "driveB", fds[1]->bdrv);
-    }
+    isa = DO_UPCAST(FDCtrlISABus, busdev, dev);
+    fdctrl_create_fdd(OBJECT(isa), isa->state.drives, fds, 2);
     qdev_init_nofail(&dev->qdev);
 
     return dev;
@@ -1917,12 +1956,7 @@ void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
     sys = DO_UPCAST(FDCtrlSysBus, busdev.qdev, dev);
     fdctrl = &sys->state;
     fdctrl->dma_chann = dma_chann; /* FIXME */
-    if (fds[0]) {
-        qdev_prop_set_drive_nofail(dev, "driveA", fds[0]->bdrv);
-    }
-    if (fds[1]) {
-        qdev_prop_set_drive_nofail(dev, "driveB", fds[1]->bdrv);
-    }
+    fdctrl_create_fdd(OBJECT(sys), sys->state.drives, fds, 2);
     qdev_init_nofail(dev);
     sysbus_connect_irq(&sys->busdev, 0, irq);
     sysbus_mmio_map(&sys->busdev, 0, mmio_base);
@@ -1935,11 +1969,9 @@ void sun4m_fdctrl_init(qemu_irq irq, target_phys_addr_t 
io_base,
     FDCtrlSysBus *sys;
 
     dev = qdev_create(NULL, "SUNW,fdtwo");
-    if (fds[0]) {
-        qdev_prop_set_drive_nofail(dev, "drive", fds[0]->bdrv);
-    }
-    qdev_init_nofail(dev);
     sys = DO_UPCAST(FDCtrlSysBus, busdev.qdev, dev);
+    fdctrl_create_fdd(OBJECT(sys), sys->state.drives, fds, 1);
+    qdev_init_nofail(dev);
     sysbus_connect_irq(&sys->busdev, 0, irq);
     sysbus_mmio_map(&sys->busdev, 0, io_base);
     *fdc_tc = qdev_get_gpio_in(dev, 0);
@@ -2043,7 +2075,7 @@ void fdc_get_bs(BlockDriverState *bs[], ISADevice *dev)
     int i;
 
     for (i = 0; i < MAX_FD; i++) {
-        bs[i] = fdctrl->drives[i].bs;
+        bs[i] = fdctrl->drives[i].fdd ? fdctrl->drives[i].fdd->bs : NULL;
     }
 }
 
@@ -2062,8 +2094,6 @@ static Property isa_fdc_properties[] = {
     DEFINE_PROP_HEX32("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].bs),
-    DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.drives[1].bs),
     DEFINE_PROP_INT32("bootindexA", FDCtrlISABus, bootindexA, -1),
     DEFINE_PROP_INT32("bootindexB", FDCtrlISABus, bootindexB, -1),
     DEFINE_PROP_BIT("check_media_rate", FDCtrlISABus, state.check_media_rate,
@@ -2100,12 +2130,6 @@ static const VMStateDescription vmstate_sysbus_fdc ={
     }
 };
 
-static Property sysbus_fdc_properties[] = {
-    DEFINE_PROP_DRIVE("driveA", FDCtrlSysBus, state.drives[0].bs),
-    DEFINE_PROP_DRIVE("driveB", FDCtrlSysBus, state.drives[1].bs),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
 static void sysbus_fdc_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -2114,7 +2138,6 @@ static void sysbus_fdc_class_init(ObjectClass *klass, 
void *data)
     k->init = sysbus_fdc_init1;
     dc->reset = fdctrl_external_reset_sysbus;
     dc->vmsd = &vmstate_sysbus_fdc;
-    dc->props = sysbus_fdc_properties;
 }
 
 static TypeInfo sysbus_fdc_info = {
@@ -2124,11 +2147,6 @@ static TypeInfo sysbus_fdc_info = {
     .class_init    = sysbus_fdc_class_init,
 };
 
-static Property sun4m_fdc_properties[] = {
-    DEFINE_PROP_DRIVE("drive", FDCtrlSysBus, state.drives[0].bs),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
 static void sun4m_fdc_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -2137,7 +2155,6 @@ static void sun4m_fdc_class_init(ObjectClass *klass, void 
*data)
     k->init = sun4m_fdc_init1;
     dc->reset = fdctrl_external_reset_sysbus;
     dc->vmsd = &vmstate_sysbus_fdc;
-    dc->props = sun4m_fdc_properties;
 }
 
 static TypeInfo sun4m_fdc_info = {
@@ -2149,6 +2166,7 @@ static TypeInfo sun4m_fdc_info = {
 
 static void fdc_register_types(void)
 {
+    type_register_static(&fdd_info);
     type_register_static(&isa_fdc_info);
     type_register_static(&sysbus_fdc_info);
     type_register_static(&sun4m_fdc_info);
-- 
1.7.6.5