Re: [RFC PATCH v4 14/19] qemu/target_info: Add %target_arch field to TargetInfo

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 4/23/25 02:14, Philippe Mathieu-Daudé wrote:
> On 23/4/25 08:24, Pierrick Bouvier wrote:
>> On 4/22/25 22:34, Philippe Mathieu-Daudé wrote:
>>> On 22/4/25 20:30, Pierrick Bouvier wrote:
>>>> On 4/22/25 11:24, Philippe Mathieu-Daudé wrote:
>>>>> On 22/4/25 20:20, Pierrick Bouvier wrote:
>>>>>> On 4/22/25 07:54, Philippe Mathieu-Daudé wrote:
>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>> ---
>>>>>>>      include/qemu/target-info-impl.h   | 4 ++++
>>>>>>>      configs/targets/aarch64-softmmu.c | 1 +
>>>>>>>      configs/targets/arm-softmmu.c     | 1 +
>>>>>>>      target-info-stub.c                | 1 +
>>>>>>>      4 files changed, 7 insertions(+)
>>>>>>>
>>>>>>> diff --git a/include/qemu/target-info-impl.h b/include/qemu/target-
>>>>>>> info-impl.h
>>>>>>> index 4ef54c5136a..e5cd169b49a 100644
>>>>>>> --- a/include/qemu/target-info-impl.h
>>>>>>> +++ b/include/qemu/target-info-impl.h
>>>>>>> @@ -10,12 +10,16 @@
>>>>>>>      #define QEMU_TARGET_INFO_IMPL_H
>>>>>>>      #include "qemu/target-info.h"
>>>>>>> +#include "qapi/qapi-types-machine.h"
>>>>>>>      typedef struct TargetInfo {
>>>>>>>          /* runtime equivalent of TARGET_NAME definition */
>>>>>>>          const char *const target_name;
>>>>>>> +    /* related to TARGET_ARCH definition */
>>>>>>> +    SysEmuTarget target_arch;
>>>>>>> +
>>>>>>>          /* QOM typename machines for this binary must implement */
>>>>>>>          const char *const machine_typename;
>>>>>>> diff --git a/configs/targets/aarch64-softmmu.c b/configs/targets/
>>>>>>> aarch64-softmmu.c
>>>>>>> index 375e6fa0b7b..ff89401ea34 100644
>>>>>>> --- a/configs/targets/aarch64-softmmu.c
>>>>>>> +++ b/configs/targets/aarch64-softmmu.c
>>>>>>> @@ -13,6 +13,7 @@
>>>>>>>      static const TargetInfo target_info_aarch64_system = {
>>>>>>>          .target_name = "aarch64",
>>>>>>> +    .target_arch = SYS_EMU_TARGET_AARCH64,
>>>>>>>          .machine_typename = TYPE_TARGET_AARCH64_MACHINE,
>>>>>>>      };
>>>>>>> diff --git a/configs/targets/arm-softmmu.c b/configs/targets/arm-
>>>>>>> softmmu.c
>>>>>>> index d4acdae64f3..22ec9e4faa3 100644
>>>>>>> --- a/configs/targets/arm-softmmu.c
>>>>>>> +++ b/configs/targets/arm-softmmu.c
>>>>>>> @@ -13,6 +13,7 @@
>>>>>>>      static const TargetInfo target_info_arm_system = {
>>>>>>>          .target_name = "arm",
>>>>>>> +    .target_arch = SYS_EMU_TARGET_ARM,
>>>>>>>          .machine_typename = TYPE_TARGET_ARM_MACHINE,
>>>>>>>      };
>>>>>>> diff --git a/target-info-stub.c b/target-info-stub.c
>>>>>>> index 218e5898e7f..e573f5c1975 100644
>>>>>>> --- a/target-info-stub.c
>>>>>>> +++ b/target-info-stub.c
>>>>>>> @@ -12,6 +12,7 @@
>>>>>>>      static const TargetInfo target_info_stub = {
>>>>>>>          .target_name = TARGET_NAME,
>>>>>>> +    .target_arch = -1,
>>>>>>
>>>>>> I think we should have a full ifdef ladder here, to handle all
>>>>>> architectures. Setting -1 is not a safe default.
>>>>>
>>>>> TargetInfo definition is internal to "qemu/target-info-impl.h",
>>>>> otherwise its type is forward-declared as opaque.
>>>>>
>>>>
>>>> Fine, but we need to be able to access to target_arch(), which returns
>>>> the enum value, without having to deal with -1 situation, which is not a
>>>> proper enum value.
>>>>
>>>> switch (target_arch()) {
>>>> case SYS_EMU_TARGET_ARM:
>>>> case SYS_EMU_TARGET_AARCH64:
>>>> ...
>>>> default:
>>>>        break;
>>>> }
>>>
>>> I didn't mentioned that because in
>>> https://lore.kernel.org/qemu-devel/3242cee6-7485-4958-
>>> a198-38d0fc68e8cd@linaro.org/
>>> you said:
>>>
>>>      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.
>>>
>>> What is your "full ifdef ladder" suggestion to avoid -1?
>>
>> #ifdef TARGET_AARCH64
>> # define TARGET_ARCH SYS_EMU_TARGET_AARCH64
>> #elif TARGET_ARCH_ALPHA
>> # define TARGET_ARCH SYS_EMU_TARGET_ALPHA
>> ...
>> #else
>> #error Target architecture can't be detected
> 
> I'm sorry but I don't understand what we gain doing that.
> 

For QAPI generated files, we'll need a various bunch of TARGET_X runtime 
conversion, to enable some commands only for targets.
Some of the concerned targets are different from the one we focus on for 
single binary.

My hope was to include target_arch() function in this series, so it's 
not a debate in next series that will touch to QAPI.
In short, move the noise here and now compared to later with another 
unrelated topic, that will have a lot of heated opinions already.

List of needed (target_()) for QAPI:
TARGET_S390X
TARGET_PPC
TARGET_ARM
TARGET_I386
TARGET_LOONGARCH64
TARGET_MIPS
TARGET_RISCV
TARGET_I386

If it's too much work on your side, would you be open that I provide 
this patch on top of your series, and that we include it with it?

Thanks,
Pierrick