| Message ID | 1334238263-28572-1-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
Am 12.04.2012 16:47, schrieb Anthony Liguori: > On 04/12/2012 08:44 AM, Andreas Färber wrote: >> Wrap setting of Object::realized property, error reporting and exit(1) >> into a helper function. It is the equivalent of qdev_init_nofail(). > > I don't like this. > > If for no reason other than, a much more specific justification is > needed for this. I absolutely don't want to repeat the error handling > mistakes of qdev. I would rather we refactor all of the users of > qdev_init_nofail() to propagate errors. I didn't get around to replying to your other mail yet: The big clash that Paolo and I had turned out to stem from tackling two virtually inconsolable goals, both under the banner of "realize": 1) Me and Peter need a way to do two-stage construction of non-qdev objects. Inlining the below code into lots of machine init functions is a really bad suggestion IMO. 2) Paolo is working towards the model envisioned by you where only a single realize call is made in vl.c and propagates to all children. I don't see 2) working at this stage due to the very simple fact that we create objects in the second stage depending on properties. The one-realize model is also incompatible with the static properties concept that Paolo is moving into object in this series - we'd need to make them dynamic properties that actually do something when set (e.g., modify the offset of a particular child MemoryRegion). So unless all devices are reworked to fit into the envisioned realize model, we will need some function like this. Current pressing use cases are: i) Moving vcpu_init() from cpu_*_init() into a realizefn. ii) Instantiating an SoC container object with varying CPU. iii) Moving ARM feature inference into a realizefn. Andreas
Am 12.04.2012 16:59, schrieb Paolo Bonzini: > Il 12/04/2012 16:47, Anthony Liguori ha scritto: >> >>> Wrap setting of Object::realized property, error reporting and exit(1) >>> into a helper function. It is the equivalent of qdev_init_nofail(). >> >> I don't like this. >> >> If for no reason other than, a much more specific justification is >> needed for this. I absolutely don't want to repeat the error handling >> mistakes of qdev. I would rather we refactor all of the users of >> qdev_init_nofail() to propagate errors. > > I agree about this in general, but for a different reason. There should be > only one call to object_realize_nofail, in vl.c, which might as well be > inlined---I'll include it in my series. All calls to qdev_init_nofail > and qdev_init should disappear from boards that are properly converted > to QOM. We had talked about this on IRC and found that it won't work for selecting the type of an object. I can work around it in the sh4 case but the problem stays. Andreas
On 04/12/2012 08:44 AM, Andreas Färber wrote: > Wrap setting of Object::realized property, error reporting and exit(1) > into a helper function. It is the equivalent of qdev_init_nofail(). I don't like this. If for no reason other than, a much more specific justification is needed for this. I absolutely don't want to repeat the error handling mistakes of qdev. I would rather we refactor all of the users of qdev_init_nofail() to propagate errors. Regards, Anthony Liguori > > Signed-off-by: Andreas Färber<afaerber@suse.de> > Cc: Anthony Liguori<anthony@codemonkey.ws> > Cc: Paolo Bonzini<pbonzini@redhat.com> > Cc: Peter Maydell<peter.maydell@linaro.org> > --- > Hi Paolo, please consider appending this patch to your push-push series. > It then provides equivalent second-stage init functionality for non-qdev > objects, as desired for ARMCPU and SH7750 SoC. > > include/qemu/object.h | 10 ++++++++++ > qom/object.c | 11 +++++++++++ > 2 files changed, 21 insertions(+), 0 deletions(-) > > diff --git a/include/qemu/object.h b/include/qemu/object.h > index 360181d..524d3b0 100644 > --- a/include/qemu/object.h > +++ b/include/qemu/object.h > @@ -504,6 +504,16 @@ void object_initialize_with_type(void *data, Type type); > void object_initialize(void *obj, const char *typename); > > /** > + * object_realize_nofail: > + * @obj: The object to realize. > + * > + * This function will complete the initialization of an object based on > + * properties set by setting the "realized" property to true. > + * Like #qdev_init_nofail it will terminate the process in case of errors. > + */ > +void object_realize_nofail(Object *obj); > + > +/** > * object_finalize: > * @obj: The object to finalize. > * > diff --git a/qom/object.c b/qom/object.c > index f4f29ab..1ef0b9b 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -381,6 +381,17 @@ void object_initialize(void *data, const char *typename) > object_initialize_with_type(data, type); > } > > +void object_realize_nofail(Object *obj) > +{ > + Error *err = NULL; > + > + object_property_set_bool(obj, true, "realized",&err); > + if (error_is_set(&err)) { > + qerror_report_err(err); > + exit(1); > + } > +} > + > static void object_property_del_all(Object *obj) > { > while (!QTAILQ_EMPTY(&obj->properties)) {
Am 12.04.2012 17:43, schrieb Anthony Liguori: > On 04/12/2012 10:15 AM, Paolo Bonzini wrote: >> Il 12/04/2012 15:58, Andreas Färber ha scritto: >>> The big clash that Paolo and I had turned out to stem from tackling two >>> virtually inconsolable goals, both under the banner of "realize": >> >> I think the code was similar enough that the goals are not >> unreconcilable (though QOM goals can certainly become sad if they aren't >> achieved in a few years :). >> >>> 1) Me and Peter need a way to do two-stage construction of non-qdev >>> objects. Inlining the below code into lots of machine init functions is >>> a really bad suggestion IMO. >> >> FWIW, I agree. > > I don't think machines should be objects. Don't know how you got there suddenly? I disagree. In my mind machines can be replaced by mainboard objects derived from "board" derived from "container" derived from object, instantiating a set of child objects. We already have /machine be a container, so moving code to an initfn rather than having the slightly similar machine registration and initialization mechanism would be a code consolidation. The thinking that I had to postpone to a 1.2 now is how to handle default vs. -M machine instantiation and setting of properties / childs on that object (the kernel / bootloader ABI discussion). > Chipsets should be QOM objects. What the machines currently do does not > map well to modeling as an object. I'd say: What qdev devices currently do does not map well to modelling as a QOM object with realize support. IMO that has little to do with the machines being the main users of these devices today. Andreas
Il 12/04/2012 16:47, Anthony Liguori ha scritto: > >> Wrap setting of Object::realized property, error reporting and exit(1) >> into a helper function. It is the equivalent of qdev_init_nofail(). > > I don't like this. > > If for no reason other than, a much more specific justification is > needed for this. I absolutely don't want to repeat the error handling > mistakes of qdev. I would rather we refactor all of the users of > qdev_init_nofail() to propagate errors. I agree about this in general, but for a different reason. There should be only one call to object_realize_nofail, in vl.c, which might as well be inlined---I'll include it in my series. All calls to qdev_init_nofail and qdev_init should disappear from boards that are properly converted to QOM. Next on my QOM list (for 1.2) is a scripted refactoring to ISADevice *rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq); to ISADevice *rtc_new(ISABus *bus, int base_year, qemu_irq intercept_irq); static ISADevice *rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq) { ISADevice *dev = rtc_new(bus, base_year, intercept_irq); qdev_init_nofail(dev); return dev; } Paolo
Il 12/04/2012 15:58, Andreas Färber ha scritto: > The big clash that Paolo and I had turned out to stem from tackling two > virtually inconsolable goals, both under the banner of "realize": I think the code was similar enough that the goals are not unreconcilable (though QOM goals can certainly become sad if they aren't achieved in a few years :). > 1) Me and Peter need a way to do two-stage construction of non-qdev > objects. Inlining the below code into lots of machine init functions is > a really bad suggestion IMO. FWIW, I agree. > I don't see 2) working at this stage due to the very simple fact that we > create objects in the second stage depending on properties. Do you have examples? As long as the objects are created in the realize method itself you can do that. > The one-realize model is also incompatible with the static properties > concept that Paolo is moving into object in this series - we'd need to > make them dynamic properties that actually do something when set (e.g., > modify the offset of a particular child MemoryRegion). The child MemoryRegion could be created at realize time as long as the composition tree hierarchy is correct. A child's MR can use a MR property that the parent creates at realize time, because the parent is always realized first and unrealized last. Some properties however might need to become dynamic, for example a SOC's number_of_uarts property would create children before realization. > i) Moving vcpu_init() from cpu_*_init() into a realizefn. This is compatible with realize-once-at-machine-creation. > ii) Instantiating an SoC container object with varying CPU. This seems similar to the MemoryRegion case. > iii) Moving ARM feature inference into a realizefn. I don't know anything at all about this. :) If you need it as a stopgap measure, you could have cpu_realize_nofail in qom/cpu.h. I wouldn't at all oppose to that. But it shouldn't be encouraged, IMO. Paolo
On 04/12/2012 09:04 AM, Andreas Färber wrote: > Am 12.04.2012 16:59, schrieb Paolo Bonzini: >> Il 12/04/2012 16:47, Anthony Liguori ha scritto: >>> >>>> Wrap setting of Object::realized property, error reporting and exit(1) >>>> into a helper function. It is the equivalent of qdev_init_nofail(). >>> >>> I don't like this. >>> >>> If for no reason other than, a much more specific justification is >>> needed for this. I absolutely don't want to repeat the error handling >>> mistakes of qdev. I would rather we refactor all of the users of >>> qdev_init_nofail() to propagate errors. >> >> I agree about this in general, but for a different reason. There should be >> only one call to object_realize_nofail, in vl.c, which might as well be >> inlined---I'll include it in my series. All calls to qdev_init_nofail >> and qdev_init should disappear from boards that are properly converted >> to QOM. > > We had talked about this on IRC and found that it won't work for > selecting the type of an object. Can you be more specific? Selecting the type of an object should be done by having a link<> property and letting the user create an object and setup the link. Properties should not be used to magically fill in link<> properties. Regards, Anthony Liguori > > I can work around it in the sh4 case but the problem stays. > > Andreas >
On 04/12/2012 10:15 AM, Paolo Bonzini wrote: > Il 12/04/2012 15:58, Andreas Färber ha scritto: >> The big clash that Paolo and I had turned out to stem from tackling two >> virtually inconsolable goals, both under the banner of "realize": > > I think the code was similar enough that the goals are not > unreconcilable (though QOM goals can certainly become sad if they aren't > achieved in a few years :). > >> 1) Me and Peter need a way to do two-stage construction of non-qdev >> objects. Inlining the below code into lots of machine init functions is >> a really bad suggestion IMO. > > FWIW, I agree. I don't think machines should be objects. Chipsets should be QOM objects. What the machines currently do does not map well to modeling as an object. Regards, Anthony Liguori
On 12 April 2012 16:43, Anthony Liguori <anthony@codemonkey.ws> wrote: > I don't think machines should be objects. > > Chipsets should be QOM objects. What the machines currently do does not map > well to modeling as an object. Why would you design an infrastructure that lets you coherently bundle together a collection of devices and have configurable properties on that bundle as well as on the devices, and then *not* use it for machines? To me it just seems obvious that you'd make machine models be a single top level QOM object. -- PMM
On 04/12/2012 10:52 AM, Peter Maydell wrote: > On 12 April 2012 16:43, Anthony Liguori<anthony@codemonkey.ws> wrote: >> I don't think machines should be objects. >> >> Chipsets should be QOM objects. What the machines currently do does not map >> well to modeling as an object. > > Why would you design an infrastructure that lets you coherently bundle > together a collection of devices and have configurable properties on > that bundle as well as on the devices, and then *not* use it for machines? The machine concept in QEMU is "broken" IMHO. If we want to maintain compatibility (and we do), we need to let machines act as a bridge. Here's how I expect the PC to work: qemu --no-machine -readconfig my-system.cfg [device "root"] driver=i440fx cpu[0]=cpu0 slot[3]=e1000 memory=2G biosname=bios.bin [device "cpu0"] driver=qemu64 [device "e1000"] bus=/i440fx netdev=eth0 [netdev "eth0"] type=tap ... There is no real need to have a '-machine' option and no need to model a machine. -M pc ends up being a compatibility bridge which takes a bunch of options that really do lots of different things (like choosing network device models). I see machines as a function that takes a QemuOpts and then does the equivalent of the above. Regards, Anthony Liguori
On 12 April 2012 17:02, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 04/12/2012 10:52 AM, Peter Maydell wrote: >> Why would you design an infrastructure that lets you coherently bundle >> together a collection of devices and have configurable properties on >> that bundle as well as on the devices, and then *not* use it for machines? > > > The machine concept in QEMU is "broken" IMHO. If we want to maintain > compatibility (and we do), we need to let machines act as a bridge. > > Here's how I expect the PC to work: > > qemu --no-machine -readconfig my-system.cfg > > [device "root"] > driver=i440fx > cpu[0]=cpu0 > slot[3]=e1000 > memory=2G > biosname=bios.bin > > [device "cpu0"] > driver=qemu64 > > [device "e1000"] > bus=/i440fx > netdev=eth0 > > [netdev "eth0"] > type=tap Isn't this just defining a machine in a config file without naming it? > There is no real need to have a '-machine' option and no need to model a > machine. This doesn't make sense to me. We need a -machine option because it's the major way for the user to say what kind of model they want. We need to model machines because without that we just have a pile of useless devices. > -M pc ends up being a compatibility bridge which takes a bunch of options > that really do lots of different things (like choosing network device > models). I see machines as a function that takes a QemuOpts and then does > the equivalent of the above. -- PMM
Il 12/04/2012 18:09, Peter Maydell ha scritto: > On 12 April 2012 17:02, Anthony Liguori <anthony@codemonkey.ws> wrote: >> On 04/12/2012 10:52 AM, Peter Maydell wrote: >>> Why would you design an infrastructure that lets you coherently bundle >>> together a collection of devices and have configurable properties on >>> that bundle as well as on the devices, and then *not* use it for machines? >> >> >> The machine concept in QEMU is "broken" IMHO. If we want to maintain >> compatibility (and we do), we need to let machines act as a bridge. >> >> Here's how I expect the PC to work: >> >> qemu --no-machine -readconfig my-system.cfg >> >> [device "root"] >> driver=i440fx >> cpu[0]=cpu0 >> slot[3]=e1000 >> memory=2G >> biosname=bios.bin >> >> [device "cpu0"] >> driver=qemu64 >> >> [device "e1000"] >> bus=/i440fx >> netdev=eth0 >> >> [netdev "eth0"] >> type=tap > > Isn't this just defining a machine in a config file without > naming it? > >> There is no real need to have a '-machine' option and no need to model a >> machine. > > This doesn't make sense to me. We need a -machine option because it's > the major way for the user to say what kind of model they want. I think that's a difference between "PC" and "SoC" views that is difficult to reconcile... I think what Anthony is saying makes a lot of sense, and there's probably a way to make it work for SoCs too with some changes. However there's no need to be so Draconian, we know that we'll never get even close for most boards... Paolo
On 04/12/2012 11:09 AM, Peter Maydell wrote: > On 12 April 2012 17:02, Anthony Liguori<anthony@codemonkey.ws> wrote: >> On 04/12/2012 10:52 AM, Peter Maydell wrote: >>> Why would you design an infrastructure that lets you coherently bundle >>> together a collection of devices and have configurable properties on >>> that bundle as well as on the devices, and then *not* use it for machines? >> >> >> The machine concept in QEMU is "broken" IMHO. If we want to maintain >> compatibility (and we do), we need to let machines act as a bridge. >> >> Here's how I expect the PC to work: >> >> qemu --no-machine -readconfig my-system.cfg >> >> [device "root"] >> driver=i440fx >> cpu[0]=cpu0 >> slot[3]=e1000 >> memory=2G >> biosname=bios.bin >> >> [device "cpu0"] >> driver=qemu64 >> >> [device "e1000"] >> bus=/i440fx >> netdev=eth0 >> >> [netdev "eth0"] >> type=tap > > Isn't this just defining a machine in a config file without > naming it? Yup. And what we think of as "machines' should basically just be doing the same thing a config file does. It doesn't need to be part of the object model. >> There is no real need to have a '-machine' option and no need to model a >> machine. > > This doesn't make sense to me. We need a -machine option because it's > the major way for the user to say what kind of model they want. We need > to model machines because without that we just have a pile of useless > devices. Machine wouldn't go away. The point I'm making is that -machine doesn't have to be an alias of a single -device option. Regards, Anthony Liguori >> -M pc ends up being a compatibility bridge which takes a bunch of options >> that really do lots of different things (like choosing network device >> models). I see machines as a function that takes a QemuOpts and then does >> the equivalent of the above. > > -- PMM
Am 12.04.2012 17:41, schrieb Anthony Liguori: > On 04/12/2012 09:04 AM, Andreas Färber wrote: >> Am 12.04.2012 16:59, schrieb Paolo Bonzini: >>> Il 12/04/2012 16:47, Anthony Liguori ha scritto: >>>> >>>>> Wrap setting of Object::realized property, error reporting and exit(1) >>>>> into a helper function. It is the equivalent of qdev_init_nofail(). >>>> >>>> I don't like this. >>>> >>>> If for no reason other than, a much more specific justification is >>>> needed for this. I absolutely don't want to repeat the error handling >>>> mistakes of qdev. I would rather we refactor all of the users of >>>> qdev_init_nofail() to propagate errors. >>> >>> I agree about this in general, but for a different reason. There >>> should be >>> only one call to object_realize_nofail, in vl.c, which might as well be >>> inlined---I'll include it in my series. All calls to qdev_init_nofail >>> and qdev_init should disappear from boards that are properly converted >>> to QOM. >> >> We had talked about this on IRC and found that it won't work for >> selecting the type of an object. > > Can you be more specific? > > Selecting the type of an object should be done by having a link<> > property and letting the user create an object and setup the link. No, that's not what a link does. A link<> property lets the user associate one instance, not a type that can be used for multiple instances. We'd need a template<> or clone<> mechanism for that! For example, the user specifies -cpu cortex-a9,+neon,-fpu and we have a dual-core exynos4210 SoC. I originally had a patch on my qom-cpu-sh4 v1 series where exactly such a link got criticized by Peter: http://patchwork.ozlabs.org/patch/146669/ And I agree, it was an ugly workaround around the missing two-stage constructor support. Therefore I had posted my object_realize() series! https://github.com/afaerber/qemu-cpu/commit/32e4dd701694fffcf312d111c79eedad243be2b3 Andreas
Il 12/04/2012 18:52, Andreas Färber ha scritto: >> Selecting the type of an object should be done by having a link<> >> property and letting the user create an object and setup the link. > > No, that's not what a link does. A link<> property lets the user > associate one instance, not a type that can be used for multiple > instances. We'd need a template<> or clone<> mechanism for that! > > For example, the user specifies -cpu cortex-a9,+neon,-fpu and we have a > dual-core exynos4210 SoC. > > I originally had a patch on my qom-cpu-sh4 v1 series where exactly such > a link got criticized by Peter: > > http://patchwork.ozlabs.org/patch/146669/ > > And I agree, it was an ugly workaround around the missing two-stage > constructor support. Therefore I had posted my object_realize() series! I think this is difficult to reconcile for now. I can see Anthony's point, which makes a lot of sense from the PC point of view, but then "magic" configuration can also be important when working with SoC boards. I've added a missing_property hook to my series that can be used to this end. Accessing a property can trigger creation of the device with the appropriate class before realization. Paolo
On 04/12/2012 12:02 PM, Paolo Bonzini wrote: > Il 12/04/2012 18:52, Andreas Färber ha scritto: >>> Selecting the type of an object should be done by having a link<> >>> property and letting the user create an object and setup the link. >> >> No, that's not what a link does. A link<> property lets the user >> associate one instance, not a type that can be used for multiple >> instances. We'd need a template<> or clone<> mechanism for that! >> >> For example, the user specifies -cpu cortex-a9,+neon,-fpu and we have a >> dual-core exynos4210 SoC. >> >> I originally had a patch on my qom-cpu-sh4 v1 series where exactly such >> a link got criticized by Peter: >> >> http://patchwork.ozlabs.org/patch/146669/ >> >> And I agree, it was an ugly workaround around the missing two-stage >> constructor support. Therefore I had posted my object_realize() series! > > I think this is difficult to reconcile for now. I can see Anthony's > point, which makes a lot of sense from the PC point of view, but then > "magic" configuration can also be important when working with SoC boards. I don't see such "magic" configurations. Either a SoC has a chip that's soldered to the board and cannot be changed (it's a child<>) or the SoC has a socket and you can add in a different component (it's a link<>). Where does the "magic" come in? > > I've added a missing_property hook to my series that can be used to this > end. Accessing a property can trigger creation of the device with the > appropriate class before realization. I think we need to stick close to something that looks like it could be a real piece of hardware. Once you add too much magic, it's too easy to do things the wrong way. Regards, Anthony Liguori > Paolo
On 04/12/2012 11:52 AM, Andreas Färber wrote: > Am 12.04.2012 17:41, schrieb Anthony Liguori: >> On 04/12/2012 09:04 AM, Andreas Färber wrote: >>> Am 12.04.2012 16:59, schrieb Paolo Bonzini: >>>> Il 12/04/2012 16:47, Anthony Liguori ha scritto: >>>>> >>>>>> Wrap setting of Object::realized property, error reporting and exit(1) >>>>>> into a helper function. It is the equivalent of qdev_init_nofail(). >>>>> >>>>> I don't like this. >>>>> >>>>> If for no reason other than, a much more specific justification is >>>>> needed for this. I absolutely don't want to repeat the error handling >>>>> mistakes of qdev. I would rather we refactor all of the users of >>>>> qdev_init_nofail() to propagate errors. >>>> >>>> I agree about this in general, but for a different reason. There >>>> should be >>>> only one call to object_realize_nofail, in vl.c, which might as well be >>>> inlined---I'll include it in my series. All calls to qdev_init_nofail >>>> and qdev_init should disappear from boards that are properly converted >>>> to QOM. >>> >>> We had talked about this on IRC and found that it won't work for >>> selecting the type of an object. >> >> Can you be more specific? >> >> Selecting the type of an object should be done by having a link<> >> property and letting the user create an object and setup the link. > > No, that's not what a link does. A link<> property lets the user > associate one instance, not a type that can be used for multiple > instances. We'd need a template<> or clone<> mechanism for that! No, you misunderstood. A link allows a user to create an object and make an associate. If you want to let a user choose a type, just have them make an object. Since you need no parameters to create an object, there's no advantage of telling something about a type name vs. giving it an object of that type. Regards, Anthony Liguori > > For example, the user specifies -cpu cortex-a9,+neon,-fpu and we have a > dual-core exynos4210 SoC. > > I originally had a patch on my qom-cpu-sh4 v1 series where exactly such > a link got criticized by Peter: > > http://patchwork.ozlabs.org/patch/146669/ > > And I agree, it was an ugly workaround around the missing two-stage > constructor support. Therefore I had posted my object_realize() series! > > https://github.com/afaerber/qemu-cpu/commit/32e4dd701694fffcf312d111c79eedad243be2b3 > > Andreas >
Am 12.04.2012 21:59, schrieb Anthony Liguori: > On 04/12/2012 11:52 AM, Andreas Färber wrote: >> Am 12.04.2012 17:41, schrieb Anthony Liguori: >>> On 04/12/2012 09:04 AM, Andreas Färber wrote: >>>> Am 12.04.2012 16:59, schrieb Paolo Bonzini: >>>>> Il 12/04/2012 16:47, Anthony Liguori ha scritto: >>>>>> >>>>>>> Wrap setting of Object::realized property, error reporting and >>>>>>> exit(1) >>>>>>> into a helper function. It is the equivalent of qdev_init_nofail(). >>>>>> >>>>>> I don't like this. >>>>>> >>>>>> If for no reason other than, a much more specific justification is >>>>>> needed for this. I absolutely don't want to repeat the error >>>>>> handling >>>>>> mistakes of qdev. I would rather we refactor all of the users of >>>>>> qdev_init_nofail() to propagate errors. >>>>> >>>>> I agree about this in general, but for a different reason. There >>>>> should be >>>>> only one call to object_realize_nofail, in vl.c, which might as >>>>> well be >>>>> inlined---I'll include it in my series. All calls to qdev_init_nofail >>>>> and qdev_init should disappear from boards that are properly converted >>>>> to QOM. >>>> >>>> We had talked about this on IRC and found that it won't work for >>>> selecting the type of an object. >>> >>> Can you be more specific? >>> >>> Selecting the type of an object should be done by having a link<> >>> property and letting the user create an object and setup the link. >> >> No, that's not what a link does. A link<> property lets the user >> associate one instance, not a type that can be used for multiple >> instances. We'd need a template<> or clone<> mechanism for that! > > No, you misunderstood. A link allows a user to create an object and > make an associate. If you want to let a user choose a type, just have > them make an object. Since you need no parameters to create an object, > there's no advantage of telling something about a type name vs. giving > it an object of that type. You're missing the point: You are advocating putting all configuration into a config file, having the user call --readconfig /path/to/board.conf and either fiddling with the config file or fiddling with (looping) QMP scripts to apply machine-wide changes. By contrast I am talking about backwards-compatible convenience options that influence more than one object in such a machine, wherever that hierarchy is being built up. Reality with SysBus is multi-stage constructors: A = qdev_create() A.a = x qdev_init_nofail(A) -> A_initfn() -> B = qdev_create() B.b = y qdev_init_nofail(B) -> B_initfn() ... while you are dreaming of a flat hierarchy: A = qdev_create() B = qdev_create() ... A.a = x B.b = y ... object_property_set_bool(qdev_get_machine(), "realized", true) But getting there requires more than denying me object_realize(), it requires rewriting all qdev devices *first*, which I do not see Paolo doing. You started for i440fx but did not finish either. Calling realize recursively shouldn't hurt us if realizing an already realized object is no-op and if the child iteration can cope with a growing child list for the current parent and any node in /machine. Meaning we can't just call one inlined object_realize_nofail() but we'd need to loop doing that until no node in the subgraph is left that is not realized. Andreas
On 04/12/2012 04:08 PM, Andreas Färber wrote: > Am 12.04.2012 21:59, schrieb Anthony Liguori: >> On 04/12/2012 11:52 AM, Andreas Färber wrote: >>> Am 12.04.2012 17:41, schrieb Anthony Liguori: >>>> On 04/12/2012 09:04 AM, Andreas Färber wrote: >>>>> Am 12.04.2012 16:59, schrieb Paolo Bonzini: >>>>>> Il 12/04/2012 16:47, Anthony Liguori ha scritto: >>>>>>> >>>>>>>> Wrap setting of Object::realized property, error reporting and >>>>>>>> exit(1) >>>>>>>> into a helper function. It is the equivalent of qdev_init_nofail(). >>>>>>> >>>>>>> I don't like this. >>>>>>> >>>>>>> If for no reason other than, a much more specific justification is >>>>>>> needed for this. I absolutely don't want to repeat the error >>>>>>> handling >>>>>>> mistakes of qdev. I would rather we refactor all of the users of >>>>>>> qdev_init_nofail() to propagate errors. >>>>>> >>>>>> I agree about this in general, but for a different reason. There >>>>>> should be >>>>>> only one call to object_realize_nofail, in vl.c, which might as >>>>>> well be >>>>>> inlined---I'll include it in my series. All calls to qdev_init_nofail >>>>>> and qdev_init should disappear from boards that are properly converted >>>>>> to QOM. >>>>> >>>>> We had talked about this on IRC and found that it won't work for >>>>> selecting the type of an object. >>>> >>>> Can you be more specific? >>>> >>>> Selecting the type of an object should be done by having a link<> >>>> property and letting the user create an object and setup the link. >>> >>> No, that's not what a link does. A link<> property lets the user >>> associate one instance, not a type that can be used for multiple >>> instances. We'd need a template<> or clone<> mechanism for that! >> >> No, you misunderstood. A link allows a user to create an object and >> make an associate. If you want to let a user choose a type, just have >> them make an object. Since you need no parameters to create an object, >> there's no advantage of telling something about a type name vs. giving >> it an object of that type. > > You're missing the point: You are advocating putting all configuration > into a config file, having the user call --readconfig > /path/to/board.conf and either fiddling with the config file or fiddling > with (looping) QMP scripts to apply machine-wide changes. > > By contrast I am talking about backwards-compatible convenience options > that influence more than one object in such a machine, wherever that > hierarchy is being built up. I think you're trying to run before we walk. I don't want the user to call --readconfig. I want to leave QEMUMachine alone for now. I think we should focus on converting the core devices to a two stage initialization and then make use of that in the appropriate machine init function. We don't need to convert every device to realize the benefits of this. We just have to convert all the devices used by a single machine type, then we can introduce a --no-machine option and let the user avoid dealing with machines at all. > > Reality with SysBus is multi-stage constructors: > A = qdev_create() > A.a = x > qdev_init_nofail(A) -> A_initfn() -> B = qdev_create() > B.b = y > qdev_init_nofail(B) -> B_initfn() > ... > > while you are dreaming of a flat hierarchy: I'm not dreaming. It's not a tremendous amount of work. > A = qdev_create() > B = qdev_create() > ... > A.a = x > B.b = y > ... > object_property_set_bool(qdev_get_machine(), "realized", true) > > But getting there requires more than denying me object_realize(), it > requires rewriting all qdev devices *first*, which I do not see Paolo > doing. You started for i440fx but did not finish either. Heh, you can't complain that I'm changing too much infrastructure too quickly and then complain that I'm not changing it quickly enough :-) From the beginning I stated a clear goal for 1.1, that was core conversion and bus state. I'll post bus state patches real soon (I think I posted them once already). We should have no problem hitting 1.1. The end goal shouldn't be s/qdev/object/g. That doesn't make things better by just using a new shiny infrastructure. Splitting device initialization into two stages is the fundamental reason for introducing QOM in the first place. It will allow us to introspect the full device model and then regenerate the same machine based on that introspection. This is the fundamental problem to solve. We shouldn't be trying to avoid it--it's the real end goal. > Calling realize recursively shouldn't hurt us if realizing an already > realized object is no-op and if the child iteration can cope with a > growing child list for the current parent and any node in /machine. > Meaning we can't just call one inlined object_realize_nofail() but we'd > need to loop doing that until no node in the subgraph is left that is > not realized. I guess I don't understand what the problem we're trying to solve is. Why can't we introduce an Object::realize() and just have it not automatically call DeviceState::init()? That way we have a way to clearly indicate whether a device needs to be refactored. Regards, Anthony Liguori > > Andreas >
On 12 April 2012 22:24, Anthony Liguori <anthony@codemonkey.ws> wrote: > The end goal shouldn't be s/qdev/object/g. That doesn't make things better > by just using a new shiny infrastructure. Splitting device initialization > into two stages is the fundamental reason for introducing QOM in the first > place. It might be yours, doesn't mean it's everybody else's :-) My main desire from shifting to QOM is named connections between devices and killing off the strict hierarchy of qbus buses; I haven't been particularly bitten by device init issues. -- PMM
On 04/12/2012 04:50 PM, Peter Maydell wrote: > On 12 April 2012 22:24, Anthony Liguori<anthony@codemonkey.ws> wrote: >> The end goal shouldn't be s/qdev/object/g. That doesn't make things better >> by just using a new shiny infrastructure. Splitting device initialization >> into two stages is the fundamental reason for introducing QOM in the first >> place. > > It might be yours, doesn't mean it's everybody else's :-) But clearly my own selfish goals are the only thing that matters :-) > My main desire from shifting to QOM is named connections between > devices and killing off the strict hierarchy of qbus buses; We're pretty much there, no? I think the only thing really left is introducing Pins and qomifying MemoryRegions. > I haven't been particularly bitten by device init issues. You will be... In order to let a user make those connections you'll need a mechanism to set link properties before realize but after instance_init. Regards, Anthony Liguori > > -- PMM
Am 12.04.2012 23:24, schrieb Anthony Liguori: > I guess I don't understand what the problem we're trying to solve is. > > Why can't we introduce an Object::realize() and just have it not > automatically call DeviceState::init()? That way we have a way to > clearly indicate whether a device needs to be refactored. That would be perfect, but means we should drop Paolo's 23/24 v2 and due to legacy compatibility I do need either object_realize_nofail() or an inline version thereof. Andreas
On 04/12/2012 05:02 PM, Andreas Färber wrote: > Am 12.04.2012 23:24, schrieb Anthony Liguori: >> I guess I don't understand what the problem we're trying to solve is. >> >> Why can't we introduce an Object::realize() and just have it not >> automatically call DeviceState::init()? That way we have a way to >> clearly indicate whether a device needs to be refactored. > > That would be perfect, but means we should drop Paolo's 23/24 v2 and due > to legacy compatibility I do need either object_realize_nofail() or an > inline version thereof. I don't understand which "legacy compatibility" you're referring to. Can you be specific? Regards, Anthony Liguori > > Andreas >
On 12 April 2012 22:57, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 04/12/2012 04:50 PM, Peter Maydell wrote: >> My main desire from shifting to QOM is named connections between >> devices and killing off the strict hierarchy of qbus buses; > > We're pretty much there, no? > > I think the only thing really left is introducing Pins and qomifying > MemoryRegions. Well, until we've introduced Pins we're not there at all :-) Also a coherent infrastructure for defining and using more complicated interfaces than 'raise/lower' would be good, Pin is just the degenerate special case of that. That would let us get rid of the annoying pointer properties we're using for omap clocks, for instance. >> I haven't been particularly bitten by device init issues. > > You will be... In order to let a user make those connections you'll need a > mechanism to set link properties before realize but after instance_init. Yeah, but qdev has that already, so I wasn't feeling the lack of it under the old regime. -- PMM
Am 12.04.2012 23:24, schrieb Anthony Liguori:
> I think you're trying to run before we walk.
The opposite is the case: I am trying to walk by improving code that was
not even qdev'ified before I touched it (see the sh4 commit in my
previous message). But now you are telling me I can't get a function for
that because Paolo and you want to run to a stage where realize doesn't
create new objects, which in reality today it does.
So what about applying a modified version of my patch that clearly marks
it as "obsolete"? Then we can at least move forward with sh4 (and prep
and ...). There's still quite some state around that just g_malloc()s
some struct and passes it around in opaques - that's also where I was
wishing for a QOM board object so that board state can be properly modelled.
I really don't see the point why I should uselessly make objects SysBus
without needing MemoryRegions or GPIO qemu_irqs and in the knowledge
that SysBus as an articial non-bus is supposed to die in QOM, just so
that I can use the existing qdev_init_nofail() for two-stage construction.
initfn is unable to handle errors btw, which is another reason to do
object creations in a second-stage constructor.
Maybe we need to decouple that from realize altogether and move to
three-stage constructors instead? With ObjectClass::realize() being for
new and reviewed code, like you just suggested, and simply moving
DeviceClass::init() to ObjectClass::init() in addition to .instance_init?
Andreas
Am 13.04.2012 00:09, schrieb Anthony Liguori: > On 04/12/2012 05:02 PM, Andreas Färber wrote: >> Am 12.04.2012 23:24, schrieb Anthony Liguori: >>> I guess I don't understand what the problem we're trying to solve is. >>> >>> Why can't we introduce an Object::realize() and just have it not >>> automatically call DeviceState::init()? That way we have a way to >>> clearly indicate whether a device needs to be refactored. >> >> That would be perfect, but means we should drop Paolo's 23/24 v2 and due >> to legacy compatibility I do need either object_realize_nofail() or an >> inline version thereof. > > I don't understand which "legacy compatibility" you're referring to. > Can you be specific? hw/sh7750.c is creating all kinds of realize-unaware SysBus devices that are virtually unmaintained, none of you will have reviewed or updated them to match some shiny new realize model. My current work is CPU modelling, not redoing devices I have no clue about in 14 targets. There's a non-qdev struct with a cpu field that I'm updating from CPUSH4State to QOM SuperHCPU and, for inspecting and testing and evaluating general SoC modelling that was calling for a child<SuperHCPU> property of a QOM object. And unfortunately that object has a user-specifiable CPU (and an unresponsive maintainer, but this is intentionially a test run in a part of code where conflicts were unlikely to occur, as opposed to target-arm, so I don't complain). So either I need a property that decides on what parameter to pass to cpu_init() or I need SoC types for each CPU type and in that case the mentioned convenience issue of how to tweak CPU properties on a SoC object comes into play. (We surely do not want the user to iterate over the DAG searching for CPUs and setting properties on each one when -cpu does that for all cores in an easy and central way, that would be a step backwards.) We can surely postpone that part of my sh4 series (leaving out subclasses and leaving the SoC non-qdev), working on that right now, but that doesn't solve the fundamental issue of the Paolo-Anthony realize model that I'm trying to point out with this example, apparently in vein. :( Andreas
Il 12/04/2012 23:08, Andreas Färber ha scritto: > Reality with SysBus is multi-stage constructors: > A = qdev_create() > A.a = x > qdev_init_nofail(A) -> A_initfn() -> B = qdev_create() > B.b = y > qdev_init_nofail(B) -> B_initfn() > ... > > while you are dreaming of a flat hierarchy: > > A = qdev_create() > B = qdev_create() > ... > A.a = x > B.b = y > ... > object_property_set_bool(qdev_get_machine(), "realized", true) > > But getting there requires more than denying me object_realize() I'm pretty sure we're talking past each other, so I'm rewinding before the last few messages. If you look at the initialization of a typical qdev object, it goes like this: dev = qdev_create(NULL, "mv88w8618_eth"); qdev_set_nic_properties(dev, &nd_table[0]); qdev_init_nofail(dev); sysbus_mmio_map(sysbus_from_qdev(dev), 0, MP_ETH_BASE); sysbus_connect_irq(sysbus_from_qdev(dev), 0, pic[MP_ETH_IRQ]); Now, there is no good reason to have that qdev_init_nofail. It is there because sysbus_init_irq and sysbus_init_mmio are called in the device init function (mv88w8618_eth_init): static int mv88w8618_eth_init(SysBusDevice *dev) { mv88w8618_eth_state *s = FROM_SYSBUS(mv88w8618_eth_state, dev); sysbus_init_irq(dev, &s->irq); s->nic = qemu_new_nic(&net_mv88w8618_info, &s->conf, object_get_typename(OBJECT(dev)), dev->qdev.id, s); memory_region_init_io(&s->iomem, &mv88w8618_eth_ops, s, "mv88w8618-eth", MP_ETH_SIZE); sysbus_init_mmio(dev, &s->iomem); return 0; } ... and those calls are prerequisites for sysbus_mmio_map and sysbus_connect_irq. Similarly, there is no reason why qdev_get_gpio_in can only be called after qdev_init_nofail. It's only like that because qdev_init_gpio_in is called later than in the instance_init function. Only a handful of devices need a dynamic number of GPIO pins, everything else can call it in instance_init. On the other hand, qemu_new_nic for example needs to be done at realize time (or at least at first access). Converting a device to remove those qdev_init_nofail seems to be little more work than just moving bits of the sysbus init function to instance_init. We can also introduce new functions sysbus_new_simple etc. to replace sysbus_create_simple. It does not even need to occur a board at a time, it can be done a device at a time. There may be indeed cases where you need multi-stage construction. However: 1) if the construction resembles the composition tree, which should usually be the case, the construction _can_ happen in the realize callback. 2) if you also need lazy construction of the composition tree before realization (i.e. initialize it on first use), we can have tools to make it simple. See the example from above of gpio pins whose number depends on properties. Until we're there, nobody is denying anyone anything. You already have qdev_init_nofail, and I said clearly that adding cpu_init_nofail is fine for me; all I object to is to adding a generic function. > initfn is unable to handle errors btw, which is another reason to do > object creations in a second-stage constructor. What error handling do you need specifically? You could add an Error ** argument to object_{new,initialize{,_with_type} too as soon as you have a use for that. Paolo
On 13 April 2012 08:17, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 12/04/2012 23:08, Andreas Färber ha scritto: > If you look at the initialization of a typical qdev object, it goes like > this: > > dev = qdev_create(NULL, "mv88w8618_eth"); > qdev_set_nic_properties(dev, &nd_table[0]); > qdev_init_nofail(dev); > sysbus_mmio_map(sysbus_from_qdev(dev), 0, MP_ETH_BASE); > sysbus_connect_irq(sysbus_from_qdev(dev), 0, pic[MP_ETH_IRQ]); > > Now, there is no good reason to have that qdev_init_nofail. It is there > because sysbus_init_irq and sysbus_init_mmio are called in the device > init function (mv88w8618_eth_init): > > static int mv88w8618_eth_init(SysBusDevice *dev) > { > mv88w8618_eth_state *s = FROM_SYSBUS(mv88w8618_eth_state, dev); > > sysbus_init_irq(dev, &s->irq); > s->nic = qemu_new_nic(&net_mv88w8618_info, &s->conf, > object_get_typename(OBJECT(dev)), > dev->qdev.id, s); > memory_region_init_io(&s->iomem, &mv88w8618_eth_ops, s, > "mv88w8618-eth", MP_ETH_SIZE); > sysbus_init_mmio(dev, &s->iomem); > return 0; > } > > ... and those calls are prerequisites for sysbus_mmio_map and > sysbus_connect_irq. ...but it's not uncommon for the number of memory regions or IRQs to depend on some qdev property, so you can't put these calls in instance_init, that would be too early. > Similarly, there is no reason why qdev_get_gpio_in can only be called > after qdev_init_nofail. It's only like that because qdev_init_gpio_in > is called later than in the instance_init function. Only a handful of > devices need a dynamic number of GPIO pins, everything else can call it > in instance_init. I think a model that says "half our devices set up memory and IRQs in one function and the other half do it somewhere else" is pretty confusing. I'd rather have a single standard place where they can all set these up. -- PMM
Il 13/04/2012 09:30, Peter Maydell ha scritto: > ...but it's not uncommon for the number of memory regions or > IRQs to depend on some qdev property, so you can't put these > calls in instance_init, that would be too early. Do you have examples? The only example I can find in the tree from a quick grep is the OMAP modulecount. >> Similarly, there is no reason why qdev_get_gpio_in can only be called >> after qdev_init_nofail. It's only like that because qdev_init_gpio_in >> is called later than in the instance_init function. Only a handful of >> devices need a dynamic number of GPIO pins, everything else can call it >> in instance_init. > > I think a model that says "half our devices set up memory and > IRQs in one function and the other half do it somewhere else" > is pretty confusing. I'd rather have a single standard place > where they can all set these up. I don't think this is any different from "half our devices use #defines and half our devices use qdev properties". Children can be created in initialization or creates children, and realization Paolo
On 13 April 2012 08:59, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 13/04/2012 09:30, Peter Maydell ha scritto: >> ...but it's not uncommon for the number of memory regions or >> IRQs to depend on some qdev property, so you can't put these >> calls in instance_init, that would be too early. > > Do you have examples? The only example I can find in the tree from a > quick grep is the OMAP modulecount. Half a dozen from random grep: "a9mpcore_priv" (and its equivalents for a15, 11mpcore, etc) "xilinx,timer" -- memory region size depends on a property "SUNW,tcx" -- number of memory regions depend on depth property "arm_mptimer" -- number of regions and irqs depends on 'num-cpu' prop "grlib,gptimer" -- number of irqs depends on "nr-timers" prop "lan9118" -- which MemoryRegionOps we use for the region depends on the "mode_16bit" property. -- PMM
Il 13/04/2012 10:12, Peter Maydell ha scritto: > Half a dozen from random grep: > "xilinx,timer" -- memory region size depends on a property > "lan9118" -- which MemoryRegionOps we use for the region depends > on the "mode_16bit" property. You can set these at realize time, before the memory region itself is realized (realization uses pre-order for this reason). > "SUNW,tcx" -- number of memory regions depend on depth property The regions can still exist in the device, whether they're mapped depends on the depth property. > "a9mpcore_priv" (and its equivalents for a15, 11mpcore, etc) > "arm_mptimer" -- number of regions and irqs depends on 'num-cpu' prop Why wouldn't you model this as a parent SoC object with either: - 1 GIC child, N CPU children, N MPTimer children, and N GICInterface children (with links to the GIC); - or 1 GIC + N CPU children, each of the latter having 1 MPTimer child and 1 GICInterface child (with links to the GIC); > "grlib,gptimer" -- number of irqs depends on "nr-timers" prop This one is a good example. Paolo
On 13 April 2012 09:33, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 13/04/2012 10:12, Peter Maydell ha scritto: >> Half a dozen from random grep: >> "xilinx,timer" -- memory region size depends on a property >> "lan9118" -- which MemoryRegionOps we use for the region depends >> on the "mode_16bit" property. > > You can set these at realize time, before the memory region itself is > realized (realization uses pre-order for this reason). > >> "SUNW,tcx" -- number of memory regions depend on depth property > > The regions can still exist in the device, whether they're mapped > depends on the depth property. This is like saying that you should still have 512 IRQ lines on every IRQ array, or that omap should have 6 GPIO modules even on variants with only 4, and so on. Devices shouldn't expose connections that don't actually exist for the config you've picked, otherwise we can't report attempts to wire up nonexistent connections as errors. We have this ability for qdev/sysbus currently, we shouldn't drop it in the move to QOM. >> "a9mpcore_priv" (and its equivalents for a15, 11mpcore, etc) >> "arm_mptimer" -- number of regions and irqs depends on 'num-cpu' prop > > Why wouldn't you model this as a parent SoC object with either: > > - 1 GIC child, N CPU children, N MPTimer children, and N GICInterface > children (with links to the GIC); > > - or 1 GIC + N CPU children, each of the latter having 1 MPTimer child > and 1 GICInterface child (with links to the GIC); You're right in general that we should be modelling these as container objects (I posted a series the other week that starts to move in that direction by dropping the weird subclassing of the GIC.) But the overall container object for a cortex-a9 will still be an object whose number of exposed IRQ lines depends on a device parameter, and the GIC device must still have a number of exposed memory regions/input lines/output lines that depends on the number of CPUs, which is going to be a device parameter. I'm not sure what your GICInterface children are? -- PMM
Il 13/04/2012 11:09, Peter Maydell ha scritto: > On 13 April 2012 09:33, Paolo Bonzini <pbonzini@redhat.com> wrote: >> Il 13/04/2012 10:12, Peter Maydell ha scritto: >>> Half a dozen from random grep: >>> "xilinx,timer" -- memory region size depends on a property >>> "lan9118" -- which MemoryRegionOps we use for the region depends >>> on the "mode_16bit" property. >> >> You can set these at realize time, before the memory region itself is >> realized (realization uses pre-order for this reason). >> >>> "SUNW,tcx" -- number of memory regions depend on depth property >> >> The regions can still exist in the device, whether they're mapped >> depends on the depth property. > > This is like saying that you should still have 512 IRQ lines on > every IRQ array, or that omap should have 6 GPIO modules even > on variants with only 4, and so on. As far as I understood, the fact that you cannot change the depth of this particular framebuffer is a limitation of QEMU. The framebuffer can do both 8 and 24-bit, the current model is the wrong one. The ARM cases are more interesting. >>> "a9mpcore_priv" (and its equivalents for a15, 11mpcore, etc) >>> "arm_mptimer" -- number of regions and irqs depends on 'num-cpu' prop >> >> Why wouldn't you model this as a parent SoC object with either: >> >> - 1 GIC child, N CPU children, N MPTimer children, and N GICInterface >> children (with links to the GIC); >> >> - or 1 GIC + N CPU children, each of the latter having 1 MPTimer child >> and 1 GICInterface child (with links to the GIC); > > You're right in general that we should be modelling these as > container objects (I posted a series the other week that starts > to move in that direction by dropping the weird subclassing of > the GIC.) But the overall container object for a cortex-a9 will > still be an object whose number of exposed IRQ lines depends on > a device parameter Yes, that would require a little more coding, just like the grlib. > , and the GIC device must still have a number > of exposed memory regions/input lines/output lines that depends > on the number of CPUs, which is going to be a device parameter. That would be a device parameter of the SoC. As long as the GIC is only created after the device parameter has been set on the parent, it can just read it from there. (BTW perhaps it is cheating, but on the PC you just have "-smp 4" for a four-core so it's not a device property). > I'm not sure what your GICInterface children are? Those would model the GIC<->CPU interface memory regions. for (i = 0; i < NUM_CPU(s); i++) { s->backref[i] = s; memory_region_init_io(&s->cpuiomem[i+1], &gic_cpu_ops, &s->backref[i], "gic_cpu", 0x100); } Paolo
On 13 April 2012 10:32, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 13/04/2012 11:09, Peter Maydell ha scritto: >> You're right in general that we should be modelling these as >> container objects (I posted a series the other week that starts >> to move in that direction by dropping the weird subclassing of >> the GIC.) But the overall container object for a cortex-a9 will >> still be an object whose number of exposed IRQ lines depends on >> a device parameter > > Yes, that would require a little more coding, just like the grlib. Well, if we can do it then that's fine (assuming that the actual implementation doesn't look too dreadful and doesn't look wildly different from "number of irq lines is fixed"). >> , and the GIC device must still have a number >> of exposed memory regions/input lines/output lines that depends >> on the number of CPUs, which is going to be a device parameter. > > That would be a device parameter of the SoC. As long as the GIC > is only created after the device parameter has been set on the > parent, it can just read it from there. I don't understand how this would work? The GIC can't read parameters from its parent, surely, that would be breaking encapsulation. > (BTW perhaps it is cheating, but on the PC you just have > "-smp 4" for a four-core so it's not a device property). I am assuming that '-smp 4' will eventually convert into setting a device property on something (probably on the QOM object which corresponds to the top level machine?). >> I'm not sure what your GICInterface children are? > > Those would model the GIC<->CPU interface memory regions. MemoryRegions model memory regions :-) I'm not sure what more there is to do there. -- PMM
Il 13/04/2012 11:53, Peter Maydell ha scritto: > On 13 April 2012 10:32, Paolo Bonzini <pbonzini@redhat.com> wrote: >> Il 13/04/2012 11:09, Peter Maydell ha scritto: >>> You're right in general that we should be modelling these as >>> container objects (I posted a series the other week that starts >>> to move in that direction by dropping the weird subclassing of >>> the GIC.) But the overall container object for a cortex-a9 will >>> still be an object whose number of exposed IRQ lines depends on >>> a device parameter >> >> Yes, that would require a little more coding, just like the grlib. > > Well, if we can do it then that's fine (assuming that the actual > implementation doesn't look too dreadful and doesn't look wildly > different from "number of irq lines is fixed"). Dreadful, no. Different, no idea as of now. :) >>> , and the GIC device must still have a number >>> of exposed memory regions/input lines/output lines that depends >>> on the number of CPUs, which is going to be a device parameter. >> >> That would be a device parameter of the SoC. As long as the GIC >> is only created after the device parameter has been set on the >> parent, it can just read it from there. > > I don't understand how this would work? The GIC can't read > parameters from its parent, surely, that would be breaking > encapsulation. The GIC can have a (strongly-typed) backlink to the SoC, and can call object_property_get on it. >> (BTW perhaps it is cheating, but on the PC you just have >> "-smp 4" for a four-core so it's not a device property). > > I am assuming that '-smp 4' will eventually convert into > setting a device property on something (probably on the > QOM object which corresponds to the top level machine?). Yeah, you would still know the type of that object though, and you would still be able to retrieve the property. >>> I'm not sure what your GICInterface children are? >> >> Those would model the GIC<->CPU interface memory regions. > > MemoryRegions model memory regions :-) I'm not sure what > more there is to do there. MemoryRegion + how to create them + ops != MemoryRegion. :) Paolo
On 13 April 2012 11:00, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 13/04/2012 11:53, Peter Maydell ha scritto: >> On 13 April 2012 10:32, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> Il 13/04/2012 11:09, Peter Maydell ha scritto: >>>> You're right in general that we should be modelling these as >>>> container objects (I posted a series the other week that starts >>>> to move in that direction by dropping the weird subclassing of >>>> the GIC.) But the overall container object for a cortex-a9 will >>>> still be an object whose number of exposed IRQ lines depends on >>>> a device parameter >>> >>> Yes, that would require a little more coding, just like the grlib. >> >> Well, if we can do it then that's fine (assuming that the actual >> implementation doesn't look too dreadful and doesn't look wildly >> different from "number of irq lines is fixed"). > > Dreadful, no. Different, no idea as of now. :) > >>>> , and the GIC device must still have a number >>>> of exposed memory regions/input lines/output lines that depends >>>> on the number of CPUs, which is going to be a device parameter. >>> >>> That would be a device parameter of the SoC. As long as the GIC >>> is only created after the device parameter has been set on the >>> parent, it can just read it from there. >> >> I don't understand how this would work? The GIC can't read >> parameters from its parent, surely, that would be breaking >> encapsulation. > > The GIC can have a (strongly-typed) backlink to the SoC, and can call > object_property_get on it. The trouble with this is that the GIC is embedded into about half a dozen different parent devices, all of which obviously have different types. And having an otherwise self-contained GIC object have a random backlink out to the SoC purely for purposes of finding something out about the component that's using it *is* breaking encapsulation, even if it's a strongly typed backdoor... >>> (BTW perhaps it is cheating, but on the PC you just have >>> "-smp 4" for a four-core so it's not a device property). >> >> I am assuming that '-smp 4' will eventually convert into >> setting a device property on something (probably on the >> QOM object which corresponds to the top level machine?). > > Yeah, you would still know the type of that object though, and you would > still be able to retrieve the property. > >>>> I'm not sure what your GICInterface children are? >>> >>> Those would model the GIC<->CPU interface memory regions. >> >> MemoryRegions model memory regions :-) I'm not sure what >> more there is to do there. > > MemoryRegion + how to create them + ops != MemoryRegion. :) Er, the ops are part of the core GIC -- what we're modelling here is a memory-mapped set of registers which provide a window (interface) onto the device. That's exactly what exposing a MemoryRegion is. (I'd like to have better support for exposing "named" MemoryRegions, in the same way that a Pin to me is support for "named" irq/gpio lines, but I don't think that's what you're getting at.) -- PMM
Il 13/04/2012 12:09, Peter Maydell ha scritto: >> The GIC can have a (strongly-typed) backlink to the SoC, and can call >> object_property_get on it. > > The trouble with this is that the GIC is embedded into about > half a dozen different parent devices, all of which obviously > have different types. They can have a common superclass. But you _can_ also make it a property; right now there is no way to guess how comfortable that will be, but we *should* make it comfortable. It can be done by using three-stage initialization, either in the few objects that need it or in DeviceState. >>>>> I'm not sure what your GICInterface children are? >>>> >>>> Those would model the GIC<->CPU interface memory regions. >>> >>> MemoryRegions model memory regions :-) I'm not sure what >>> more there is to do there. >> >> MemoryRegion + how to create them + ops != MemoryRegion. :) > > Er, the ops are part of the core GIC -- what we're modelling > here is a memory-mapped set of registers which provide a > window (interface) onto the device. That's exactly what > exposing a MemoryRegion is. (I'd like to have better support > for exposing "named" MemoryRegions, in the same way that > a Pin to me is support for "named" irq/gpio lines, but I don't > think that's what you're getting at.) The PC has a similar model but it is implemented with N+1 devices, one APIC per CPU plus the IO-APIC. For the ARM, it seems cleaner to me to also split it into a per-CPU object (gic_cpu_ops) and a connection object (gic_dist_ops + gic_thiscpu_ops)---even if the per-CPU object are just thin forwarders because the logic is embedded in a single device. Paolo
On 13 April 2012 11:24, Paolo Bonzini <pbonzini@redhat.com> wrote: > The PC has a similar model but it is implemented with N+1 devices, one > APIC per CPU plus the IO-APIC. > > For the ARM, it seems cleaner to me to also split it into a per-CPU > object (gic_cpu_ops) and a connection object (gic_dist_ops + > gic_thiscpu_ops)---even if the per-CPU object are just thin forwarders > because the logic is embedded in a single device. I'm against this because it's not modelling the hardware. The actual silicon is a single block of functionality with a number of configurable knobs/pins/memory regions, and I think trying to impose a boundary that doesn't exist in reality isn't going to work. The only sane forwarding you could do for the gic_cpu_ops objects would be to forward the reads and writes entirely untouched. That's just a silly special case wrapper around a MemoryRegion and I don't see that it helps. Also from the point of view of a user of the GIC (ie the SoC level containers) you don't want to be messing about instantiating lots of different objects and connecting them together. You want to configure and instantiate one object, the GIC, and wire it up in the obvious way: by mapping the memory regions it exposes at the locations you require and by wiring up the irq/gpio lines to your own irq/gpio lines. -- PMM
Il 13/04/2012 12:43, Peter Maydell ha scritto: > On 13 April 2012 11:24, Paolo Bonzini <pbonzini@redhat.com> wrote: >> The PC has a similar model but it is implemented with N+1 devices, one >> APIC per CPU plus the IO-APIC. >> >> For the ARM, it seems cleaner to me to also split it into a per-CPU >> object (gic_cpu_ops) and a connection object (gic_dist_ops + >> gic_thiscpu_ops)---even if the per-CPU object are just thin forwarders >> because the logic is embedded in a single device. > > I'm against this because it's not modelling the hardware. The > actual silicon is a single block of functionality with a number > of configurable knobs/pins/memory regions Well, it's configurable at synthesis time. You're trying to model things like VHDL generate statements, which just do not exist in C. Modeling what you do in VHDL *and* keeping configurability is going to be hard anyway. In fact, if you want to model real hardware the real GIC does have a "backdoor" to the SoC that contains it (the "number of CPUs" constant is probably set in some VHDL file that is shared by SoC and GIC source code). > , and I think trying > to impose a boundary that doesn't exist in reality isn't going > to work. The only sane forwarding you could do for the gic_cpu_ops > objects would be to forward the reads and writes entirely untouched. Yes, that would be it. For the MPTimer it could also make sense to do things the other way round: place the timer as a child of the CPU, and add a global device that just forwards to the current CPU's timers. You could choose between consistency and simplicity. > That's just a silly special case wrapper around a MemoryRegion and > I don't see that it helps. It helps because it avoids having N of this, N of that, and N of the other. You can just have N CPUBlock objects, each of which has-a CPU, an interface to the GIC, an interface to the MPTimer, etc. /machine the SoC object /cpu[0] a CPUBlock object /cpu the actual ARMCPU /gic_intf a forwarder to the global GIC object /timer an MPTimerBlock /watchdog an MPTimerBlock /cpu[1] another CPUBlock object /cpu /gic_intf /timer /watchdog ... /gic the configurable GIC /mptimer a forwarder to the current CPU's timer objects (The above view is only the children: you would also have a /machine/cpu[0]/gic link to /machine/gic etc. However, you wouldn't need a link from /machine/cpu[0]/timer to /machine/mptimer). > Also from the point of view of a user of the GIC (ie the SoC > level containers) you don't want to be messing about instantiating > lots of different objects and connecting them together. You want > to configure and instantiate one object, the GIC, and wire it up in > the obvious way: by mapping the memory regions it exposes at > the locations you require and by wiring up the irq/gpio lines > to your own irq/gpio lines. Yes, all this would be hidden in the init and/or realize functions for CPUBlock. Paolo
On 04/13/2012 04:09 AM, Peter Maydell wrote: > On 13 April 2012 09:33, Paolo Bonzini<pbonzini@redhat.com> wrote: >> Il 13/04/2012 10:12, Peter Maydell ha scritto: >>> Half a dozen from random grep: >>> "xilinx,timer" -- memory region size depends on a property >>> "lan9118" -- which MemoryRegionOps we use for the region depends >>> on the "mode_16bit" property. >> >> You can set these at realize time, before the memory region itself is >> realized (realization uses pre-order for this reason). >> >>> "SUNW,tcx" -- number of memory regions depend on depth property >> >> The regions can still exist in the device, whether they're mapped >> depends on the depth property. > > This is like saying that you should still have 512 IRQ lines on > every IRQ array, There's a fairly nasty dependency problem here. Let's say that we did: [device "pic"] irq_num = 16 irq[0] = /mydevice/irq irq[1] = /foodev/irq irq[2] = /bardev/irq irq[16] = /baz/irq # should throw an error This is clearly something we want to be able to express. The trouble is that since the irq_num property affects whether irq[16] is valid, you would need to have a multi-stage construction process or have a strict property assignment ordering that was preserved in the config file. Since we're dealing with a graph, relying on ordering will probably never actually work. There are alternative ways to handle this. We could make use of Paolo's "default property" hook and then at realize time, validate that we don't have irq's assigned that violate irq_num. This still gives you the nice end user error without requiring crazy dependency mapping. The downside is that if you introspect the properties, you'll get a single generic irq[*] or something before realize. But since this is a relatively uncommon requirement, I think it's a reasonable compromise. Regards, Anthony Liguori
Am 13.04.2012 09:17, schrieb Paolo Bonzini: >> initfn is unable to handle errors btw, which is another reason to do >> object creations in a second-stage constructor. > > What error handling do you need specifically? You could add an Error ** > argument to object_{new,initialize{,_with_type} too as soon as you have > a use for that. My point was that .instance_init functions always need to succeed (they're void and have no Error** argument and object_new() does not return NULL). Allocating a new object in an initfn may fail though. When at startup, an abort() is considered acceptable. However since Jan and others are talking about CPU hotplug, having e.g. a PowerPCCPU fail to initialize its large opcodes table should IMO mid-/long-term not abort but signal to the user that there was not sufficient memory for the desired runtime operation. Bad example. Point is, if realize fails we can delete the object and continue; if something in initfn fails we currently need to abort. Yes, it's possible to change all initializers, but not last minute before 1.1 please. Andreas
On 04/13/2012 08:36 AM, Andreas Färber wrote: > Am 13.04.2012 09:17, schrieb Paolo Bonzini: >>> initfn is unable to handle errors btw, which is another reason to do >>> object creations in a second-stage constructor. >> >> What error handling do you need specifically? You could add an Error ** >> argument to object_{new,initialize{,_with_type} too as soon as you have >> a use for that. > > My point was that .instance_init functions always need to succeed > (they're void and have no Error** argument and object_new() does not > return NULL). Allocating a new object in an initfn may fail though. How would it fail? The instance_init shouldn't have any side effects nor should it be affected by external factors. Regards, Anthony Liguori > > When at startup, an abort() is considered acceptable. However since Jan > and others are talking about CPU hotplug, having e.g. a PowerPCCPU fail > to initialize its large opcodes table should IMO mid-/long-term not > abort but signal to the user that there was not sufficient memory for > the desired runtime operation. Bad example. Point is, if realize fails > we can delete the object and continue; if something in initfn fails we > currently need to abort. Yes, it's possible to change all initializers, > but not last minute before 1.1 please. > > Andreas >
Am 13.04.2012 16:00, schrieb Anthony Liguori: > On 04/13/2012 08:36 AM, Andreas Färber wrote: >> Am 13.04.2012 09:17, schrieb Paolo Bonzini: >>>> initfn is unable to handle errors btw, which is another reason to do >>>> object creations in a second-stage constructor. >>> >>> What error handling do you need specifically? You could add an Error ** >>> argument to object_{new,initialize{,_with_type} too as soon as you have >>> a use for that. >> >> My point was that .instance_init functions always need to succeed >> (they're void and have no Error** argument and object_new() does not >> return NULL). Allocating a new object in an initfn may fail though. > > How would it fail? > > The instance_init shouldn't have any side effects nor should it be > affected by external factors. I'm still talking about the (pretty clear to me) graph that I posted. There, object A's init function creates a new qdev object - . Creating an object can fail - fatally or non-fatally. And yes, exactly my point, currently initfn (first stage) cannot fail, only the second stage (DeviceClass::init). Which is why I've been saying we'll need to refactor those "fake composition" usages first before we declare that we can defer qdev initialization to vl.c. Andreas
Il 13/04/2012 16:06, Andreas Färber ha scritto: > I'm still talking about the (pretty clear to me) graph that I posted. > There, object A's init function creates a new qdev object - . Creating > an object can fail - fatally or non-fatally. > > And yes, exactly my point, currently initfn (first stage) cannot fail, > only the second stage (DeviceClass::init). Which is why I've been saying > we'll need to refactor those "fake composition" usages first before we > declare that we can defer qdev initialization to vl.c. But why should they fail? This is what I also asked. If instance-init is deterministic, it will either always or never fail (besides cases like memory allocation which cannot really be handled correctly). Paolo
Am 13.04.2012 16:08, schrieb Paolo Bonzini: > Il 13/04/2012 16:06, Andreas Färber ha scritto: >> I'm still talking about the (pretty clear to me) graph that I posted. >> There, object A's init function creates a new qdev object - . Creating >> an object can fail - fatally or non-fatally. >> >> And yes, exactly my point, currently initfn (first stage) cannot fail, >> only the second stage (DeviceClass::init). Which is why I've been saying >> we'll need to refactor those "fake composition" usages first before we >> declare that we can defer qdev initialization to vl.c. > > But why should they fail? This is what I also asked. If instance-init > is deterministic, it will either always or never fail (besides cases > like memory allocation which cannot really be handled correctly). Indeed I am thinking of trivial memory allocation for starters, yes. This is not just a theoretical issue as I have two such reports in my Bugzilla already. I do think we need an object_try_new() for said runtime operations such as CPU hotplug, and the PowerPCCPU dynamically allocates the opcodes table using malloc() that may fail as well (same for g_try_malloc). I see three solutions: a) make children a field of the object's state struct (Anthony's i440fx approach, requires to know object size in advance) or b) allocate in realize (being done in at least some SysBus devices) or c) let initfn fail. On a related matter, another solution to my SoC problem would be to be able to add a NULL child property as placeholder (so that the layering of naming the property is preserved) and to have an object_property_set_child() to fill that in; still needs a second-stage constructor function for the CPU-dependent devices though... Andreas
Il 13/04/2012 16:21, Andreas Färber ha scritto: > Am 13.04.2012 16:08, schrieb Paolo Bonzini: >> Il 13/04/2012 16:06, Andreas Färber ha scritto: >>> I'm still talking about the (pretty clear to me) graph that I posted. >>> There, object A's init function creates a new qdev object - . Creating >>> an object can fail - fatally or non-fatally. >>> >>> And yes, exactly my point, currently initfn (first stage) cannot fail, >>> only the second stage (DeviceClass::init). Which is why I've been saying >>> we'll need to refactor those "fake composition" usages first before we >>> declare that we can defer qdev initialization to vl.c. >> >> But why should they fail? This is what I also asked. If instance-init >> is deterministic, it will either always or never fail (besides cases >> like memory allocation which cannot really be handled correctly). > > Indeed I am thinking of trivial memory allocation for starters, yes. > This is not just a theoretical issue as I have two such reports in my > Bugzilla already. Are they public? Haven't we long agreed that exit(1) is the right thing to do on OOM? Paolo
Am 13.04.2012 16:25, schrieb Paolo Bonzini: > Il 13/04/2012 16:21, Andreas Färber ha scritto: >> Am 13.04.2012 16:08, schrieb Paolo Bonzini: >>> Il 13/04/2012 16:06, Andreas Färber ha scritto: >>>> I'm still talking about the (pretty clear to me) graph that I posted. >>>> There, object A's init function creates a new qdev object - . Creating >>>> an object can fail - fatally or non-fatally. >>>> >>>> And yes, exactly my point, currently initfn (first stage) cannot fail, >>>> only the second stage (DeviceClass::init). Which is why I've been saying >>>> we'll need to refactor those "fake composition" usages first before we >>>> declare that we can defer qdev initialization to vl.c. >>> >>> But why should they fail? This is what I also asked. If instance-init >>> is deterministic, it will either always or never fail (besides cases >>> like memory allocation which cannot really be handled correctly). >> >> Indeed I am thinking of trivial memory allocation for starters, yes. >> This is not just a theoretical issue as I have two such reports in my >> Bugzilla already. > > Are they public? No. But the issue is that creating the machine worked fine, customer installs SLES, qemu-kvm crashes at runtime. > Haven't we long agreed that exit(1) is the right thing > to do on OOM? There seemed to be agreement that (i) for creating objects at startup exit(1) or abort() is acceptable, (ii) abort() were not acceptable for user-supplied -m values (but still no patch), (iii) runtime memory usage should not be as dynamic as shown in my trace graph causing OOM (search your inbox for "gnuplot", mentioned the thread name on IRC recently too). There's also a related recent thread to which I haven't replied yet. I already fixed one QOM bug where qdev_try_create() or so would abort() if the type doesn't exist simply because qom/object.c has assert()s all over it and is lacking some non-asserting versions. This also affects my currently-deferred -cpu subclassing series, for which I had to inline some class existence checks before object_new() so that cpu_init() can return NULL as currently expected. Andreas
On 04/13/2012 09:06 AM, Andreas Färber wrote: > Am 13.04.2012 16:00, schrieb Anthony Liguori: >> On 04/13/2012 08:36 AM, Andreas Färber wrote: >>> Am 13.04.2012 09:17, schrieb Paolo Bonzini: >>>>> initfn is unable to handle errors btw, which is another reason to do >>>>> object creations in a second-stage constructor. >>>> >>>> What error handling do you need specifically? You could add an Error ** >>>> argument to object_{new,initialize{,_with_type} too as soon as you have >>>> a use for that. >>> >>> My point was that .instance_init functions always need to succeed >>> (they're void and have no Error** argument and object_new() does not >>> return NULL). Allocating a new object in an initfn may fail though. >> >> How would it fail? >> >> The instance_init shouldn't have any side effects nor should it be >> affected by external factors. > > I'm still talking about the (pretty clear to me) graph that I posted. > There, object A's init function creates a new qdev object - . instance_init should not call qdev_init(). That would be a bug. Regards, Anthony Liguori Creating > an object can fail - fatally or non-fatally. > > And yes, exactly my point, currently initfn (first stage) cannot fail, > only the second stage (DeviceClass::init). Which is why I've been saying > we'll need to refactor those "fake composition" usages first before we > declare that we can defer qdev initialization to vl.c. > > Andreas >
On 04/13/2012 09:21 AM, Andreas Färber wrote: > Am 13.04.2012 16:08, schrieb Paolo Bonzini: >> Il 13/04/2012 16:06, Andreas Färber ha scritto: >>> I'm still talking about the (pretty clear to me) graph that I posted. >>> There, object A's init function creates a new qdev object - . Creating >>> an object can fail - fatally or non-fatally. >>> >>> And yes, exactly my point, currently initfn (first stage) cannot fail, >>> only the second stage (DeviceClass::init). Which is why I've been saying >>> we'll need to refactor those "fake composition" usages first before we >>> declare that we can defer qdev initialization to vl.c. >> >> But why should they fail? This is what I also asked. If instance-init >> is deterministic, it will either always or never fail (besides cases >> like memory allocation which cannot really be handled correctly). > > Indeed I am thinking of trivial memory allocation for starters, yes. > This is not just a theoretical issue as I have two such reports in my > Bugzilla already. > > I do think we need an object_try_new() for said runtime operations such > as CPU hotplug, and the PowerPCCPU dynamically allocates the opcodes > table using malloc() that may fail as well (same for g_try_malloc). Why doesn't it use g_malloc()? > > I see three solutions: > a) make children a field of the object's state struct (Anthony's i440fx > approach, requires to know object size in advance) or > b) allocate in realize (being done in at least some SysBus devices) or > c) let initfn fail. Regards, Anthony Liguori > On a related matter, another solution to my SoC problem would be to be > able to add a NULL child property as placeholder (so that the layering > of naming the property is preserved) and to have an > object_property_set_child() to fill that in; still needs a second-stage > constructor function for the CPU-dependent devices though... > > Andreas >
On Fri, Apr 13, 2012 at 09:32, Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 13/04/2012 11:09, Peter Maydell ha scritto: >> On 13 April 2012 09:33, Paolo Bonzini <pbonzini@redhat.com> wrote: >>> Il 13/04/2012 10:12, Peter Maydell ha scritto: >>>> Half a dozen from random grep: >>>> "xilinx,timer" -- memory region size depends on a property >>>> "lan9118" -- which MemoryRegionOps we use for the region depends >>>> on the "mode_16bit" property. >>> >>> You can set these at realize time, before the memory region itself is >>> realized (realization uses pre-order for this reason). >>> >>>> "SUNW,tcx" -- number of memory regions depend on depth property >>> >>> The regions can still exist in the device, whether they're mapped >>> depends on the depth property. >> >> This is like saying that you should still have 512 IRQ lines on >> every IRQ array, or that omap should have 6 GPIO modules even >> on variants with only 4, and so on. > > As far as I understood, the fact that you cannot change the depth > of this particular framebuffer is a limitation of QEMU. The > framebuffer can do both 8 and 24-bit, the current model is the wrong > one. Actually there were two different TCX devices, standard 8 bit model and more expensive 24 bit version. The 24 bit version was downwards compatible, it had a separate control plane where you could select for each pixel whether it used 8 bit mode or 24 bit mode. The 24 bit plane data resided also in a separate region to 8 bit data. In the 8 bit version these regions did not exist. We could make the devices separate, then the depth would be fixed per device but the memory regions would not change. At sun4m.c we could choose the device based on the depth. Because we also pass the depth to OpenBIOS (it uses that to determine the device type), everything should work as before. > The ARM cases are more interesting. > >>>> "a9mpcore_priv" (and its equivalents for a15, 11mpcore, etc) >>>> "arm_mptimer" -- number of regions and irqs depends on 'num-cpu' prop >>> >>> Why wouldn't you model this as a parent SoC object with either: >>> >>> - 1 GIC child, N CPU children, N MPTimer children, and N GICInterface >>> children (with links to the GIC); >>> >>> - or 1 GIC + N CPU children, each of the latter having 1 MPTimer child >>> and 1 GICInterface child (with links to the GIC); >> >> You're right in general that we should be modelling these as >> container objects (I posted a series the other week that starts >> to move in that direction by dropping the weird subclassing of >> the GIC.) But the overall container object for a cortex-a9 will >> still be an object whose number of exposed IRQ lines depends on >> a device parameter > > Yes, that would require a little more coding, just like the grlib. > >> , and the GIC device must still have a number >> of exposed memory regions/input lines/output lines that depends >> on the number of CPUs, which is going to be a device parameter. > > That would be a device parameter of the SoC. As long as the GIC > is only created after the device parameter has been set on the > parent, it can just read it from there. > > (BTW perhaps it is cheating, but on the PC you just have > "-smp 4" for a four-core so it's not a device property). > >> I'm not sure what your GICInterface children are? > > Those would model the GIC<->CPU interface memory regions. > > for (i = 0; i < NUM_CPU(s); i++) { > s->backref[i] = s; > memory_region_init_io(&s->cpuiomem[i+1], &gic_cpu_ops, &s->backref[i], > "gic_cpu", 0x100); > } > > Paolo >
diff --git a/include/qemu/object.h b/include/qemu/object.h index 360181d..524d3b0 100644 --- a/include/qemu/object.h +++ b/include/qemu/object.h @@ -504,6 +504,16 @@ void object_initialize_with_type(void *data, Type type); void object_initialize(void *obj, const char *typename); /** + * object_realize_nofail: + * @obj: The object to realize. + * + * This function will complete the initialization of an object based on + * properties set by setting the "realized" property to true. + * Like #qdev_init_nofail it will terminate the process in case of errors. + */ +void object_realize_nofail(Object *obj); + +/** * object_finalize: * @obj: The object to finalize. * diff --git a/qom/object.c b/qom/object.c index f4f29ab..1ef0b9b 100644 --- a/qom/object.c +++ b/qom/object.c @@ -381,6 +381,17 @@ void object_initialize(void *data, const char *typename) object_initialize_with_type(data, type); } +void object_realize_nofail(Object *obj) +{ + Error *err = NULL; + + object_property_set_bool(obj, true, "realized", &err); + if (error_is_set(&err)) { + qerror_report_err(err); + exit(1); + } +} + static void object_property_del_all(Object *obj) { while (!QTAILQ_EMPTY(&obj->properties)) {
Wrap setting of Object::realized property, error reporting and exit(1) into a helper function. It is the equivalent of qdev_init_nofail(). Signed-off-by: Andreas Färber <afaerber@suse.de> Cc: Anthony Liguori <anthony@codemonkey.ws> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Peter Maydell <peter.maydell@linaro.org> --- Hi Paolo, please consider appending this patch to your push-push series. It then provides equivalent second-stage init functionality for non-qdev objects, as desired for ARMCPU and SH7750 SoC. include/qemu/object.h | 10 ++++++++++ qom/object.c | 11 +++++++++++ 2 files changed, 21 insertions(+), 0 deletions(-)