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

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

[Markus Armbruster]

This is probaly more about QOM than about floppy.  It's not a working
solution, yet.  I'm posting it to give us something concrete to
discuss.

Based on Kevin's block branch, commit ad431215.

Markus Armbruster (2):
  Un-inline fdctrl_init_isa()
  Split fdd devices off the floppy controller

 hw/fdc.c      |  132 ++++++++++++++++++++++++++++++++++++--------------------
 hw/fdc.h      |   24 +---------
 hw/ide/piix.c |    3 +-
 hw/isa.h      |    2 -
 hw/pc_sysfw.c |    1 +
 qemu-common.h |    1 +
 6 files changed, 91 insertions(+), 72 deletions(-)

-- 
1.7.6.5

[Qemu-devel] [RFC PATCH 1/2] Un-inline fdctrl_init_isa()

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/fdc.c      |   20 ++++++++++++++++++++
 hw/fdc.h      |   24 ++----------------------
 hw/ide/piix.c |    3 ++-
 hw/isa.h      |    2 --
 hw/pc_sysfw.c |    1 +
 qemu-common.h |    1 +
 6 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index cb4cd25..d9c4fbf 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -1886,6 +1886,26 @@ static int fdctrl_connect_drives(FDCtrl *fdctrl)
     return 0;
 }
 
+ISADevice *fdctrl_init_isa(ISABus *bus, DriveInfo **fds)
+{
+    ISADevice *dev;
+
+    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);
+    }
+    qdev_init_nofail(&dev->qdev);
+
+    return dev;
+}
+
 void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
                         target_phys_addr_t mmio_base, DriveInfo **fds)
 {
diff --git a/hw/fdc.h b/hw/fdc.h
index 55a8d73..1b32b17 100644
--- a/hw/fdc.h
+++ b/hw/fdc.h
@@ -1,32 +1,12 @@
 #ifndef HW_FDC_H
 #define HW_FDC_H
 
-#include "isa.h"
-#include "blockdev.h"
+#include "qemu-common.h"
 
 /* fdc.c */
 #define MAX_FD 2
 
-static inline ISADevice *fdctrl_init_isa(ISABus *bus, DriveInfo **fds)
-{
-    ISADevice *dev;
-
-    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);
-    }
-    qdev_init_nofail(&dev->qdev);
-
-    return dev;
-}
-
+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,
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index bcaa400..f5a74c2 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -22,11 +22,12 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
+
 #include <hw/hw.h>
 #include <hw/pc.h>
 #include <hw/pci.h>
 #include <hw/isa.h>
-#include "block.h"
+#include "blockdev.h"
 #include "sysemu.h"
 #include "dma.h"
 
diff --git a/hw/isa.h b/hw/isa.h
index f7bc4b5..6c6fd7f 100644
--- a/hw/isa.h
+++ b/hw/isa.h
@@ -9,8 +9,6 @@
 
 #define ISA_NUM_IRQS 16
 
-typedef struct ISADevice ISADevice;
-
 #define TYPE_ISA_DEVICE "isa-device"
 #define ISA_DEVICE(obj) \
      OBJECT_CHECK(ISADevice, (obj), TYPE_ISA_DEVICE)
diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
index f0d7c21..b45f0ac 100644
--- a/hw/pc_sysfw.c
+++ b/hw/pc_sysfw.c
@@ -23,6 +23,7 @@
  * THE SOFTWARE.
  */
 
+#include "blockdev.h"
 #include "sysbus.h"
 #include "hw.h"
 #include "pc.h"
diff --git a/qemu-common.h b/qemu-common.h
index cccfb42..0bb1078 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -239,6 +239,7 @@ typedef struct VLANState VLANState;
 typedef struct VLANClientState VLANClientState;
 typedef struct i2c_bus i2c_bus;
 typedef struct ISABus ISABus;
+typedef struct ISADevice ISADevice;
 typedef struct SMBusDevice SMBusDevice;
 typedef struct PCIHostState PCIHostState;
 typedef struct PCIExpressHost PCIExpressHost;
-- 
1.7.6.5

[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 <armbru@redhat.com>
---
 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

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

[Markus Armbruster]

Forgot to cc: Paolo and Mike, apologies!

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

[Kevin Wolf]

Am 11.05.2012 17:22, schrieb 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.

I like the idea of splitting the drives from the controller. In fact, I
think we could even try to split them into a separate hw/fdd.c

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

As long as this is true, committing a patch like this doesn't help a
lot, so I hope Anthony's patches will go in before this is ready.

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

We might need it, I seem to remember that libvirt uses it.

Kevin

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

[Daniel P. Berrange]

On Mon, May 14, 2012 at 10:47:35AM +0200, Kevin Wolf wrote:
> Am 11.05.2012 17:22, schrieb 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.
> 
> I like the idea of splitting the drives from the controller. In fact, I
> think we could even try to split them into a separate hw/fdd.c
> 
> > 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.
> 
> As long as this is true, committing a patch like this doesn't help a
> lot, so I hope Anthony's patches will go in before this is ready.
> 
> > Note: I *break* -global isa-fdc.driveA=...  The properties are simply
> > gone.  Fixable if we need backwards compatibility there.
> 
> We might need it, I seem to remember that libvirt uses it.

Yes, since we had no other way to configure floppys, we used the -global
options. I welcome a move to bring floppys into line with other disks,
but would like us to have a little bit of overlap where -global still
works, before finally being removed in a later release.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

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

[Andreas Färber]

Am 14.05.2012 10:47, schrieb Kevin Wolf:
> Am 11.05.2012 17:22, schrieb 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.
> 
> I like the idea of splitting the drives from the controller. In fact, I
> think we could even try to split them into a separate hw/fdd.c

Seconded, however that might make the patch harder to read due to the
code movements.

>> 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.
> 
> As long as this is true, committing a patch like this doesn't help a
> lot, so I hope Anthony's patches will go in before this is ready.

Having gone through nearly all QOM patches on the list for qom-next,
this is not something I remember seeing yet.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Kevin Wolf]

Am 14.05.2012 13:58, schrieb Andreas Färber:
> Am 14.05.2012 10:47, schrieb Kevin Wolf:
>> Am 11.05.2012 17:22, schrieb 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.
>>
>> I like the idea of splitting the drives from the controller. In fact, I
>> think we could even try to split them into a separate hw/fdd.c
> 
> Seconded, however that might make the patch harder to read due to the
> code movements.

I didn't mean that we should do it in this patch, of course, but as a
second step.

Kevin

Re: [Qemu-devel] [RFC PATCH 1/2] Un-inline fdctrl_init_isa()

[Anthony Liguori]

On 05/11/2012 10:22 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster<armbru@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

I know this is a pervasive idiom, but it makes little sense to me and I've got a 
number of similar changes queued up myself.

Regards,

Anthony Liguori

> ---
>   hw/fdc.c      |   20 ++++++++++++++++++++
>   hw/fdc.h      |   24 ++----------------------
>   hw/ide/piix.c |    3 ++-
>   hw/isa.h      |    2 --
>   hw/pc_sysfw.c |    1 +
>   qemu-common.h |    1 +
>   6 files changed, 26 insertions(+), 25 deletions(-)
>
> diff --git a/hw/fdc.c b/hw/fdc.c
> index cb4cd25..d9c4fbf 100644
> --- a/hw/fdc.c
> +++ b/hw/fdc.c
> @@ -1886,6 +1886,26 @@ static int fdctrl_connect_drives(FDCtrl *fdctrl)
>       return 0;
>   }
>
> +ISADevice *fdctrl_init_isa(ISABus *bus, DriveInfo **fds)
> +{
> +    ISADevice *dev;
> +
> +    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);
> +    }
> +    qdev_init_nofail(&dev->qdev);
> +
> +    return dev;
> +}
> +
>   void fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
>                           target_phys_addr_t mmio_base, DriveInfo **fds)
>   {
> diff --git a/hw/fdc.h b/hw/fdc.h
> index 55a8d73..1b32b17 100644
> --- a/hw/fdc.h
> +++ b/hw/fdc.h
> @@ -1,32 +1,12 @@
>   #ifndef HW_FDC_H
>   #define HW_FDC_H
>
> -#include "isa.h"
> -#include "blockdev.h"
> +#include "qemu-common.h"
>
>   /* fdc.c */
>   #define MAX_FD 2
>
> -static inline ISADevice *fdctrl_init_isa(ISABus *bus, DriveInfo **fds)
> -{
> -    ISADevice *dev;
> -
> -    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);
> -    }
> -    qdev_init_nofail(&dev->qdev);
> -
> -    return dev;
> -}
> -
> +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,
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index bcaa400..f5a74c2 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -22,11 +22,12 @@
>    * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>    * THE SOFTWARE.
>    */
> +
>   #include<hw/hw.h>
>   #include<hw/pc.h>
>   #include<hw/pci.h>
>   #include<hw/isa.h>
> -#include "block.h"
> +#include "blockdev.h"
>   #include "sysemu.h"
>   #include "dma.h"
>
> diff --git a/hw/isa.h b/hw/isa.h
> index f7bc4b5..6c6fd7f 100644
> --- a/hw/isa.h
> +++ b/hw/isa.h
> @@ -9,8 +9,6 @@
>
>   #define ISA_NUM_IRQS 16
>
> -typedef struct ISADevice ISADevice;
> -
>   #define TYPE_ISA_DEVICE "isa-device"
>   #define ISA_DEVICE(obj) \
>        OBJECT_CHECK(ISADevice, (obj), TYPE_ISA_DEVICE)
> diff --git a/hw/pc_sysfw.c b/hw/pc_sysfw.c
> index f0d7c21..b45f0ac 100644
> --- a/hw/pc_sysfw.c
> +++ b/hw/pc_sysfw.c
> @@ -23,6 +23,7 @@
>    * THE SOFTWARE.
>    */
>
> +#include "blockdev.h"
>   #include "sysbus.h"
>   #include "hw.h"
>   #include "pc.h"
> diff --git a/qemu-common.h b/qemu-common.h
> index cccfb42..0bb1078 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -239,6 +239,7 @@ typedef struct VLANState VLANState;
>   typedef struct VLANClientState VLANClientState;
>   typedef struct i2c_bus i2c_bus;
>   typedef struct ISABus ISABus;
> +typedef struct ISADevice ISADevice;
>   typedef struct SMBusDevice SMBusDevice;
>   typedef struct PCIHostState PCIHostState;
>   typedef struct PCIExpressHost PCIExpressHost;

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

[Anthony Liguori]

On 05/11/2012 10:22 AM, Markus Armbruster wrote:
> 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.

That's a bug in qom-fuse.  It's an empty link and I unconditionally prepend the 
relative path to the root to make the non-empty links work.  It's an easy fix.

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

Yup.  It's queued in qom-next actually (which is probably where you want to work 
now).

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

They're thoroughly documented actually...

##
# @qom-get:
#
# This command will get a property from a object model path and return the
# value.
#
# @path: The path within the object model.  There are two forms of supported
#        paths--absolute and partial paths.
#
#        Absolute paths are derived from the root object and can follow child<>
#        or link<> properties.  Since they can follow link<> properties, they
#        can be arbitrarily long.  Absolute paths look like absolute filenames
#        and are prefixed  with a leading slash.
#
#        Partial paths look like relative filenames.  They do not begin
#        with a prefix.  The matching rules for partial paths are subtle but
#        designed to make specifying objects easy.  At each level of the
#        composition tree, the partial path is matched as an absolute path.
#        The first match is not returned.  At least two matches are searched
#        for.  A successful result is only returned if only one match is
#        found.  If more than one match is found, a flag is return to
#        indicate that the match was ambiguous.
#
# @property: The property name to read
#
# Returns: The property value.  The type depends on the property type.  legacy<>
#          properties are returned as #str.  child<> and link<> properties are
#          returns as #str pathnames.  All integer property types (u8, u16, etc)
#          are returned as #int.
#
# Since: 1.1
#
# Notes: This command is experimental and may change syntax in future releases.
##
{ 'command': 'qom-get',
   'data': { 'path': 'str', 'property': 'str' },
   'returns': 'visitor',
   'gen': 'no' }

I assume you're looking in qmp-commands.hx...  At this point, we should just 
remove all of the docs in that file to avoid future confusion.

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

We do need to make sure we preserve backwards compat here.

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

I looked through the PIIX4 documentation the other day and it doesn't appear 
that there is a floppy controller in the PIIX4.  I think it must have been an 
ISA device so I think inheriting from ISA and being a child of /machine makes 
sense conceptionally.

>
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
> ---
>   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);
> +}

This is not quite right.  You want to do the actual initialization in 
instance_init as a method.  You should make the BlockDriverState a property too. 
  The fdd_create() call then because object_new() + setting properties only.

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC PATCH 1/2] Un-inline fdctrl_init_isa()

[Kevin Wolf]

Am 14.05.2012 15:32, schrieb Anthony Liguori:
> On 05/11/2012 10:22 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster<armbru@redhat.com>
> 
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> I know this is a pervasive idiom, but it makes little sense to me and I've got a 
> number of similar changes queued up myself.

The patch makes sense on its own, so I applied it to block-next now even
though the series is just an RFC. We can still update or drop it in the
unlikely case that we find that it's actually a stupid thing to do.

Kevin

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.05.2012 13:58, schrieb Andreas Färber:
>> Am 14.05.2012 10:47, schrieb Kevin Wolf:
>>> Am 11.05.2012 17:22, schrieb 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.
>>>
>>> I like the idea of splitting the drives from the controller. In fact, I
>>> think we could even try to split them into a separate hw/fdd.c
>> 
>> Seconded, however that might make the patch harder to read due to the
>> code movements.
>
> I didn't mean that we should do it in this patch, of course, but as a
> second step.

Sounds like a plan.

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

[Markus Armbruster]

Anthony Liguori <aliguori@us.ibm.com> writes:

> On 05/11/2012 10:22 AM, Markus Armbruster wrote:
>> 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.
>
> That's a bug in qom-fuse.  It's an empty link and I unconditionally
> prepend the relative path to the root to make the non-empty links
> work.  It's an easy fix.
>
>> 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.
>
> Yup.  It's queued in qom-next actually (which is probably where you
> want to work now).

Thanks, I'll rebase.

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

Shouldn't there be one?

>> 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 ;)
>
> They're thoroughly documented actually...
>
> ##
> # @qom-get:
[...]
> I assume you're looking in qmp-commands.hx...  At this point, we
> should just remove all of the docs in that file to avoid future
> confusion.

Yes, please.

>> Note: I *break* -global isa-fdc.driveA=...  The properties are simply
>> gone.  Fixable if we need backwards compatibility there.
>
> We do need to make sure we preserve backwards compat here.

I'll cook up something.

>> 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.
>
> I looked through the PIIX4 documentation the other day and it doesn't
> appear that there is a floppy controller in the PIIX4.  I think it
> must have been an ISA device so I think inheriting from ISA and being
> a child of /machine makes sense conceptionally.

No problem.

I guess for q35 we could model the fdc as part of the super I/O device,
which connected to the south bridge via LPC.

>> Signed-off-by: Markus Armbruster<armbru@redhat.com>
>> ---
>>   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);
>> +}
>
> This is not quite right.  You want to do the actual initialization in
> instance_init as a method.

Will do, thanks.

>                             You should make the BlockDriverState a
> property too. The fdd_create() call then because object_new() +
> setting properties only.

Last sentence no verb?

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

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Anthony Liguori <aliguori@us.ibm.com> writes:
>
>> On 05/11/2012 10:22 AM, Markus Armbruster wrote:
[...]
>>> 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);
>>> +}
>>
>> This is not quite right.  You want to do the actual initialization in
>> instance_init as a method.
>
> Will do, thanks.
>
>>                             You should make the BlockDriverState a
>> property too. The fdd_create() call then because object_new() +
>> setting properties only.
>
> Last sentence no verb?

I'm guessing s/because/becomes/

I'm afraid adding a drive property would require way too much surgery
right now.  The existing drive property code is encapsulated in
qdev-properties.c, and works only for subtypes of TYPE_DEVICE.  I guess
I have to shelve this work until I can make TYPE_FDD one.