On Thu, 19 Dec, 2024, 9:16 pm Philippe Mathieu-Daudé, <phi...@linaro.org>
wrote:
> On 19/12/24 15:07, Ani Sinha wrote:
> > On Thu, Dec 19, 2024 at 6:25 PM Marc-André Lureau
> > <marcandre.lur...@redhat.com> wrote:
> >>
> >> Hi
> >>
> >> On Thu, Dec 19, 2024 at 2:03 PM Philippe Mathieu-Daudé
> >> <phi...@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=<optimized 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:
>
> https://lore.kernel.org/qemu-devel/20241219153857.57450-1-phi...@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.
>
>
