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

[Claudio Fontana]

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"
if we agree to use "accel-x86_64" aka "accel-" CPU_RESOLVING_TYPE, I would 
lookup "kvm-accel-" CPU_RESOLVING_TYPE

* 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-cpu", ac_name);
    acc = object_class_by_name(acc_name);
    g_free(acc_name);

    if (acc) {
        object_class_foreach(accel_init_cpu_interfaces_aux, cpu_type, false, 
acc);
    }
}

Ciao,

CLaudio