Re: [RFC v5 11/12] i386: centralize initialization of cpu accel interfaces

[Claudio Fontana]

On 11/24/20 5:59 PM, Eduardo Habkost wrote:
> On Tue, Nov 24, 2020 at 05:22:09PM +0100, Claudio Fontana wrote:
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> 
> Probably this can be squashed into patch 10/12.


Yes, you are right, no point building things fragmented and then merging 
together later.


> 
>> ---
>>  target/i386/cpu-qom.h |  2 --
>>  target/i386/cpu.c     | 27 ++++++++++++++++++++-------
>>  target/i386/hvf/cpu.c |  9 ---------
>>  target/i386/kvm/cpu.c |  8 --------
>>  target/i386/tcg/cpu.c |  9 ---------
>>  5 files changed, 20 insertions(+), 35 deletions(-)
>>
>> diff --git a/target/i386/cpu-qom.h b/target/i386/cpu-qom.h
>> index 9316e78e71..2cea5394c6 100644
>> --- a/target/i386/cpu-qom.h
>> +++ b/target/i386/cpu-qom.h
>> @@ -98,6 +98,4 @@ struct X86CPUAccelClass {
>>      void (*cpu_realizefn)(X86CPU *cpu, Error **errp);
>>  };
>>  
>> -void x86_cpu_accel_init(const char *accel_name);
>> -
>>  #endif
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index b799723e53..f6fd055046 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -7066,18 +7066,31 @@ type_init(x86_cpu_register_types)
>>  static void x86_cpu_accel_init_aux(ObjectClass *klass, void *opaque)
>>  {
>>      X86CPUClass *xcc = X86_CPU_CLASS(klass);
>> -    const X86CPUAccelClass **accel = opaque;
>> +    X86CPUAccelClass *accel = opaque;
>>  
>> -    xcc->accel = *accel;
>> +    xcc->accel = accel;
>>      xcc->accel->cpu_common_class_init(xcc);
>>  }
>>  
>> -void x86_cpu_accel_init(const char *accel_name)
>> +static void x86_cpu_accel_init(void)
>>  {
>> -    X86CPUAccelClass *acc;
>> +    const char *ac_name;
>> +    ObjectClass *ac;
>> +    char *xac_name;
>> +    ObjectClass *xac;
>>  
>> -    acc = X86_CPU_ACCEL_CLASS(object_class_by_name(accel_name));
>> -    g_assert(acc != NULL);
>> +    ac = object_get_class(OBJECT(current_accel()));
>> +    g_assert(ac != NULL);
>> +    ac_name = object_class_get_name(ac);
>> +    g_assert(ac_name != NULL);
>>  
>> -    object_class_foreach(x86_cpu_accel_init_aux, TYPE_X86_CPU, false, &acc);
>> +    xac_name = g_strdup_printf("%s-%s", ac_name, TYPE_X86_CPU);
>> +    xac = object_class_by_name(xac_name);
>> +    g_free(xac_name);
>> +
>> +    if (xac) {
>> +        object_class_foreach(x86_cpu_accel_init_aux, TYPE_X86_CPU, false, 
>> xac);
>> +    }
>>  }
>> +
>> +accel_cpu_init(x86_cpu_accel_init);
> 
> This keeps the hidden initialization ordering dependency between
> MODULE_INIT_ACCEL_CPU and current_accel().  I thought we were
> going to get rid of module init functions that depend on runtime
> state.
> 
> This is an improvement to the code in patch 10/12, though.  If
> others believe it is an acceptable (temporary) solution, I won't
> block it.


In the way I thought about it, MODULE_INIT_ACCEL_CPU meant exactly that: 
initializations to be done after accel is chosen.
So in my view the relationship with current_accel() was then following 
naturally.



> 
> I would still prefer to have a
>   void arch_accel_cpu_init(AccelState*)
> function which would call a
>   void x86_cpu_accel_init(AccelState*)
> function.  That would make the dependency between
> x86_cpu_accel_init() and accelerator creation explicit.
> 


not a bad idea either,
what I would lose here is a single point to discover the codebase, ie

MODULE_INIT_ACCEL_CPU via a simple grep or gid MODULE_INIT_ACCEL_CPU gives me 
all initializations done
for this phase, not only the arch_ stuff, but also currently the Ops stuff.


> 
>> diff --git a/target/i386/hvf/cpu.c b/target/i386/hvf/cpu.c
>> index 7e7dc044d3..70b6dbfc10 100644
>> --- a/target/i386/hvf/cpu.c
>> +++ b/target/i386/hvf/cpu.c
>> @@ -65,12 +65,3 @@ static void hvf_cpu_accel_register_types(void)
>>      type_register_static(&hvf_cpu_accel_type_info);
>>  }
>>  type_init(hvf_cpu_accel_register_types);
>> -
>> -static void hvf_cpu_accel_init(void)
>> -{
>> -    if (hvf_enabled()) {
>> -        x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("hvf"));
>> -    }
>> -}
>> -
>> -accel_cpu_init(hvf_cpu_accel_init);
>> diff --git a/target/i386/kvm/cpu.c b/target/i386/kvm/cpu.c
>> index bc5f519479..c17ed5a3f2 100644
>> --- a/target/i386/kvm/cpu.c
>> +++ b/target/i386/kvm/cpu.c
>> @@ -147,11 +147,3 @@ static void kvm_cpu_accel_register_types(void)
>>      type_register_static(&kvm_cpu_accel_type_info);
>>  }
>>  type_init(kvm_cpu_accel_register_types);
>> -
>> -static void kvm_cpu_accel_init(void)
>> -{
>> -    if (kvm_enabled()) {
>> -        x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("kvm"));
>> -    }
>> -}
>> -accel_cpu_init(kvm_cpu_accel_init);
>> diff --git a/target/i386/tcg/cpu.c b/target/i386/tcg/cpu.c
>> index e7d4effdd0..00166c36e9 100644
>> --- a/target/i386/tcg/cpu.c
>> +++ b/target/i386/tcg/cpu.c
>> @@ -170,12 +170,3 @@ static void tcg_cpu_accel_register_types(void)
>>      type_register_static(&tcg_cpu_accel_type_info);
>>  }
>>  type_init(tcg_cpu_accel_register_types);
>> -
>> -static void tcg_cpu_accel_init(void)
>> -{
>> -    if (tcg_enabled()) {
>> -        x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("tcg"));
>> -    }
>> -}
>> -
>> -accel_cpu_init(tcg_cpu_accel_init);
>> -- 
>> 2.26.2
>>
>