Alex Bennée <alex.ben...@linaro.org> writes:
> Peter Maydell <peter.mayd...@linaro.org> writes: > >> On Mon, 27 Mar 2023 at 23:10, Alex Bennée <alex.ben...@linaro.org> wrote: >>> >>> >>> Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> writes: >>> >>> > On 27/03/2023 17:12, Alex Bennée wrote: >>> > >>> >> Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> writes: >>> >> >>> >>> On 27/03/2023 14:15, Alex Bennée wrote: >>> >>> >>> >>>> I'm still not sure how I achieve by use case of the parent class >>> >>>> defining the following properties: >>> >>>> static Property vud_properties[] = { >>> >>>> DEFINE_PROP_CHR("chardev", VHostUserDevice, chardev), >>> >>>> DEFINE_PROP_UINT16("id", VHostUserDevice, id, 0), >>> >>>> DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), >>> >>>> DEFINE_PROP_END_OF_LIST(), >>> >>>> }; >>> >>>> But for the specialisation of the class I want the id to default to >>> >>>> the actual device id, e.g.: >>> >>>> static Property vu_rng_properties[] = { >>> >>>> DEFINE_PROP_UINT16("id", VHostUserDevice, id, VIRTIO_ID_RNG), >>> >>>> DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1), >>> >>>> DEFINE_PROP_END_OF_LIST(), >>> >>>> }; >>> >>>> And so far the API for doing that isn't super clear. >>> >>>> Signed-off-by: Alex Bennée <alex.ben...@linaro.org> >>> >>>> --- >>> >>>> include/hw/qdev-core.h | 9 +++++++++ >>> >>>> 1 file changed, 9 insertions(+) >>> >>>> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h >>> >>>> index bd50ad5ee1..d4bbc30c92 100644 >>> >>>> --- a/include/hw/qdev-core.h >>> >>>> +++ b/include/hw/qdev-core.h >>> >>>> @@ -776,6 +776,15 @@ BusState *sysbus_get_default(void); >>> >>>> char *qdev_get_fw_dev_path(DeviceState *dev); >>> >>>> char *qdev_get_own_fw_dev_path_from_handler(BusState *bus, >>> >>>> DeviceState *dev); >>> >>>> +/** >>> >>>> + * device_class_set_props(): add a set of properties to an device >>> >>>> + * @dc: the parent DeviceClass all devices inherit >>> >>>> + * @props: an array of properties, terminate by >>> >>>> DEFINE_PROP_END_OF_LIST() >>> >>>> + * >>> >>>> + * This will add a set of properties to the object. It will fault if >>> >>>> + * you attempt to add an existing property defined by a parent class. >>> >>>> + * To modify an inherited property you need to use???? >>> >>>> + */ >>> >>>> void device_class_set_props(DeviceClass *dc, Property *props); >>> >>>> /** >>> >>> >>> >>> Hmmm that's an interesting one. Looking at the source in >>> >>> hw/core/qdev-properties.c you could possibly get away with something >>> >>> like this in vu_rng_class_init(): >>> >>> >>> >>> ObjectProperty *op = object_class_property_find(klass, "id"); >>> >>> object_property_set_default_uint(op, VIRTIO_ID_RNG); >>> >>> >>> >>> Of course this is all completely untested :) >>> >> Sadly we assert on the existing prop->defval: >>> >> static void object_property_set_default(ObjectProperty *prop, >>> >> QObject *defval) >>> >> { >>> >> assert(!prop->defval); >>> >> assert(!prop->init); >>> >> prop->defval = defval; >>> >> prop->init = object_property_init_defval; >>> >> } >>> >> Maybe the assert is too aggressive or we need a different helper, >>> >> maybe >>> >> a: >>> >> void object_property_update_default_uint(ObjectProperty *prop, >>> >> uint64_t value) >>> >> ? >>> > >>> > It seems in that case once the default has been set, it is impossible >>> > to change. The only other immediate option I can think of is to define >>> > a specific DEFINE_VHOST_PROPERTIES macro in a similar way to >>> > DEFINE_AUDIO_PROPERTIES which you can use to set the common properties >>> > for all VHostUserDevice devices, including providing the default ID. >>> >>> I tried this: allow the default to change >>> >>> modified qom/object.c >>> @@ -1557,11 +1557,16 @@ static void object_property_init_defval(Object >>> *obj, ObjectProperty *prop) >>> >>> static void object_property_set_default(ObjectProperty *prop, QObject >>> *defval) >>> { >>> - assert(!prop->defval); >>> - assert(!prop->init); >>> + if (prop->init == object_property_init_defval) { >>> + fprintf(stderr, "%s: updating existing defval\n", __func__); >>> + prop->defval = defval; >>> + } else { >>> + assert(!prop->defval); >>> + assert(!prop->init); >>> >>> - prop->defval = defval; >>> - prop->init = object_property_init_defval; >>> + prop->defval = defval; >>> + prop->init = object_property_init_defval; >>> + } >>> } >> >> I think this leaves the door open to bugs where you create >> the property, somebody looks at it, and then you update >> the default value afterwards... > > Really the pattern I have is: > > vhost-user-device has the property and is configurable > vhost-user-rng-device specialises vhost-user-device and fixes the value > > I'm not sure how best to represent that. This should all be happening at > class_init time. Of course enabling my second derived class I ran into: ERROR:../../qom/object.c:1590:object_property_fix_default: assertion failed: (!prop->fixed) Bail out! ERROR:../../qom/object.c:1590:object_property_fix_default: assertion failed: (!prop->fixed) [New Thread 2088694.2088695] Thread 1 received signal SIGABRT, Aborted. __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 50 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory. (rr) bt #0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50 #1 0x00007f50194bd537 in __GI_abort () at abort.c:79 #2 0x00007f501b03fdcc in g_assertion_message (domain=<optimized out>, file=0x557b85d1ef1b "../../qom/object.c", line=<optimized out>, func=<optimized out>, message=<optimized out>) at ../../../glib/gtestutils.c:2937 #3 0x00007f501b09d2fb in g_assertion_message_expr (domain=0x0, file=0x557b85d1ef1b "../../qom/object.c", line=1590, func=0x557b85d1f8b0 <__func__.10> "object_property_fix_default", expr=<optimized out>) at ../../../glib/gtestutils.c:2963 #4 0x0000557b8589260e in object_property_fix_default (prop=0x557b88402f30, defval=0x557b885bda80) at ../../qom/object.c:1590 #5 0x0000557b8589266e in object_property_fix_default_uint (prop=0x557b88402f30, value=41) at ../../qom/object.c:1598 #6 0x0000557b857ccc28 in vu_gpio_class_init (klass=0x557b885bd360, data=0x0) at ../../hw/virtio/vhost-user-gpio.c:32 #7 0x0000557b8588fad5 in type_initialize (ti=0x557b883aec50) at ../../qom/object.c:366 #8 0x0000557b85891038 in object_class_foreach_tramp (key=0x557b883aedd0, value=0x557b883aec50, opaque=0x7ffe065f4290) at ../../qom/object.c:1081 #9 0x00007f501b062fa0 in g_hash_table_foreach (hash_table=0x557b882a7240 = {...}, func=0x557b85891008 <object_class_foreach_tramp>, user_data=0x7ffe065f4290) at ../../../glib/ghash.c:2067 #10 0x0000557b85891117 in object_class_foreach (fn=0x557b85891274 <object_class_get_list_tramp>, implements_type=0x557b85c579f5 "machine", include_abstract=false, opaque=0x7ffe065f42e0) at ../../qom/object.c:1103 #11 0x0000557b858912ef in object_class_get_list (implements_type=0x557b85c579f5 "machine", include_abstract=false) at ../../qom/object.c:1160 #12 0x0000557b85395077 in select_machine (qdict=0x557b883c0e80, errp=0x557b86657e40 <error_fatal>) at ../../softmmu/vl.c:1580 #13 0x0000557b85396068 in qemu_create_machine (qdict=0x557b883c0e80) at ../../softmmu/vl.c:2013 #14 0x0000557b85399c5d in qemu_init (argc=33, argv=0x7ffe065f4628) at ../../softmmu/vl.c:3544 #15 0x0000557b84fdda9f in main (argc=33, argv=0x7ffe065f4628) at ../../softmmu/main.c:47 (rr) f 4 #4 0x0000557b8589260e in object_property_fix_default (prop=0x557b88402f30, defval=0x557b885bda80) at ../../qom/object.c:1590 1590 g_assert(!prop->fixed); (rr) p &prop->fixed $1 = (_Bool *) 0x557b88402f80 (rr) watch *(_Bool *) 0x557b88402f80 Hardware watchpoint 1: *(_Bool *) 0x557b88402f80 (rr) rc Continuing. Thread 1 hit Hardware watchpoint 1: *(_Bool *) 0x557b88402f80 Old value = true New value = false 0x0000557b8589261e in object_property_fix_default (prop=0x557b88402f30, defval=0x557b88403780) at ../../qom/object.c:1593 1593 prop->fixed = true; (rr) bt #0 0x0000557b8589261e in object_property_fix_default (prop=0x557b88402f30, defval=0x557b88403780) at ../../qom/object.c:1593 #1 0x0000557b8589266e in object_property_fix_default_uint (prop=0x557b88402f30, value=4) at ../../qom/object.c:1598 #2 0x0000557b857ccad5 in vu_rng_class_init (klass=0x557b884015c0, data=0x0) at ../../hw/virtio/vhost-user-rng.c:33 #3 0x0000557b8588fad5 in type_initialize (ti=0x557b883aea90) at ../../qom/object.c:366 #4 0x0000557b85891038 in object_class_foreach_tramp (key=0x557b883aec10, value=0x557b883aea90, opaque=0x7ffe065f4290) at ../../qom/object.c:1081 #5 0x00007f501b062fa0 in g_hash_table_foreach (hash_table=0x557b882a7240 = {...}, func=0x557b85891008 <object_class_foreach_tramp>, user_data=0x7ffe065f4290) at ../../../glib/ghash.c:2067 #6 0x0000557b85891117 in object_class_foreach (fn=0x557b85891274 <object_class_get_list_tramp>, implements_type=0x557b85c579f5 "machine", include_abstract=false, opaque=0x7ffe065f42e0) at ../../qom/object.c:1103 #7 0x0000557b858912ef in object_class_get_list (implements_type=0x557b85c579f5 "machine", include_abstract=false) at ../../qom/object.c:1160 #8 0x0000557b85395077 in select_machine (qdict=0x557b883c0e80, errp=0x557b86657e40 <error_fatal>) at ../../softmmu/vl.c:1580 #9 0x0000557b85396068 in qemu_create_machine (qdict=0x557b883c0e80) at ../../softmmu/vl.c:2013 #10 0x0000557b85399c5d in qemu_init (argc=33, argv=0x7ffe065f4628) at ../../softmmu/vl.c:3544 #11 0x0000557b84fdda9f in main (argc=33, argv=0x7ffe065f4628) at ../../softmmu/main.c:47 So I guess duplicating the options as per Mark is going to be the next approach to try although it doesn't quite achieve what I want. -- Alex Bennée Virtualisation Tech Lead @ Linaro