| Message ID | 20250417165430.58213-1-philmd@linaro.org |
|---|---|
| Headers | show |
| Series | cpus: Replace CPU_RESOLVING_TYPE -> target_cpu_type() | expand |
Maybe it would be preferable to focus on providing a minimal but *complete* TargetInfo before upstreaming any of this, as it's really blocking the rest of the work for single binary. Minimal requirements to have a complete series would be: - Rename QMP TargetInfo struct to use that name - Be able to query target cpu type (what this series does) - Be able to query machine cpu type - Modify generic functions listing machines/cpus to take this into account - Tag labeled boards/cpu in hw/arm to prove this is working (without doing any other cleanup to those files and *not* make them common) - No other additional target information for the v1, let's keep that for later. Note: target_cpu_type will not be TYPE_ARM_CPU, as it wrongly wraps arm32 and aarch64 cpus, while it should correctly identify one or the other. I suggested TYPE_TARGET_CPU_ARM, TYPE_TARGET_CPU_AARCH64, and same for machines: TYPE_TARGET_MACHINE_ARM, TYPE_TARGET_MACHINE_AARCH64. So we can reuse this naming convention with any other target we'll reuse in the future. Pierrick On 4/17/25 09:54, Philippe Mathieu-Daudé wrote: > (Series fully reviewed, I plan to queue it via my tree) > > This series replace the target-specific CPU_RESOLVING_TYPE > by calls to the target-agnostic target_cpu_type() method. > > Since RFCv1: > - Split from bigger/unrelated TargetInfo series (Pierrick) > - Added Pierrick R-b tags > - Added commit descriptions > > Philippe Mathieu-Daudé (7): > qemu: Introduce target_cpu_type() > cpus: Replace CPU_RESOLVING_TYPE -> target_cpu_type() > cpus: Move target-agnostic methods out of cpu-target.c > accel: Implement accel_init_ops_interfaces() for both system/user mode > accel: Include missing 'qemu/accel.h' header in accel-internal.h > accel: Make AccelCPUClass structure target-agnostic > accel: Move target-agnostic code from accel-target.c -> accel-common.c > > MAINTAINERS | 4 +- > meson.build | 2 + > accel/{accel-system.h => accel-internal.h} | 10 +- > include/accel/accel-cpu-target.h | 12 +- > include/accel/accel-cpu.h | 23 ++++ > include/qemu/target_info.h | 19 +++ > accel/accel-common.c | 142 +++++++++++++++++++++ > accel/accel-system.c | 4 +- > accel/accel-target.c | 134 ------------------- > accel/accel-user.c | 6 + > accel/tcg/tcg-all.c | 5 +- > cpu-target.c | 76 +---------- > hw/core/cpu-common.c | 74 +++++++++++ > target_info-defs.c | 16 +++ > accel/meson.build | 1 + > 15 files changed, 299 insertions(+), 229 deletions(-) > rename accel/{accel-system.h => accel-internal.h} (56%) > create mode 100644 include/accel/accel-cpu.h > create mode 100644 include/qemu/target_info.h > create mode 100644 accel/accel-common.c > create mode 100644 target_info-defs.c >
On 4/17/25 11:28, Pierrick Bouvier wrote: > Maybe it would be preferable to focus on providing a minimal but > *complete* TargetInfo before upstreaming any of this, as it's really > blocking the rest of the work for single binary. > > Minimal requirements to have a complete series would be: > - Rename QMP TargetInfo struct to use that name > - Be able to query target cpu type (what this series does) > - Be able to query machine cpu type s/machine cpu/target machine
On 17/4/25 20:28, Pierrick Bouvier wrote: > Maybe it would be preferable to focus on providing a minimal but > *complete* TargetInfo before upstreaming any of this, as it's really > blocking the rest of the work for single binary. I suppose I misunderstood you asking to post these reviewed patches as separate of the TargetInfo series which need more work: "I just feel the last 3 commits, and this one, are a bit disconnected from the series." https://lore.kernel.org/qemu-devel/0b4376ee-504b-4096-a590-8a509ec7894d@linaro.org/ > > Minimal requirements to have a complete series would be: > - Rename QMP TargetInfo struct to use that name > - Be able to query target cpu type (what this series does) > - Be able to query machine cpu type > - Modify generic functions listing machines/cpus to take this into account > - Tag labeled boards/cpu in hw/arm to prove this is working (without > doing any other cleanup to those files and *not* make them common) > - No other additional target information for the v1, let's keep that for > later. > > Note: target_cpu_type will not be TYPE_ARM_CPU, as it wrongly wraps > arm32 and aarch64 cpus, while it should correctly identify one or the > other. I suggested TYPE_TARGET_CPU_ARM, TYPE_TARGET_CPU_AARCH64, and > same for machines: TYPE_TARGET_MACHINE_ARM, TYPE_TARGET_MACHINE_AARCH64. > So we can reuse this naming convention with any other target we'll reuse > in the future. Got it. > > Pierrick
On 4/17/25 11:38, Philippe Mathieu-Daudé wrote: > On 17/4/25 20:28, Pierrick Bouvier wrote: >> Maybe it would be preferable to focus on providing a minimal but >> *complete* TargetInfo before upstreaming any of this, as it's really >> blocking the rest of the work for single binary. > > I suppose I misunderstood you asking to post these reviewed patches as > separate of the TargetInfo series which need more work: > > "I just feel the last 3 commits, and this one, are a bit disconnected > from the series." > You're right, it meant that the 4 commits (accel: *) are extra cleanups and not really related to the series title "Introduce TargetInfo API (for single binary)", which is fine to upstream by itself. However, introducing target_info.h partially just to do this cleanup first sounds a bit weird to me. My complete thought was "This cleanup is ok, but please postpone it once we have TargetInfo API upstream". It's a remark I'll keep doing for TargetInfo, as the goal for v1 is to have a minimal API, introducing the concept of Machine and CPU types, and apply it to hw/arm, which was our initial need and why we talked about this in the first place. Once it's there upstream, we can all enhance it in parallel, with the various needs we'll have for cleaning up other parts of the codebase. My rationale is that it's easy to rebase our conflicting code against a common file, but it's hard to do it if it doesn't exist, thus my insistence on getting a minimal API first. For instance, once it's upstream, we can easily add in different series (and different people): - target_current() -> enum Target - target_aarch64() -> bool and progress without having to first be blocked by the existence of TargetInfo itself. We can as well cherry pick our mutual patches touching TargetInfo without getting blocked by the upstream process itself, and the first series to be pulled will make it available for everyone. > https://lore.kernel.org/qemu-devel/0b4376ee-504b-4096-a590-8a509ec7894d@linaro.org/ > >> >> Minimal requirements to have a complete series would be: >> - Rename QMP TargetInfo struct to use that name >> - Be able to query target cpu type (what this series does) >> - Be able to query machine cpu type >> - Modify generic functions listing machines/cpus to take this into account >> - Tag labeled boards/cpu in hw/arm to prove this is working (without >> doing any other cleanup to those files and *not* make them common) >> - No other additional target information for the v1, let's keep that for >> later. >> >> Note: target_cpu_type will not be TYPE_ARM_CPU, as it wrongly wraps >> arm32 and aarch64 cpus, while it should correctly identify one or the >> other. I suggested TYPE_TARGET_CPU_ARM, TYPE_TARGET_CPU_AARCH64, and >> same for machines: TYPE_TARGET_MACHINE_ARM, TYPE_TARGET_MACHINE_AARCH64. >> So we can reuse this naming convention with any other target we'll reuse >> in the future. > > Got it. > >> >> Pierrick
On 4/17/25 09:54, Philippe Mathieu-Daudé wrote: > Philippe Mathieu-Daudé (7): > qemu: Introduce target_cpu_type() > cpus: Replace CPU_RESOLVING_TYPE -> target_cpu_type() > cpus: Move target-agnostic methods out of cpu-target.c > accel: Implement accel_init_ops_interfaces() for both system/user mode > accel: Include missing 'qemu/accel.h' header in accel-internal.h > accel: Make AccelCPUClass structure target-agnostic > accel: Move target-agnostic code from accel-target.c -> accel-common.c Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
(Series fully reviewed, I plan to queue it via my tree) This series replace the target-specific CPU_RESOLVING_TYPE by calls to the target-agnostic target_cpu_type() method. Since RFCv1: - Split from bigger/unrelated TargetInfo series (Pierrick) - Added Pierrick R-b tags - Added commit descriptions Philippe Mathieu-Daudé (7): qemu: Introduce target_cpu_type() cpus: Replace CPU_RESOLVING_TYPE -> target_cpu_type() cpus: Move target-agnostic methods out of cpu-target.c accel: Implement accel_init_ops_interfaces() for both system/user mode accel: Include missing 'qemu/accel.h' header in accel-internal.h accel: Make AccelCPUClass structure target-agnostic accel: Move target-agnostic code from accel-target.c -> accel-common.c MAINTAINERS | 4 +- meson.build | 2 + accel/{accel-system.h => accel-internal.h} | 10 +- include/accel/accel-cpu-target.h | 12 +- include/accel/accel-cpu.h | 23 ++++ include/qemu/target_info.h | 19 +++ accel/accel-common.c | 142 +++++++++++++++++++++ accel/accel-system.c | 4 +- accel/accel-target.c | 134 ------------------- accel/accel-user.c | 6 + accel/tcg/tcg-all.c | 5 +- cpu-target.c | 76 +---------- hw/core/cpu-common.c | 74 +++++++++++ target_info-defs.c | 16 +++ accel/meson.build | 1 + 15 files changed, 299 insertions(+), 229 deletions(-) rename accel/{accel-system.h => accel-internal.h} (56%) create mode 100644 include/accel/accel-cpu.h create mode 100644 include/qemu/target_info.h create mode 100644 accel/accel-common.c create mode 100644 target_info-defs.c