Re: [Qemu-devel] [RFC] QOM design - add instance data to Object (-> add constructors)

[Liviu Ionescu]

> On 14 Jun 2015, at 04:49, Peter Crosthwaite <address@hidden> wrote:
> 
> 
> ...
> Infact, I have an application of constructor arugments myself. This is
> for the ARM A9 MPCore container where the number of CPUs is variable
> per-instance. Same problem as you are facing, realize is too late,
> init is too early.

init is clearly too early. why is realize() to late?

my current understanding of the existing implementation is that the mechanism 
including the properties setters followed by realize() is the equivalent of a 
variable args, very generic, constructor.

if this was the intention, then ok, the entire construction logic should 
happen, in the usual linear way, in realize(), using all the data collected so 
far, provided during .instance_init or via the property setters.

this also means that realize() cannot be too late, since **it is** the 
constructor.

so I ask again: is realize() the constructor or not?

my entire suggestion was based on the recommendation to avoid adding logic to 
realize(), but if this is no longer true, and you think that realize() is never 
too late for any use case, then the entire discussion is objectless.

this also means that only after realize() the object has a consistent state and 
can be used.

in practical terms this means that realize() should be called right after 
creation, immediately after setting the properties:

   DeviceState *mcu = qdev_create(NULL, TYPE_STM32F103RB);
   {
       qdev_prop_set_uint32(mcu, "hse-freq-hz", 8000000); /* 8.0 MHz */
       qdev_prop_set_uint32(mcu, "lse-freq-hz", 32768); /* 32 KHz */
   }
   qdev_realize(mcu);

which is ok as long as it is strictly adhered.

to summarise the requirements for realize() to be a constructor:

1 - it should not be allowed to use the object for other purposes between the 
creation and realize().

2 - for hierarchical objects the child realize() should always call the parent 
realize() at the beginning.

3 - inside realize() it should be perfectly legal to create as many children 
objects, as long as they respect the same requirements

are these requirements reasonable?

if so, the comments in qdev-core.h should be updated:

 * As an interim step, the #DeviceState:realized property is set by deprecated
 * functions qdev_init() and qdev_init_nofail().
 * In the future, devices will propagate this state change to their children
 * and along busses they expose.
 * The point in time will be deferred to machine creation, so that values
 * set in @realize will not be introspectable beforehand. Therefore devices
 * must not create children during @realize; they should initialize them via
 * object_initialize() in their own #TypeInfo.instance_init and forward the
 * realization events appropriately.

especially the "Therefore devices must not create children during @realize;" 
must be removed, since this is the statement that made me consider adding C++ 
style constructors (I clearly need to create new objects during construction).

and also "In the future, devices will propagate this state change to their 
children and along busses they expose" is very misleading, since it implies the 
realize() mechanism was intended for other purposes, not for object creation.

> I'm not saying that constructor arguments are a bad idea, just I am
> still seeing a different solution to your problem.

so currently there are two ways:

- use realize() as a delayed, generic, variable args, constructor (and no 
longer plan to use it for other purposes)
- use C++ style constructors with arguments (with the variant to set properties 
and call an explicit constructor without arguments)

if delaying the construction till realize() is not ok, or you plan to use 
realize() for other purposes, like propagating it over buses (and it is up to 
you to judge this, I do not have the overall view on the project), then I'm 
afraid we don't have a better alternative then explicit constructors (possibly 
with arguments).

> ... The number of machine init args blew out of control at one stage so
> they were structified. It was natural to roll this struct into the
> machine-state struct.

Q: given the fact that init args are known at machine level, and they are 
needed during the construction of the cortexm-mcu object (the parent of 
stm32-mcu, the parent of STM32F103RB), how do you pass the init args to be 
available at construction time? setting a property automatically implies the 
object construction should be delayed till realize().

> 
>>>> GenericGPIOLEDInfo h103_machine_green_led = {
>>>>   .desc = "STM32-H103 Green LED, GPIOC[12], active low",
>>>>   .port_index = STM32_GPIO_PORT_C,
>>>>   .port_bit = 12,
>>>>   .active_low = true,
>>>>   .on_message = "[Green LED On]\n",
>>>>   .off_message = "[Green LED Off]\n",
>>>>   .use_stderr = true };
>>> 
>>> Is any of this information needed for construction of can these fields
>>> just be qdev properties?
>> 
>> yes, the port index and bit.
>> 
> 
> So it sounds like the LED is managing its own connectivity. This is
> not normal esp, trying to do it as part of object construction, the
> machine level should manage the connectivity after the components are
> constructed.

this is again OO philosophical talk.

the GenericGPIOLED is an abstract class. to be instantiated, it requires a 
derived class to implement a virtual (get_gpio_dev(port_index)) that provides 
the actual gpio peripheral to be used.

for all STM32 MCUs, this derived class is STM32GPIOLED; for any other different 
MCU a new such class will be written and then all LEDs of all boards will be 
instantiated using the same procedure.

Q: the code performing the connectivity is exactly the same for all LEDs for 
all MCUs, why not implement it in the base class and have to manually 
copy/paste it in all places where the instance is used?

>>> 
>>> You should try and avoid fishing sub-devices out of the container like
>>> this if you can.
>>> 
>>>>       qdev_prop_set_uint32(rcc, "hse-freq-hz", 8000000); /* 8.0 MHz */
>>>>       qdev_prop_set_uint32(rcc, "lse-freq-hz", 32768); /* 32 KHz */
>>> 
>>> If these are crystal frequencies, then they should be properties of
>>> the MCU object itself.
>> 
>> these are properties specific to some of the STM32 MCUs, other families have 
>> different properties.
>> 
>> they are definitely not properties of the generic cortexm-mcu object.
>> 
> 
> Yes, but this code is accessing a module of a container rather than
> talking to the container itself.

so your suggestion is to define the two properties in the stm32-mcu, and 
somehow forward them internally to the specific device, the stm32-rcc in the ST 
case.

Q: except triggers, which are complicated, what other mechanisms are available 
for such properties forwarding?

>> 
>> a led requires a port bit to work.
> 
> Maybe not :). In your use case, an LED is a GPIO sink that just does a
> printf. This should be independent of any SoC or even architecture.
> Cna you make just this think a device in its own right without any
> "port" awareness?

sure, but this is a completely different use case.

the object name is "generic-gpio-led" and the ST specific implementation is 
named "stm32-gpio-led", so it was designed to be connected to a gpio 
peripheral; for other use cases obviously other object can be defined. 

what you should keep in mind is that this same LED is planned to be used for 
several tens of boards, from multiple vendors, and instantiating such LEDs 
should be as easy as possible.

in C++ I would write:

   DeviceState *led = new STM32GPIOLED(NULL, &h103_machine_green_led, mcu);


for QOM the shortest instantiating method I found was:

   DeviceState *led = qdev_create(NULL, TYPE_STM32_GPIO_LED);
   {
       qdev_prop_set_ptr(led, "defs", &h103_machine_green_led); 
       qdev_prop_set_ptr(led, "mcu", mcu);
   }
   qdev_realize(led);


Q: how would you instantiate your super generic LED and connect it to the GPIO? 
can you do it using less than 4 statements?

>> what you basically try to say is that all objects should be created at 
>> .instance_init, using static recipes, always the same, all other 
>> configuration should be done by setting properties, and there should be no 
>> references from one object to another.
>> 
> 
> The last part is incorrect. You are allowed refs between objects just
> having to ref another object for the sake of your own construction has
> no precedent.

sorry, I don't understand this phrase.

> I never said you cant ref one object from another. QOM links exist for
> this exact purpose.

Q: any example of QOM links?

> But clock enables are probably best implemented as
> GPIOs anyway (I have had similar discussions on list WRT reset pins).

Q: same question, can you point to such an implementation?

Q: where is defined the QEMU GPIO functionality? 

it seems we have another communication problem, all my previous references to 
GPIO were meant to refer to the MCU peripheral named GPIO, while you probably 
referred to QEMU GPIO (which I currently do not know very well, but the name 
seems misleading, they are in fact some kind of interrupts)
 

> patches will help. C code is sometimes a better language than English.

sure.  ;-)

to quote a living classic:

  git clone git://git.code.sf.net/p/gnuarmeclipse/qemu gnuarmeclipse-qemu.git
  cd gnuarmeclipse-qemu.git
  git checkout master
  git checkout gnuarmeclipse-dev
  git diff master gnuarmeclipse-dev

but please be warned that currently there are 15724 lines in this diff. (I 
personally feel too lazy to visually inspect such diffs and prefer graphical 
tools)

>> perhaps a skype call, using screen sharing, would be more efficient to 
>> clarify these issues. would you be available for such a call? (my skype id 
>> is 'ilg-ul', you can call me at any time).
> 
> I'm ok with that. I am on USA PDT time (-8:00).

I'm on +3:00, but it does not matter, you can call me at any time you see me 
active on skype.


regards,

Liviu