diff mbox series

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

Message ID 20210201100903.17309-19-cfontana@suse.de (mailing list archive)
State New, archived
Headers show
Series i386 cleanup PART 2 | expand

Commit Message

Claudio Fontana Feb. 1, 2021, 10:08 a.m. UTC 
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

Comments

Philippe Mathieu-Daudé Feb. 3, 2021, 2:27 p.m. UTC | #1 
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".
Claudio Fontana Feb. 3, 2021, 2:49 p.m. UTC | #2 
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

Ciao,

Claudio
Philippe Mathieu-Daudé Feb. 3, 2021, 2:51 p.m. UTC | #3 
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?
Claudio Fontana Feb. 3, 2021, 2:56 p.m. UTC | #4 
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
diff mbox series

Patch

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;
+
+#endif /* ACCEL_CPU_H */
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 3873428330..0477ce402e 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -190,6 +190,7 @@  struct CPUClass {
     /* Keep non-pointer data at the end to minimize holes.  */
     int gdb_num_core_regs;
     bool gdb_stop_before_watchpoint;
+    struct AccelCPUClass *accel_cpu;
 
     /*
      * NB: this should be wrapped by CONFIG_TCG, but it is unsafe to do it here,
diff --git a/accel/accel-common.c b/accel/accel-common.c
index 6b59873419..9901b0531c 100644
--- a/accel/accel-common.c
+++ b/accel/accel-common.c
@@ -26,6 +26,9 @@ 
 #include "qemu/osdep.h"
 #include "qemu/accel.h"
 
+#include "cpu.h"
+#include "hw/core/accel-cpu.h"
+
 #ifndef CONFIG_USER_ONLY
 #include "accel-softmmu.h"
 #endif /* !CONFIG_USER_ONLY */
@@ -46,16 +49,57 @@  AccelClass *accel_find(const char *opt_name)
     return ac;
 }
 
+static void accel_init_cpu_int_aux(ObjectClass *klass, void *opaque)
+{
+    CPUClass *cc = CPU_CLASS(klass);
+    AccelCPUClass *accel_cpu = opaque;
+
+    cc->accel_cpu = accel_cpu;
+    if (accel_cpu->cpu_class_init) {
+        accel_cpu->cpu_class_init(cc);
+    }
+}
+
+/* initialize the arch-specific accel CpuClass interfaces */
+static void accel_init_cpu_interfaces(AccelClass *ac)
+{
+    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-%s", ac_name, CPU_RESOLVING_TYPE);
+    acc = object_class_by_name(acc_name);
+    g_free(acc_name);
+
+    if (acc) {
+        object_class_foreach(accel_init_cpu_int_aux,
+                             CPU_RESOLVING_TYPE, false, acc);
+    }
+}
+
 void accel_init_interfaces(AccelClass *ac)
 {
 #ifndef CONFIG_USER_ONLY
     accel_init_ops_interfaces(ac);
 #endif /* !CONFIG_USER_ONLY */
+
+    accel_init_cpu_interfaces(ac);
 }
 
+static const TypeInfo accel_cpu_type = {
+    .name = TYPE_ACCEL_CPU,
+    .parent = TYPE_OBJECT,
+    .abstract = true,
+    .class_size = sizeof(AccelCPUClass),
+};
+
 static void register_accel_types(void)
 {
     type_register_static(&accel_type);
+    type_register_static(&accel_cpu_type);
 }
 
 type_init(register_accel_types);
diff --git a/MAINTAINERS b/MAINTAINERS
index fc611640ad..b1b64acd05 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -440,6 +440,7 @@  R: Paolo Bonzini <pbonzini@redhat.com>
 S: Maintained
 F: include/qemu/accel.h
 F: include/sysemu/accel-ops.h
+F: include/hw/core/accel-cpu.h
 F: accel/accel-*.c
 F: accel/Makefile.objs
 F: accel/stubs/Makefile.objs