Re: Suspicious QOM types without instance/class size
On Mon, Aug 24, 2020 at 08:06:42PM +0300, Roman Bolshakov wrote:
> On Mon, Aug 24, 2020 at 12:45:52PM -0400, Eduardo Habkost wrote:
> > On Mon, Aug 24, 2020 at 07:41:34PM +0300, Roman Bolshakov wrote:
> > > On Fri, Aug 21, 2020 at 01:48:02PM -0400, Eduardo Habkost wrote:
> > > > Signed-off-by: Eduardo Habkost
> > > > ---
> > > > diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
> > > > index d81f569aed..81d1662d06 100644
> > > > --- a/target/i386/hvf/hvf.c
> > > > +++ b/target/i386/hvf/hvf.c
> > > > @@ -878,13 +878,11 @@ static int hvf_accel_init(MachineState *ms)
> > > > {
> > > > int x;
> > > > hv_return_t ret;
> > > > -HVFState *s;
> > > > +HVFState *s = HVF_STATE(ms->accelerator);
> > >
> > > The file also needs definition of MachineState:
> > > #include "hw/boards.h"
> > >
> > > >
> > > > ret = hv_vm_create(HV_VM_DEFAULT);
> > > > assert_hvf_ok(ret);
> > > >
> > > > -s = g_new0(HVFState, 1);
> > > > -
> > > > s->num_slots = 32;
> > > > for (x = 0; x < s->num_slots; ++x) {
> > > > s->slots[x].size = 0;
> > > > @@ -908,6 +906,7 @@ static void hvf_accel_class_init(ObjectClass *oc,
> > > > void *data)
> > > > static const TypeInfo hvf_accel_type = {
> > > > .name = TYPE_HVF_ACCEL,
> > > > .parent = TYPE_ACCEL,
> > > > +.instance_size = sizeof(HVFState),
> > > > .class_init = hvf_accel_class_init,
> > > > };
> > > >
> > > >
> >
> > However, the hvf patch above shouldn't require it. You should be
> > able to apply and test it on top of qemu.git master.
> >
>
> Yeah, that's correct, thanks.
>
> With the include fix for hw/boards.h, the patch works:
> Reviewed-By: Roman Bolshakov
> Tested-By: Roman Bolshakov
>
> BTW, am I expected to see the accel in "info qtree" (or qom-tree)? It's
> not there for a reason.
I don't know if you are expect to see it. I don't think there's
explicit code to attach the accel object to the user-visible QOM
tree.
--
Eduardo
Re: Suspicious QOM types without instance/class size
On Mon, Aug 24, 2020 at 12:45:52PM -0400, Eduardo Habkost wrote:
> On Mon, Aug 24, 2020 at 07:41:34PM +0300, Roman Bolshakov wrote:
> > On Fri, Aug 21, 2020 at 01:48:02PM -0400, Eduardo Habkost wrote:
> > > Signed-off-by: Eduardo Habkost
> > > ---
> > > diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
> > > index d81f569aed..81d1662d06 100644
> > > --- a/target/i386/hvf/hvf.c
> > > +++ b/target/i386/hvf/hvf.c
> > > @@ -878,13 +878,11 @@ static int hvf_accel_init(MachineState *ms)
> > > {
> > > int x;
> > > hv_return_t ret;
> > > -HVFState *s;
> > > +HVFState *s = HVF_STATE(ms->accelerator);
> >
> > The file also needs definition of MachineState:
> > #include "hw/boards.h"
> >
> > >
> > > ret = hv_vm_create(HV_VM_DEFAULT);
> > > assert_hvf_ok(ret);
> > >
> > > -s = g_new0(HVFState, 1);
> > > -
> > > s->num_slots = 32;
> > > for (x = 0; x < s->num_slots; ++x) {
> > > s->slots[x].size = 0;
> > > @@ -908,6 +906,7 @@ static void hvf_accel_class_init(ObjectClass *oc,
> > > void *data)
> > > static const TypeInfo hvf_accel_type = {
> > > .name = TYPE_HVF_ACCEL,
> > > .parent = TYPE_ACCEL,
> > > +.instance_size = sizeof(HVFState),
> > > .class_init = hvf_accel_class_init,
> > > };
> > >
> > >
>
> However, the hvf patch above shouldn't require it. You should be
> able to apply and test it on top of qemu.git master.
>
Yeah, that's correct, thanks.
With the include fix for hw/boards.h, the patch works:
Reviewed-By: Roman Bolshakov
Tested-By: Roman Bolshakov
BTW, am I expected to see the accel in "info qtree" (or qom-tree)? It's
not there for a reason.
Regards,
Roman
Re: Suspicious QOM types without instance/class size
On Mon, Aug 24, 2020 at 07:41:34PM +0300, Roman Bolshakov wrote:
> On Fri, Aug 21, 2020 at 01:48:02PM -0400, Eduardo Habkost wrote:
> > On Fri, Aug 21, 2020 at 01:29:38PM -0400, Eduardo Habkost wrote:
> > > On Fri, Aug 21, 2020 at 01:53:52PM +0300, Roman Bolshakov wrote:
> > > > On Thu, Aug 20, 2020 at 05:55:29PM -0400, Eduardo Habkost wrote:
> > > > > While trying to convert TypeInfo declarations to the new
> > > > > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases
> > > > > where instance_size or class_size is not set, despite having type
> > > > > checker macros that use a specific type.
> > > > >
> > > > > The ones with "WARNING" are abstract types (maybe not serious if
> > > > > subclasses set the appropriate sizes). The ones with "ERROR"
> > > > > don't seem to be abstract types.
> > > > >
> > > >
> > > > > ERROR: target/i386/hvf/hvf.c:908:1: instance_size should be set to
> > > > > sizeof(HVFState)?
> > > >
> > >
> > > > BTW, the object definition for hvf seems different from KVM (and perhaps
> > > > wrong?), e.g. HVFState is allocated within init_machine handler and then
> > > > assigned to a global variable:
> > >
> > > Interesting. It looks like hvf_state is _not_ the actual QOM
> > > object instance. The actual TYPE_HVF_ACCEL instance is created
> > > by do_configure_accelerator(). That would explain why the lack
> > > of instance_init never caused any problems.
> > >
> > > Luckily, no code ever used the HVF_STATE macro. If
> > > HVF_STATE(hvf_state) got called, it would crash because of
> > > uninitialized object instance data. If HVF_STATE(machine->accel)
> > > got called, it would return an invalid HVFState pointer (not
> > > hvf_state).
> > >
> > > I believe the simplest short term solution here is to just delete
> > > the HVF_STATE macro and HVFState::parent field. We can worry
> > > about actually moving hvf_state to the machine->accel QOM object
> > > later.
> >
> > Actually, it might be easier to do the full QOM conversion in a
> > single patch instead of deleting the incomplete code.
> >
>
> I agree full QOM conversion is better, perhaps we can later move
> certains bits to accel/hvf.c like it's done for kvm/tcg/qtest.
>
> > Can you check if the following patch works? I don't have a host
> > where I can test it.
> >
>
> Sure, thanks :)
>
> > Signed-off-by: Eduardo Habkost
> > ---
> > diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
> > index d81f569aed..81d1662d06 100644
> > --- a/target/i386/hvf/hvf.c
> > +++ b/target/i386/hvf/hvf.c
> > @@ -878,13 +878,11 @@ static int hvf_accel_init(MachineState *ms)
> > {
> > int x;
> > hv_return_t ret;
> > -HVFState *s;
> > +HVFState *s = HVF_STATE(ms->accelerator);
>
> The file also needs definition of MachineState:
> #include "hw/boards.h"
>
> >
> > ret = hv_vm_create(HV_VM_DEFAULT);
> > assert_hvf_ok(ret);
> >
> > -s = g_new0(HVFState, 1);
> > -
> > s->num_slots = 32;
> > for (x = 0; x < s->num_slots; ++x) {
> > s->slots[x].size = 0;
> > @@ -908,6 +906,7 @@ static void hvf_accel_class_init(ObjectClass *oc, void
> > *data)
> > static const TypeInfo hvf_accel_type = {
> > .name = TYPE_HVF_ACCEL,
> > .parent = TYPE_ACCEL,
> > +.instance_size = sizeof(HVFState),
> > .class_init = hvf_accel_class_init,
> > };
> >
> >
>
> Unfortunately it fails to start (even without the HVF patch):
> ERROR:../qom/object.c:314:type_initialize: assertion failed:
> (parent->class_size <= ti->class_size)
> Bail out! ERROR:../qom/object.c:314:type_initialize: assertion failed:
> (parent->class_size <= ti->class_size)
[...]
> It doesn't seem related to HVF QOM changes 樂
>
> Bisected it to:
>
> b717702de21461138ac0e1d6775da0bd0482c013 is the first bad commit
> commit b717702de21461138ac0e1d6775da0bd0482c013
> Author: Daniel P. Berrangé
> Date: Wed Aug 19 20:12:35 2020 -0400
>
> crypto: use QOM macros for declaration/definition of secret types
>
> This introduces the use of the OBJECT_DEFINE and OBJECT_DECLARE macro
> families in the secret types, in order to eliminate boilerplate code.
>
> Signed-off-by: Daniel P. Berrangé
> Message-Id: <20200723181410.3145233-4-berra...@redhat.com>
> [ehabkost: rebase, update to pass additional arguments to macro]
> Signed-off-by: Eduardo Habkost
> Message-Id: <20200820001236.1284548-58-ehabk...@redhat.com>
Oh, that's a bug in my QOM series. Thanks for debugging it! I
will fix it in v3.
However, the hvf patch above shouldn't require it. You should be
able to apply and test it on top of qemu.git master.
--
Eduardo
Re: Suspicious QOM types without instance/class size
On Fri, Aug 21, 2020 at 01:48:02PM -0400, Eduardo Habkost wrote:
> On Fri, Aug 21, 2020 at 01:29:38PM -0400, Eduardo Habkost wrote:
> > On Fri, Aug 21, 2020 at 01:53:52PM +0300, Roman Bolshakov wrote:
> > > On Thu, Aug 20, 2020 at 05:55:29PM -0400, Eduardo Habkost wrote:
> > > > While trying to convert TypeInfo declarations to the new
> > > > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases
> > > > where instance_size or class_size is not set, despite having type
> > > > checker macros that use a specific type.
> > > >
> > > > The ones with "WARNING" are abstract types (maybe not serious if
> > > > subclasses set the appropriate sizes). The ones with "ERROR"
> > > > don't seem to be abstract types.
> > > >
> > >
> > > > ERROR: target/i386/hvf/hvf.c:908:1: instance_size should be set to
> > > > sizeof(HVFState)?
> > >
> >
> > > BTW, the object definition for hvf seems different from KVM (and perhaps
> > > wrong?), e.g. HVFState is allocated within init_machine handler and then
> > > assigned to a global variable:
> >
> > Interesting. It looks like hvf_state is _not_ the actual QOM
> > object instance. The actual TYPE_HVF_ACCEL instance is created
> > by do_configure_accelerator(). That would explain why the lack
> > of instance_init never caused any problems.
> >
> > Luckily, no code ever used the HVF_STATE macro. If
> > HVF_STATE(hvf_state) got called, it would crash because of
> > uninitialized object instance data. If HVF_STATE(machine->accel)
> > got called, it would return an invalid HVFState pointer (not
> > hvf_state).
> >
> > I believe the simplest short term solution here is to just delete
> > the HVF_STATE macro and HVFState::parent field. We can worry
> > about actually moving hvf_state to the machine->accel QOM object
> > later.
>
> Actually, it might be easier to do the full QOM conversion in a
> single patch instead of deleting the incomplete code.
>
I agree full QOM conversion is better, perhaps we can later move
certains bits to accel/hvf.c like it's done for kvm/tcg/qtest.
> Can you check if the following patch works? I don't have a host
> where I can test it.
>
Sure, thanks :)
> Signed-off-by: Eduardo Habkost
> ---
> diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
> index d81f569aed..81d1662d06 100644
> --- a/target/i386/hvf/hvf.c
> +++ b/target/i386/hvf/hvf.c
> @@ -878,13 +878,11 @@ static int hvf_accel_init(MachineState *ms)
> {
> int x;
> hv_return_t ret;
> -HVFState *s;
> +HVFState *s = HVF_STATE(ms->accelerator);
The file also needs definition of MachineState:
#include "hw/boards.h"
>
> ret = hv_vm_create(HV_VM_DEFAULT);
> assert_hvf_ok(ret);
>
> -s = g_new0(HVFState, 1);
> -
> s->num_slots = 32;
> for (x = 0; x < s->num_slots; ++x) {
> s->slots[x].size = 0;
> @@ -908,6 +906,7 @@ static void hvf_accel_class_init(ObjectClass *oc, void
> *data)
> static const TypeInfo hvf_accel_type = {
> .name = TYPE_HVF_ACCEL,
> .parent = TYPE_ACCEL,
> +.instance_size = sizeof(HVFState),
> .class_init = hvf_accel_class_init,
> };
>
>
Unfortunately it fails to start (even without the HVF patch):
ERROR:../qom/object.c:314:type_initialize: assertion failed:
(parent->class_size <= ti->class_size)
Bail out! ERROR:../qom/object.c:314:type_initialize: assertion failed:
(parent->class_size <= ti->class_size)
(lldb) bt
* thread #3, stop reason = signal SIGABRT
* frame #0: 0x7fff6a75e33a libsystem_kernel.dylib`__pthread_kill + 10
frame #1: 0x7fff6a81ae60 libsystem_pthread.dylib`pthread_kill + 430
frame #2: 0x7fff6a6e5808 libsystem_c.dylib`abort + 120
frame #3: 0x000101289c36 libglib-2.0.0.dylib`g_assertion_message + 406
frame #4: 0x000101289c9e libglib-2.0.0.dylib`g_assertion_message_expr +
94
frame #5: 0x000100353c00
qemu-system-x86_64`type_initialize(ti=0x0001032403e0) at object.c:314:9
[opt]
frame #6: 0x00010035378b
qemu-system-x86_64`type_initialize(ti=0x00010323fd70) at object.c:310:9
[opt]
frame #7: 0x000100353de8
qemu-system-x86_64`object_class_foreach_tramp(key=,
value=0x00010323fd70, opaque=0x75cb8d98) at object.c:1030:5 [opt]
frame #8: 0x00010124b83d libglib-2.0.0.dylib`g_hash_table_foreach + 125
frame #9: 0x000100354079 qemu-system-x86_64`object_class_get_list
[inlined] object_class_foreach(fn=, implements_type=,
include_abstract=, opaque=0x75cb8d90) at object.c:1052:5
[opt]
frame #10: 0x00010035401e
qemu-system-x86_64`object_class_get_list(implements_type=,
include_abstract=) at object.c:1109 [opt]
frame #11: 0x00010030875d qemu-system-x86_64`qemu_init [inlined]
select_machine at vl.c:2438:24 [opt]
frame #12: 0x00010030874c
qemu-system-x86_64`qemu_init(argc=, argv=,
envp=) at vl.c:3842 [opt]
frame #13: 0x00018ef9
qemu-system-x86_64`qemu_main(argc=, argv=,
envp=) at main.c:49:5 [opt]
frame #14:
Re: Suspicious QOM types without instance/class size
On Fri, 21 Aug 2020 17:01:49 -0400 Eduardo Habkost wrote: > On Fri, Aug 21, 2020 at 11:43:35AM +0200, Cornelia Huck wrote: > > On Thu, 20 Aug 2020 17:55:29 -0400 > > Eduardo Habkost wrote: > > > > > While trying to convert TypeInfo declarations to the new > > > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases > > > where instance_size or class_size is not set, despite having type > > > checker macros that use a specific type. > > > > > > The ones with "WARNING" are abstract types (maybe not serious if > > > subclasses set the appropriate sizes). The ones with "ERROR" > > > don't seem to be abstract types. > > > > > > ERROR: hw/s390x/virtio-ccw.c:1237:1: class_size should be set to > > > sizeof(VirtioCcwBusClass)? > > > ERROR: hw/virtio/virtio-pci.c:2101:1: class_size should be set to > > > sizeof(VirtioPCIBusClass)? > > > > VirtioCcwBusClass and VirtioPCIBusClass are both simple typedefs of > > VirtioBusClass (it's likely that I copied the ccw definition from the > > pci one). virtio-mmio instead uses VirtioBusClass directly in its > > checker macros. > > > > I don't see a real reason for the typedefs, maybe ccw and pci should > > use the mmio approach as well? > > I think it's OK to keep the typedefs if the code is consistent > (i.e. we set instance_size and class_size just in case the > typedefs are replaced by a real struct one day). AFAIU, VirtioBusClass is providing functionality needed for all virtio buses, and they should not need to add anything on top. We can however try to make it safe, as it is only a line of code for both pci and ccw. > > I'm not sure about the TYPE_VIRTIO_MMIO_BUS approach. If the > code just needs VirtioBusState or VirtioBusClass pointers, it can > already use the VIRTIO_BUS* macros. We could go ahead and ditch the bus-specific macros if there's no real need for it. At least, I don't see a real need for *Class. OTOH, having types and macros defined everywhere makes it more symmetric. > > The OBJECT_DECLARE_TYPE macro Daniel sent expects each QOM type > to have a separate struct being defined, which isn't true in many > cases. I'm considering removing the "typedef struct Foo Foo" > lines from OBJECT_DECLARE_TYPE(), to make initial conversion > easier. Would be interesting to figure out the individual reasons why there's no separate struct, just to make sure we're not operating from wrong assumptions.
Re: Suspicious QOM types without instance/class size
On Fri, Aug 21, 2020 at 11:43:35AM +0200, Cornelia Huck wrote: > On Thu, 20 Aug 2020 17:55:29 -0400 > Eduardo Habkost wrote: > > > While trying to convert TypeInfo declarations to the new > > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases > > where instance_size or class_size is not set, despite having type > > checker macros that use a specific type. > > > > The ones with "WARNING" are abstract types (maybe not serious if > > subclasses set the appropriate sizes). The ones with "ERROR" > > don't seem to be abstract types. > > > > ERROR: hw/s390x/virtio-ccw.c:1237:1: class_size should be set to > > sizeof(VirtioCcwBusClass)? > > ERROR: hw/virtio/virtio-pci.c:2101:1: class_size should be set to > > sizeof(VirtioPCIBusClass)? > > VirtioCcwBusClass and VirtioPCIBusClass are both simple typedefs of > VirtioBusClass (it's likely that I copied the ccw definition from the > pci one). virtio-mmio instead uses VirtioBusClass directly in its > checker macros. > > I don't see a real reason for the typedefs, maybe ccw and pci should > use the mmio approach as well? I think it's OK to keep the typedefs if the code is consistent (i.e. we set instance_size and class_size just in case the typedefs are replaced by a real struct one day). I'm not sure about the TYPE_VIRTIO_MMIO_BUS approach. If the code just needs VirtioBusState or VirtioBusClass pointers, it can already use the VIRTIO_BUS* macros. The OBJECT_DECLARE_TYPE macro Daniel sent expects each QOM type to have a separate struct being defined, which isn't true in many cases. I'm considering removing the "typedef struct Foo Foo" lines from OBJECT_DECLARE_TYPE(), to make initial conversion easier. -- Eduardo
Re: Suspicious QOM types without instance/class size
On Fri, Aug 21, 2020 at 11:40:12AM +0200, David Hildenbrand wrote: > On 20.08.20 23:55, Eduardo Habkost wrote: > > While trying to convert TypeInfo declarations to the new > > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases > > where instance_size or class_size is not set, despite having type > > checker macros that use a specific type. > > > > The ones with "WARNING" are abstract types (maybe not serious if > > subclasses set the appropriate sizes). The ones with "ERROR" > > don't seem to be abstract types. [...] > > ERROR: hw/s390x/virtio-ccw.c:1237:1: class_size should be set to > > sizeof(VirtioCcwBusClass)? > > The parent of TYPE_VIRTIO_CCW_BUS is TYPE_VIRTIO_BUS. > > typedef struct VirtioBusClass VirtioCcwBusClass; > > So I guess the sizes match? Anyhow, setting doesn't hurt. Thanks for checking. Yeah, the sizes match today. It's a good idea to set it, just in case a real VirtioCcwBusClass struct gets created one day. -- Eduardo
Re: Suspicious QOM types without instance/class size
On Fri, Aug 21, 2020 at 11:47:32AM +1000, David Gibson wrote: > On Thu, Aug 20, 2020 at 05:55:29PM -0400, Eduardo Habkost wrote: > > While trying to convert TypeInfo declarations to the new > > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases > > where instance_size or class_size is not set, despite having type > > checker macros that use a specific type. > > > > The ones with "WARNING" are abstract types (maybe not serious if > > subclasses set the appropriate sizes). The ones with "ERROR" > > don't seem to be abstract types. > > > Comment on the ones within my area: > > > > WARNING: hw/input/adb.c:310:1: class_size should be set to > > sizeof(ADBDeviceClass)? > > Yeah, that looks like a bug (though we'll get away with it because > it's abstract). Right, luckily we are not touching any ADBDeviceClass field inside adb_device_class_init(). > > > WARNING: hw/ppc/pnv_lpc.c:771:1: instance_size should be set to > > sizeof(PnvLpcController)? > > Ditto. Agreed. > > Should I make fixes for these, or will you? Please send the fixes, and I will apply them before running the TypeInfo conversion script. > > > ERROR: hw/ppc/spapr_drc.c:771:1: instance_size should be set to > > sizeof(SpaprDrc)? > > I'm confused by this one. I'm not exactly sure which definition is > tripping the error, and AFAICT they should all be correctly inheriting > instance_size from either TYPE_SPAPR_DR_CONNECTOR or > TYPE_SPAPR_DRC_PHSYICAL. If anything, it looks like > TYPE_SPAPR_DRC_PHB could drop it's explicit override of instance_size. The error is triggered because of this type checking macro at include/hw/ppc/spapr_drc.h: #define SPAPR_DRC_PCI(obj) OBJECT_CHECK(SpaprDrc, (obj), \ TYPE_SPAPR_DRC_PCI) The expectation is that whatever type you use in OBJECT_CHECK will be the one used for instance_size. The script also looks at the parent type, to reduce false positives, but this case was flagged because SPAPR_DRC_PCI uses SpaprDrc, but the parent type (SPAPR_DRC_PHYSICAL) uses SpaprDrcPhysical. Now, I don't understand why we have so many instance checker macros that use the same typedef (SpaprDrc). If the code needs a valid SpaprDrc pointer, it can just use SPAPR_DR_CONNECTOR(). -- Eduardo
Re: Suspicious QOM types without instance/class size
On Fri, Aug 21, 2020 at 01:29:38PM -0400, Eduardo Habkost wrote:
> On Fri, Aug 21, 2020 at 01:53:52PM +0300, Roman Bolshakov wrote:
> > On Thu, Aug 20, 2020 at 05:55:29PM -0400, Eduardo Habkost wrote:
> > > While trying to convert TypeInfo declarations to the new
> > > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases
> > > where instance_size or class_size is not set, despite having type
> > > checker macros that use a specific type.
> > >
> > > The ones with "WARNING" are abstract types (maybe not serious if
> > > subclasses set the appropriate sizes). The ones with "ERROR"
> > > don't seem to be abstract types.
> > >
> >
> > > ERROR: target/i386/hvf/hvf.c:908:1: instance_size should be set to
> > > sizeof(HVFState)?
> >
> > Hi Eduardo,
> >
> > How do you get the error?
>
> My script looks for corresponding type checking macros, and check
> if instance_size is set to sizeof(T) with the right type from the
> type checking macro.
>
> The code is here:
> https://github.com/ehabkost/qemu-hacks/blob/920b2c521ad2a29fa663256854e24ed2059ba9cd/scripts/codeconverter/codeconverter/qom_type_info.py#L136
>
>
> >
> > Given your changes, instance size should really be sizeof(HVFState).
> >
>
> The changes I've made shouldn't make any difference (if there's
> an issue, it is there before or after my series).
>
> > BTW, the object definition for hvf seems different from KVM (and perhaps
> > wrong?), e.g. HVFState is allocated within init_machine handler and then
> > assigned to a global variable:
>
> Interesting. It looks like hvf_state is _not_ the actual QOM
> object instance. The actual TYPE_HVF_ACCEL instance is created
> by do_configure_accelerator(). That would explain why the lack
> of instance_init never caused any problems.
>
> Luckily, no code ever used the HVF_STATE macro. If
> HVF_STATE(hvf_state) got called, it would crash because of
> uninitialized object instance data. If HVF_STATE(machine->accel)
> got called, it would return an invalid HVFState pointer (not
> hvf_state).
>
> I believe the simplest short term solution here is to just delete
> the HVF_STATE macro and HVFState::parent field. We can worry
> about actually moving hvf_state to the machine->accel QOM object
> later.
Actually, it might be easier to do the full QOM conversion in a
single patch instead of deleting the incomplete code.
Can you check if the following patch works? I don't have a host
where I can test it.
Signed-off-by: Eduardo Habkost
---
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index d81f569aed..81d1662d06 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -878,13 +878,11 @@ static int hvf_accel_init(MachineState *ms)
{
int x;
hv_return_t ret;
-HVFState *s;
+HVFState *s = HVF_STATE(ms->accelerator);
ret = hv_vm_create(HV_VM_DEFAULT);
assert_hvf_ok(ret);
-s = g_new0(HVFState, 1);
-
s->num_slots = 32;
for (x = 0; x < s->num_slots; ++x) {
s->slots[x].size = 0;
@@ -908,6 +906,7 @@ static void hvf_accel_class_init(ObjectClass *oc, void
*data)
static const TypeInfo hvf_accel_type = {
.name = TYPE_HVF_ACCEL,
.parent = TYPE_ACCEL,
+.instance_size = sizeof(HVFState),
.class_init = hvf_accel_class_init,
};
--
Eduardo
Re: Suspicious QOM types without instance/class size
On Fri, Aug 21, 2020 at 01:53:52PM +0300, Roman Bolshakov wrote:
> On Thu, Aug 20, 2020 at 05:55:29PM -0400, Eduardo Habkost wrote:
> > While trying to convert TypeInfo declarations to the new
> > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases
> > where instance_size or class_size is not set, despite having type
> > checker macros that use a specific type.
> >
> > The ones with "WARNING" are abstract types (maybe not serious if
> > subclasses set the appropriate sizes). The ones with "ERROR"
> > don't seem to be abstract types.
> >
>
> > ERROR: target/i386/hvf/hvf.c:908:1: instance_size should be set to
> > sizeof(HVFState)?
>
> Hi Eduardo,
>
> How do you get the error?
My script looks for corresponding type checking macros, and check
if instance_size is set to sizeof(T) with the right type from the
type checking macro.
The code is here:
https://github.com/ehabkost/qemu-hacks/blob/920b2c521ad2a29fa663256854e24ed2059ba9cd/scripts/codeconverter/codeconverter/qom_type_info.py#L136
>
> Given your changes, instance size should really be sizeof(HVFState).
>
The changes I've made shouldn't make any difference (if there's
an issue, it is there before or after my series).
> BTW, the object definition for hvf seems different from KVM (and perhaps
> wrong?), e.g. HVFState is allocated within init_machine handler and then
> assigned to a global variable:
Interesting. It looks like hvf_state is _not_ the actual QOM
object instance. The actual TYPE_HVF_ACCEL instance is created
by do_configure_accelerator(). That would explain why the lack
of instance_init never caused any problems.
Luckily, no code ever used the HVF_STATE macro. If
HVF_STATE(hvf_state) got called, it would crash because of
uninitialized object instance data. If HVF_STATE(machine->accel)
got called, it would return an invalid HVFState pointer (not
hvf_state).
I believe the simplest short term solution here is to just delete
the HVF_STATE macro and HVFState::parent field. We can worry
about actually moving hvf_state to the machine->accel QOM object
later.
>
> static int hvf_accel_init(MachineState *ms)
> {
> int x;
> hv_return_t ret;
> HVFState *s;
>
> ret = hv_vm_create(HV_VM_DEFAULT);
> assert_hvf_ok(ret);
>
> s = g_new0(HVFState, 1);
>
> s->num_slots = 32;
> for (x = 0; x < s->num_slots; ++x) {
> s->slots[x].size = 0;
> s->slots[x].slot_id = x;
> }
>
> hvf_state = s;
> cpu_interrupt_handler = hvf_handle_interrupt;
> memory_listener_register(_memory_listener, _space_memory);
> return 0;
> }
>
> static void hvf_accel_class_init(ObjectClass *oc, void *data)
> {
> AccelClass *ac = ACCEL_CLASS(oc);
> ac->name = "HVF";
> ac->init_machine = hvf_accel_init;
> ac->allowed = _allowed;
> }
>
> static const TypeInfo hvf_accel_type = {
> .name = TYPE_HVF_ACCEL,
> .parent = TYPE_ACCEL,
> .class_init = hvf_accel_class_init,
> };
>
> Thanks,
> Roman
>
--
Eduardo
Re: Suspicious QOM types without instance/class size
On Fri, Aug 21, 2020 at 09:06:51AM -0700, Alistair Francis wrote: > On Thu, Aug 20, 2020 at 2:56 PM Eduardo Habkost wrote: > > > > While trying to convert TypeInfo declarations to the new > > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases > > where instance_size or class_size is not set, despite having type > > checker macros that use a specific type. > > > > The ones with "WARNING" are abstract types (maybe not serious if > > subclasses set the appropriate sizes). The ones with "ERROR" > > don't seem to be abstract types. > > > > > ERROR: hw/core/register.c:328:1: instance_size should be set to > > sizeof(RegisterInfo)? > > I'll send a patch out for this one today. > > If you are fixing all of these as part of a series I'm also happy to > just let you do that. Feel free to send the fix, and I will include it as part of my series if necessary. Note that register_init_block() relies on the fact that register_init() won't touch any RegisterInfo field except parent_obj, so this won't be a one line patch. -- Eduardo
Re: Suspicious QOM types without instance/class size
On Thu, Aug 20, 2020 at 2:56 PM Eduardo Habkost wrote: > > While trying to convert TypeInfo declarations to the new > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases > where instance_size or class_size is not set, despite having type > checker macros that use a specific type. > > The ones with "WARNING" are abstract types (maybe not serious if > subclasses set the appropriate sizes). The ones with "ERROR" > don't seem to be abstract types. > > ERROR: hw/core/register.c:328:1: instance_size should be set to > sizeof(RegisterInfo)? I'll send a patch out for this one today. If you are fixing all of these as part of a series I'm also happy to just let you do that. Alistair > > -- > Eduardo > >
Re: Suspicious QOM types without instance/class size
On Thu, Aug 20, 2020 at 05:55:29PM -0400, Eduardo Habkost wrote:
> While trying to convert TypeInfo declarations to the new
> OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases
> where instance_size or class_size is not set, despite having type
> checker macros that use a specific type.
>
> The ones with "WARNING" are abstract types (maybe not serious if
> subclasses set the appropriate sizes). The ones with "ERROR"
> don't seem to be abstract types.
>
> ERROR: target/i386/hvf/hvf.c:908:1: instance_size should be set to
> sizeof(HVFState)?
Hi Eduardo,
How do you get the error?
Given your changes, instance size should really be sizeof(HVFState).
BTW, the object definition for hvf seems different from KVM (and perhaps
wrong?), e.g. HVFState is allocated within init_machine handler and then
assigned to a global variable:
static int hvf_accel_init(MachineState *ms)
{
int x;
hv_return_t ret;
HVFState *s;
ret = hv_vm_create(HV_VM_DEFAULT);
assert_hvf_ok(ret);
s = g_new0(HVFState, 1);
s->num_slots = 32;
for (x = 0; x < s->num_slots; ++x) {
s->slots[x].size = 0;
s->slots[x].slot_id = x;
}
hvf_state = s;
cpu_interrupt_handler = hvf_handle_interrupt;
memory_listener_register(_memory_listener, _space_memory);
return 0;
}
static void hvf_accel_class_init(ObjectClass *oc, void *data)
{
AccelClass *ac = ACCEL_CLASS(oc);
ac->name = "HVF";
ac->init_machine = hvf_accel_init;
ac->allowed = _allowed;
}
static const TypeInfo hvf_accel_type = {
.name = TYPE_HVF_ACCEL,
.parent = TYPE_ACCEL,
.class_init = hvf_accel_class_init,
};
Thanks,
Roman
Re: Suspicious QOM types without instance/class size
On Thu, 20 Aug 2020 17:55:29 -0400 Eduardo Habkost wrote: > While trying to convert TypeInfo declarations to the new > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases > where instance_size or class_size is not set, despite having type > checker macros that use a specific type. > > The ones with "WARNING" are abstract types (maybe not serious if > subclasses set the appropriate sizes). The ones with "ERROR" > don't seem to be abstract types. > > ERROR: hw/s390x/virtio-ccw.c:1237:1: class_size should be set to > sizeof(VirtioCcwBusClass)? > ERROR: hw/virtio/virtio-pci.c:2101:1: class_size should be set to > sizeof(VirtioPCIBusClass)? VirtioCcwBusClass and VirtioPCIBusClass are both simple typedefs of VirtioBusClass (it's likely that I copied the ccw definition from the pci one). virtio-mmio instead uses VirtioBusClass directly in its checker macros. I don't see a real reason for the typedefs, maybe ccw and pci should use the mmio approach as well?
Re: Suspicious QOM types without instance/class size
On 20.08.20 23:55, Eduardo Habkost wrote: > While trying to convert TypeInfo declarations to the new > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases > where instance_size or class_size is not set, despite having type > checker macros that use a specific type. > > The ones with "WARNING" are abstract types (maybe not serious if > subclasses set the appropriate sizes). The ones with "ERROR" > don't seem to be abstract types. > > WARNING: hw/arm/armsse.c:1159:1: class_size should be set to > sizeof(ARMSSEClass)? > WARNING: hw/audio/hda-codec.c:900:1: instance_size should be set to > sizeof(HDAAudioState)? > ERROR: hw/core/register.c:328:1: instance_size should be set to > sizeof(RegisterInfo)? > WARNING: hw/input/adb.c:310:1: class_size should be set to > sizeof(ADBDeviceClass)? > WARNING: hw/isa/isa-superio.c:181:1: instance_size should be set to > sizeof(ISASuperIODevice)? > WARNING: hw/ppc/pnv_lpc.c:771:1: instance_size should be set to > sizeof(PnvLpcController)? > ERROR: hw/ppc/spapr_drc.c:771:1: instance_size should be set to > sizeof(SpaprDrc)? > WARNING: hw/rtc/m48t59-isa.c:156:1: class_size should be set to > sizeof(M48txxISADeviceClass)? > WARNING: hw/rtc/m48t59.c:691:1: class_size should be set to > sizeof(M48txxSysBusDeviceClass)? > ERROR: hw/s390x/virtio-ccw.c:1237:1: class_size should be set to > sizeof(VirtioCcwBusClass)? The parent of TYPE_VIRTIO_CCW_BUS is TYPE_VIRTIO_BUS. typedef struct VirtioBusClass VirtioCcwBusClass; So I guess the sizes match? Anyhow, setting doesn't hurt. -- Thanks, David / dhildenb
Re: Suspicious QOM types without instance/class size
On Thu, 20 Aug 2020 at 22:55, Eduardo Habkost wrote: > > While trying to convert TypeInfo declarations to the new > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases > where instance_size or class_size is not set, despite having type > checker macros that use a specific type. > > The ones with "WARNING" are abstract types (maybe not serious if > subclasses set the appropriate sizes). The ones with "ERROR" > don't seem to be abstract types. > > WARNING: hw/arm/armsse.c:1159:1: class_size should be set to > sizeof(ARMSSEClass)? Seems like a bug, yes. Here the subclasses are simple leaf classes which (correctly) don't set class_size, so the abstract parent class must set class_size. > ERROR: hw/core/register.c:328:1: instance_size should be set to > sizeof(RegisterInfo)? Bug (we haven't noticed because the only thing that creates them is register_info() and it does object_initialize() into an existing struct rather than trying to object_new() it). thanks -- PMM
Re: Suspicious QOM types without instance/class size
On Thu, Aug 20, 2020 at 05:55:29PM -0400, Eduardo Habkost wrote: > While trying to convert TypeInfo declarations to the new > OBJECT_DECLARE* macros, I've stumbled on a few suspicious cases > where instance_size or class_size is not set, despite having type > checker macros that use a specific type. > > The ones with "WARNING" are abstract types (maybe not serious if > subclasses set the appropriate sizes). The ones with "ERROR" > don't seem to be abstract types. Comment on the ones within my area: > > WARNING: hw/input/adb.c:310:1: class_size should be set to > sizeof(ADBDeviceClass)? Yeah, that looks like a bug (though we'll get away with it because it's abstract). > WARNING: hw/ppc/pnv_lpc.c:771:1: instance_size should be set to > sizeof(PnvLpcController)? Ditto. Should I make fixes for these, or will you? > ERROR: hw/ppc/spapr_drc.c:771:1: instance_size should be set to > sizeof(SpaprDrc)? I'm confused by this one. I'm not exactly sure which definition is tripping the error, and AFAICT they should all be correctly inheriting instance_size from either TYPE_SPAPR_DR_CONNECTOR or TYPE_SPAPR_DRC_PHSYICAL. If anything, it looks like TYPE_SPAPR_DRC_PHB could drop it's explicit override of instance_size. -- David Gibson| I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson signature.asc Description: PGP signature
