From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51452) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1STvyT-0008AI-Dp for qemu-devel@nongnu.org; Mon, 14 May 2012 10:10:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1STvYo-0007Rq-2l for qemu-devel@nongnu.org; Mon, 14 May 2012 09:44:30 -0400 Received: from e37.co.us.ibm.com ([32.97.110.158]:38918) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1STvYn-0007Qj-LS for qemu-devel@nongnu.org; Mon, 14 May 2012 09:43:37 -0400 Received: from /spool/local by e37.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 14 May 2012 07:43:29 -0600 Received: from d03relay02.boulder.ibm.com (d03relay02.boulder.ibm.com [9.17.195.227]) by d03dlp01.boulder.ibm.com (Postfix) with ESMTP id 8752A1FF0025 for ; Mon, 14 May 2012 07:42:33 -0600 (MDT) Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by d03relay02.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q4EDgXGd150208 for ; Mon, 14 May 2012 07:42:34 -0600 Received: from d03av01.boulder.ibm.com (loopback [127.0.0.1]) by d03av01.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q4EDgTa0002197 for ; Mon, 14 May 2012 07:42:29 -0600 Message-ID: <4FB10BC2.6080306@us.ibm.com> Date: Mon, 14 May 2012 08:42:26 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1336749740-18474-1-git-send-email-armbru@redhat.com> <1336749740-18474-3-git-send-email-armbru@redhat.com> In-Reply-To: <1336749740-18474-3-git-send-email-armbru@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH 2/2] Split fdd devices off the floppy controller List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, hpoussin@reactos.org, qemu-devel@nongnu.org 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 > --- > 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