Re: [Xen-devel] [Qemu-devel] [PATCH for-3.2 v4 15/28] hw: apply accel compat properties without touching globals
On Wed, Nov 28, 2018 at 12:02:21AM +0400, Marc-André Lureau wrote: > Hi > > On Tue, Nov 27, 2018 at 11:40 PM Eduardo Habkost wrote: > > > > On Tue, Nov 27, 2018 at 01:27:48PM +0400, Marc-André Lureau wrote: > > > Introduce object_apply_global_props() function, to apply compatibility > > > properties from a GPtrArray. > > > > > > For accel compatibility properties, apply them during > > > device_post_init(), after accel_register_compat_props() has set them. > > > > > > To populate the compatibility properties, introduce SET_COMPAT(), a > > > more generic version of SET_MACHINE_COMPAT() that can set compat > > > properties on other objects than Machine, and using GPtrArray. > > > > > > Signed-off-by: Marc-André Lureau > > > --- > > > include/hw/qdev-core.h | 13 + > > > include/qom/object.h | 3 +++ > > > include/sysemu/accel.h | 4 +--- > > > accel/accel.c | 12 > > > hw/core/qdev.c | 11 +++ > > > hw/xen/xen-common.c| 38 +++--- > > > qom/object.c | 25 + > > > vl.c | 2 +- > > > 8 files changed, 73 insertions(+), 35 deletions(-) > > > > > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > > index a24d0dd566..82afd3c50d 100644 > > > --- a/include/hw/qdev-core.h > > > +++ b/include/hw/qdev-core.h > > > @@ -267,6 +267,19 @@ typedef struct GlobalProperty { > > > Error **errp; > > > } GlobalProperty; > > > > > > +#define SET_COMPAT(S, COMPAT) \ > > > +do {\ > > > +int i; \ > > > +static GlobalProperty props[] = { \ > > > +COMPAT \ > > > +}; \ > > > +for (i = 0; i < G_N_ELEMENTS(props); i++) { \ > > > +g_ptr_array_add((S)->compat_props, (void *)&props[i]); \ > > > +} \ > > > +} while (0) > > > > I think this macro would be an acceptable alternative to the > > existing SET_MACHINE_COMPAT macro trickery, but: > > > > > + > > > +void accel_register_compat_props(const GPtrArray *props); > > [...] > > > @@ -185,7 +183,9 @@ static void xen_accel_class_init(ObjectClass *oc, > > > void *data) > > > ac->init_machine = xen_init; > > > ac->setup_post = xen_setup_post; > > > ac->allowed = &xen_allowed; > > > -ac->global_props = xen_compat_props; > > > +ac->compat_props = g_ptr_array_new(); > > > + > > > +SET_COMPAT(ac, XEN_COMPAT); > > > > I think this is a step backwards. I like us to be able to > > register compat properties without macro magic. The existence of > > SET_MACHINE_COMPAT is a bug and not a feature. > > > > If you really want to use GPtrArray instead of a simple > > GlobalProperty* field (I'm not sure I understand the reasoning > > behind the choice to use GPtrArray), what about: > > Except in the Xen case, It needs to register multiple GlobalProperty*, > not necessarily from contiguous in memory. That's why we have an array > of ptr. If you also need to register multiple properties not from a contiguous array, would a simple wrapper that does a single g_ptr_array_add() call be enough? > > > > > static GPtrArray *build_compat_props_array(GlobalProperty *props) > > { > > GlobalProperty *p = props; > > GPtrArray *array = g_ptr_array_new(); > > while (p->driver) { > > g_ptr_array_add(array, (void *)p); > > } > > return array; > > } > > > > > > static void xen_accel_class_init(ObjectClass *oc, void *data) > > { > > ... > > ac->compat_props = build_compat_props_array(xen_compat_props); > > If we would register from one place, that would be fine. > > We could replace the macro by a function, then we would have to > declare the GlobalProperty arrays manually basically. I would prefer to replace the macro with a function, if possible. What do you mean by declaring the GlobalProperty arrays manually? > > > } > > > > > > > > > } > > > > > > #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen") > > > diff --git a/qom/object.c b/qom/object.c > > > index 17921c0a71..dbdab0aead 100644 > > > --- a/qom/object.c > > > +++ b/qom/object.c > > > @@ -370,6 +370,31 @@ static void object_post_init_with_type(Object *obj, > > > TypeImpl *ti) > > > } > > > } > > > > > > +void object_apply_global_props(Object *obj, const GPtrArray *props, > > > Error **errp) > > > +{ > > > +Error *err = NULL; > > > +int i; > > > + > > > +if (!props) { > > > +return; > > > +} > > > + > > > +for (i = 0; i < props->len; i++) { > > > +GlobalProperty *p = g_ptr_array_index(props, i); > > > + > > > +if (object_dynamic_cast(obj, p->driver) == NULL)
Re: [Xen-devel] [Qemu-devel] [PATCH for-3.2 v4 15/28] hw: apply accel compat properties without touching globals
Hi On Tue, Nov 27, 2018 at 11:40 PM Eduardo Habkost wrote: > > On Tue, Nov 27, 2018 at 01:27:48PM +0400, Marc-André Lureau wrote: > > Introduce object_apply_global_props() function, to apply compatibility > > properties from a GPtrArray. > > > > For accel compatibility properties, apply them during > > device_post_init(), after accel_register_compat_props() has set them. > > > > To populate the compatibility properties, introduce SET_COMPAT(), a > > more generic version of SET_MACHINE_COMPAT() that can set compat > > properties on other objects than Machine, and using GPtrArray. > > > > Signed-off-by: Marc-André Lureau > > --- > > include/hw/qdev-core.h | 13 + > > include/qom/object.h | 3 +++ > > include/sysemu/accel.h | 4 +--- > > accel/accel.c | 12 > > hw/core/qdev.c | 11 +++ > > hw/xen/xen-common.c| 38 +++--- > > qom/object.c | 25 + > > vl.c | 2 +- > > 8 files changed, 73 insertions(+), 35 deletions(-) > > > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > > index a24d0dd566..82afd3c50d 100644 > > --- a/include/hw/qdev-core.h > > +++ b/include/hw/qdev-core.h > > @@ -267,6 +267,19 @@ typedef struct GlobalProperty { > > Error **errp; > > } GlobalProperty; > > > > +#define SET_COMPAT(S, COMPAT) \ > > +do {\ > > +int i; \ > > +static GlobalProperty props[] = { \ > > +COMPAT \ > > +}; \ > > +for (i = 0; i < G_N_ELEMENTS(props); i++) { \ > > +g_ptr_array_add((S)->compat_props, (void *)&props[i]); \ > > +} \ > > +} while (0) > > I think this macro would be an acceptable alternative to the > existing SET_MACHINE_COMPAT macro trickery, but: > > > + > > +void accel_register_compat_props(const GPtrArray *props); > [...] > > @@ -185,7 +183,9 @@ static void xen_accel_class_init(ObjectClass *oc, void > > *data) > > ac->init_machine = xen_init; > > ac->setup_post = xen_setup_post; > > ac->allowed = &xen_allowed; > > -ac->global_props = xen_compat_props; > > +ac->compat_props = g_ptr_array_new(); > > + > > +SET_COMPAT(ac, XEN_COMPAT); > > I think this is a step backwards. I like us to be able to > register compat properties without macro magic. The existence of > SET_MACHINE_COMPAT is a bug and not a feature. > > If you really want to use GPtrArray instead of a simple > GlobalProperty* field (I'm not sure I understand the reasoning > behind the choice to use GPtrArray), what about: Except in the Xen case, It needs to register multiple GlobalProperty*, not necessarily from contiguous in memory. That's why we have an array of ptr. > > static GPtrArray *build_compat_props_array(GlobalProperty *props) > { > GlobalProperty *p = props; > GPtrArray *array = g_ptr_array_new(); > while (p->driver) { > g_ptr_array_add(array, (void *)p); > } > return array; > } > > > static void xen_accel_class_init(ObjectClass *oc, void *data) > { > ... > ac->compat_props = build_compat_props_array(xen_compat_props); If we would register from one place, that would be fine. We could replace the macro by a function, then we would have to declare the GlobalProperty arrays manually basically. > } > > > > > } > > > > #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen") > > diff --git a/qom/object.c b/qom/object.c > > index 17921c0a71..dbdab0aead 100644 > > --- a/qom/object.c > > +++ b/qom/object.c > > @@ -370,6 +370,31 @@ static void object_post_init_with_type(Object *obj, > > TypeImpl *ti) > > } > > } > > > > +void object_apply_global_props(Object *obj, const GPtrArray *props, Error > > **errp) > > +{ > > +Error *err = NULL; > > +int i; > > + > > +if (!props) { > > +return; > > +} > > + > > +for (i = 0; i < props->len; i++) { > > +GlobalProperty *p = g_ptr_array_index(props, i); > > + > > +if (object_dynamic_cast(obj, p->driver) == NULL) { > > +continue; > > +} > > +p->used = true; > > +object_property_parse(obj, p->value, p->property, &err); > > +if (err != NULL) { > > +error_prepend(&err, "can't apply global %s.%s=%s: ", > > + p->driver, p->property, p->value); > > +error_propagate(errp, err); > > +} > > +} > > +} > > + > > static void object_initialize_with_type(void *data, size_t size, TypeImpl > > *type) > > { > > Object *obj = data; > > diff --git a/vl.c b/vl.c > > index fa25d1ae2d..c06e94271c 100644 > > --- a/vl.c > > +
Re: [Xen-devel] [Qemu-devel] [PATCH for-3.2 v4 15/28] hw: apply accel compat properties without touching globals
On Tue, Nov 27, 2018 at 01:27:48PM +0400, Marc-André Lureau wrote: > Introduce object_apply_global_props() function, to apply compatibility > properties from a GPtrArray. > > For accel compatibility properties, apply them during > device_post_init(), after accel_register_compat_props() has set them. > > To populate the compatibility properties, introduce SET_COMPAT(), a > more generic version of SET_MACHINE_COMPAT() that can set compat > properties on other objects than Machine, and using GPtrArray. > > Signed-off-by: Marc-André Lureau > --- > include/hw/qdev-core.h | 13 + > include/qom/object.h | 3 +++ > include/sysemu/accel.h | 4 +--- > accel/accel.c | 12 > hw/core/qdev.c | 11 +++ > hw/xen/xen-common.c| 38 +++--- > qom/object.c | 25 + > vl.c | 2 +- > 8 files changed, 73 insertions(+), 35 deletions(-) > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index a24d0dd566..82afd3c50d 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -267,6 +267,19 @@ typedef struct GlobalProperty { > Error **errp; > } GlobalProperty; > > +#define SET_COMPAT(S, COMPAT) \ > +do {\ > +int i; \ > +static GlobalProperty props[] = { \ > +COMPAT \ > +}; \ > +for (i = 0; i < G_N_ELEMENTS(props); i++) { \ > +g_ptr_array_add((S)->compat_props, (void *)&props[i]); \ > +} \ > +} while (0) I think this macro would be an acceptable alternative to the existing SET_MACHINE_COMPAT macro trickery, but: > + > +void accel_register_compat_props(const GPtrArray *props); [...] > @@ -185,7 +183,9 @@ static void xen_accel_class_init(ObjectClass *oc, void > *data) > ac->init_machine = xen_init; > ac->setup_post = xen_setup_post; > ac->allowed = &xen_allowed; > -ac->global_props = xen_compat_props; > +ac->compat_props = g_ptr_array_new(); > + > +SET_COMPAT(ac, XEN_COMPAT); I think this is a step backwards. I like us to be able to register compat properties without macro magic. The existence of SET_MACHINE_COMPAT is a bug and not a feature. If you really want to use GPtrArray instead of a simple GlobalProperty* field (I'm not sure I understand the reasoning behind the choice to use GPtrArray), what about: static GPtrArray *build_compat_props_array(GlobalProperty *props) { GlobalProperty *p = props; GPtrArray *array = g_ptr_array_new(); while (p->driver) { g_ptr_array_add(array, (void *)p); } return array; } static void xen_accel_class_init(ObjectClass *oc, void *data) { ... ac->compat_props = build_compat_props_array(xen_compat_props); } > } > > #define TYPE_XEN_ACCEL ACCEL_CLASS_NAME("xen") > diff --git a/qom/object.c b/qom/object.c > index 17921c0a71..dbdab0aead 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -370,6 +370,31 @@ static void object_post_init_with_type(Object *obj, > TypeImpl *ti) > } > } > > +void object_apply_global_props(Object *obj, const GPtrArray *props, Error > **errp) > +{ > +Error *err = NULL; > +int i; > + > +if (!props) { > +return; > +} > + > +for (i = 0; i < props->len; i++) { > +GlobalProperty *p = g_ptr_array_index(props, i); > + > +if (object_dynamic_cast(obj, p->driver) == NULL) { > +continue; > +} > +p->used = true; > +object_property_parse(obj, p->value, p->property, &err); > +if (err != NULL) { > +error_prepend(&err, "can't apply global %s.%s=%s: ", > + p->driver, p->property, p->value); > +error_propagate(errp, err); > +} > +} > +} > + > static void object_initialize_with_type(void *data, size_t size, TypeImpl > *type) > { > Object *obj = data; > diff --git a/vl.c b/vl.c > index fa25d1ae2d..c06e94271c 100644 > --- a/vl.c > +++ b/vl.c > @@ -2963,7 +2963,7 @@ static void user_register_global_props(void) > */ > static void register_global_properties(MachineState *ms) > { > -accel_register_compat_props(ms->accelerator); > + > accel_register_compat_props(ACCEL_GET_CLASS(ms->accelerator)->compat_props); > machine_register_compat_props(ms); > user_register_global_props(); > } > -- > 2.20.0.rc1 > > -- Eduardo ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel