include/hw/core/cpu.h | 2 ++ target/i386/cpu.h | 3 --- target/ppc/cpu.h | 4 ---- target/s390x/cpu.h | 4 ---- target/s390x/cpu_models.h | 3 +++ target/sparc/cpu.h | 3 --- cpu-target.c | 25 ++++++++++++------------- hw/s390x/s390-virtio-ccw.c | 2 +- target/i386/cpu.c | 3 ++- target/ppc/cpu_init.c | 3 ++- target/s390x/cpu.c | 1 + target/sparc/cpu.c | 3 ++- 12 files changed, 25 insertions(+), 31 deletions(-)
Since v1 (Thomas review comments) - Move s390_set_qemu_cpu_model/s390_cpu_list to "cpu_models.h" - Correct 'target/s390x: Register CPUClass:list_cpus' subject 'cpu_list' might be defined per target, and force code to be built per-target. We can avoid that by introducing a CPUClass callback. This series combined with another which converts CPU_RESOLVING_TYPE to a runtime helper, allows to move most of cpu-target to common. Based-on: <20250324165356.39540-1-philmd@linaro.org> Philippe Mathieu-Daudé (7): cpus: Introduce CPUClass::list_cpus() callback target/i386: Register CPUClass:list_cpus target/ppc: Register CPUClass:list_cpus target/sparc: Register CPUClass:list_cpus target/s390x: Declare s390_set_qemu_cpu_model/cpu_list in cpu_models.h target/s390x: Register CPUClass:list_cpus cpus: Remove #ifdef check on cpu_list definition include/hw/core/cpu.h | 2 ++ target/i386/cpu.h | 3 --- target/ppc/cpu.h | 4 ---- target/s390x/cpu.h | 4 ---- target/s390x/cpu_models.h | 3 +++ target/sparc/cpu.h | 3 --- cpu-target.c | 25 ++++++++++++------------- hw/s390x/s390-virtio-ccw.c | 2 +- target/i386/cpu.c | 3 ++- target/ppc/cpu_init.c | 3 ++- target/s390x/cpu.c | 1 + target/sparc/cpu.c | 3 ++- 12 files changed, 25 insertions(+), 31 deletions(-) -- 2.47.1
On 3/24/25 11:46, Philippe Mathieu-Daudé wrote: > Since v1 (Thomas review comments) > - Move s390_set_qemu_cpu_model/s390_cpu_list to "cpu_models.h" > - Correct 'target/s390x: Register CPUClass:list_cpus' subject > > 'cpu_list' might be defined per target, and force code to be > built per-target. We can avoid that by introducing a CPUClass > callback. > > This series combined with another which converts CPU_RESOLVING_TYPE > to a runtime helper, allows to move most of cpu-target to common. > I think we should focus on a more general solution, usable with machines, cpus, and devices, like the interface based approached we talked about. Having an optional callback, implemented on half target, looks like a half baked solution. And it would not be desirable to do the same thing for machines, and devices later. In case CPU_RESOLVING_TYPE is ambiguous (because 64 bit type inherit from 32 bit type, like TYPE_AARCH64_CPU inherits from TYPE_ARM_CPU), it's better to declare another interface, non ambiguous, like: - TYPE_MACHINE_TARGET_AARCH64 - TYPE_MACHINE_TARGET_ARM - TYPE_CPU_TARGET_AARCH64 - TYPE_CPU_TARGET_ARM - TYPE_DEVICE_TARGET_AARCH64 - TYPE_DEVICE_TARGET_ARM I would be in favor to introduce this for all targets, so all of them are implemented in the exact same way, and the pattern is easy to follow. By using this approach and tagging machines/cpus/devices accordingly, there is no need for any callback anywhere, and the listing code can be generic. As a bonus, we can assert all machines/cpus are properly tagged, something impossible to do with the callbacks approach, which opens room for a possible error, if one of the callback is buggy.
On 24/3/25 19:59, Pierrick Bouvier wrote: > On 3/24/25 11:46, Philippe Mathieu-Daudé wrote: >> Since v1 (Thomas review comments) >> - Move s390_set_qemu_cpu_model/s390_cpu_list to "cpu_models.h" >> - Correct 'target/s390x: Register CPUClass:list_cpus' subject >> >> 'cpu_list' might be defined per target, and force code to be >> built per-target. We can avoid that by introducing a CPUClass >> callback. >> >> This series combined with another which converts CPU_RESOLVING_TYPE >> to a runtime helper, allows to move most of cpu-target to common. >> > > I think we should focus on a more general solution, usable with > machines, cpus, and devices, like the interface based approached we > talked about. > > Having an optional callback, implemented on half target, looks like a > half baked solution. And it would not be desirable to do the same thing > for machines, and devices later. > > In case CPU_RESOLVING_TYPE is ambiguous (because 64 bit type inherit > from 32 bit type, like TYPE_AARCH64_CPU inherits from TYPE_ARM_CPU), > it's better to declare another interface, non ambiguous, like: > - TYPE_MACHINE_TARGET_AARCH64 > - TYPE_MACHINE_TARGET_ARM > - TYPE_CPU_TARGET_AARCH64 > - TYPE_CPU_TARGET_ARM > - TYPE_DEVICE_TARGET_AARCH64 > - TYPE_DEVICE_TARGET_ARM > > I would be in favor to introduce this for all targets, so all of them > are implemented in the exact same way, and the pattern is easy to follow. > > By using this approach and tagging machines/cpus/devices accordingly, > there is no need for any callback anywhere, and the listing code can be > generic. > > As a bonus, we can assert all machines/cpus are properly tagged, > something impossible to do with the callbacks approach, which opens room > for a possible error, if one of the callback is buggy. This is exactly the plan (and what I've in my local branch). But since we need to remove the list_cpus definition (otherwise clash) I went with this surgical cleanup first.
On 3/24/25 12:03, Philippe Mathieu-Daudé wrote: > On 24/3/25 19:59, Pierrick Bouvier wrote: >> On 3/24/25 11:46, Philippe Mathieu-Daudé wrote: >>> Since v1 (Thomas review comments) >>> - Move s390_set_qemu_cpu_model/s390_cpu_list to "cpu_models.h" >>> - Correct 'target/s390x: Register CPUClass:list_cpus' subject >>> >>> 'cpu_list' might be defined per target, and force code to be >>> built per-target. We can avoid that by introducing a CPUClass >>> callback. >>> >>> This series combined with another which converts CPU_RESOLVING_TYPE >>> to a runtime helper, allows to move most of cpu-target to common. >>> >> >> I think we should focus on a more general solution, usable with >> machines, cpus, and devices, like the interface based approached we >> talked about. >> >> Having an optional callback, implemented on half target, looks like a >> half baked solution. And it would not be desirable to do the same thing >> for machines, and devices later. >> >> In case CPU_RESOLVING_TYPE is ambiguous (because 64 bit type inherit >> from 32 bit type, like TYPE_AARCH64_CPU inherits from TYPE_ARM_CPU), >> it's better to declare another interface, non ambiguous, like: >> - TYPE_MACHINE_TARGET_AARCH64 >> - TYPE_MACHINE_TARGET_ARM >> - TYPE_CPU_TARGET_AARCH64 >> - TYPE_CPU_TARGET_ARM >> - TYPE_DEVICE_TARGET_AARCH64 >> - TYPE_DEVICE_TARGET_ARM >> >> I would be in favor to introduce this for all targets, so all of them >> are implemented in the exact same way, and the pattern is easy to follow. >> >> By using this approach and tagging machines/cpus/devices accordingly, >> there is no need for any callback anywhere, and the listing code can be >> generic. >> >> As a bonus, we can assert all machines/cpus are properly tagged, >> something impossible to do with the callbacks approach, which opens room >> for a possible error, if one of the callback is buggy. > > This is exactly the plan (and what I've in my local branch). But since > we need to remove the list_cpus definition (otherwise clash) I went with > this surgical cleanup first. I don't see why it's absolutely needed as a first step. Once we get interface approach implemented, all that is needed is to remove cpu_list code completely (in cpu-target) and replace with the simple new cpu_list() function, based on type tagged by the target. The cpu_list macros and stuff can exist until there without any harm.
On 3/24/25 11:46, Philippe Mathieu-Daudé wrote: > Philippe Mathieu-Daudé (7): > cpus: IntroduceCPUClass::list_cpus() callback > target/i386: RegisterCPUClass:list_cpus > target/ppc: RegisterCPUClass:list_cpus > target/sparc: RegisterCPUClass:list_cpus > target/s390x: Declare s390_set_qemu_cpu_model/cpu_list in cpu_models.h > target/s390x: RegisterCPUClass:list_cpus > cpus: Remove #ifdef check on cpu_list definition Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
© 2016 - 2025 Red Hat, Inc.