qdev instance_init vs realize split

qdev instance_init vs realize split

[Damien Hedde]

Hi,

I'm wondering if there are rules or convention about what we put in the 
instance_init() vs realize() for simple devices ? (For complex ones we 
generally have no choice to put everything in realize())

For example we can declare irqs and mmios in instance_init() or 
realize() if they do not depend on some property.

This is not a big deal, but given how works the help message generation 
in the monitor, there are difference if the device is user-creatable.

If we leave irqs and mmios declaration in the instance_init(). They 
appear in the help message.
 > (qemu) device_add ibex-timer,help
 > ibex-timer options:
 >   ibex-timer[0]=<child<memory-region>>
 >   sysbus-irq[0]=<link<irq>>
 >   timebase-freq=<uint32> -  (default: 10000)

If we delay the declaration in realize(), we only have the declared 
qdev-properties (which is maybe more what we expect at this point):

 > (qemu) device_add ibex-timer,help
 > ibex-timer options:
 >   timebase-freq=<uint32> -  (default: 10000)

Any comments ?

Thanks,
Damien

Re: qdev instance_init vs realize split

[Philippe Mathieu-Daudé via]

On 15/2/22 11:19, Damien Hedde wrote:
> Hi,
> 
> I'm wondering if there are rules or convention about what we put in the 
> instance_init() vs realize() for simple devices ? (For complex ones we 
> generally have no choice to put everything in realize())

See Peter's recommendations here:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg723958.html

> For example we can declare irqs and mmios in instance_init() or 
> realize() if they do not depend on some property.
> 
> This is not a big deal, but given how works the help message generation 
> in the monitor, there are difference if the device is user-creatable.
> 
> If we leave irqs and mmios declaration in the instance_init(). They 
> appear in the help message.
>  > (qemu) device_add ibex-timer,help
>  > ibex-timer options:
>  >   ibex-timer[0]=<child<memory-region>>
>  >   sysbus-irq[0]=<link<irq>>
>  >   timebase-freq=<uint32> -  (default: 10000)
> 
> If we delay the declaration in realize(), we only have the declared 
> qdev-properties (which is maybe more what we expect at this point):
> 
>  > (qemu) device_add ibex-timer,help
>  > ibex-timer options:
>  >   timebase-freq=<uint32> -  (default: 10000)
> 
> Any comments ?
> 
> Thanks,
> Damien

Re: qdev instance_init vs realize split

[Peter Maydell]

On Tue, 15 Feb 2022 at 10:32, Damien Hedde <damien.hedde@greensocs.com> wrote:
> I'm wondering if there are rules or convention about what we put in the
> instance_init() vs realize() for simple devices ? (For complex ones we
> generally have no choice to put everything in realize())
>
> For example we can declare irqs and mmios in instance_init() or
> realize() if they do not depend on some property.

We don't, unfortunately, have a clear set of conventions for this.
We really ideally ought to write some up, because the question
keeps coming up. There are a few absolute rules:
 * things that can fail must be done in realize
 * things that depend on property values must be done in realize
 * things that affect the simulation must be done in realize
 * if you do something that needs a corresponding manual deinit
   step in instance_init then you must provide an instance_finalize
   even if the device would otherwise be "create once, lasts for
   entire simulation" as many of our devices are

But in many cases actions can be done in either method, and we
end up with devices being inconsistent and people wondering whether
there's a reason for it.

I vaguely think it would be good to get into the habit of writing
all our devices to have the full lifecycle code including supporting
init-realize-unrealize-finalize, but on the other hand that implies
a bunch of code (unrealize) which is never executed or tested...
I also suspect we have a bunch of buggy code in realize methods
which isn't correctly undoing things it has done already in the
error-exit-from-realize case.

> This is not a big deal, but given how works the help message generation
> in the monitor, there are difference if the device is user-creatable.
>
> If we leave irqs and mmios declaration in the instance_init(). They
> appear in the help message.
>  > (qemu) device_add ibex-timer,help
>  > ibex-timer options:
>  >   ibex-timer[0]=<child<memory-region>>
>  >   sysbus-irq[0]=<link<irq>>
>  >   timebase-freq=<uint32> -  (default: 10000)
>
> If we delay the declaration in realize(), we only have the declared
> qdev-properties (which is maybe more what we expect at this point):
>
>  > (qemu) device_add ibex-timer,help
>  > ibex-timer options:
>  >   timebase-freq=<uint32> -  (default: 10000)

This seems to me to be a problem with the help message generation.
IRQ and MMIO properties shouldn't be being presented to the user
whether we set them up in instance-init or realize: there is nothing
the user can usefully do with them.

thanks
-- PMM

Re: qdev instance_init vs realize split

[Damien Hedde]

On 2/15/22 12:32, Peter Maydell wrote:
> On Tue, 15 Feb 2022 at 10:32, Damien Hedde <damien.hedde@greensocs.com> wrote:
>> I'm wondering if there are rules or convention about what we put in the
>> instance_init() vs realize() for simple devices ? (For complex ones we
>> generally have no choice to put everything in realize())
>>
>> For example we can declare irqs and mmios in instance_init() or
>> realize() if they do not depend on some property.
> 
> We don't, unfortunately, have a clear set of conventions for this.
> We really ideally ought to write some up, because the question
> keeps coming up. There are a few absolute rules:
>   * things that can fail must be done in realize
>   * things that depend on property values must be done in realize
>   * things that affect the simulation must be done in realize
>   * if you do something that needs a corresponding manual deinit
>     step in instance_init then you must provide an instance_finalize
>     even if the device would otherwise be "create once, lasts for
>     entire simulation" as many of our devices are
> 
> But in many cases actions can be done in either method, and we
> end up with devices being inconsistent and people wondering whether
> there's a reason for it.
> 
> I vaguely think it would be good to get into the habit of writing
> all our devices to have the full lifecycle code including supporting
> init-realize-unrealize-finalize, but on the other hand that implies
> a bunch of code (unrealize) which is never executed or tested...
> I also suspect we have a bunch of buggy code in realize methods
> which isn't correctly undoing things it has done already in the
> error-exit-from-realize case.

Are you saying that: if an operation like a mmio/irq definition is done 
in realize(), in theory, we should have the unrealize() counterpart ?

Thanks,
--
Damien

Re: qdev instance_init vs realize split

[Peter Maydell]

On Tue, 15 Feb 2022 at 13:21, Damien Hedde <damien.hedde@greensocs.com> wrote:
> Are you saying that: if an operation like a mmio/irq definition is done
> in realize(), in theory, we should have the unrealize() counterpart ?

I'm saying that at the moment we have two categories of device:
 * ones which are intended to be pluggable and so might be
   destroyed at runtime: these have to support the full
   instance_init->realize->unrealize->finalize cycle
 * ones which are only created in machine models and then exist
   for the lifetime of the QEMU process: these have to support
   instance_init->finalize (for the benefit of monitor introspection
   which can create and delete devices in that way) but otherwise
   only need to support the instance_init->realize and don't
   need to handle the unrealize->finalize part

and maybe we should consider whether it would be better to write
all our devices in the same way to handle the full set of
state transitions.

But if we do decide that then we'd need to have a testing framework
that actually exercised the whole lifecycle for all devices, and
it would probably be a lot of work, so maybe it's not worthwhile.

We'd also want where we can to have APIs that arrange to do
the handling of destruction for you. I think the GPIO line
APIs are like this, for instance. That's much less prone to
"forgot to clean it up" bugs.

-- PMM

Re: qdev instance_init vs realize split

[Damien Hedde]

On 2/15/22 14:35, Peter Maydell wrote:
> On Tue, 15 Feb 2022 at 13:21, Damien Hedde <damien.hedde@greensocs.com> wrote:
>> Are you saying that: if an operation like a mmio/irq definition is done
>> in realize(), in theory, we should have the unrealize() counterpart ?
> 
> I'm saying that at the moment we have two categories of device:
>   * ones which are intended to be pluggable and so might be
>     destroyed at runtime: these have to support the full
>     instance_init->realize->unrealize->finalize cycle
>   * ones which are only created in machine models and then exist
>     for the lifetime of the QEMU process: these have to support
>     instance_init->finalize (for the benefit of monitor introspection
>     which can create and delete devices in that way) but otherwise
>     only need to support the instance_init->realize and don't
>     need to handle the unrealize->finalize part
> 
> and maybe we should consider whether it would be better to write
> all our devices in the same way to handle the full set of
> state transitions.
> 
> But if we do decide that then we'd need to have a testing framework
> that actually exercised the whole lifecycle for all devices, and
> it would probably be a lot of work, so maybe it's not worthwhile.
> 
> We'd also want where we can to have APIs that arrange to do
> the handling of destruction for you. I think the GPIO line
> APIs are like this, for instance. That's much less prone to
> "forgot to clean it up" bugs.
> 
> -- PMM

Thanks for the clarification,
--
Damien