Re: [RFC v6 10/11] accel: introduce AccelCPUClass extending CPUClass

[Claudio Fontana]

On 11/27/20 6:06 PM, Eduardo Habkost wrote:
> On Thu, Nov 26, 2020 at 11:32:17PM +0100, Claudio Fontana wrote:
>> add a new optional interface to CPUClass,
>> which allows accelerators to extend the CPUClass
>> with additional accelerator-specific initializations.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
> [...]
>> +static void accel_init_cpu_int_aux(ObjectClass *klass, void *opaque)
>> +{
>> +    CPUClass *cc = CPU_CLASS(klass);
>> +    AccelCPUClass *accel_cpu_interface = opaque;
>> +
>> +    cc->accel_cpu_interface = accel_cpu_interface;
>> +    if (accel_cpu_interface->cpu_class_init) {
>> +        accel_cpu_interface->cpu_class_init(cc);
>> +    }
>> +}
> 
> So, now that the approach we're following to trigger the
> accel_init_cpu*() call is less controversial (thanks for your
> patience!), we can try to address the monkey patching issue:
> 
> Monkey patching classes like this is acceptable as an initial
> solution, but I'd like us to have a plan to eventually get rid of
> it.  Monkey patching CPU classes makes querying of CPU model
> information less predictable and subtly dependent on QEMU
> initialization state.


The question of QEMU initialization state and the querying of supported 
functionality, also in relationship with the loadable modules, is I think a 
larger discussion.

Regardless of the amount of glue code and lipstick, this is hiding the fact 
that the fundamentals of the object hierarchy for cpus are wrong,
and are (unfortunately) codified as part of the external interface.


> 
> Removing CPUClass.accel_cpu_interface may be easy, because it
> should be possible to just call current_accel() when realizing
> CPUs.  Getting rid of CPUClass.cpu_class_init might be more
> difficult, depending on what the ->cpu_class_init() function is
> doing.


This seems to be for a next step to me.

Ciao,

Claudio


> 
>> +
>> +/* initialize the arch-specific accel CpuClass interfaces */
>> +static void accel_init_cpu_interfaces(AccelClass *ac, const char *cpu_type)
>> +{
>> +    const char *ac_name; /* AccelClass name */
>> +    char *acc_name;      /* AccelCPUClass name */
>> +    ObjectClass *acc;    /* AccelCPUClass */
>> +
>> +    ac_name = object_class_get_name(OBJECT_CLASS(ac));
>> +    g_assert(ac_name != NULL);
>> +
>> +    acc_name = g_strdup_printf("%s-%s", ac_name, CPU_RESOLVING_TYPE);
>> +    acc = object_class_by_name(acc_name);
>> +    g_free(acc_name);
>> +
>> +    if (acc) {
>> +        object_class_foreach(accel_init_cpu_int_aux, cpu_type, false, acc);
>> +    }
>> +}
>> +
>>  void accel_init_interfaces(AccelClass *ac, const char *cpu_type)
>>  {
>>  #ifndef CONFIG_USER_ONLY
>>      accel_init_ops_interfaces(ac);
>>  #endif /* !CONFIG_USER_ONLY */
>> +
>> +    accel_init_cpu_interfaces(ac, cpu_type);
>>  }
> [...]
>