[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] hw/sysbus.h: New sysbus_init_child() helper
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] hw/sysbus.h: New sysbus_init_child() helper function |
Date: |
Mon, 26 Mar 2018 14:41:02 +0200 |
On Sat, 24 Mar 2018 12:35:22 -0300
Philippe Mathieu-Daudé <address@hidden> wrote:
> Hi,
>
> On 02/20/2018 10:23 AM, Igor Mammedov wrote:
> > On Tue, 20 Feb 2018 13:13:49 +0100
> > Paolo Bonzini <address@hidden> 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.