Am 26.07.2013 14:19, schrieb Andreas Färber:
> Am 25.07.2013 18:19, schrieb Michael S. Tsirkin:
>> On Thu, Jul 25, 2013 at 05:50:55PM +0200, Andreas Färber wrote:
>>> Am 24.07.2013 18:01, schrieb Michael S. Tsirkin:
>>>> This code can also be found here:
>>>> git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git acpi
>>>>
>>>> Please review, and consider for 1.6.
>>>
>>> Quite frankly, this is still not looking the way I imagined it based on
>>> the KVM call discussion and Anthony's comments that I remember:
>>>
>>> I believe Anthony asked to extract the information from the QOM tree,
>>> originally from the SeaBIOS side, then later agreeing to do it on the
>>> QEMU side.
>>>
>>> However here I am still seeing *functions* added in device code to check
>>> device existence and to extract individual fields. I was assuming (and
>>> clearly prefer) such code to live in a central place, be it acpi-build.c
>>> or something else, and to use QOM *API*s to obtain information when
>>> needed rather than building up lots of new structs duplicating that
>>> data. That would at the same time be a test case for how useful the QOM
>>> tree is
>>>
>>> I'm not sure if there was a misunderstanding or whether the PC QOM model
>>> still sucks^W is incomplete? Anthony and Ping Fang(?) had both posted
>>> patches to improve the composition tree once. If there's properties
>>> missing that you need to access for ACPI, we should simply add them.
>>> For i440fx we have /machine/i440fx.
>>> For q35 I encountered an mch child on q35-pcihost, but what's trivially
>>> missing apparently is to add q35-pcihost as a child to /machine, e.g.
>>> /machine/q35.
>>> Then you'll end up doing
>>> Object *obj = object_resolve_path_component(qdev_get_machine(), "q35/mch");
>>> object_property_get_int(obj, "foo", &err);
>>> object_property_get_string(obj, "bar", &err);
>>> and so on. No need to do the TYPE_... based search for everything.
>>>
>>> User-added -devices will show up in /machine/peripheral or
>>> /machine/peripheral-anon depending on whether id= is used, so there a
>>> type-based search probably makes sense. And there is nothing wrong with
>>> moving the TYPE_* constants to a device header where not yet the case,
>>> to allow that from generic code.
>>>
>>> Similarly, please don't open-code OBJECT_CHECK()s, do a trivial patch
>>> with a macro that we can then reuse elsewhere. I'd be happy to review
>>> such QOM patches and help fast-track them into master.
>>>
>>> Will take a closer look at the implementation later.
>>
>> This is not my understanding of previous comments on list
>> or on KVM call.
>>
>> Basically it sounds like you want to make my work depend on completion
>> of QOM conversion.
>> I think we explicitly agreed full QOM convertion is not a blocker.
> 
> Not sure what you mean with "completion of QOM conversion" or "full QOM
> conversion". What I am saying is that instead of spending time adding
> functions to devices that fulfill your own ACPI needs only, that time
> were better spent adding QOM properties where not yet existent.
> 
> Because then what you can access for ACPI can also be accessed by
> libvirt and other management tools as well as qtest - I consider it a
> test case. QMP does not offer an instance/path search by type.

To clarify for everyone what we're talking about here, I'm attaching
/machine composition tree dumps for pc,accel=kvm and q35,accel=kvm plus
the rudimentary script I used to generate it.

It shows for instance the mentioned /machine/i440fx and lack of
/machine/q35. It also shows that there would be a /machine/fw_cfg.

Paths starting with /machine/unassigned shouldn't be hardcoded anywhere
(that's the nobody-added-it-as-a-child<> bucket), except maybe for
/machine/unassigned/sysbus. But whenever there's a link from a named
device to a /machine/unassigned/device[n] that may of course be used
dynamically, e.g. /machine/icc-bridge/icc to discover CPUs and APICs.

HTH,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg