On Mon, Jun 19, 2017 at 08:49:37PM +0800, Peter Xu wrote:
> Originally it can only alloc new entries and insert. Let it be smarter
> that it can update existing fields, and even delete elements. This is
> preparation of (finally) the replacement of x86_cpu_apply_props(). If
> you see x86_cpu_apply_props(), it allows to skip entries when value is
> NULL. Here, it works just like deleting an existing entry.
>
> Signed-off-by: Peter Xu <pet...@redhat.com>
This makes the global property API much more complex, making the
interactions between different callers much more subtle.
Currently, the global property list was always an append-only
list, and we had only two possible sources of global properties:
1) MachineClass::compat_props;
2) User-provided globals (including -global and other
command-line options that are simply translated to globals).
So it is easy to calculate the results of a given QEMU
configuration: just append both lists.
With this patch, the rules become more complex, and calculating
the actual results of multiple register_compat_prop() calls is
not trivial. Tracking the actual register_compat_prop() calls
scattered around the code (and figuring out the order in which
they are called) makes it even more difficult. IMO, this defeats
the purpose of introducing a AccelClass::global_props field.
I believe the main source of this complexity are the
x86_cpu_change_kvm_default() calls in pc_piix.c, and the rules
those calls represent are really more complex. But I would like
to find a simpler solution to represent those rules inside either
MachineClass::compat_props or AccelClass::global_props. If we
can't do that, I think we should keep that complexity inside the
PC/x86 code instead of exposing it to all users of the global
properties API.
I suggest we do this, to keep things simple in the first version:
1) Introduce AccelClass::global_props without this patch.
2) Move: kvmclock, kvm-nopiodelay, kvm-asyncpf, kvm-steal-time,
kvmclock-stable-bit, acpi, monitor to AccelClass::global_props
in kvm-accel.
3) Move: vme=off to AccelClass::global_props in tcg-accel.
4) Keep svm, x2apic and kvm-pv-eoi inside kvm_default_props in
x86 code, because their rules are more complex.
5) Look for a simpler way to represent the rules from (4) later.
> ---
> hw/core/qdev-properties.c | 28 +++++++++++++++++++++++++++-
> include/hw/qdev-properties.h | 10 ++++++++++
> 2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index 4b74382..dc3b0ac 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -1045,7 +1045,33 @@ static GList *global_props;
> GList *global_prop_list_add(GList *list, const char *driver,
> const char *property, const char *value)
> {
> - GlobalProperty *p = g_new0(GlobalProperty, 1);
> + GList *l;
> + GlobalProperty *p;
> +
> + /* Look up the (property, value) first in the list */
> + for (l = list; l; l = l->next) {
> + p = l->data;
> + if (!strcmp(driver, p->driver) && !strcmp(property, p->property)) {
> + if (value) {
> + /* Modify existing value */
> + p->value = value;
> + } else {
> + /* Delete this entry entirely */
> + list = g_list_remove_link(list, l);
> + g_free(p);
> + g_list_free(l);
> + }
> + return list;
> + }
> + }
> +
> + /* Entry not exist. If we are deleting one entry, we're done. */
> + if (!value) {
> + return list;
> + }
> +
> + /* If not found and value is set, we try to create a new entry */
> + p = g_new0(GlobalProperty, 1);
>
> /* These properties cannot fail the apply */
> p->errp = &error_abort;
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 15ee6ba..55ad507 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -201,6 +201,16 @@ void qdev_prop_set_globals(DeviceState *dev);
> void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
> Property *prop, const char *value);
>
> +/*
> + * Register global property on the list. Elements of list should be
> + * GlobalProperty.
> + *
> + * - If (driver, property) is not existing on the list, create a new
> + * one and link to the list.
> + * - If (driver, property) exists on the list, then:
> + * - if value != NULL, update with new value
> + * - if value == NULL, delete the entry
> + */
> GList *global_prop_list_add(GList *list, const char *driver,
> const char *property, const char *value);
> void register_compat_prop(const char *driver, const char *property,
> --
> 2.7.4
>
--
Eduardo
