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

[Peter Crosthwaite]

On Fri, Jun 12, 2015 at 4:33 AM, Liviu Ionescu <address@hidden> 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...
>
>
>
>
>