Patchwork [v2] qom: Introduce object_realize_nofail()

login
register
mail settings
Submitter Andreas Färber
Date April 12, 2012, 1:44 p.m.
Message ID <1334238263-28572-1-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/152085/
State New
Headers show

Comments

Andreas Färber - April 12, 2012, 1:44 p.m.
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(-)
Andreas Färber - April 12, 2012, 1:58 p.m.
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
Andreas Färber - April 12, 2012, 2:04 p.m.
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
Anthony Liguori - April 12, 2012, 2:47 p.m.
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)) {
Andreas Färber - April 12, 2012, 2:53 p.m.
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
Paolo Bonzini - April 12, 2012, 2:59 p.m.
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
Paolo Bonzini - April 12, 2012, 3:15 p.m.
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
Anthony Liguori - April 12, 2012, 3:41 p.m.
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
>
Anthony Liguori - April 12, 2012, 3:43 p.m.
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
Peter Maydell - April 12, 2012, 3:52 p.m.
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
Anthony Liguori - April 12, 2012, 4:02 p.m.
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
Peter Maydell - April 12, 2012, 4:09 p.m.
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
Paolo Bonzini - April 12, 2012, 4:13 p.m.
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
Anthony Liguori - April 12, 2012, 4:33 p.m.
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
Andreas Färber - April 12, 2012, 4:52 p.m.
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
Paolo Bonzini - April 12, 2012, 5:02 p.m.
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
Anthony Liguori - April 12, 2012, 7:59 p.m.
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
Anthony Liguori - April 12, 2012, 7:59 p.m.
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
>
Andreas Färber - April 12, 2012, 9:08 p.m.
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
Anthony Liguori - April 12, 2012, 9:24 p.m.
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
>
Peter Maydell - April 12, 2012, 9:50 p.m.
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
Anthony Liguori - April 12, 2012, 9:57 p.m.
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
Andreas Färber - April 12, 2012, 10:02 p.m.
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
Anthony Liguori - April 12, 2012, 10:09 p.m.
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
>
Peter Maydell - April 12, 2012, 10:11 p.m.
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
Andreas Färber - April 12, 2012, 10:31 p.m.
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
Andreas Färber - April 12, 2012, 10:54 p.m.
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
Paolo Bonzini - April 13, 2012, 7:17 a.m.
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
Peter Maydell - April 13, 2012, 7:30 a.m.
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
Paolo Bonzini - April 13, 2012, 7:59 a.m.
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
Peter Maydell - April 13, 2012, 8:12 a.m.
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
Paolo Bonzini - April 13, 2012, 8:33 a.m.
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
Peter Maydell - April 13, 2012, 9:09 a.m.
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
Paolo Bonzini - April 13, 2012, 9:32 a.m.
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
Peter Maydell - April 13, 2012, 9:53 a.m.
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
Paolo Bonzini - April 13, 2012, 10 a.m.
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
Peter Maydell - April 13, 2012, 10:09 a.m.
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
Paolo Bonzini - April 13, 2012, 10:24 a.m.
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
Peter Maydell - April 13, 2012, 10:43 a.m.
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
Paolo Bonzini - April 13, 2012, 11:01 a.m.
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
Anthony Liguori - April 13, 2012, 1:15 p.m.
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
Andreas Färber - April 13, 2012, 1:36 p.m.
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
Anthony Liguori - April 13, 2012, 2 p.m.
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
>
Andreas Färber - April 13, 2012, 2:06 p.m.
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
Paolo Bonzini - April 13, 2012, 2:08 p.m.
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
Andreas Färber - April 13, 2012, 2:21 p.m.
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
Paolo Bonzini - April 13, 2012, 2:25 p.m.
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
Andreas Färber - April 13, 2012, 2:51 p.m.
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
Anthony Liguori - April 13, 2012, 3:35 p.m.
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
>
Anthony Liguori - April 13, 2012, 3:36 p.m.
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
>
Blue Swirl - April 14, 2012, 11:29 a.m.
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
>

Patch

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)) {