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

[Claudio Fontana]

On 11/26/20 4:48 PM, Eduardo Habkost wrote:
> On Thu, Nov 26, 2020 at 04:34:17PM +0100, Claudio Fontana wrote:
>> On 11/26/20 4:14 PM, Eduardo Habkost wrote:
>>> On Thu, Nov 26, 2020 at 03:55:37PM +0100, Claudio Fontana wrote:
>>>> On 11/26/20 3:49 PM, Eduardo Habkost wrote:
>>>>> On Thu, Nov 26, 2020 at 03:33:17PM +0100, Claudio Fontana wrote:
>>>>>> On 11/26/20 2:44 PM, Eduardo Habkost wrote:
>>>>>>> On Thu, Nov 26, 2020 at 11:57:28AM +0100, Claudio Fontana wrote:
>>>>>>>> On 11/24/20 10:31 PM, Eduardo Habkost wrote:
>>>>>>>>> On Tue, Nov 24, 2020 at 09:13:13PM +0100, Paolo Bonzini wrote:
>>>>>>>>>> On 24/11/20 17:22, Claudio Fontana wrote:
>>>>>>>>>>> +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);
>>>>>>>>>>
>>>>>>>>>> If this and cpus_accel_ops_init are the only call to accel_cpu_init, 
>>>>>>>>>> I'd
>>>>>>>>>> rather make them functions in CPUClass (which you find and call via
>>>>>>>>>> CPU_RESOLVING_TYPE) and AccelClass respectively.
>>>>>>>>>
>>>>>>>>> Making x86_cpu_accel_init() be a CPUClass method sounds like a
>>>>>>>>> good idea.  This way we won't need a arch_cpu_accel_init() stub
>>>>>>>>> for non-x86.
>>>>>>>>>
>>>>>>>>> accel.c can't use cpu.h, correct?  We can add a:
>>>>>>>>>
>>>>>>>>>   CPUClass *arch_base_cpu_type(void)
>>>>>>>>>   {
>>>>>>>>>       return object_class_by_name(CPU_RESOLVING_TYPE);
>>>>>>>>>   }
>>>>>>>>>
>>>>>>>>> function to arch_init.c, to allow target-independent code call
>>>>>>>>> target-specific code.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Eduardo,
>>>>>>>>
>>>>>>>> we can't use arch-init because it is softmmu only, but we could put 
>>>>>>>> this in $(top_srcdir)/cpu.c
>>>>>>>
>>>>>>> That would work, too.
>>>>>>>
>>>>>>>>
>>>>>>>> however, it would be very useful to put a:
>>>>>>>>
>>>>>>>> #define TYPE_ACCEL_CPU "accel-" CPU_RESOLVING_TYPE
>>>>>>>> #define ACCEL_CPU_NAME(name) (name "-" TYPE_ACCEL_CPU)
>>>>>>>>
>>>>>>>> in an H file somewhere, for convenience for the programmer that
>>>>>>>> has to implement subclasses in target/xxx/
>>>>>>>
>>>>>>> Absolutely.
>>>>>>>
>>>>>>>>
>>>>>>>> But it is tough to find a header where CPU_RESOLVING_TYPE can be used.
>>>>>>>
>>>>>>> cpu-all.h?
>>>>>>>
>>>>>>>>
>>>>>>>> We could I guess just use plain "cpu" instead of CPU_RESOLVING_TYPE,
>>>>>>>> maybe that would be acceptable too? The interface ends up in CPUClass, 
>>>>>>>> so maybe ok?
>>>>>>>>
>>>>>>>> So we'd end up having
>>>>>>>>
>>>>>>>> accel-cpu
>>>>>>>>
>>>>>>>> instead of the previous
>>>>>>>>
>>>>>>>> accel-x86_64-cpu
>>>>>>>>
>>>>>>>> on top of the hierarchy.
>>>>>>>
>>>>>>> It seems OK to have a accel-cpu type at the top, but I don't see
>>>>>>> why it solves the problem above.  What exactly would be the value
>>>>>>> of `kvm_cpu_accel.name`?
>>>>>>>
>>>>>>
>>>>>> It does solve the problem, because we can put then all AccelOpsClass and 
>>>>>> AccelCPUClass stuff in accel.h,
>>>>>> resolve everything in accel/accel-*.c, and make a generic solution 
>>>>>> fairly self-contained (already tested, will post soonish).
>>>>>>
>>>>>> But I'll try cpu-all.h if it's preferred to have accel-x86_64-cpu, 
>>>>>> accel-XXX-cpu on top, I wonder what the preference would be?
>>>>>
>>>>> I don't have a specific preference, but I still wonder how
>>>>> exactly you would name the X86CPUAccel implemented at
>>>>> target/i386/kvm, and how exactly you would look for it when
>>>>> initializing the accelerator.
>>>>>
>>>>
>>>> If we agree to use "accel-cpu" I would lookup "kvm-accel-cpu"
>>>
>>> The structure in target/i386/kvm is x86-specific and
>>> kvm-specific.  If we name it "kvm-accel-cpu", how would you name
>>> the equivalent structures at target/s390x/kvm, target/arm/kvm,
>>> target/ppc/kvm?
>>
>> The same way; only one of them would be compiled into the target binary, so 
>> the lookup would not collide in practice,
> 
> That's not always going to be true.  Maybe for KVM it will, but
> not necessarily for TCG.
> 
>> but I wonder whether we want separate names anyway.
> 
> I believe we do.  Avoiding duplicate QOM type names is a good
> idea in either case.
> 

Ok will try, for now I CPU_RESOLVING_TYPE is not playing nice with my attempts..

Ciao,

Claudio