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

[Peter Crosthwaite]

On Sat, Jun 13, 2015 at 5:56 PM, Liviu Ionescu <address@hidden> wrote:
>
>> On 14 Jun 2015, at 01:46, Peter Crosthwaite <address@hidden> wrote:
>>
>>> not to mention that in real life situations, constructors are used to pass 
>>> non-static data, for example the 'machine' structure (for various command 
>>> line params) when constructing MCUs, or the 'mcu' when constructing LEDs.
>>>
>>
>> I think these are both questionable examples, as they are contained
>> objects taking and relying on pointers to their container which
>> reduces their re-usability as a modular component.
>
> the cortexm-mcu reference to machine is actually not a reference to the 
> container. as it is now, qemu packs some command line args into the machine 
> structure, and these values are needed during the MCU construction.
>
> I don't know why they are packed inside the machine structure, probably this 
> is another unfortunate decision, but currently there is no other way to get 
> these values inside the cortexm-mcu object.
>

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.

>>> 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.

>>
>>>
>>> static void stm32_h103_board_init_callback(MachineState *machine)
>>> {
>>>    cortexm_board_greeting(machine);
>>>    DeviceState *mcu = qdev_alloc(NULL, TYPE_STM32F103RB);
>>>    {
>>>        STM32F103RB_GET_CLASS(mcu)->construct(OBJECT(mcu), machine);
>>>
>>>        /* Set the board specific oscillator frequencies. */
>>>        DeviceState *rcc = stm32_mcu_get_rcc_dev(mcu);
>>
>> 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.

>> The MCU property setter then delegates its
>> property to the sub-device. This abstracts SoC internals away from the
>> board layer. Alias properties can make shorter work of this.
>
>>>
>>> the one-instance-per-class might be true for MCUs, but generally it isn't, 
>>> for example the LED object, you can have several LEDs in a machine, all 
>>> having a common class, but different instances. similarly for GPIOs.
>>>
>>
>> But there doesn't seem to be any demand for construction-sensitive
>> info there - at least yet.
>
> I'm not sure it is so.
>
> 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?

> it can be constructed by first constructing an object that encapsulates the 
> port & bit, later passed to the led, or, as it is now, the top led objects 
> calls a virtual in the child (stm32-led) to return the specific port & bit, 
> and then connects the interrupts. the second solution is easier to use.
>
>>> ah, just for having the information available for verbosity. here is a 
>>> typical run for a simple blinky application:
>>>
>>
>> can you use object_get_typename?
>
> the message is printed in the cortexm-mcu object, if at this level 
> object_get_typename() returns the child name and not the current object name, 
> then the answer is 'yes'.
>

Should be OK.

> ---
>
> 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.

> well, I think this is simply not realistic. and the result of this approach 
> is that all objects will be actually constructed at realize(), which is 
> exactly what I tried to avoid.
>
> how do you suggest to efficiently implement dependencies between peripherals? 
> for example in stm32 the RCC includes clock enable bits for all peripherals, 
> including gpios. the current implementation uses a pointer to the stm32-rcc 
> object from each of the stm32-gpio objects. how would you do it without this 
> pointer? using listeners and notifiers to inform each gpio when rcc bits are 
> changed? how would the gpio register to the notifier without a pointer to it? 
> by name?
>

I never said you cant ref one object from another. QOM links exist for
this exact purpose. But clock enables are probably best implemented as
GPIOs anyway (I have had similar discussions on list WRT reset pins).
The individual peripherals should not have implementation awareness of
their clock controller so it's hard to see why they should depend on
them for object construction. They should just accept a GPIO that does
clock control and the SoC container manages the wiring after
everything is constructed.

>
> how do you suggest the objects in a hierarchy communicate to each other 
> without constructors and virtuals?

Virtual pointers do exist in the form of links and are settable as properties.

>for example the STM32 capabilities are known in the STM32F103RB object (let's 
>say form the class data, so available at .instance_init), but these 
>capabilities are needed in the stm32-mcu parent object, which will create the 
>actual objects.
>
>
> my feeling is that we have a communication problem and I'm not able to 
> explain you the exact problems that I'm facing.
>

patches will help. C code is sometimes a better language than English.
Mark a patch series as RFC and explain on the cover letter what
feedback you are looking for.

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

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.

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

Regards,
Peter

>
> regards,
>
> Liviu
>
>
>
>