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

[Anthony Liguori <aliguori@us.ibm.com>]

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