Re: [Qemu-devel] [RFC] QOM design - add instance data to Object

[Peter Crosthwaite]

On Sat, Jun 13, 2015 at 4:41 AM, Liviu Ionescu <address@hidden> wrote:
>
>> On 13 Jun 2015, at 12:29, Peter Crosthwaite <address@hidden> wrote:
>>
>>>
>>> STM32F103RB* mcu = new STM32F103RB(&stm32f103rb_capabilities);
>>>
>>
>> Is the capabilities genuinely variable from instance to instance?
>
> good point.
>
> my example was not very accurate, the capabilities are indeed specific to a 
> certain class, like this:
>
> class STM32F103RB : public STM32 {
> public:
>         STM32F103RB() : STM32(&stm32f103rb_capabilities) {
>                 ...
>         }
> }
>
> and constructing a new mcu is:
>
> STM32F103RB* mcu = new STM32F103RB();
>
>> This
>> naming suggest a 1:1 between capabilities instances and MCU classes
>> which would mean he capabilities can just be rolled into the classes
>> themselves.
>
> that's correct. but in practical terms it does not help much.
>
>>> static void stm32f103rb_mcu_instance_init_callback(Object *obj)
>>
>> No need for _callback.
>
> this is a personal preference issue, I like to explicitly mark functions 
> called by the framework to easily differentiate them from regular functions.
>
>>>    CortexMState *cm_state = CORTEXM_MCU_STATE(obj);
>>>    cm_state->capabilities = (CortexMCapabilities *) 
>>> &stm32f103rb_capabilities;
>>
>> Following on from before, can you put the capabilities in the class
>> instead? Then it is immediately accessible from the CortexMState
>> instance_init.
>
> yes, this can be done, but it doesn't help much.
>

So I am struggling to see the problem any more. As all the information
you need to construct the object is constant per-concrete class, the
information can be added to the class to resolve. This is then
available to init() call immediately. I don't see a need to pass any
data at init time.

steps:

1: Create a class struct for the cortext MCU with appropriate parent
(TYPE_DEVICE?)
2: Add pointer for this data in struct. Infact, you can inline.
Something like this:

typedef struct CortexMClass {
    /*< private >*/
    DeviceClass parent_class;
    /*< public >*/
    uint64_t .flash_size_kb = 128,
    uint64_t .sram_size_kb = 20,
    ...
}

3: Create subclasses with more data as needed:

typedef struct STMClass {
    /*< private >*/
    CortexMClass parent_class;
    /*< public >*/
    const char *family;
    uint64_t hsi_freq_hz;
    uint64_t lsi_freq_hz;
    ...
}

4: In the concrete classes class_init fm (not instance_init). Set all
the data. There are many class init functions that demonstrate setting
things on multiple levels. Most set only function hooks, but in your
case you are setting data. That is OK.

Check m25p80.c for M25P80Class and its class_init for a relevant
example. You can also see in m25p80_init that the class is retrieve
from the instance pointer to get the part info for construction. Now
m25p80_init is using a legacy SSI class init function but this same
approach would work if that fn was an object _init.

>>
>>> }
>>>
>>>
>>> constructing a mcu is done with:
>>>
>>> dev = qdev_create(NULL, 'STM32F103RB');
>>>
>>> which calls in order:
>>>
>>> cortexm_mcu_instance_init_callback()
>>> stm32_mcu_instance_init_callback()
>>> stm32f103rb_mcu_instance_init_callback()
>>>
>>> as you can see, the stm32f103rb call is executed last (as it should be). 
>>> this also means that during the execution of the cortexm_ and stm_ 
>>> functions the capabilities **are not** available.
>>>
>>>
>>> the workaround I used was to move the object construction from 
>>> instance_init to instance_post_init, which are executed after the 
>>> instance_init, so when they run the capabilities pointer is already set.
>>>
>>
>> What in your construction process requires access to the capabilities
>> information? Can you give concrete examples?
>
> here is the actual capabilities structure:
>
> static STM32Capabilities stm32f103rb_capabilities = {
>     .cortexm = {
>         .device_name = TYPE_STM32F103RB,

Why is the capabilities struct aware of its container type?

>         .cortexm_model = CORTEX_M3,
>         .flash_size_kb = 128,
>         .sram_size_kb = 20,
>         .has_mpu = true,
>         .has_itm = true,
>         .num_irq = 60,
>         .nvic_bits = 4 },
>     .stm32 = {
>         .family = STM32_FAMILY_F1,
>         .hsi_freq_hz = 8000000,
>         .lsi_freq_hz = 40000,
>         .has_gpioa = true,
>         .has_gpiob = true,
>         .has_gpioc = true,
>         .has_gpiod = true,
>         .has_gpioe = true,
>         .f1 = {
>             .is_md = true } } };
>

Why do you need to make it one giant struct with its own inheritance
hierarchy? With the class idea, you would just split this to two
structs for cortexm and stm32.

Regards,
Peter