On 19/12/24 15:07, Ani Sinha wrote:
> On Thu, Dec 19, 2024 at 6:25 PM Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>>
>> Hi
>>
>> On Thu, Dec 19, 2024 at 2:03 PM Philippe Mathieu-Daudé
>> <philmd@linaro.org> wrote:
>>>>>> +static const TypeInfo vmfwupdate_device_info = {
>>>>>> + .name = TYPE_VMFWUPDATE,
>>>>>> + .parent = TYPE_DEVICE,
>>>>>
>>>>> What is the qdev API used here? Why not use a plain object?
>>>>
>>>> I wrote this taking vmcoreinfo device as starting point. I will leave this as is for now unless anyone has strong opinions.
>>>
>>> We shouldn't blindly copy/paste & spread possible design mistakes.
>>>
>>> Marc-André, any particular reason to implement vmcoreinfo using qdev
>>> and not plain object?
>>>
>>
>> I don't remember (damn 8y ago..). It seems the design changed over
>> time during review, qdev might have been necessary and stayed this
>> way.
>
> I changed it to TYPE_OBJECT and we get a crash here:
>
> #3 0x0000aaaaab207a48 [PAC] in object_class_dynamic_cast_assert
> (class=0xaaaaac608880, typename=typename@entry=0xaaaaab4b9630
> "device", file=file@entry=0xaaaaab4300d0
> "/workspace/qemu-ani/include/hw/qdev-core.h", line=line@entry=77,
> func=func@entry=0xaaaaab595a90 <__func__.0> "DEVICE_CLASS") at
> ../qom/object.c:1021
> #4 0x0000aaaaaaec2d74 in DEVICE_CLASS (klass=<optimized out>) at
> /workspace/qemu-ani/include/hw/qdev-core.h:77
> #5 vmcoreinfo_device_class_init (klass=<optimized out>,
> data="" out>) at ../hw/misc/vmcoreinfo.c:88
I believe you have enough knowledge to understand the concepts you
are mixing here. You can not change a type signature without
implementing its interface (which as you noticed, for QEMU is checked
at runtime).
Yes the point was to quickly try and see changing to DEVICE works. Turned out that more changes would be required and therefore I left it for the maintained of that device.
> Basically doing this would be illegal for vmcoreinfo and we need to
> adjust the code :
>
> DeviceClass *dc = DEVICE_CLASS(klass);
>
> dc->vmsd = &vmstate_vmcoreinfo;
> dc->realize = vmcoreinfo_realize;
> dc->hotpluggable = false;
> set_bit(DEVICE_CATEGORY_MISC, dc->categories);
See the conversion:
20241219153857.57450-1-philmd@linaro.org/" rel="noreferrer noreferrer" target="_blank">https://lore.kernel.org/qemu-devel/20241219153857.57450-1-philmd@linaro.org/
Yes I see you sent a patch and Dan's response. That was exactly also my opinion. Vmfwupdate, like vmcoreinfo is like a device not a generic object. So device type is more appropriate.
> Anyway, for vmfwupdate, it is actually like a device with device properties:
>
> + device_class_set_props(dc, vmfwupdate_properties);
>
> So I prefer to make it qdev type for now.
We have the opportunity to start with the correct model.
Consider simplifying our future (see what is required in
the suggested vmcoreinfo conversion). Except if you insist
and commit to do the vmfwupdate later.