Anthony Liguori <aligu...@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<arm...@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?
