Re: [PATCH v2] hw/misc/vmfwupdate: Introduce hypervisor fw-cfg interface support

[Philippe Mathieu-Daudé]

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=<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).

> 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/">https://lore.kernel.org/qemu-devel/20241219153857.57450-1-philmd@linaro.org/

> 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.