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

[Claudio Fontana]

On 2/3/21 3:51 PM, Philippe Mathieu-Daudé wrote:
> 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.
>>>> + */
>>>> +


This is the comment to read :-)


>>>> +#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?
> 

It is not possible (see comment above).

The header needs to be target-specific, and only can be included by target 
code, or disaster is ensured.
The part that is in hw/core/cpu.h is the part that can be safely included by 
both common and target-specific code.

The accel-cpu.h is target-specific.

This is one of the fallouts of our split of code between common, 
target-specific modules.

It gains us quite a bit in compilation time,
but with the drawback that thinking about these things is quite convoluted 
sometimes.

Ciao,

CLaudio