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