Message ID | 1336749740-18474-3-git-send-email-armbru@redhat.com |
---|---|
State | New |
Headers | show |
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
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
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
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
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
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.
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?
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.
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);
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(-)