Re: [RFC PATCH v3 13/14] qemu/target_info: Add target_aarch64() helper

[Markus Armbruster <armbru@redhat.com>]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

[...]

> At this point, I would like to focus on having a first version of TargetInfo API, and not reviewing any other changes, as things may be modified, and they would need to be reviewed again. It's hard to follow the same abstraction done multiple times in multiple series.
>
> Regarding your proposal for target_system_arch(), I understand that you tried to reuse the existing SysEmuTarget, which was a good intention.
> However, I don't think we should use any string compare for this (which qemu_api_parse does). It has several flaws:

qemu_api_parse()?  Do you mean qapi_enum_parse()?

> - The most important one: it can fail (what if -1 is returned?). Enums can be guaranteed and exhaustive at compile time.
> - It's slower than having the current arch directly known at compile time.
> As well, since SysEmuTarget is a generated enum, it makes it much harder to follow code IMHO.
> QAPI requires those things to be defined from a json file for external usage, but it's not a good reason for being forced to use it in all the codebase as the only possible abstraction.
>
> To have something fast and infallible, we can adopt this solution:
>
> In target_info.h:
>
> /* Named TargetArch to not clash with poisoned TARGET_X */
> typedef enum TargetArch {
>     TARGET_ARCH_AARCH64,
>     TARGET_ARCH_ALPHA,
>     TARGET_ARCH_ARM,
>     TARGET_ARCH_AVR,
>     TARGET_ARCH_HPPA,
>     TARGET_ARCH_I386,
>     TARGET_ARCH_LOONGARCH64,
>     TARGET_ARCH_M68K,
>     TARGET_ARCH_MICROBLAZE,
>     TARGET_ARCH_MICROBLAZEEL,
>     TARGET_ARCH_MIPS,
>     TARGET_ARCH_MIPS64,
>     TARGET_ARCH_MIPS64EL,
>     TARGET_ARCH_MIPSEL,
>     TARGET_ARCH_OR1K,
>     TARGET_ARCH_PPC,
>     TARGET_ARCH_PPC64,
>     TARGET_ARCH_RISCV32,
>     TARGET_ARCH_RISCV64,
>     TARGET_ARCH_RX,
>     TARGET_ARCH_S390X,
>     TARGET_ARCH_SH4,
>     TARGET_ARCH_SH4EB,
>     TARGET_ARCH_SPARC,
>     TARGET_ARCH_SPARC64,
>     TARGET_ARCH_TRICORE,
>     TARGET_ARCH_X86_64,
>     TARGET_ARCH_XTENSA,
>     TARGET_ARCH_XTENSAEB,
> } TargetArch;

Effectively duplicates generated enum SysEmuTarget.  Can you explain why
that's useful?

>
> typedef struct TargetInfo {
> ...
> 	TargetArch target_arch;
> ...
> }
>
> static inline target_arch() {
> 	return target_info()->target_arch;
> }
>
> static inline target_aarch64() {
> 	return target_arch() == TARGET_ARCH_AARCH64;
> }
>
>
> In target_info-stub.c:
>
> #ifdef TARGET_AARCH64
> # define TARGET_ARCH TARGET_ARCH_AARCH64
> #elif TARGET_ARCH_ALPHA
> # define TARGET_ARCH TARGET_ARCH_ALPHA
> ...
> #endif
>
> static const TargetInfo target_info_stub = {
>     ...
>     .target_arch = TARGET_ARCH;
>     ...
> }