(+Kevin / Markus / Mark for previous discussions)
On 23/2/26 18:01, Peter Maydell wrote:
> Currently we allow devices to define "link properties" with
> DEFINE_PROP_LINK(): these are a way to give a device a pointer to
> another QOM object. (Under the hood this is done by handing it the
> canonical QOM path for the object.)
>
> We also allow devices to define "array properties" with
> DEFINE_PROP_ARRAY(): these are a way to give a device a
> variable-length array of properties.
>
> However, there is no way to define an array of link properties. If
> you try to do it by passing qdev_prop_link as the arrayprop argument
> to DEFINE_PROP_ARRAY() you will get a crash because qdev_prop_link
> does not provide the .set and .get methods in its PropertyInfo
> struct.
>
> This patch implements a new DEFINE_PROP_LINK_ARRAY(). In
> a device you can use it like this:
>
> struct MyDevice {
> ...
> uint32_t num_cpus;
> ARMCPU **cpus;
> }
>
> and in your Property array:
> DEFINE_PROP_LINK_ARRAY("cpus", MyDevice, num_cpus, cpus,
> TYPE_ARM_CPU, ARMCPU *),
>
> The array property code will fill in s->num_cpus, allocate memory in
> s->cpus, and populate it with pointers.
>
> On the device-creation side you set the property in the same way as
> the existing array properties, using the new qlist_append_link()
> function to append to the QList:
>
> QList *cpulist = qlist_new();
> for (int i = 0; i < cpus; i++) {
> qlist_append_link(cpulist, OBJECT(cpu[i]));
> }
> qdev_prop_set_array(mydev, "cpus", cpulist);
>
> The implementation is mostly in the provision of the .set and
> .get methods to the qdev_prop_link PropertyInfo struct. The
> code of these methods parallels the code in
> object_set_link_property() and object_get_link_property(). We can't
> completely share the code with those functions because of differences
> in where we get the information like the target QOM type, but I have
> pulled out a new function object_resolve_and_typecheck() for the
> shared "given a QOM path and a type, give me the object or an error"
> code.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I want this specifically for the GICv5 interrupt controller device
> I'm working on. I'd like to be able to pass in links to the CPUs
> connected to the GIC, so that we don't have to have the code assume
> that it's connected to CPU numbers 0,1,2... the way the current
> GICv3 code does.
>
> I think Phil also had a proposed series a while back that wanted
> to do something similar with one of the existing devices.
FTR previous discussions:
https://lore.kernel.org/qemu-devel/47615329-9ae4-f9e0-117d-f7d4300bfdf1@ilande.co.uk/
https://lore.kernel.org/qemu-devel/CAFEAcA9FT+QMyQSLCeLjd7tEfaoS9JazmkYWQE++s1AmF7Nfvw@mail.gmail.com/
>
> I figured I'd post this early for review, though obviously it's a
> little unmotivated until I have my new GICv5 code into enough shape
> to submit. This is tested, but with my work-in-progress code.
> ---
> hw/core/qdev-properties.c | 78 +++++++++++++++++++++++++++++++
> include/hw/core/qdev-properties.h | 40 ++++++++++++++++
> include/qom/object.h | 19 ++++++++
> qom/object.c | 42 ++++++++++-------
> 4 files changed, 161 insertions(+), 18 deletions(-)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index ba8461e9a4..f8181b0d91 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -669,6 +669,7 @@ static Property array_elem_prop(Object *obj, const Property *parent_prop,
> * being inside the device struct.
> */
> .offset = (uintptr_t)elem - (uintptr_t)obj,
> + .link_type = parent_prop->link_type,
> };
> }
>
> @@ -950,6 +951,12 @@ void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values)
> qobject_unref(values);
> }
>
> +void qlist_append_link(QList *qlist, Object *obj)
> +{
> + g_autofree char *path = object_get_canonical_path(obj);
> + qlist_append_str(qlist, path);
> +}
> +
> static GPtrArray *global_props(void)
> {
> static GPtrArray *gp;
> @@ -1059,9 +1066,80 @@ static ObjectProperty *create_link_property(ObjectClass *oc, const char *name,
> OBJ_PROP_LINK_STRONG);
> }
>
> +/*
> + * The logic in these get_link() and set_link() functions is similar
> + * to that used for single-element link properties in the
> + * object_get_link_property() and object_set_link_property() functions.
> + * The difference is largely in how we get the expected type of the
> + * link: for us it is in the Property struct, and for a single link
> + * property it is part of the property name on the object.
> + */
> +static void get_link(Object *obj, Visitor *v, const char *name, void *opaque,
> + Error **errp)
> +{
> + const Property *prop = opaque;
> + Object **targetp = object_field_prop_ptr(obj, prop);
> + g_autofree char *path = NULL;
> +
> + if (*targetp) {
> + path = object_get_canonical_path(*targetp);
> + visit_type_str(v, name, &path, errp);
> + } else {
> + path = g_strdup("");
> + visit_type_str(v, name, &path, errp);
> + }
> +}
> +
> +static void set_link(Object *obj, Visitor *v, const char *name, void *opaque,
> + Error **errp)
> +{
> + const Property *prop = opaque;
> + Object **targetp = object_field_prop_ptr(obj, prop);
> + g_autofree char *path = NULL;
> + Object *new_target, *old_target = *targetp;
> +
> + ERRP_GUARD();
> +
> + /* Get the path to the object we want to set the link to */
> + if (!visit_type_str(v, name, &path, errp)) {
> + return;
> + }
> +
> + /* Now get the pointer to the actual object */
> + if (*path) {
> + new_target = object_resolve_and_typecheck(path, prop->name,
> + prop->link_type, errp);
> + if (!new_target) {
> + return;
> + }
> + } else {
> + new_target = NULL;
> + }
> +
> + /*
> + * Our link properties are always OBJ_PROP_LINK_STRONG and
> + * have the allow_set_link_before_realize check.
> + */
> + qdev_prop_allow_set_link_before_realize(obj, prop->name, new_target, errp);
> + if (*errp) {
> + return;
> + }
> +
> + *targetp = new_target;
> + object_ref(new_target);
> + object_unref(old_target);
> +}
> +
> const PropertyInfo qdev_prop_link = {
> .type = "link",
> .create = create_link_property,
> + /*
> + * Since we have a create method, the get and set are used
> + * only in get_prop_array() and set_prop_array() for the case
> + * where we have an array of link properties.
> + */
> + .get = get_link,
> + .set = set_link,
> };
>
> void qdev_property_add_static(DeviceState *dev, const Property *prop)
> diff --git a/include/hw/core/qdev-properties.h b/include/hw/core/qdev-properties.h
> index d8745d4c65..936d06023a 100644
> --- a/include/hw/core/qdev-properties.h
> +++ b/include/hw/core/qdev-properties.h
> @@ -168,6 +168,31 @@ extern const PropertyInfo qdev_prop_link;
> DEFINE_PROP(_name, _state, _field, qdev_prop_link, _ptr_type, \
> .link_type = _type)
>
> +/**
> + * DEFINE_PROP_LINK_ARRAY:
> + * @_name: name of the array
> + * @_state: name of the device state structure type
> + * @_field: uint32_t field in @_state to hold the array length
> + * @_arrayfield: field in @_state (of type '@_arraytype *') which
> + * will point to the array
> + * @_linktype: QOM type name of the link type
> + * @_arraytype: C type of the array elements
> + *
> + * Define device properties for a variable-length array _name of links
> + * (i.e. this is the array version of DEFINE_PROP_LINK).
> + * The array is represented as a list of QStrings in the visitor interface,
> + * where each string is the QOM path of the object to be linked.
> + */
> +#define DEFINE_PROP_LINK_ARRAY(_name, _state, _field, _arrayfield, \
> + _linktype, _arraytype) \
> + DEFINE_PROP(_name, _state, _field, qdev_prop_array, uint32_t, \
> + .set_default = true, \
> + .defval.u = 0, \
> + .arrayinfo = &qdev_prop_link, \
> + .arrayfieldsize = sizeof(_arraytype), \
> + .arrayoffset = offsetof(_state, _arrayfield), \
> + .link_type = _linktype)
> +
> #define DEFINE_PROP_UINT8(_n, _s, _f, _d) \
> DEFINE_PROP_UNSIGNED(_n, _s, _f, _d, qdev_prop_uint8, uint8_t)
> #define DEFINE_PROP_UINT16(_n, _s, _f, _d) \
> @@ -219,6 +244,21 @@ void qdev_prop_set_enum(DeviceState *dev, const char *name, int value);
> /* Takes ownership of @values */
> void qdev_prop_set_array(DeviceState *dev, const char *name, QList *values);
>
> +/**
> + * qlist_append_link: Add a QOM object to a QList of link properties
> + * @qlist: list to append to
> + * @obj: object to append
> + *
> + * This is a helper function for constructing a QList to pass to
> + * qdev_prop_set_array() when the qdev property array is an array of link
> + * properties (i.e. one defined with DEFINE_PROP_LINK_ARRAY).
> + *
> + * The object is encoded into the list as a QString which is the
> + * canonical path of the object; this is the same encoding that
> + * object_set_link_property() and object_get_link_property() use.
> + */
> +void qlist_append_link(QList *qlist, Object *obj);
> +
> void *object_field_prop_ptr(Object *obj, const Property *prop);
>
> void qdev_prop_register_global(GlobalProperty *prop);
> diff --git a/include/qom/object.h b/include/qom/object.h
> index 26df6137b9..48fdbf353e 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -1750,6 +1750,25 @@ ObjectProperty *object_class_property_add_link(ObjectClass *oc,
> Object *val, Error **errp),
> ObjectPropertyLinkFlags flags);
>
> +
> +/**
> + * object_resolve_and_typecheck:
> + * @path: path to look up
> + * @name: name of property we are resolving for (used only in error messages)
> + * @target_type: QOM type we expect @path to resolve to
> + * @errp: error
> + *
> + * Look up the object at @path and return it. If it does not have
> + * the correct type @target_type, return NULL and set @errp.
> + *
> + * This is similar to object_resolve_path_type(), but it insists on
> + * a non-ambiguous path and it produces error messages that are specialised
> + * to the use case of setting a link property on an object.
> + */
> +Object *object_resolve_and_typecheck(const char *path,
> + const char *name,
> + const char *target_type, Error **errp);
> +
> /**
> * object_property_add_str:
> * @obj: the object to add a property to
> diff --git a/qom/object.c b/qom/object.c
> index ff8ede8a32..90b8f3461e 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1895,26 +1895,13 @@ static void object_get_link_property(Object *obj, Visitor *v,
> }
> }
>
> -/*
> - * object_resolve_link:
> - *
> - * Lookup an object and ensure its type matches the link property type. This
> - * is similar to object_resolve_path() except type verification against the
> - * link property is performed.
> - *
> - * Returns: The matched object or NULL on path lookup failures.
> - */
> -static Object *object_resolve_link(Object *obj, const char *name,
> - const char *path, Error **errp)
> +Object *object_resolve_and_typecheck(const char *path,
> + const char *name,
> + const char *target_type, Error **errp)
> {
> - const char *type;
> - char *target_type;
> bool ambiguous = false;
> Object *target;
>
> - /* Go from link<FOO> to FOO. */
> - type = object_property_get_type(obj, name, NULL);
> - target_type = g_strndup(&type[5], strlen(type) - 6);
> target = object_resolve_path_type(path, target_type, &ambiguous);
>
> if (ambiguous) {
> @@ -1931,11 +1918,30 @@ static Object *object_resolve_link(Object *obj, const char *name,
> }
> target = NULL;
> }
> - g_free(target_type);
> -
> return target;
> }
>
> +/*
> + * object_resolve_link:
> + *
> + * Lookup an object and ensure its type matches the link property type. This
> + * is similar to object_resolve_path() except type verification against the
> + * link property is performed.
> + *
> + * Returns: The matched object or NULL on path lookup failures.
> + */
> +static Object *object_resolve_link(Object *obj, const char *name,
> + const char *path, Error **errp)
> +{
> + const char *type;
> + g_autofree char *target_type = NULL;
> +
> + /* Go from link<FOO> to FOO. */
> + type = object_property_get_type(obj, name, NULL);
> + target_type = g_strndup(&type[5], strlen(type) - 6);
> + return object_resolve_and_typecheck(path, name, target_type, errp);
> +}
> +
> static void object_set_link_property(Object *obj, Visitor *v,
> const char *name, void *opaque,
> Error **errp)
Nitpicking, I'd add object_resolve_and_typecheck() in a preliminary
patch. Otherwise patch LGTM!
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>