qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Eric Blake <eblake@redhat.com>
To: "Andreas Färber" <afaerber@suse.de>
Cc: Eduardo Habkost <ehabkost@redhat.com>,
	libvir-list@redhat.com, Markus Armbruster <armbru@redhat.com>,
	qemu-devel@nongnu.org, Luiz Capitulino <lcapitulino@redhat.com>,
	Anthony Liguori <anthony@codemonkey.ws>,
	Igor Mammedov <imammedo@redhat.com>,
	Jiri Denemark <jdenemar@redhat.com>
Subject: Re: [Qemu-devel] [PATCH qom-cpu 0/9] x86: feature words array (v11) + "feature-words" property
Date: Fri, 03 May 2013 09:31:43 -0600	[thread overview]
Message-ID: <5183D85F.9080905@redhat.com> (raw)
In-Reply-To: <5183D0A4.7000200@suse.de>

[-- Attachment #1: Type: text/plain, Size: 3854 bytes --]

On 05/03/2013 08:58 AM, Andreas Färber wrote:
> 
>> I agree that libvirt would very much like to have this in 1.5.  How
>> can I help in reviewing things?
> 
> Apart from the usual QMP considerations that you will know much better
> than me, I have two concerns here:
> 1) Polluting the QOM namespace with this dump-all implementation for
> libvirt and interfering with more fine-grained property getters/setters.

Ultimately, it would be nice to have full QOM introspection.  We are
part way there: we know WHAT properties we can query ('qom-list' exists
now), but right now it returns details on the type of a property as a
string, rather than as a full-blown JSON representation of the full
details of that type.  Maybe if we had an additional QMP command that
gave back a full JSON representation of any type, so you can feed the
'type':'str' field from ObjectPropertyInfo in 'qom-list' into the new
command to get a full idea of what each property contains.  With full
introspection, we could then have the option of changing the layout of
the feature-words and filtered-features properties, but if we do make a
change in the future, it would be nice to remain backwards compatible
(adding features, rather than removing).

> 2) Basing its design on current code of which we are not sure yet how
> it may evolve and having to live with that for ABI stability.
> Like I said, I hadn't reviewed that part yet, so couldn't pick it up
> on short notice. If we get it respun and reviewed today, I can (try
> to) prepare a PULL on Sunday.

I guess I see where you are coming from - once we bake in the QOM
property name and libvirt starts relying on it, it becomes harder to
support the generation of that property in the future if the underlying
implementation of how feature bits are represented in qemu changed.  But
the hope is that we have already sanitized the feature word generation
into something that is a lot more maintainable (iterating over a struct
of descriptions of how to get at each feature), and where future changes
would merely be extending (not deleting) from that struct (and therefore
making the corresponding JSON type returned via the QOM property
larger).  And since these two properties are read-only, extending the
JSON struct shouldn't be a problem.

> 
> On Igor's series (latest: v7 from Feb 25) I had more or less nack'ed
> the attempt to introduce f-* properties due to Anthony asking for
> verbose QOM property names, so we're in need of a better name, likely
> something with "feature" in it, similar to what is being proposed here.
> I had also argued with Anthony that QOM's object_property_add_bool()
> should allow us to create a container object for accessing features in
> a more simple way, such as .../icc/child[0]/cpuid-features/foo rather
> than f-foo or feature-foo or foo-feature to avoid the constant
> repetition and an unreadable long list of CPU properties, but the
> addition of an opaque to support this was turned down.

The problem with adding container objects is that you can only access
one feature at a time, instead of all of them in a single array.  I
guess libvirt would cope with either style, but it is nicer to be able
to get everything at once via JSON struct than it is to have to make
multiple qom-get calls.

> 
> So it boils down to the questions of where do we want to expose which
> information, how should it be structured and where does/will that
> information come from. Thanks.

I guess that decision is up to the qemu maintainers - all I can do is
say whether the proposed interface is usable, not whether it is the
ideal interface compared to some other layout of where the property is
attached.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

      parent reply	other threads:[~2013-05-03 15:31 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-04-22 19:00 [Qemu-devel] [PATCH qom-cpu 0/9] x86: feature words array (v11) + "feature-words" property Eduardo Habkost
2013-04-22 19:00 ` [Qemu-devel] [PATCH qom-cpu 1/9] target-i386: cleanup: Group together level, xlevel, xlevel2 fields Eduardo Habkost
2013-04-22 19:00 ` [Qemu-devel] [PATCH qom-cpu 2/9] target-i386/kvm.c: Code formatting changes Eduardo Habkost
2013-05-01 22:55   ` Andreas Färber
2013-04-22 19:00 ` [Qemu-devel] [PATCH qom-cpu 3/9] target-i386/cpu.c: Break lines so they don't get too long Eduardo Habkost
2013-04-22 19:00 ` [Qemu-devel] [PATCH qom-cpu 4/9] target-i386: Replace cpuid_*features fields with a feature word array Eduardo Habkost
2013-05-01 23:03   ` Andreas Färber
2013-05-02 15:06     ` Eduardo Habkost
2013-04-22 19:00 ` [Qemu-devel] [PATCH qom-cpu 5/9] target-i386: Add ECX information to FeatureWordInfo Eduardo Habkost
2013-05-03 15:16   ` Andreas Färber
2013-05-03 15:54     ` Eduardo Habkost
2013-05-06 16:27       ` Andreas Färber
2013-04-22 19:00 ` [Qemu-devel] [PATCH qom-cpu 6/9] target-i386: Add "feature-words" property Eduardo Habkost
2013-04-22 20:37   ` [Qemu-devel] [libvirt] " Eric Blake
2013-04-23 19:25     ` Eduardo Habkost
2013-05-03 11:34   ` [Qemu-devel] " Igor Mammedov
2013-05-03 13:17     ` Eduardo Habkost
2013-05-03 14:25       ` Eric Blake
2013-05-03 14:57   ` Eric Blake
2013-04-22 19:00 ` [Qemu-devel] [PATCH qom-cpu 7/9] target-i386: Use FeatureWord loop on filter_features_for_kvm() Eduardo Habkost
2013-05-03 15:01   ` Eric Blake
2013-05-06 16:28     ` Andreas Färber
2013-04-22 19:00 ` [Qemu-devel] [PATCH qom-cpu 8/9] target-i386: Introduce X86CPU.filtered_features field Eduardo Habkost
2013-05-03 15:03   ` Eric Blake
2013-04-22 19:00 ` [Qemu-devel] [PATCH qom-cpu 9/9] target-i386: Add "filtered-features" property to X86CPU Eduardo Habkost
2013-05-03 15:10   ` Eric Blake
2013-05-01 22:53 ` [Qemu-devel] [PATCH qom-cpu 0/9] x86: feature words array (v11) + "feature-words" property Andreas Färber
2013-05-02 19:43   ` Eduardo Habkost
2013-05-02 19:48     ` Eric Blake
2013-05-03 14:58       ` Andreas Färber
2013-05-03 15:23         ` Igor Mammedov
2013-05-03 15:31         ` Eric Blake [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5183D85F.9080905@redhat.com \
    --to=eblake@redhat.com \
    --cc=afaerber@suse.de \
    --cc=anthony@codemonkey.ws \
    --cc=armbru@redhat.com \
    --cc=ehabkost@redhat.com \
    --cc=imammedo@redhat.com \
    --cc=jdenemar@redhat.com \
    --cc=lcapitulino@redhat.com \
    --cc=libvir-list@redhat.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).