Re: [Qemu-devel] [PATCH 1/2] hw/sysbus.h: New sysbus_init_child() helper function

[Igor Mammedov <imammedo@redhat.com>]

On Sat, 24 Mar 2018 12:35:22 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> Hi,
> 
> On 02/20/2018 10:23 AM, Igor Mammedov wrote:
> > On Tue, 20 Feb 2018 13:13:49 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >   
> >> On 16/02/2018 18:40, Philippe Mathieu-Daudé wrote:  
> >>> we can keep object_initialize() when no parent,
> >>> and add object_initialize_child(, const char *childname, Object *parent)
> >>> 'parent' last because all previous args are child-related.
> >>>     
> >>>>    
> >>>>> +    qdev_set_parent_bus(DEVICE(child), sysbus_get_default());    
> >>>> and then assuming we don't create sysbus devices, nor should be able to,
> >>>> which are not attached to sysbus_get_default() this one can go sysbus' instance_init()    
> >>> good idea.
> >>>     
> > [...]
> >   
> >> I'm not sure about moving qdev_set_parent_bus to instance_init().  It
> >> would be a bit different from other buses and possibly confusing.  
> > That's what this series sort of does, i.e. creating a type/class
> > specific helper(s). Which becomes confusing as number of helpers
> > increases (frankly it's just 2 different approaches to code i.e.
> > functional vs OOM).
> > 
> > It could be better to keep single QOM API and let SYSBUS base class
> > instance_init() to do all implicit initialization that must be done
> > for inherited classes.  
> 
> What is the consensus on this series once 2.12 gets released?
At least we should add and make current code use it
  object_initialize_child()
It should save quite a bit of code in ARM target

As for
  qdev_set_parent_bus(DEVICE, sysbus_get_default())
it is a separate issue, but I'd still go with
TYPE_SYS_BUS_DEVICE setting bus in its instance_init so that
inherited types nor their users would have to deal with it.

> > Yes, it will be different from other devices with buses but
> > users won't really care (there is no other buss to assign to),
> > for them it will look like a typical bus-less device and it
> > would be less error-prone to code.
> > 
> >    
> >> Potentially there could be a "hierarchy" of *_initialize_child
> >> functions, adding or removing arguments as needed for the specific kind
> >> of bus:
> >>
> >> 	/* adds qdev_set_parent_bus */
> >> 	device_initialize_child(Object *parent, const char *childname,
> >> 				BusState *bus, void *data, size_t size,
> >> 				const char *typename);
> >> 	/* uses sysbus_get_default() */
> >> 	sysbus_initialize_child(Object *parent, const char *childname,
> >> 				void *data, size_t size,
> >> 				const char *typename);  
> > 
> >   
> >> 	/* initializes "addr" property too */
> >> 	pci_initialize_child(Object *parent, const char *childname,
> >> 			     BusState *bus, int addr,
> >> 			     void *data, size_t size,
> >> 			     const char *typename);  
> > PCI could also incorporate PCI specific bus setting/
> > verification logic inside of base class.
> > 
> > It could allow us to drop bus assigning magic in qdev_device_add(),
> > bringing it closer to simple QOM object handling, with specifics
> > abstracted by respective TYPE implementations.
> > Maybe we would be able to unify -device with -object in the end.  
> 
> Thanks,
> 
> Phil.