[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info |
Date: |
Sun, 28 Jul 2013 15:19:04 +0300 |
On Sun, Jul 28, 2013 at 01:08:13PM +0200, Andreas Färber wrote:
> Am 28.07.2013 12:31, schrieb Andreas Färber:
> > Am 28.07.2013 12:14, schrieb Michael S. Tsirkin:
> >> On Sun, Jul 28, 2013 at 11:38:17AM +0200, Andreas Färber wrote:
> >>> Am 28.07.2013 09:30, schrieb Michael S. Tsirkin:
> >>>> On Sun, Jul 28, 2013 at 02:12:49AM +0200, Andreas Färber 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
> >>>>>> **pi440fx_state, int *piix3_devfn,
> >>>>>> return b;
> >>>>>> }
> >>>>>>
> >>>>>> +PCIBus *find_i440fx(void)
> >>>>>> +{
> >>>>>> + PCIHostState *s = OBJECT_CHECK(PCIHostState,
> >>>>>> +
> >>>>>> object_resolve_path("/machine/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 trivial
> >>>>> addition to the path that's already being used here. You can do:
> >>>>> PCIBus *bus = 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-list
> >>> 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?
> >
> > 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, so
> > we need name-based paths to be available.
>
> 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.
>
> 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).
>
> 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:
>
> 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.
>
> 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 cases
> per device where this would apply and I'm saying this with our
> longer-term QOM goals in mind.
>
> 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 become
> very explicit in my explanations...) as that would bring ABI stability
> rules onto us.
>
> Andreas
>
> > 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 property,
> > 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).
> >
> > Andreas
> >
> >>> 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 node.
> >>>
> >>> The suggested "pci.0" is considered stable for -device ...,bus=pci.0
> >>> according to review feedback the Xen people have received for libxl.
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
- Re: [Qemu-devel] [PATCH v3 13/14] hpet: add API to find it, (continued)
- [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Michael S. Tsirkin, 2013/07/24
- Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Michael S. Tsirkin, 2013/07/25
- Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Andreas Färber, 2013/07/27
- Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Michael S. Tsirkin, 2013/07/28
- Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Andreas Färber, 2013/07/28
- Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Michael S. Tsirkin, 2013/07/28
- Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Andreas Färber, 2013/07/28
- Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Andreas Färber, 2013/07/28
- Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info,
Michael S. Tsirkin <=
Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info, Gerd Hoffmann, 2013/07/25
[Qemu-devel] [PATCH v3 14/14] i386: ACPI table generation code from seabios, Michael S. Tsirkin, 2013/07/24
- Re: [Qemu-devel] [PATCH v3 14/14] i386: ACPI table generation code from seabios, Gerd Hoffmann, 2013/07/25
- Re: [Qemu-devel] [PATCH v3 14/14] i386: ACPI table generation code from seabios, Michael S. Tsirkin, 2013/07/25
- Re: [Qemu-devel] [PATCH v3 14/14] i386: ACPI table generation code from seabios, Gerd Hoffmann, 2013/07/25
- Re: [Qemu-devel] [PATCH v3 14/14] i386: ACPI table generation code from seabios, Michael S. Tsirkin, 2013/07/25
- Re: [Qemu-devel] [PATCH v3 14/14] i386: ACPI table generation code from seabios, Gerd Hoffmann, 2013/07/26
- Re: [Qemu-devel] [PATCH v3 14/14] i386: ACPI table generation code from seabios, Gerd Hoffmann, 2013/07/26
- Re: [Qemu-devel] [PATCH v3 14/14] i386: ACPI table generation code from seabios, Michael S. Tsirkin, 2013/07/28
[Qemu-devel] [PATCH v3 02/14] i386: add ACPI table files from seabios, Michael S. Tsirkin, 2013/07/24