qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [RFC PATCH 2/2] Split fdd devices off the floppy controller
Date: Wed, 16 May 2012 22:30:51 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux)

Anthony Liguori <address@hidden> 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<address@hidden>
>> ---
>>   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?



reply via email to

[Prev in Thread] Current Thread [Next in Thread]