From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44563) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3PvG-0004ZK-Uu for qemu-devel@nongnu.org; Sun, 28 Jul 2013 08:18:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V3Pv8-0001Cd-0K for qemu-devel@nongnu.org; Sun, 28 Jul 2013 08:18:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:28667) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V3Pv7-0001CR-P7 for qemu-devel@nongnu.org; Sun, 28 Jul 2013 08:17:53 -0400 Date: Sun, 28 Jul 2013 15:19:04 +0300 From: "Michael S. Tsirkin" Message-ID: <20130728121904.GB15065@redhat.com> References: <1374681580-17439-1-git-send-email-mst@redhat.com> <1374681580-17439-12-git-send-email-mst@redhat.com> <20130725093248.GA27524@redhat.com> <51F46201.2010106@suse.de> <20130728073028.GF12087@redhat.com> <51F4E689.80405@suse.de> <20130728101427.GA15065@redhat.com> <51F4F2FB.5010800@suse.de> <51F4FB9D.2080401@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <51F4FB9D.2080401@suse.de> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: Aurelien Jarno , qemu-devel@nongnu.org, Anthony Liguori , Gerd Hoffmann On Sun, Jul 28, 2013 at 01:08:13PM +0200, Andreas F=E4rber wrote: > Am 28.07.2013 12:31, schrieb Andreas F=E4rber: > > Am 28.07.2013 12:14, schrieb Michael S. Tsirkin: > >> On Sun, Jul 28, 2013 at 11:38:17AM +0200, Andreas F=E4rber wrote: > >>> Am 28.07.2013 09:30, schrieb Michael S. Tsirkin: > >>>> On Sun, Jul 28, 2013 at 02:12:49AM +0200, Andreas F=E4rber wrote: > >>>>> Am 25.07.2013 11:32, schrieb Michael S. Tsirkin: > >>>>>> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c > >>>>>> index 3908860..daefdfb 100644 > >>>>>> --- a/hw/pci-host/piix.c > >>>>>> +++ b/hw/pci-host/piix.c > >>>>>> @@ -349,6 +349,14 @@ PCIBus *i440fx_init(PCII440FXState **pi440f= x_state, int *piix3_devfn, > >>>>>> return b; > >>>>>> } > >>>>>> =20 > >>>>>> +PCIBus *find_i440fx(void) > >>>>>> +{ > >>>>>> + PCIHostState *s =3D OBJECT_CHECK(PCIHostState, > >>>>>> + object_resolve_path("/machin= e/i440fx", NULL), > >>>>>> + TYPE_PCI_HOST_BRIDGE); > >>>>> > >>>>> Open-coded OBJECT_CHECK() - should use existing PCI_HOST_BRIDGE(.= ..). > >>>>> > >>>>>> + return s ? s->bus : NULL; > >>>>>> +} > >>>>> > >>>>> Is this function really necessary? /machine/i440fx/pci.0 is a tri= vial > >>>>> addition to the path that's already being used here. You can do: > >>>>> PCIBus *bus =3D PCI_BUS(object_resolve_path("/machine/i440fx/pci.= 0")); > >>>>> where you actually need to access it. > >>>> > >>>> > >>>> I don't mind but I would like to avoid callers hard-coding > >>>> paths, in this case "i440fx". > >>>> Why the aversion to functions? > >>> > >>> Simply because QMP cannot call functions. It has to work with qom-l= ist > >>> and qom-get, so this is a test case showing what is missing and can= IMO > >>> easily be addressed for both parties. > >> > >> Fine but if the function calls QOM things internally > >> then where's the problem? > >=20 > > The grief with object_path_resolve_type() is that it's a "hack" Paolo > > has added for his convenience (in audio code?) that QMP cannot use, s= o > > we need name-based paths to be available. >=20 > Add to that, the way I see QOM devices (as opposed to qdev or pre-qdev > devices) is that they are instantiated from the outside - a different > source file or command line or config file - via QOM APIs, and they > allow other source files to interact with them via mydev_foo(MyDev *d, > ...) APIs that operate on an instance. >=20 > Having functions to create devices often hints at legacy code from > pre-qdev times (we had such a discussion with Alex when I tried to > qdev'ify the prep_pci device), indicating that either something is not > yet accessible via qdev/QOM APIs (e.g., IRQs) or that the device is not > yet implementing composition properly (e.g., creating a bus after the > bridge device rather than in the bridge, or a PCIDevice after the PHB > rather than by the PHB). >=20 > Having functions in the device's file to obtain a random instance of > that device in the form MyDev *mydev_get(void) is what I dislike here. Absolutely but what would you do? E.g. we can't handle more than one instance of PIIX ATM. > My personal preference (which you may ignore if others disagree) would > be to have accessors, where unavoidable, in the form: >=20 > foo mydev_get_bar(MyDev *s) > { > return s->baz; > } So how about implementing mydev_get_bar(void) and make that do the lookup internally using a path? Also mydev_present to test that. >=20 > which we can then later easily convert into dynamic property getters, > and it would hopefully spare us new per-device ...Info structs by > obtaining the info where and when you need it. > I admit it's a bit of boilerplate to code, but I've seen at most 4 case= s > per device where this would apply and I'm saying this with our > longer-term QOM goals in mind. >=20 > I'm skeptical towards Igor's suggestion of adding Last Minute 1.6 qdev > properties (as opposed to a composition property, you force me to becom= e > very explicit in my explanations...) as that would bring ABI stability > rules onto us. >=20 > Andreas >=20 > > And to clarify, I have been talking about two different time frames: > > Small adjustments that you can make for 1.6 (e.g., casts, q35 propert= y, > > different QOM function/callsite used; if Anthony is okay with taking > > ACPI at this late point) and post-1.6 cleanups to replace interim > > constructs that involve refactorings outside your control (e.g., > > MemoryRegion QOM'ification, adding properties to random devices). > >=20 > > Andreas > >=20 > >>> The suggested cast to PCI_BUS() lets you use regular qdev functions= btw > >>> as a shortcut, QMP users would need to iterate children of that nod= e. > >>> > >>> The suggested "pci.0" is considered stable for -device ...,bus=3Dpc= i.0 > >>> according to review feedback the Xen people have received for libxl. >=20 > --=20 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrn= berg