Suspicious QOM types without instance/class size

Suspicious QOM types without instance/class size

[Eduardo Habkost]

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)?
WARNING: hw/ssi/ssi.c:88:1: instance_size should be set to sizeof(SSISlave)?
ERROR: hw/virtio/virtio-pci.c:2101:1: class_size should be set to sizeof(VirtioPCIBusClass)?
WARNING: scsi/pr-manager.c:76:1: instance_size should be set to sizeof(PRManager)?
ERROR: target/i386/hvf/hvf.c:908:1: instance_size should be set to sizeof(HVFState)?

-- 
Eduardo

Re: Suspicious QOM types without instance/class size

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1455 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Suspicious QOM types without instance/class size

[Peter Maydell]

On Thu, 20 Aug 2020 at 22:55, Eduardo Habkost <ehabkost@redhat.com> 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

[David Hildenbrand]

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

[Cornelia Huck]

On Thu, 20 Aug 2020 17:55:29 -0400
Eduardo Habkost <ehabkost@redhat.com> 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

[Roman Bolshakov]

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(&hvf_memory_listener, &address_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 = &hvf_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

[Alistair Francis]

On Thu, Aug 20, 2020 at 2:56 PM Eduardo Habkost <ehabkost@redhat.com> 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

[Eduardo Habkost]

On Fri, Aug 21, 2020 at 09:06:51AM -0700, Alistair Francis wrote:
> On Thu, Aug 20, 2020 at 2:56 PM Eduardo Habkost <ehabkost@redhat.com> 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

[Eduardo Habkost]

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(&hvf_memory_listener, &address_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 = &hvf_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

[Eduardo Habkost]

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 <ehabkost@redhat.com>
---
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

[Eduardo Habkost]

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

[Eduardo Habkost]

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

[Eduardo Habkost]

On Fri, Aug 21, 2020 at 11:43:35AM +0200, Cornelia Huck wrote:
> On Thu, 20 Aug 2020 17:55:29 -0400
> Eduardo Habkost <ehabkost@redhat.com> 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

[Cornelia Huck]

On Fri, 21 Aug 2020 17:01:49 -0400
Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com> 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

[Roman Bolshakov]

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 <ehabkost@redhat.com>
> ---
> 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: 0x00007fff6a75e33a libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff6a81ae60 libsystem_pthread.dylib`pthread_kill + 430
    frame #2: 0x00007fff6a6e5808 libsystem_c.dylib`abort + 120
    frame #3: 0x0000000101289c36 libglib-2.0.0.dylib`g_assertion_message + 406
    frame #4: 0x0000000101289c9e libglib-2.0.0.dylib`g_assertion_message_expr + 94
    frame #5: 0x0000000100353c00 qemu-system-x86_64`type_initialize(ti=0x00000001032403e0) at object.c:314:9 [opt]
    frame #6: 0x000000010035378b qemu-system-x86_64`type_initialize(ti=0x000000010323fd70) at object.c:310:9 [opt]
    frame #7: 0x0000000100353de8 qemu-system-x86_64`object_class_foreach_tramp(key=<unavailable>, value=0x000000010323fd70, opaque=0x0000700005cb8d98) at object.c:1030:5 [opt]
    frame #8: 0x000000010124b83d libglib-2.0.0.dylib`g_hash_table_foreach + 125
    frame #9: 0x0000000100354079 qemu-system-x86_64`object_class_get_list [inlined] object_class_foreach(fn=<unavailable>, implements_type=<unavailable>, include_abstract=<unavailable>, opaque=0x0000700005cb8d90) at object.c:1052:5 [opt]
    frame #10: 0x000000010035401e qemu-system-x86_64`object_class_get_list(implements_type=<unavailable>, include_abstract=<unavailable>) at object.c:1109 [opt]
    frame #11: 0x000000010030875d qemu-system-x86_64`qemu_init [inlined] select_machine at vl.c:2438:24 [opt]
    frame #12: 0x000000010030874c qemu-system-x86_64`qemu_init(argc=<unavailable>, argv=<unavailable>, envp=<unavailable>) at vl.c:3842 [opt]
    frame #13: 0x0000000100008ef9 qemu-system-x86_64`qemu_main(argc=<unavailable>, argv=<unavailable>, envp=<unavailable>) at main.c:49:5 [opt]
    frame #14: 0x00000001000386f6 qemu-system-x86_64`call_qemu_main(opaque=<unavailable>) at cocoa.m:1710:14 [opt]
    frame #15: 0x000000010045f7ae qemu-system-x86_64`qemu_thread_start(args=<unavailable>) at qemu-thread-posix.c:521:9 [opt]
    frame #16: 0x00007fff6a81b109 libsystem_pthread.dylib`_pthread_start + 148
    frame #17: 0x00007fff6a816b8b libsystem_pthread.dylib`thread_start + 15

(lldb) f 6
frame #6: 0x000000010035378b qemu-system-x86_64`type_initialize(ti=0x000000010323fd70) at object.c:310:9 [opt]
   307
   308      parent = type_get_parent(ti);
   309      if (parent) {
-> 310          type_initialize(parent);
   311          GSList *e;
   312          int i;
   313
(lldb) p *ti
(TypeImpl) $3 = {
  name = 0x000000010323fee0 "tls-creds-anon"
  class_size = 80
  instance_size = 88
  class_init = 0x0000000100365160 (qemu-system-x86_64`qcrypto_tls_creds_anon_class_init at tlscredsanon.c:186)
  class_base_init = 0x0000000000000000
  class_data = 0x0000000000000000
  instance_init = 0x00000001003650d0 (qemu-system-x86_64`qcrypto_tls_creds_anon_init at tlscredsanon.c:199)
  instance_post_init = 0x0000000000000000
  instance_finalize = 0x00000001003650e0 (qemu-system-x86_64`qcrypto_tls_creds_anon_finalize at tlscredsanon.c:177)
  abstract = false
  parent = 0x000000010323fef0 "tls-creds"
  parent_type = 0x00000001032403e0
  class = 0x00000001032844f0
  num_interfaces = 1
  interfaces = {
    [0] = (typename = "user-creatable")
    [1] = (typename = 0x0000000000000000)
    [2] = (typename = 0x0000000000000000)
    [3] = (typename = 0x0000000000000000)
    [4] = (typename = 0x0000000000000000)
    [5] = (typename = 0x0000000000000000)
    [6] = (typename = 0x0000000000000000)
    [7] = (typename = 0x0000000000000000)
    [8] = (typename = 0x0000000000000000)
    [9] = (typename = 0x0000000000000000)
    [10] = (typename = 0x0000000000000000)
    [11] = (typename = 0x0000000000000000)
    [12] = (typename = 0x0000000000000000)
    [13] = (typename = 0x0000000000000000)
    [14] = (typename = 0x0000000000000000)
    [15] = (typename = 0x0000000000000000)
    [16] = (typename = 0x0000000000000000)
    [17] = (typename = 0x0000000000000000)
    [18] = (typename = 0x0000000000000000)
    [19] = (typename = 0x0000000000000000)
    [20] = (typename = 0x0000000000000000)
    [21] = (typename = 0x0000000000000000)
    [22] = (typename = 0x0000000000000000)
    [23] = (typename = 0x0000000000000000)
    [24] = (typename = 0x0000000000000000)
    [25] = (typename = 0x0000000000000000)
    [26] = (typename = 0x0000000000000000)
    [27] = (typename = 0x0000000000000000)
    [28] = (typename = 0x0000000000000000)
    [29] = (typename = 0x0000000000000000)
    [30] = (typename = 0x0000000000000000)
    [31] = (typename = 0x0000000000000000)
  }
}

(lldb) f 5 (NB, it's from the different run of lldb, so addresses can be
different due to KASLR)
qemu-system-x86_64 was compiled with optimization - stepping may behave oddly; variables may not be available.
frame #5: 0x0000000100353c00 qemu-system-x86_64`type_initialize(ti=0x0000000101e409d0) at object.c:314:9 [opt]
   311          GSList *e;
   312          int i;
   313
-> 314          g_assert(parent->class_size <= ti->class_size);
   315          g_assert(parent->instance_size <= ti->instance_size);
   316          memcpy(ti->class, parent->class, parent->class_size);
   317          ti->class->interfaces = NULL;
(lldb) p *ti
(TypeImpl) $0 = {
  name = 0x0000000101e40b40 "tls-creds"
  class_size = 40
  instance_size = 80
  class_init = 0x0000000100369740 (qemu-system-x86_64`qcrypto_tls_creds_class_init at tlscreds.c:229)
  class_base_init = 0x0000000000000000
  class_data = 0x0000000000000000
  instance_init = 0x00000001003696d0 (qemu-system-x86_64`qcrypto_tls_creds_init at tlscreds.c:249)
  instance_post_init = 0x0000000000000000
  instance_finalize = 0x0000000100369700 (qemu-system-x86_64`qcrypto_tls_creds_finalize at tlscreds.c:258)
  abstract = true
  parent = 0x0000000101e40b50 "object"
  parent_type = 0x0000000101e3f360
  class = 0x0000000101e84b30
  num_interfaces = 0
  interfaces = {
    [0] = (typename = 0x0000000000000000)
    [1] = (typename = 0x0000000000000000)
    [2] = (typename = 0x0000000000000000)
    [3] = (typename = 0x0000000000000000)
    [4] = (typename = 0x0000000000000000)
    [5] = (typename = 0x0000000000000000)
    [6] = (typename = 0x0000000000000000)
    [7] = (typename = 0x0000000000000000)
    [8] = (typename = 0x0000000000000000)
    [9] = (typename = 0x0000000000000000)
    [10] = (typename = 0x0000000000000000)
    [11] = (typename = 0x0000000000000000)
    [12] = (typename = 0x0000000000000000)
    [13] = (typename = 0x0000000000000000)
    [14] = (typename = 0x0000000000000000)
    [15] = (typename = 0x0000000000000000)
    [16] = (typename = 0x0000000000000000)
    [17] = (typename = 0x0000000000000000)
    [18] = (typename = 0x0000000000000000)
    [19] = (typename = 0x0000000000000000)
    [20] = (typename = 0x0000000000000000)
    [21] = (typename = 0x0000000000000000)
    [22] = (typename = 0x0000000000000000)
    [23] = (typename = 0x0000000000000000)
    [24] = (typename = 0x0000000000000000)
    [25] = (typename = 0x0000000000000000)
    [26] = (typename = 0x0000000000000000)
    [27] = (typename = 0x0000000000000000)
    [28] = (typename = 0x0000000000000000)
    [29] = (typename = 0x0000000000000000)
    [30] = (typename = 0x0000000000000000)
    [31] = (typename = 0x0000000000000000)
  }
}

It doesn't seem related to HVF QOM changes 🤔

Bisected it to:

b717702de21461138ac0e1d6775da0bd0482c013 is the first bad commit
commit b717702de21461138ac0e1d6775da0bd0482c013
Author: Daniel P. Berrangé <berrange@redhat.com>
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é <berrange@redhat.com>
    Message-Id: <20200723181410.3145233-4-berrange@redhat.com>
    [ehabkost: rebase, update to pass additional arguments to macro]
    Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
    Message-Id: <20200820001236.1284548-58-ehabkost@redhat.com>

Regards,
Roman

Re: Suspicious QOM types without instance/class size

[Eduardo Habkost]

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 <ehabkost@redhat.com>
> > ---
> > 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é <berrange@redhat.com>
> 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é <berrange@redhat.com>
>     Message-Id: <20200723181410.3145233-4-berrange@redhat.com>
>     [ehabkost: rebase, update to pass additional arguments to macro]
>     Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>     Message-Id: <20200820001236.1284548-58-ehabkost@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

[Roman Bolshakov]

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 <ehabkost@redhat.com>
> > > ---
> > > 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 <r.bolshakov@yadro.com>
Tested-By: Roman Bolshakov <r.bolshakov@yadro.com>

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

[Eduardo Habkost]

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 <ehabkost@redhat.com>
> > > > ---
> > > > 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 <r.bolshakov@yadro.com>
> Tested-By: Roman Bolshakov <r.bolshakov@yadro.com>
> 
> 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