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

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

[Liviu Ionescu]

while implementing the cortex-m hierarchical objects I faced several problems that required some hack, and I wanted to know your comments on them.

for convenience I'll explain the problem in C++ and then return to the issues of the C implementation. (the C++ syntax may not be very strict).

one of my goals was to emulate a representative number of MCUs, from several vendors. the model I used is hierarchical, grouping vendor common characteristics. for example for ST I have:

class CortexM {};
class STM32 : public CortexM {};
class STM32F103RB : public STM32 {};

to facilitate configuration of the CortexM and STM objects, I defined some 'capabilities' classes:

class CortexMCapabilities {};
class STM32Capabilities : public CortexMCapabilities {};

in the CortexMCapabilities I store the Cortex-M family (M0/M0+/M1/M3/M4/M4F/M7/M7F), flash/sram memory sizes, number of interrupts, flags telling if MPU, ITP, or other ARM peripherals are present.

in the STM32Capabilities I store STM family (F1/F2/F4/F4/etc), number of GPIOs, etc.

a capabilities object must be defined for each device, for example:

STM32Capabilities stm32f103rb_capabilities; ...

this pointer is passed to the constructors and as such would be available to all objects:

class CortexM 
{
public:
	CortexM(CortexMCapabilities* capabilities) {
		fCapabilities = capabilities;
		...
	}

	CortexMCapabilities* fCapabilities;
	...
}

class STM32 : public CortexM {
public:
	STM32(STM32Capabilities* capabilities) : CortexM(capabilities) {
		...
	}
}

class STM32F103RB : public STM32 {
public:
	STM32F103RB(STM32Capabilities* capabilities) : STM32(capabilities) {
		...
	}
}

when constructing a new mcu:

STM32F103RB* mcu = new STM32F103RB(&stm32f103rb_capabilities);

the constructors are executed in the natural 'parent first' order:

CortexM(...)
STM32(...)
STM32F103RB(...)

and each object gets access to the capabilities during the construction, as expected.


now let's return to the qemu objects I used, where I have

- a 'cortex-mcu' object derived from 'sys-bus-device', which has as member a pointer to the capabilities
- a 'stm32-mcu' object, derived from 'cortex-mcu'
- a 'STM32F103RB' object, derived from 'stm32-mcu' (the name matches the CMSIS device name).

the natural place to define the capabilities is the more specfic instance_init

static void stm32f103rb_mcu_instance_init_callback(Object *obj)
{
    qemu_log_function_name();

    CortexMState *cm_state = CORTEXM_MCU_STATE(obj);
    cm_state->capabilities = (CortexMCapabilities *) &stm32f103rb_capabilities;
}


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.

unfortunately, these calls are executed in reverse order of the constructors, 'child first', which might create problems if references to parent objects are required.

stm32f103rb_mcu_instance_post_init_callback()
stm32_mcu_instance_post_init_callback()
...
cortexm_mcu_instance_post_init_callback()
...

to solve the problem of references to parent objects, the third construction step is performed during realize().

unfortunately realize() is a simple virtual call, and since pointers to virtuals are stored in the class structure and not as separate vtbls, access to parent realize() is not possible directly, but only by manually storing the pointer in a separate parent_realize(0 and explicitly calling it at the beginning of each realize().


---

another similar problem that required a hack was passing some of the machine fields (kernel_filename & cpu_model) to the mcu constructor.

so, the actual mcu costructor has, in addition to the mcu name ('STM32F103RB'), a pointer to the machine.

for not being able to pass it to the constructor, I had to store it in a static variable and in the constructor refer to it, but this is definitely a non-reentrant kludge:

static MachineState *global_machine;

DeviceState *cortexm_mcu_create(MachineState *machine, const char *mcu_type)
{
    /*
     * Kludge, since it is not possible to pass data to object creation,
     * we use a static variable.
     */
    global_machine = machine;
    DeviceState *dev;
    dev = qdev_create(NULL, mcu_type);

    return dev;
}

static void cortexm_mcu_instance_init_callback(Object *obj)
{
    qemu_log_function_name();

    CortexMState *cm_state = CORTEXM_MCU_STATE(obj);
    assert(global_machine != NULL);

    if (global_machine->kernel_filename) {
        cm_state->kernel_filename = global_machine->kernel_filename;
    }

    if (global_machine->cpu_model) {
        cm_state->cpu_model = global_machine->cpu_model;
    }
}

---

I think that both the above cases show that a mechanism to pass instance data to the constructor is missing from the QOM design.

a bit curious is that the designer considered a similar mechanism to pass class data when creating class objects, but this cannot be used for classes that have multiple instances.


to avoid breaking compatibility, the solution that I suggest is to add separate initialisation functions
- add a new "void* instance_data;" member to Object, 
- add a new function to object_initialize_with_instance_data(), to pass this pointer
- add a new function qdev_create_with_instance_data() to pass this pointer

calling the existing constructor functions will create objects with this pointer set to NULL, calling the new functions will create objects with this pointer set to the desired value.

please note that this pointer must be set before any call to the instance_init() callbacks.

access to the instance data would be easy, using something like "OBJECT(dev)->instance_data", or, even better, a specialised getter 

	void* object_get_instance_data(Object*);

---

as a conclusion, a very simple thing like passing initialisation data to a C++ constructor requires a pretty sustained effort with the Qemu C implementation. unfortunately, this is also true for some other QOM aspects, like calling parent virtual methods.

a solution to simplify things is to add instance data to the object, and functions to set/get it.

my initial guess is that this solution can be implemented without breaking compatibility with existing code, by adding new functions; the suggested names are obviously not mandatory, any other more inspired names may be used.


regards,

Liviu


p.s. I generally appreciate creativity and the desire to invent new solutions for various problems; Stroustrup solution to the need of objects was to create a new programming language; your solution to the need of objects, instead of choosing a more appropriate language, was to insist on using C and invent a very, very complicated infrastructure, that requires a lot of explicit code to be manually written, it is relatively error prone, and it still has several design flaws; please believe me, it is quite difficult to outsmart Stroustrup...

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

[Liviu Ionescu]

any object oriented gurus around?

regards,

Liviu


> On 12 Jun 2015, at 14:33, Liviu Ionescu <ilg@livius.net> wrote:
> 
> while implementing the cortex-m hierarchical objects I faced several problems that required some hack, and I wanted to know your comments on them.
> 
> for convenience I'll explain the problem in C++ and then return to the issues of the C implementation. (the C++ syntax may not be very strict).
> 
> one of my goals was to emulate a representative number of MCUs, from several vendors. the model I used is hierarchical, grouping vendor common characteristics. for example for ST I have:
> 
> class CortexM {};
> class STM32 : public CortexM {};
> class STM32F103RB : public STM32 {};
> 
> to facilitate configuration of the CortexM and STM objects, I defined some 'capabilities' classes:
> 
> class CortexMCapabilities {};
> class STM32Capabilities : public CortexMCapabilities {};
> 
> in the CortexMCapabilities I store the Cortex-M family (M0/M0+/M1/M3/M4/M4F/M7/M7F), flash/sram memory sizes, number of interrupts, flags telling if MPU, ITP, or other ARM peripherals are present.
> 
> in the STM32Capabilities I store STM family (F1/F2/F4/F4/etc), number of GPIOs, etc.
> 
> a capabilities object must be defined for each device, for example:
> 
> STM32Capabilities stm32f103rb_capabilities; ...
> 
> this pointer is passed to the constructors and as such would be available to all objects:
> 
> class CortexM 
> {
> public:
> 	CortexM(CortexMCapabilities* capabilities) {
> 		fCapabilities = capabilities;
> 		...
> 	}
> 
> 	CortexMCapabilities* fCapabilities;
> 	...
> }
> 
> class STM32 : public CortexM {
> public:
> 	STM32(STM32Capabilities* capabilities) : CortexM(capabilities) {
> 		...
> 	}
> }
> 
> class STM32F103RB : public STM32 {
> public:
> 	STM32F103RB(STM32Capabilities* capabilities) : STM32(capabilities) {
> 		...
> 	}
> }
> 
> when constructing a new mcu:
> 
> STM32F103RB* mcu = new STM32F103RB(&stm32f103rb_capabilities);
> 
> the constructors are executed in the natural 'parent first' order:
> 
> CortexM(...)
> STM32(...)
> STM32F103RB(...)
> 
> and each object gets access to the capabilities during the construction, as expected.
> 
> 
> now let's return to the qemu objects I used, where I have
> 
> - a 'cortex-mcu' object derived from 'sys-bus-device', which has as member a pointer to the capabilities
> - a 'stm32-mcu' object, derived from 'cortex-mcu'
> - a 'STM32F103RB' object, derived from 'stm32-mcu' (the name matches the CMSIS device name).
> 
> the natural place to define the capabilities is the more specfic instance_init
> 
> static void stm32f103rb_mcu_instance_init_callback(Object *obj)
> {
>    qemu_log_function_name();
> 
>    CortexMState *cm_state = CORTEXM_MCU_STATE(obj);
>    cm_state->capabilities = (CortexMCapabilities *) &stm32f103rb_capabilities;
> }
> 
> 
> 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.
> 
> unfortunately, these calls are executed in reverse order of the constructors, 'child first', which might create problems if references to parent objects are required.
> 
> stm32f103rb_mcu_instance_post_init_callback()
> stm32_mcu_instance_post_init_callback()
> ...
> cortexm_mcu_instance_post_init_callback()
> ...
> 
> to solve the problem of references to parent objects, the third construction step is performed during realize().
> 
> unfortunately realize() is a simple virtual call, and since pointers to virtuals are stored in the class structure and not as separate vtbls, access to parent realize() is not possible directly, but only by manually storing the pointer in a separate parent_realize(0 and explicitly calling it at the beginning of each realize().
> 
> 
> ---
> 
> another similar problem that required a hack was passing some of the machine fields (kernel_filename & cpu_model) to the mcu constructor.
> 
> so, the actual mcu costructor has, in addition to the mcu name ('STM32F103RB'), a pointer to the machine.
> 
> for not being able to pass it to the constructor, I had to store it in a static variable and in the constructor refer to it, but this is definitely a non-reentrant kludge:
> 
> static MachineState *global_machine;
> 
> DeviceState *cortexm_mcu_create(MachineState *machine, const char *mcu_type)
> {
>    /*
>     * Kludge, since it is not possible to pass data to object creation,
>     * we use a static variable.
>     */
>    global_machine = machine;
>    DeviceState *dev;
>    dev = qdev_create(NULL, mcu_type);
> 
>    return dev;
> }
> 
> static void cortexm_mcu_instance_init_callback(Object *obj)
> {
>    qemu_log_function_name();
> 
>    CortexMState *cm_state = CORTEXM_MCU_STATE(obj);
>    assert(global_machine != NULL);
> 
>    if (global_machine->kernel_filename) {
>        cm_state->kernel_filename = global_machine->kernel_filename;
>    }
> 
>    if (global_machine->cpu_model) {
>        cm_state->cpu_model = global_machine->cpu_model;
>    }
> }
> 
> ---
> 
> I think that both the above cases show that a mechanism to pass instance data to the constructor is missing from the QOM design.
> 
> a bit curious is that the designer considered a similar mechanism to pass class data when creating class objects, but this cannot be used for classes that have multiple instances.
> 
> 
> to avoid breaking compatibility, the solution that I suggest is to add separate initialisation functions
> - add a new "void* instance_data;" member to Object, 
> - add a new function to object_initialize_with_instance_data(), to pass this pointer
> - add a new function qdev_create_with_instance_data() to pass this pointer
> 
> calling the existing constructor functions will create objects with this pointer set to NULL, calling the new functions will create objects with this pointer set to the desired value.
> 
> please note that this pointer must be set before any call to the instance_init() callbacks.
> 
> access to the instance data would be easy, using something like "OBJECT(dev)->instance_data", or, even better, a specialised getter 
> 
> 	void* object_get_instance_data(Object*);
> 
> ---
> 
> as a conclusion, a very simple thing like passing initialisation data to a C++ constructor requires a pretty sustained effort with the Qemu C implementation. unfortunately, this is also true for some other QOM aspects, like calling parent virtual methods.
> 
> a solution to simplify things is to add instance data to the object, and functions to set/get it.
> 
> my initial guess is that this solution can be implemented without breaking compatibility with existing code, by adding new functions; the suggested names are obviously not mandatory, any other more inspired names may be used.
> 
> 
> regards,
> 
> Liviu
> 
> 
> p.s. I generally appreciate creativity and the desire to invent new solutions for various problems; Stroustrup solution to the need of objects was to create a new programming language; your solution to the need of objects, instead of choosing a more appropriate language, was to insist on using C and invent a very, very complicated infrastructure, that requires a lot of explicit code to be manually written, it is relatively error prone, and it still has several design flaws; please believe me, it is quite difficult to outsmart Stroustrup...
> 
> 
> 
> 

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

[Peter Crosthwaite]

On Fri, Jun 12, 2015 at 4:33 AM, Liviu Ionescu <ilg@livius.net> wrote:
> while implementing the cortex-m hierarchical objects I faced several problems that required some hack, and I wanted to know your comments on them.
>
> for convenience I'll explain the problem in C++ and then return to the issues of the C implementation. (the C++ syntax may not be very strict).
>
> one of my goals was to emulate a representative number of MCUs, from several vendors. the model I used is hierarchical, grouping vendor common characteristics. for example for ST I have:
>
> class CortexM {};
> class STM32 : public CortexM {};
> class STM32F103RB : public STM32 {};
>
> to facilitate configuration of the CortexM and STM objects, I defined some 'capabilities' classes:
>
> class CortexMCapabilities {};
> class STM32Capabilities : public CortexMCapabilities {};
>
> in the CortexMCapabilities I store the Cortex-M family (M0/M0+/M1/M3/M4/M4F/M7/M7F), flash/sram memory sizes, number of interrupts, flags telling if MPU, ITP, or other ARM peripherals are present.
>
> in the STM32Capabilities I store STM family (F1/F2/F4/F4/etc), number of GPIOs, etc.
>
> a capabilities object must be defined for each device, for example:
>
> STM32Capabilities stm32f103rb_capabilities; ...
>
> this pointer is passed to the constructors and as such would be available to all objects:
>
> class CortexM
> {
> public:
>         CortexM(CortexMCapabilities* capabilities) {
>                 fCapabilities = capabilities;
>                 ...
>         }
>
>         CortexMCapabilities* fCapabilities;
>         ...
> }
>
> class STM32 : public CortexM {
> public:
>         STM32(STM32Capabilities* capabilities) : CortexM(capabilities) {
>                 ...
>         }
> }
>
> class STM32F103RB : public STM32 {
> public:
>         STM32F103RB(STM32Capabilities* capabilities) : STM32(capabilities) {
>                 ...
>         }
> }
>
> when constructing a new mcu:
>
> STM32F103RB* mcu = new STM32F103RB(&stm32f103rb_capabilities);
>

Is the capabilities genuinely variable from instance to instance? 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.

> the constructors are executed in the natural 'parent first' order:
>
> CortexM(...)
> STM32(...)
> STM32F103RB(...)
>
> and each object gets access to the capabilities during the construction, as expected.
>
>
> now let's return to the qemu objects I used, where I have
>
> - a 'cortex-mcu' object derived from 'sys-bus-device', which has as member a pointer to the capabilities
> - a 'stm32-mcu' object, derived from 'cortex-mcu'
> - a 'STM32F103RB' object, derived from 'stm32-mcu' (the name matches the CMSIS device name).
>
> the natural place to define the capabilities is the more specfic instance_init
>
> static void stm32f103rb_mcu_instance_init_callback(Object *obj)

No need for _callback.

> {
>     qemu_log_function_name();
>
>     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.

> }
>
>
> 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?

> unfortunately, these calls are executed in reverse order of the constructors, 'child first', which might create problems if references to parent objects are required.
>
> stm32f103rb_mcu_instance_post_init_callback()
> stm32_mcu_instance_post_init_callback()
> ...
> cortexm_mcu_instance_post_init_callback()
> ...
>
> to solve the problem of references to parent objects, the third construction step is performed during realize().
>
> unfortunately realize() is a simple virtual call, and since pointers to virtuals are stored in the class structure and not as separate vtbls, access to parent realize() is not possible directly, but only by manually storing the pointer in a separate parent_realize(0 and explicitly calling it at the beginning of each realize().
>

There is a way :). Check object_class_get_parent. qdev.c uses it to
implement device properties on multiple levels without overriding. You
still need to explicit call to superclass in each child class but that
is normal of OO syntaxes.

>
> ---
>
> another similar problem that required a hack was passing some of the machine fields (kernel_filename & cpu_model) to the mcu constructor.
>

So should kernel_filename instead be handled on the machine level
rather then going into the MCU class? cpu_model should be a class
property, I don't see why instances of a same MCU class need varying
cpu_models.

Even if you want to do this, one way is to use a property
setter-callback on the string which does the cpu init late. property
setters can let you invent new passes to the initialisation if needed
but it is not pretty.

> so, the actual mcu costructor has, in addition to the mcu name ('STM32F103RB'), a pointer to the machine.
>
> for not being able to pass it to the constructor, I had to store it in a static variable and in the constructor refer to it, but this is definitely a non-reentrant kludge:
>
> static MachineState *global_machine;
>
> DeviceState *cortexm_mcu_create(MachineState *machine, const char *mcu_type)
> {
>     /*
>      * Kludge, since it is not possible to pass data to object creation,
>      * we use a static variable.
>      */
>     global_machine = machine;
>     DeviceState *dev;
>     dev = qdev_create(NULL, mcu_type);
>
>     return dev;
> }
>
> static void cortexm_mcu_instance_init_callback(Object *obj)
> {
>     qemu_log_function_name();
>
>     CortexMState *cm_state = CORTEXM_MCU_STATE(obj);
>     assert(global_machine != NULL);
>
>     if (global_machine->kernel_filename) {
>         cm_state->kernel_filename = global_machine->kernel_filename;
>     }
>
>     if (global_machine->cpu_model) {
>         cm_state->cpu_model = global_machine->cpu_model;
>     }
> }
>
> ---
>
> I think that both the above cases show that a mechanism to pass instance data to the constructor is missing from the QOM design.
>
> a bit curious is that the designer considered a similar mechanism to pass class data when creating class objects, but this cannot be used for classes that have multiple instances.
>
>
> to avoid breaking compatibility, the solution that I suggest is to add separate initialisation functions
> - add a new "void* instance_data;" member to Object,

I don't think it should persist in the Object struct like this. If is
analagous to constructor fn arguments, then it is up the specific
class instance_init functions to take the state into their subclass
data.

It doesn't solve the problem of the subclass being able to specify
superclass construction arguments either.

> - add a new function to object_initialize_with_instance_data(), to pass this pointer
> - add a new function qdev_create_with_instance_data() to pass this pointer
>

Better to not patch the qdev API and rather use QOM directly for this
capability.

Regards,
Peter

> calling the existing constructor functions will create objects with this pointer set to NULL, calling the new functions will create objects with this pointer set to the desired value.
>
> please note that this pointer must be set before any call to the instance_init() callbacks.
>
> access to the instance data would be easy, using something like "OBJECT(dev)->instance_data", or, even better, a specialised getter
>
>         void* object_get_instance_data(Object*);
>
> ---
>
> as a conclusion, a very simple thing like passing initialisation data to a C++ constructor requires a pretty sustained effort with the Qemu C implementation. unfortunately, this is also true for some other QOM aspects, like calling parent virtual methods.
>
> a solution to simplify things is to add instance data to the object, and functions to set/get it.
>
> my initial guess is that this solution can be implemented without breaking compatibility with existing code, by adding new functions; the suggested names are obviously not mandatory, any other more inspired names may be used.
>
>
> regards,
>
> Liviu
>
>
> p.s. I generally appreciate creativity and the desire to invent new solutions for various problems; Stroustrup solution to the need of objects was to create a new programming language; your solution to the need of objects, instead of choosing a more appropriate language, was to insist on using C and invent a very, very complicated infrastructure, that requires a lot of explicit code to be manually written, it is relatively error prone, and it still has several design flaws; please believe me, it is quite difficult to outsmart Stroustrup...
>
>
>
>
>

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

[Liviu Ionescu]

> On 13 Jun 2015, at 12:29, Peter Crosthwaite <peter.crosthwaite@xilinx.com> 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.
 
> 
>> }
>> 
>> 
>> 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,
        .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 } } };


the cortexm-mcu constructor needs to know a lot of things to construct the core object and some related devices, and the stm32-mcu constructor needs to know another set of details, to construct specific devices.


>> unfortunately realize() is a simple virtual call, and since pointers to virtuals are stored in the class structure and not as separate vtbls, access to parent realize() is not possible directly, but only by manually storing the pointer in a separate parent_realize(0 and explicitly calling it at the beginning of each realize().
>> 
> 
> There is a way :). Check object_class_get_parent. qdev.c uses it to
> implement device properties on multiple levels without overriding. You
> still need to explicit call to superclass in each child class but that
> is normal of OO syntaxes.

thank you for the hint, I'll try this, but in this case why isn't this mechanism used for realize()? all examples I could find used a local copy  like parent_realize().

> So should kernel_filename instead be handled on the machine level
> rather then going into the MCU class?

the image is currently loaded in the mcu class, but indeed, it might not be needed that deep.

I'll try to move it to the machine level.

> cpu_model should be a class
> property, I don't see why instances of a same MCU class need varying
> cpu_models.

the point is to allow the command line cpu_model to overwrite the board mcu definition.

in other words, even if the board refers to a mcu that is marked as cortex-m4, the command line may change it to a cortex-m3.

> Even if you want to do this, one way is to use a property
> setter-callback on the string which does the cpu init late. property
> setters can let you invent new passes to the initialisation if needed
> but it is not pretty.

I considered this, but it looked even more complicated.

>> to avoid breaking compatibility, the solution that I suggest is to add separate initialisation functions
>> - add a new "void* instance_data;" member to Object,
> 
> I don't think it should persist in the Object struct like this. If is
> analagous to constructor fn arguments, then it is up the specific
> class instance_init functions to take the state into their subclass
> data.
> 
> It doesn't solve the problem of the subclass being able to specify
> superclass construction arguments either.

yes, I also noticed this (during the jogging session I just completed!).

so I invented another solution:

- do not define any of the instance_init() and instance_post_init() 
- add custom construct(...) callbacks to all my classes
- make each instance call parent construct(...) at the beginning, as for any OO constructor, than perform the specific construction
- move the code that now is split between instance_post_init() and realize() into this new construct(...)
- make the realize() functions just call realize() for all its children, without performing any other operations

the use case would look like:

  DeviceState *mcu = cortexm_mcu_alloc(machine, TYPE_STM32F103RB);
  {
    STM32F103R8_CLASS(mcu)->construct();

    qdev_prop_set_uint32(mcu, "some-property", "some value");
    ...
  }
  qdev_realize(mcu);

the stm32f103rb_construct_callback() will call:
 
  stm32_construct_callback(&stm32f103rb_capabilities);
  ...

the stm32_construct_callback(STM32Capabilities *p) will call something like:

  cortexm_construct_callback((CortexMCapabilities*)&(p->cortexm));
  ...

with this approach the alloc() step will be the equivalent of the C++ new() memory allocation and the construct() will be the equivalent of the C++ object construction.

I think this mechanism is generic enough to easily accommodate any number of arguments for the constructors, if needed.


I'll test it shortly and let you know how it behaves.


regards,

Liviu

*) qdev_realize() is the same as qdev_init_nofail(), but with a more intuitive name.

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

[Liviu Ionescu]

> On 13 Jun 2015, at 14:41, Liviu Ionescu <ilg@livius.net> wrote:
> 
> ... so I invented another solution:
> 
> - do not define any of the instance_init() and instance_post_init() 
> - add custom construct(...) callbacks to all my classes
> - make each instance call parent construct(...) at the beginning, as for any OO constructor, than perform the specific construction
> - move the code that now is split between instance_post_init() and realize() into this new construct(...)
> - make the realize() functions just call realize() for all its children, without performing any other operations
> 
> ... with this approach the alloc() step will be the equivalent of the C++ new() memory allocation and the construct() will be the equivalent of the C++ object construction.
> 
> I think this mechanism is generic enough to easily accommodate any number of arguments for the constructors, if needed.
> 
> 
> I'll test it shortly and let you know how it behaves.

well, to me it seems quite fine now, I'm curious what is your opinion.

the board creation code is exactly the following:

------------------------------------------------------
/* ----- Olimex STM32-H103 ----- */

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 };

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);
        qdev_prop_set_uint32(rcc, "hse-freq-hz", 8000000); /* 8.0 MHz */
        qdev_prop_set_uint32(rcc, "lse-freq-hz", 32768); /* 32 KHz */
    }
    qdev_realize(mcu);

    /* Board peripheral objects */
    DeviceState *led = qdev_alloc(NULL, TYPE_STM32_GPIO_LED);
    {
        STM32_GPIO_LED_GET_CLASS(led)->construct(OBJECT(led),
                &h103_machine_green_led, mcu);
    }
    qdev_realize(led);
}

static QEMUMachine stm32_h103_machine = {
    .name = "STM32-H103",
    .desc = "Olimex Header Board for STM32F103RBT6 (Experimental)",
    .init = stm32_h103_board_init_callback };

------------------------------------------------------

the steps to create new objects are:
- alloc
- construct
- optionally set props
- realize

there is a small discussion related to what can be configured with separate properties and what should be passed in the constructor. well, my recommendation is to try to pass as much as possible with properties. if something is needed during construction, make it a constructor argument.

for a complete example, I'll also show the other constructors used in this implementation:

------------------------------------------------------
/* ----- STM32F103RB ----- */
static STM32Capabilities stm32f103rb_capabilities = {
    .cortexm = {
        .device_name = TYPE_STM32F103RB,
        .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 } } };

static void stm32f103rb_mcu_construct_callback(Object *obj,
        MachineState *machine)
{
    qemu_log_function_name();

    STM32_MCU_GET_CLASS(obj)->construct(obj, &stm32f103rb_capabilities,
            machine);
}

------------------------------------------------------

static void stm32_mcu_construct_callback(Object *obj,
        STM32Capabilities* capabilities, MachineState *machine)
{
    qemu_log_function_name();

    CORTEXM_MCU_GET_CLASS(obj)->construct(obj, &(capabilities->cortexm),
            machine);

    assert(capabilities != NULL);

    /* ... construct the STM32 MCU based on capabilities  ... */
}

------------------------------------------------------

static void cortexm_mcu_construct_callback(Object *obj,
        CortexMCapabilities* capabilities, MachineState *machine)
{
    qemu_log_function_name();

    CortexMState *cm_state = CORTEXM_MCU_STATE(obj);

    if (machine->kernel_filename) {
        cm_state->kernel_filename = machine->kernel_filename;
    }

    if (machine->cpu_model) {
        cm_state->cpu_model = machine->cpu_model;
    }

    cm_state->capabilities = capabilities;

    /* ... construct the Cortex-M MCU based on capabilities  ... */
}

------------------------------------------------------

static void stm_gpio_led_construct_callback(Object *obj,
        GenericGPIOLEDInfo* info, DeviceState *mcu)
{
    qemu_log_function_name();

    GENERIC_GPIO_LED_GET_CLASS(obj)->construct(obj, info, mcu);
}

------------------------------------------------------

static void generic_gpio_led_construct_callback(Object *obj,
        GenericGPIOLEDInfo* info, DeviceState *mcu)
{
    qemu_log_function_name();

    GenericGPIOLEDState *state = GENERIC_GPIO_LED_STATE(obj);

    state->info = info;
    state->mcu = mcu;
}

------------------------------------------------------


as it can be seen, calling the parent constructor is relatively straight forward.


to me this mechanism seems ok, it provides the required control on object creation (order and data availability), it is relatively easy to use (within the QOM limits), and the code seems reasonably maintainable. 

if you have no better suggestions, I'll probably use this model for all my future objects.


regards,

Liviu


p.s. please note the introduction of the LED object, intended to greatly simplify the board definitions.

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

[Peter Crosthwaite]

On Sat, Jun 13, 2015 at 4:41 AM, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 13 Jun 2015, at 12:29, Peter Crosthwaite <peter.crosthwaite@xilinx.com> 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

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

[Liviu Ionescu]

> On 13 Jun 2015, at 21:29, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 
> 
> ... 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.

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.

so, no, the class structure is not suitable for multi-instances objects, and even for singletons I think that using it for passing such configuration data is generally abusive.

>> static STM32Capabilities stm32f103rb_capabilities = {
>>    .cortexm = {
>>        .device_name = TYPE_STM32F103RB,
> 
> Why is the capabilities struct aware of its container type?

ah, just for having the information available for verbosity. here is a typical run for a simple blinky application:

GNU ARM Eclipse 64-bits QEMU v2.3.50 (qemu-system-gnuarmeclipse).
Board: 'STM32-H103' (Olimex Header Board for STM32F103RBT6 (Experimental)).
Device: 'STM32F103RB' (Cortex-M3, MPU), Flash: 128 KB, RAM: 20 KB.
Image: '/Users/ilg/My Files/MacBookPro Projects/GNU ARM Eclipse/Eclipse Workspaces/qemu-images-44-osx/f1-h103-blink-sh-shd/Debug/f1-h103-blink-sh-shd.elf'.
Command line: 'cm3 4' (5 bytes).
Cortex-M3 core initialised.
Cortex-M core reset.

main(argc=2, argv=["cm3", "4"]);
Hello ARM World!
Standard output message.
Standard error message.
System clock: 72000000 Hz.
[Green LED Off]
[Green LED On]
[Green LED Off]
Second 1
[Green LED On]
[Green LED Off]
Second 2
[Green LED On]
[Green LED Off]
Second 3
[Green LED On]
[Green LED Off]
Second 4
QEMU exit(0)

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

sure, this can be done, but when using constructors, this requires passing two pointers instead of one.


regards,

Liviu

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

[Liviu Ionescu]

> On 13 Jun 2015, at 21:57, Liviu Ionescu <ilg@livius.net> wrote:
> 
> ... so, no, the class structure is not suitable for multi-instances objects, and even for singletons I think that using it for passing such configuration data is generally abusive.

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.

passing these pointers orderly to the entire hierarchy is very simple when using standard constructors.
 
how would you pass them with the existing .instance_init or .instance_post_init support? I don't think it is possible, which means the entire object construction needs to be delegated to realize(), which is way too late.


for a better understanding of the problem, I'm copying again the same (fully functional) code to create boards:

/* ----- Olimex STM32-H103 ----- */

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 };

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);
        qdev_prop_set_uint32(rcc, "hse-freq-hz", 8000000); /* 8.0 MHz */
        qdev_prop_set_uint32(rcc, "lse-freq-hz", 32768); /* 32 KHz */
    }
    qdev_realize(mcu);

    /* Board peripheral objects */
    DeviceState *led = qdev_alloc(NULL, TYPE_STM32_GPIO_LED);
    {
        STM32_GPIO_LED_GET_CLASS(led)->construct(OBJECT(led),
                &h103_machine_green_led, mcu);
    }
    qdev_realize(led);
}

static QEMUMachine stm32_h103_machine = {
    .name = "STM32-H103",
    .desc = "Olimex Header Board for STM32F103RBT6 (Experimental)",
    .init = stm32_h103_board_init_callback };

-----------------------------------------------

regards,

Liviu

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

[Peter Crosthwaite]

On Sat, Jun 13, 2015 at 12:52 PM, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 13 Jun 2015, at 21:57, Liviu Ionescu <ilg@livius.net> wrote:
>>
>> ... so, no, the class structure is not suitable for multi-instances objects, and even for singletons I think that using it for passing such configuration data is generally abusive.
>
> 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.

> passing these pointers orderly to the entire hierarchy is very simple when using standard constructors.
>
> how would you pass them with the existing .instance_init or .instance_post_init support? I don't think it is possible, which means the entire object construction needs to be delegated to realize(), which is way too late.
>
>
> for a better understanding of the problem, I'm copying again the same (fully functional) code to create boards:
>
> /* ----- Olimex STM32-H103 ----- */
>
> 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?

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

Regards,
Peter

>     }
>     qdev_realize(mcu);
>
>     /* Board peripheral objects */
>     DeviceState *led = qdev_alloc(NULL, TYPE_STM32_GPIO_LED);
>     {
>         STM32_GPIO_LED_GET_CLASS(led)->construct(OBJECT(led),
>                 &h103_machine_green_led, mcu);
>     }
>     qdev_realize(led);
> }
>
> static QEMUMachine stm32_h103_machine = {
>     .name = "STM32-H103",
>     .desc = "Olimex Header Board for STM32F103RBT6 (Experimental)",
>     .init = stm32_h103_board_init_callback };
>
> -----------------------------------------------
>
> regards,
>
> Liviu
>
>

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

[Liviu Ionescu]

> On 14 Jun 2015, at 01:46, Peter Crosthwaite <peter.crosthwaite@xilinx.com> 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.

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

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

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

---

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.

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?


how do you suggest the objects in a hierarchy communicate to each other without constructors and virtuals? 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. 

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


regards,

Liviu

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 <ilg@livius.net> wrote:
>
>> On 14 Jun 2015, at 01:46, Peter Crosthwaite <peter.crosthwaite@xilinx.com> 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
>
>
>
>

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

[Liviu Ionescu]

> On 14 Jun 2015, at 04:49, Peter Crosthwaite <peter.crosthwaite@xilinx.com> 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

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

[Liviu Ionescu]

> On 14 Jun 2015, at 15:43, Liviu Ionescu <ilg@livius.net> wrote:
> 
> 
>> I never said you cant ref one object from another. QOM links exist for
>> this exact purpose.
> 
> Q: any example of QOM links?

ok, I found them, they are not part of qdev but only in qom. 

they look like this:

    object_property_set_link(OBJECT(ds), OBJECT(s), "dma", &local_err);

and apparently they set a string property with the canonical path to the object.

why is this preferred to a direct pointer to the object?


regards,

Liviu

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

[Liviu Ionescu]

> On 14 Jun 2015, at 15:43, Liviu Ionescu <ilg@livius.net> wrote:
> 
> 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.

after adding aliases from the mcu to the internal rcc, I discovered that this is no longer true, if the object creation is done at realize(), the target object and the aliases do not exist before realize, so these settings are not possible.

so, from my point of view, this topic is clear now, realize() is not the right place for construction and the only viable solution is to use explicit constructors.

    DeviceState *mcu = qdev_alloc(NULL, TYPE_STM32F103RB);
    {
        STM32F103RB_GET_CLASS(mcu)->construct(OBJECT(mcu), machine);

        /* Set the board specific oscillator frequencies. */
        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);


one less issue to worry about. :-)


regards,

Liviu

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

[Liviu Ionescu]

> On 14 Jun 2015, at 04:49, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 
> ...
> 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.

can we have multiple machine instances simultaneously, each with its different init args?

if not, a global get_init_args() would do the job even more naturally, and be accessible from anywhere, including from the MCU constructor, without having to bother forwarding it from the machine through multiple constructors.


regards,

Liviu

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

[Peter Crosthwaite]

On Sat, Jun 13, 2015 at 11:57 AM, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 13 Jun 2015, at 21:29, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>>
>>
>> ... 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.
>
> 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.

> so, no, the class structure is not suitable for multi-instances objects, and even for singletons I think that using it for passing such configuration data is generally abusive.
>
>>> static STM32Capabilities stm32f103rb_capabilities = {
>>>    .cortexm = {
>>>        .device_name = TYPE_STM32F103RB,
>>
>> Why is the capabilities struct aware of its container type?
>
> 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?

Regards,
Peter

> GNU ARM Eclipse 64-bits QEMU v2.3.50 (qemu-system-gnuarmeclipse).
> Board: 'STM32-H103' (Olimex Header Board for STM32F103RBT6 (Experimental)).
> Device: 'STM32F103RB' (Cortex-M3, MPU), Flash: 128 KB, RAM: 20 KB.
> Image: '/Users/ilg/My Files/MacBookPro Projects/GNU ARM Eclipse/Eclipse Workspaces/qemu-images-44-osx/f1-h103-blink-sh-shd/Debug/f1-h103-blink-sh-shd.elf'.
> Command line: 'cm3 4' (5 bytes).
> Cortex-M3 core initialised.
> Cortex-M core reset.
>
> main(argc=2, argv=["cm3", "4"]);
> Hello ARM World!
> Standard output message.
> Standard error message.
> System clock: 72000000 Hz.
> [Green LED Off]
> [Green LED On]
> [Green LED Off]
> Second 1
> [Green LED On]
> [Green LED Off]
> Second 2
> [Green LED On]
> [Green LED Off]
> Second 3
> [Green LED On]
> [Green LED Off]
> Second 4
> QEMU exit(0)
>
>> 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.
>
> sure, this can be done, but when using constructors, this requires passing two pointers instead of one.
>
>
> regards,
>
> Liviu
>
>
>
>

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

[Liviu Ionescu]

> On 13 Jun 2015, at 12:29, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> 
> There is a way :). Check object_class_get_parent. qdev.c uses it to
> implement device properties on multiple levels without overriding. You
> still need to explicit call to superclass in each child class but that
> is normal of OO syntaxes.

there is a way, but there is a catch, it must be applied to the explicit class of the current level, not of the current device object, since this might be the class of the child class.

so the detailed syntax to call the parent realize() is:

	DEVICE_CLASS(object_class_get_parent(object_class_by_name(CURRENT_CLASS_TYPE_NAME)))->realize(dev, &local_err);


regards,

Liviu

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

[Peter Crosthwaite]

On Sun, Jun 14, 2015 at 11:47 AM, Liviu Ionescu <ilg@livius.net> wrote:
>
>> On 13 Jun 2015, at 12:29, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>>
>> There is a way :). Check object_class_get_parent. qdev.c uses it to
>> implement device properties on multiple levels without overriding. You
>> still need to explicit call to superclass in each child class but that
>> is normal of OO syntaxes.
>
> there is a way, but there is a catch, it must be applied to the explicit class of the current level, not of the current device object, since this might be the class of the child class.
>
> so the detailed syntax to call the parent realize() is:
>
>         DEVICE_CLASS(object_class_get_parent(object_class_by_name(CURRENT_CLASS_TYPE_NAME)))->realize(dev, &local_err);
>

Yes. We did actually come up with some macros to make this less
verbose at one stage.

Regards,
Peter

>
> regards,
>
> Liviu
>
>
>