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

[Andreas Färber]

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.

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;
}

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