Re: [Qemu-devel] [PATCH v3 00/14] qemu: generate acpi tables for the guest

[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.

> Meanwhile, I'm adding APIs in particular so you can work on converting
> things to QOM without bothering with ACPI.  If you want to know what is
> missing, you only need to look at the patches themselves, once they are
> merged you can rework them internally without need to touch acpi code.

I do intend to look at them but did not have time yesterday.

I brought up my comments early because Hard Freeze is approaching fast
and I fear that if your v3 series is applied to 1.6 and is working, you
will not bother to send follow-up fixes after 1.6 anymore because you do
not care about whether that code is considered bad design by others, and
thus things will likely stay in a bad shape. You cannot expect me to do
everything (not to mention that it was Anthony's idea in the first place).

> As for your suggestion to poke at internal structures in a central place -

I never suggested that, on the contrary. See my example above using
Object: There is no need to even expose structs to ACPI to access things
via QOM properties.

Regards,
Andreas

> generally, I don't see why poking at internal field or properties of all
> kind of device structures or hard-coding paths in acpi-build is a good
> idea, surely accessing structures through APIs is the basic idea of data
> hiding.
> 
> Using QOM to find devices rather than passing pointers around makes
> sense to me since this removes the need to depend on initialization
> order.

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