From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34501) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bvB1k-00035X-CO for qemu-devel@nongnu.org; Fri, 14 Oct 2016 18:32:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bvB1h-0003j2-K7 for qemu-devel@nongnu.org; Fri, 14 Oct 2016 18:32:31 -0400 References: <1475264368-9838-1-git-send-email-kwolf@redhat.com> <1475264368-9838-4-git-send-email-kwolf@redhat.com> From: John Snow Message-ID: <58a36fa8-41ad-1dc3-b9ff-c9f01ef5f73a@redhat.com> Date: Fri, 14 Oct 2016 18:32:19 -0400 MIME-Version: 1.0 In-Reply-To: <1475264368-9838-4-git-send-email-kwolf@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 3/4] fdc: Move qdev properties to FloppyDrive List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf , qemu-block@nongnu.org Cc: qemu-devel@nongnu.org, armbru@redhat.com, mreitz@redhat.com On 09/30/2016 03:39 PM, Kevin Wolf wrote: > 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 > --- > hw/block/fdc.c | 112 ++++++++++++++++++++++++++++++++++++++++++--------------- > vl.c | 1 + > 2 files changed, 85 insertions(+), 28 deletions(-) > > diff --git a/hw/block/fdc.c b/hw/block/fdc.c > index 5aa8e52..00c0ec6 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,29 +525,57 @@ static int floppy_drive_init(DeviceState *qdev) > return -1; > } > > - /* TODO Check whether unit is in use */ > - > drive = get_drv(bus->fdc, dev->unit); > - > 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); Missing a 'q' here: ^ > + assert(ret == 0); > } > > - fd_init(drive); > - if (drive->blk) { > - blk_set_dev_ops(drive->blk, &fd_block_ops, drive); > - pick_drive_type(drive); > + 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); > + blk_set_dev_ops(drive->blk, &fd_block_ops, drive); > + > + /* Keep 'type' qdev property and FDrive->drive in sync */ > + drive->drive = dev->type; > + pick_drive_type(drive); > + dev->type = drive->drive; > + > fd_revalidate(drive); > > return 0; > @@ -808,6 +844,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,11 +2499,13 @@ 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; > FDrive *drive; > DeviceState *dev; > + BlockBackend *blk; > Error *local_err = NULL; > > for (i = 0; i < MAX_FD; i++) { > @@ -2472,7 +2514,8 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl, Error **errp) > > /* If the drive is not present, we skip creating the qdev device, but > * still have to initialise the controller. */ > - if (!fdctrl->drives[i].blk) { > + blk = fdctrl->qdev_for_drives[i].blk; > + if (!blk) { > fd_init(drive); > fd_revalidate(drive); > continue; > @@ -2480,6 +2523,19 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl, Error **errp) > > 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); > + > + 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); > @@ -2597,7 +2653,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[] = { > @@ -2723,14 +2779,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, > @@ -2782,8 +2838,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), ^ Does sysbus' type property not need updating ...? > diff --git a/vl.c b/vl.c > index ab0349b..7486f33 100644 > --- a/vl.c > +++ b/vl.c > @@ -218,6 +218,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 }, > wew... Floppy drives, man. Fun.