Re: [Qemu-devel] [PATCH for-3.2 v4 16/28] hw: apply machine compat properties without touching globals

[Eduardo Habkost]

On Tue, Dec 04, 2018 at 02:17:24PM +0100, Igor Mammedov wrote:
> On Fri, 30 Nov 2018 09:41:42 -0200
> Eduardo Habkost <address@hidden> wrote:
> 
> > On Fri, Nov 30, 2018 at 11:55:26AM +0100, Igor Mammedov wrote:
> > > On Fri, 30 Nov 2018 01:36:03 +0400
> > > Marc-André Lureau <address@hidden> wrote:
> > >   
> > > > Hi
> > > > 
> > > > On Thu, Nov 29, 2018 at 9:51 PM Eduardo Habkost <address@hidden> wrote: 
> > > >  
> > > > >
> > > > > On Thu, Nov 29, 2018 at 02:32:18PM +0400, Marc-André Lureau wrote:    
> > > > > > Hi
> > > > > > On Wed, Nov 28, 2018 at 9:53 PM Igor Mammedov <address@hidden> 
> > > > > > wrote:    
> > > > > > >
> > > > > > > On Tue, 27 Nov 2018 11:35:27 -0200
> > > > > > > Eduardo Habkost <address@hidden> wrote:
> > > > > > >    
> > > > > > > > On Tue, Nov 27, 2018 at 05:10:05PM +0400, Marc-André Lureau 
> > > > > > > > wrote:    
> > > > > > > > > On Tue, Nov 27, 2018 at 4:57 PM Eduardo Habkost 
> > > > > > > > > <address@hidden> wrote:    
> > > > > > > > > >
> > > > > > > > > > On Tue, Nov 27, 2018 at 01:27:49PM +0400, Marc-André Lureau 
> > > > > > > > > > wrote:    
> > > > > > > > > > > Similarly to accel properties, move compat properties out 
> > > > > > > > > > > of globals
> > > > > > > > > > > registration, and apply the machine compat properties 
> > > > > > > > > > > during
> > > > > > > > > > > device_post_init().
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Marc-André Lureau <address@hidden>    
> > > > > > > > > > [...]    
> > > > > > > > > > > diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> > > > > > > > > > > index 7066d28271..3b31b2c025 100644
> > > > > > > > > > > --- a/hw/core/qdev.c
> > > > > > > > > > > +++ b/hw/core/qdev.c
> > > > > > > > > > > @@ -971,17 +971,26 @@ static void device_initfn(Object 
> > > > > > > > > > > *obj)
> > > > > > > > > > >  }
> > > > > > > > > > >
> > > > > > > > > > >  static const GPtrArray *ac_compat_props;
> > > > > > > > > > > +static const GPtrArray *mc_compat_props;    
> > > > > > > why you didn't use just 'compat_props' for both?
> > > > > > > (it would be cleaner have single registry for compat
> > > > > > > properties, and the place that takes care of registration
> > > > > > > will take care of necessary ordering)    
> > > > > >
> > > > > > There are two arrays, one from the accelerator class, the other from
> > > > > > the machine class. We can't make it a singleton (all compats props 
> > > > > > for
> > > > > > the various machines would be mixed).
> > > > > >
> > > > > > We could create a third array that would be the set of both, but 
> > > > > > that
> > > > > > would require more copy/allocation.    
> > > > >
> > > > > I am failing to see the advantage of replacing the `global_props`
> > > > > static variable from qdev-properties.c with a collection of
> > > > > separate static variables scattered around the code.  I thought
> > > > > the main point of the changes was to reduce the amount of
> > > > > duplicate data stored in static variables.  
> > > Main point was to use compats for backends then on top of that 
> > > we added split global from compat properties goal.
> > >   
> > > > > I was expecting something like this:
> > > > >
> > > > > accel.c:
> > > > >
> > > > >   void accel_apply_compat_props(AccelState *accel, Object *obj)
> > > > >   {
> > > > >       object_apply_global_props(obj, 
> > > > > ACCEL_GET_CLASS(accel)->compat_props, &error_abort);
> > > > >   }
> > > > >
> > > > > machine.c:
> > > > >
> > > > >   /* Apply compat properties and global properties to an object */
> > > > >   void machine_apply_compat_props(MachineState *ms, Object *obj)
> > > > >   {
> > > > >       accel_apply_compat_props(ms->accel, obj);
> > > > >       object_apply_global_props(obj, 
> > > > > MACHINE_GET_CLASS(ms)->compat_props, &error_abort);
> > > > >   }
> > > > >
> > > > > compat-props.c:
> > > > >
> > > > >   static void object_apply_compat_props(Object *obj)
> > > > >   {
> > > > >       MachineState *machine = MACHINE(qdev_get_machine());
> > > > >       machine_apply_compat_props(machine, obj);
> > > > >   }
> > > > >
> > > > > qdev.c:
> > > > >
> > > > >   static void device_post_init(Object *obj)
> > > > >   {
> > > > >       object_apply_compat_props(obj);
> > > > >       apply_user_provided_globals(obj);
> > > > >   }
> > > > >
> > > > > object_interface.c:
> > > > >
> > > > >   void user_creatable_complete(Object *obj, Error **errp)
> > > > >   {
> > > > >       object_apply_compat_props(obj);
> > > > >       ...
> > > > >       ucc->complete(...)
> > > > >   }
> > > > >    
> > > > 
> > > > I like that solution too (which you also proposed in the other
> > > > thread). But we have to decide whether it's acceptable to reference
> > > > MachineState, or if the compat properties should be registered.  
> > > I dislike pulling in machine into basic object code and
> > > I think a separate compats would be cleaner interface without
> > > layer violation. But to unstuck, lets go with qdev_get_machine()
> > > for now.  
> > 
> > Which basic object code?  The only reference to machine above is
> > at compat-props.c.  I don't see any layering violation here.
> I see compat properties as just a set of arbitrary properties
> that's applied to objects depending on configuration.
> Which currently includes machine version and accel type
> but in no way inherently tied to them.

I understand you want to have a more generic abstraction layer,
but my question is: why do we need it?  I still don't see which
problems it would solve.

> 
> As for layering violation, we do it in device model cases
> as it's machine related (ugly 'acceptable' but helps to cut corners)
> and with new interface it will be used wit backends which are not
> machine related at all.

I don't see why it's ugly to have QEMU backend object code use
machine core code.  What problems does it cause?


> 
> Doing like you suggest would make it usable with current machine
> initialization order (with some fixes) but if one would consider
> a bit earlier time (preconfig time) and creating machine step by
> step from there it might be less usable (as it needs machine instance,
> it still would be possible but probably more tricky (pure speculation)).

I agree that this is a problem: we really have a
backend -> compat_props -> current_machine dependency chain here.

Using a registration mechanism that copies data, the dependency
would still exists, but be implicit in the initialization
ordering.  Then the system would silently break as soon as we
reorder the code.

> 
> So I'd vote for abstracted from machine compat props that could be
> called from any object that implements interface and explicit
> compats registration (like we do now) and explicit registration failure
> if compats happend to be applied to any object. But in case of this
> series I'm fine with any approach just to get compat properties work
> with backends for now.

Thanks.  I agree we'll want to refactor this later.  We may want
to be able to apply compat properties to backend objects before
creating machine and accel instances.

Or maybe we will want to apply only MachineClass::compat_props to
backend objects, and not AccelClass::compat_props?  This is not
clear to me yet.

-- 
Eduardo