diff mbox series

[v3,24/24] cpus: Remove CPUClass::has_work() handler

Message ID 20250125170125.32855-25-philmd@linaro.org (mailing list archive)
State New
Headers show
Series cpus: Restrict CPU has_work() handlers to system emulation | expand

Commit Message

Philippe Mathieu-Daudé Jan. 25, 2025, 5:01 p.m. UTC 
All handlers have been converted to SysemuCPUOps::has_work().
Remove CPUClass::has_work along with cpu_common_has_work()
and simplify cpu_has_work(), making SysemuCPUOps::has_work
handler mandatory.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/core/cpu.h            |  3 +--
 include/hw/core/sysemu-cpu-ops.h |  2 +-
 cpu-target.c                     |  8 ++++++++
 hw/core/cpu-common.c             | 12 ++++++------
 hw/core/cpu-system.c             |  7 +------
 5 files changed, 17 insertions(+), 15 deletions(-)

Comments

Richard Henderson Jan. 26, 2025, 12:31 p.m. UTC | #1 
On 1/25/25 09:01, Philippe Mathieu-Daudé wrote:
> diff --git a/cpu-target.c b/cpu-target.c
> index 98e9e7cc4a1..778f622b07a 100644
> --- a/cpu-target.c
> +++ b/cpu-target.c
> @@ -230,6 +230,14 @@ void cpu_class_init_props(DeviceClass *dc)
>       device_class_set_props(dc, cpu_common_props);
>   }
>   
> +void cpu_exec_class_post_init(CPUClass *cc, void *data)
> +{
> +#ifndef CONFIG_USER_ONLY
> +    /* Check mandatory SysemuCPUOps handlers */
> +    g_assert(cc->sysemu_ops->has_work);
> +#endif
> +}
> +

Does this really need to be split from...

> @@ -319,6 +313,11 @@ static void cpu_common_class_init(ObjectClass *klass, void *data)
>       dc->user_creatable = false;
>   }
>   
> +static void cpu_common_post_class_init(ObjectClass *klass, void *data)
> +{
> +    cpu_exec_class_post_init(CPU_CLASS(klass), data);
> +}

... here?


r~
Philippe Mathieu-Daudé Jan. 27, 2025, 7:50 a.m. UTC | #2 
On 26/1/25 13:31, Richard Henderson wrote:
> On 1/25/25 09:01, Philippe Mathieu-Daudé wrote:
>> diff --git a/cpu-target.c b/cpu-target.c
>> index 98e9e7cc4a1..778f622b07a 100644
>> --- a/cpu-target.c
>> +++ b/cpu-target.c
>> @@ -230,6 +230,14 @@ void cpu_class_init_props(DeviceClass *dc)
>>       device_class_set_props(dc, cpu_common_props);
>>   }
>> +void cpu_exec_class_post_init(CPUClass *cc, void *data)
>> +{
>> +#ifndef CONFIG_USER_ONLY
>> +    /* Check mandatory SysemuCPUOps handlers */
>> +    g_assert(cc->sysemu_ops->has_work);
>> +#endif
>> +}
>> +
> 
> Does this really need to be split from...
> 

 > diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
 >
>> @@ -319,6 +313,11 @@ static void cpu_common_class_init(ObjectClass 
>> *klass, void *data)
>>       dc->user_creatable = false;
>>   }
>> +static void cpu_common_post_class_init(ObjectClass *klass, void *data)
>> +{
>> +    cpu_exec_class_post_init(CPU_CLASS(klass), data);
>> +}
> 
> ... here?

cpu-target.c is in specific_ss[], cpu-common.c in common_ss[]
where CONFIG_USER_ONLY is not available thus defined.

We could define cpu_common_post_class_init() in cpu-target.c,
which is odd because not common; or name it differently 
(cpu_exec_post_class_init?) and have it registered as common
post_init handler but again it seems a dirty API mismatch, so
I went this way, hoping that eventually cpu-common.c ends
better abstracted and cpu-target.c disappearing...
Philippe Mathieu-Daudé Feb. 10, 2025, 9:36 p.m. UTC | #3 
On 27/1/25 08:50, Philippe Mathieu-Daudé wrote:
> On 26/1/25 13:31, Richard Henderson wrote:
>> On 1/25/25 09:01, Philippe Mathieu-Daudé wrote:
>>> diff --git a/cpu-target.c b/cpu-target.c
>>> index 98e9e7cc4a1..778f622b07a 100644
>>> --- a/cpu-target.c
>>> +++ b/cpu-target.c
>>> @@ -230,6 +230,14 @@ void cpu_class_init_props(DeviceClass *dc)
>>>       device_class_set_props(dc, cpu_common_props);
>>>   }
>>> +void cpu_exec_class_post_init(CPUClass *cc, void *data)
>>> +{
>>> +#ifndef CONFIG_USER_ONLY
>>> +    /* Check mandatory SysemuCPUOps handlers */
>>> +    g_assert(cc->sysemu_ops->has_work);
>>> +#endif
>>> +}
>>> +
>>
>> Does this really need to be split from...
>>
> 
>  > diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
>  >
>>> @@ -319,6 +313,11 @@ static void cpu_common_class_init(ObjectClass 
>>> *klass, void *data)
>>>       dc->user_creatable = false;
>>>   }
>>> +static void cpu_common_post_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    cpu_exec_class_post_init(CPU_CLASS(klass), data);
>>> +}
>>
>> ... here?
> 
> cpu-target.c is in specific_ss[], cpu-common.c in common_ss[]
> where CONFIG_USER_ONLY is not available thus defined.
> 
> We could define cpu_common_post_class_init() in cpu-target.c,
> which is odd because not common; or name it differently 
> (cpu_exec_post_class_init?) and have it registered as common
> post_init handler but again it seems a dirty API mismatch, so
> I went this way, hoping that eventually cpu-common.c ends
> better abstracted and cpu-target.c disappearing...

I'll amend to the description:

---
Note, since cpu-common.c is in meson's common_ss[] source set, we
must define cpu_exec_class_post_init() in cpu-target.c (which is
in the specific_ss[] source set) to have CONFIG_USER_ONLY defined.
---
Richard Henderson Feb. 10, 2025, 9:38 p.m. UTC | #4 
On 2/10/25 13:36, Philippe Mathieu-Daudé wrote:
> I'll amend to the description:
> 
> ---
> Note, since cpu-common.c is in meson's common_ss[] source set, we
> must define cpu_exec_class_post_init() in cpu-target.c (which is
> in the specific_ss[] source set) to have CONFIG_USER_ONLY defined.
> ---

Excellent, thanks.


r~
diff mbox series

Patch

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index d64c823e768..42795f12290 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -104,7 +104,6 @@  struct SysemuCPUOps;
  *                 instantiatable CPU type.
  * @parse_features: Callback to parse command line arguments.
  * @reset_dump_flags: #CPUDumpFlags to use for reset logging.
- * @has_work: Callback for checking if there is work to do.
  * @mmu_index: Callback for choosing softmmu mmu index;
  *       may be used internally by memory_rw_debug without TCG.
  * @memory_rw_debug: Callback for GDB memory access.
@@ -151,7 +150,6 @@  struct CPUClass {
     ObjectClass *(*class_by_name)(const char *cpu_model);
     void (*parse_features)(const char *typename, char *str, Error **errp);
 
-    bool (*has_work)(CPUState *cpu);
     int (*mmu_index)(CPUState *cpu, bool ifetch);
     int (*memory_rw_debug)(CPUState *cpu, vaddr addr,
                            uint8_t *buf, int len, bool is_write);
@@ -1154,6 +1152,7 @@  G_NORETURN void cpu_abort(CPUState *cpu, const char *fmt, ...)
 
 /* $(top_srcdir)/cpu.c */
 void cpu_class_init_props(DeviceClass *dc);
+void cpu_exec_class_post_init(CPUClass *cc, void *data);
 void cpu_exec_initfn(CPUState *cpu);
 bool cpu_exec_realizefn(CPUState *cpu, Error **errp);
 void cpu_exec_unrealizefn(CPUState *cpu);
diff --git a/include/hw/core/sysemu-cpu-ops.h b/include/hw/core/sysemu-cpu-ops.h
index dee8a62ca98..877892373f9 100644
--- a/include/hw/core/sysemu-cpu-ops.h
+++ b/include/hw/core/sysemu-cpu-ops.h
@@ -19,7 +19,7 @@  typedef struct SysemuCPUOps {
     /**
      * @has_work: Callback for checking if there is work to do.
      */
-    bool (*has_work)(CPUState *cpu);
+    bool (*has_work)(CPUState *cpu); /* MANDATORY NON-NULL */
     /**
      * @get_memory_mapping: Callback for obtaining the memory mappings.
      */
diff --git a/cpu-target.c b/cpu-target.c
index 98e9e7cc4a1..778f622b07a 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -230,6 +230,14 @@  void cpu_class_init_props(DeviceClass *dc)
     device_class_set_props(dc, cpu_common_props);
 }
 
+void cpu_exec_class_post_init(CPUClass *cc, void *data)
+{
+#ifndef CONFIG_USER_ONLY
+    /* Check mandatory SysemuCPUOps handlers */
+    g_assert(cc->sysemu_ops->has_work);
+#endif
+}
+
 void cpu_exec_initfn(CPUState *cpu)
 {
     cpu->as = NULL;
diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
index 886aa793c04..eae621f942f 100644
--- a/hw/core/cpu-common.c
+++ b/hw/core/cpu-common.c
@@ -134,11 +134,6 @@  static void cpu_common_reset_hold(Object *obj, ResetType type)
     cpu_exec_reset_hold(cpu);
 }
 
-static bool cpu_common_has_work(CPUState *cs)
-{
-    return false;
-}
-
 ObjectClass *cpu_class_by_name(const char *typename, const char *cpu_model)
 {
     ObjectClass *oc;
@@ -304,7 +299,6 @@  static void cpu_common_class_init(ObjectClass *klass, void *data)
 
     k->parse_features = cpu_common_parse_features;
     k->get_arch_id = cpu_common_get_arch_id;
-    k->has_work = cpu_common_has_work;
     k->gdb_read_register = cpu_common_gdb_read_register;
     k->gdb_write_register = cpu_common_gdb_write_register;
     set_bit(DEVICE_CATEGORY_CPU, dc->categories);
@@ -319,6 +313,11 @@  static void cpu_common_class_init(ObjectClass *klass, void *data)
     dc->user_creatable = false;
 }
 
+static void cpu_common_post_class_init(ObjectClass *klass, void *data)
+{
+    cpu_exec_class_post_init(CPU_CLASS(klass), data);
+}
+
 static const TypeInfo cpu_type_info = {
     .name = TYPE_CPU,
     .parent = TYPE_DEVICE,
@@ -328,6 +327,7 @@  static const TypeInfo cpu_type_info = {
     .abstract = true,
     .class_size = sizeof(CPUClass),
     .class_init = cpu_common_class_init,
+    .class_post_init = cpu_common_post_class_init,
 };
 
 static void cpu_register_types(void)
diff --git a/hw/core/cpu-system.c b/hw/core/cpu-system.c
index 7b16bda2250..32d09757169 100644
--- a/hw/core/cpu-system.c
+++ b/hw/core/cpu-system.c
@@ -25,12 +25,7 @@ 
 
 bool cpu_has_work(CPUState *cpu)
 {
-    if (cpu->cc->sysemu_ops->has_work) {
-        return cpu->cc->sysemu_ops->has_work(cpu);
-    }
-
-    g_assert(cpu->cc->has_work);
-    return cpu->cc->has_work(cpu);
+    return cpu->cc->sysemu_ops->has_work(cpu);
 }
 
 bool cpu_paging_enabled(const CPUState *cpu)