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,
but I wonder whether we want separate names anyway.
Ciao,
Claudio
>
> The same question would apply to target/*/tcg*, and to other
> accelerators.
>
>> 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
>>
>
