Re: [PATCH v15 18/23] accel: introduce AccelCPUClass extending CPUClass

[Philippe Mathieu-Daudé]

On 2/3/21 3:49 PM, Claudio Fontana wrote:
> On 2/3/21 3:27 PM, Philippe Mathieu-Daudé wrote:
>> On 2/1/21 11:08 AM, 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>
>>> ---
>>>  include/hw/core/accel-cpu.h | 35 +++++++++++++++++++++++++++++
>>>  include/hw/core/cpu.h       |  1 +
>>>  accel/accel-common.c        | 44 +++++++++++++++++++++++++++++++++++++
>>>  MAINTAINERS                 |  1 +
>>>  4 files changed, 81 insertions(+)
>>>  create mode 100644 include/hw/core/accel-cpu.h
>>>
>>> diff --git a/include/hw/core/accel-cpu.h b/include/hw/core/accel-cpu.h
>>> new file mode 100644
>>> index 0000000000..c7c137dc9a
>>> --- /dev/null
>>> +++ b/include/hw/core/accel-cpu.h
>>> @@ -0,0 +1,35 @@
>>> +/*
>>> + * Accelerator interface, specializes CPUClass
>>> + *
>>> + * Copyright 2021 SUSE LLC
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or 
>>> later.
>>> + * See the COPYING file in the top-level directory.
>>> + */
>>> +
>>> +#ifndef ACCEL_CPU_H
>>> +#define ACCEL_CPU_H
>>> +
>>> +/*
>>> + * these defines cannot be in cpu.h, because we are using
>>> + * CPU_RESOLVING_TYPE here.
>>> + * Use this header to define your accelerator-specific
>>> + * cpu-specific accelerator interfaces.
>>> + */
>>> +
>>> +#define TYPE_ACCEL_CPU "accel-" CPU_RESOLVING_TYPE
>>> +#define ACCEL_CPU_NAME(name) (name "-" TYPE_ACCEL_CPU)
>>> +typedef struct AccelCPUClass AccelCPUClass;
>>> +DECLARE_CLASS_CHECKERS(AccelCPUClass, ACCEL_CPU, TYPE_ACCEL_CPU)
>>> +
>>> +typedef struct AccelCPUClass {
>>> +    /*< private >*/
>>> +    ObjectClass parent_class;
>>> +    /*< public >*/
>>> +
>>> +    void (*cpu_class_init)(CPUClass *cc);
>>> +    void (*cpu_instance_init)(CPUState *cpu);
>>> +    void (*cpu_realizefn)(CPUState *cpu, Error **errp);
>>> +} AccelCPUClass;
>>
>> This header only makes sense if you forward-declare CPUClass
>> in "qemu/typedefs.h", so accelerators don't have to include
>> "hw/core/cpu.h".
>>
> 
> Can you clarify what you mean? I don't see how it follows that this header 
> only makes sense if I forward-declare CPUClass.
> 
> This is necessary for the accel-specific target-specific code that needs to 
> extend cpu classes with Accel CPU interfaces,
> in this series f.e.:
> 
> target/i386/kvm/kvm-cpu.c
> target/i386/hvf/hvf-cpu.c
> target/i386/tcg/tcg-cpu.c

Why not keep theses declarations in "hw/core/cpu.h", rather than
adding a new header? What is the point of the new header?