Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/4] fdc: Move qdev properties to FloppyDrive

2016-10-17 Thread Kevin Wolf 
Am 15.10.2016 um 00:32 hat John Snow geschrieben:
> 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_tunit;
> >+DeviceState qdev;
> >+uint32_tunit;
> >+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:   ^

Yes. It has the same value, but after my last pull request we need a
DeviceState* here indeed rather than a void*.

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

Doing half of the properties here felt like a good transitional step
from fully converting the PC device to completely ignoring Sun.

Well, I guess, I should fix that...

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH v2 3/4] fdc: Move qdev properties to FloppyDrive

2016-10-14 Thread John Snow 



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_tunit;
+DeviceState qdev;
+uint32_tunit;
+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, _block_ops, drive);
-pick_drive_type(drive);
+blkconf_blocksizes(>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(>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, _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];