Re: [Qemu-devel] [PATCH v3 11/14] piix: APIs for pc guest info

[Michael S. Tsirkin]

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