Re: [RFC PATCH v3 01/14] qapi: Rename TargetInfo structure as BinaryTargetInfo

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

On 4/22/25 00:35, Philippe Mathieu-Daudé wrote:
> On 22/4/25 07:55, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>>
>>> The QAPI-generated 'TargetInfo' structure name is only used
>>> in a single file. We want to heavily use another structure
>>> similarly named. Rename the QAPI one, since structure names
>>> are not part of the public API.
>>>
>>> Suggested-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>>    qapi/machine.json          | 12 ++++++------
>>>    hw/core/machine-qmp-cmds.c |  4 ++--
>>>    2 files changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/qapi/machine.json b/qapi/machine.json
>>> index a6b8795b09e..3246212f048 100644
>>> --- a/qapi/machine.json
>>> +++ b/qapi/machine.json
>>> @@ -275,15 +275,15 @@
>>>    { 'command': 'query-current-machine', 'returns': 'CurrentMachineParams' }
>>>    
>>>    ##
>>> -# @TargetInfo:
>>> +# @BinaryTargetInfo:
>>>    #
>>> -# Information describing the QEMU target.
>>> +# Information describing the QEMU binary target.
>>
>> What's "the QEMU binary target"?  The QEMU binary's target?
> 

I agree that "target" is a *very* confusing term in QEMU.
It depends if you adopt the point of view of a compiler (where you 
generate code to *run* on a given target), or the one of an emulator 
(where you translate a target architecture to *run* on your host).

For the context of the single binary here, a target is a target in the 
meaning of our build system: ./configure --target-list=X.
In other words, a target is one of the configuration present in 
configs/targets.

It's a pair of {arch, mode}, where arch is the target architecture 
emulated, and mode is {softmmu,linux,bsd-user}.

Arch could be considered ambiguous, if you consider endianness variants 
(aarch64 vs aarch64_be, or xtensaeb vs xtensa for instance), or even 
"bitness" variants (i386 vs x86_64, arm vs aarch64).
However, for everyone sanity, I think it's better if we follow the exact 
list of targets present in config/targets today, without any change. So 
we consider any variant to be a distinct target, even though they will 
have most of their implementation in common.

Does that help to understand, or is it even more confusing?

> For me 'qemu-system-aarch64' is a QEMU binary, but for Pierrick and
> Richard it is the QEMU target, so I merged both names ¯\_(ツ)_/¯
> 

Mentioning "QEMU binary" reflects an arbitrary choice of the 
architecture we have today, but not what we might have tomorrow.
It happens that at the moment, our target list *is* the existing list of 
QEMU binaries, but it might change in the future.
Indeed, we'll be able to build several targets within a single qemu 
binary. At this point, what will be the rationale to name the targets as 
"binaries"?

If we follow this logic, we should have used: ./configure 
--binary-list=X from the start.

GCC and Clang both have the same semantic for target (which architecture 
to emit code for), but gcc uses separate binaries, while clang has a 
single binary and symlinks, and allows to override target with -target 
command line parameter.

I want to point that it's not a personal preference or taste, but simply 
reflecting what exists in QEMU command line and build system. From 
there, I don't see why we should invent another name for an existing wheel.

> This structure describes the static target configuration built into
> a binary, i.e. TARGET_NAME=aarch64, TARGET_BIG_ENDIAN=false.
> 
> For the forthcoming single/heterogeneous binary, we don't have a
> particular restricted configuration in the binary.
> 
> What about "Information describing the QEMU target configuration
> built in a binary."?
> 
>>   From the QMP user's point of view, perhaps "the QEMU process's target"
>> would make more sense.
> 
> So maybe ProcessTargetInfo is a better structure name.
> 
> For heterogeneous target I suppose we'll return SYS_EMU_TARGET_HETERO
> and we'll provide new QMP commands, possibly returning array of
> ProcessTargetInfo.
> 
>>
>>>    #
>>> -# @arch: the target architecture
>>> +# @arch: the binary target architecture
>>
>> Are there non-binary target architectures?
> 
> :) I won't update this line.
> 
>>
>>>    #
>>>    # Since: 1.2
>>>    ##
>>> -{ 'struct': 'TargetInfo',
>>> +{ 'struct': 'BinaryTargetInfo',
>>>      'data': { 'arch': 'SysEmuTarget' } }
>>>

TargetInfo and query-target are good name choices.
It just happens that it would be more useful to have this name for a 
codebase wide type, instead of a specific type used only by one QMP command.

As well, (QMP) TargetInfo is a struct with a single field, that simply 
exists to workaround a QAPI limitation:
/home/user/.work/qemu/qapi/machine.json: In command 'query-target':
/home/user/.work/qemu/qapi/machine.json:286: command's 'returns' cannot 
take enum type 'SysEmuTarget'

I don't think it's necessary to fix this limitation, or do any deep 
thinking, any name change will free the TargetInfo for what we really 
need. So SysEmuTargetStruct, SysEmuTargetContainer, QMPTargetInfo, 
SysEmuTargetInfo, or any [A-Z]*TargetInfo* is ok for me, as long as we 
don't mention what a "QEMU Binary" is.

>>>    ##
>>> @@ -291,11 +291,11 @@
>>>    #
>>>    # Return information about the target for this QEMU
>>>    #
>>> -# Returns: TargetInfo
>>> +# Returns: BinaryTargetInfo
>>>    #
>>>    # Since: 1.2
>>>    ##
>>> -{ 'command': 'query-target', 'returns': 'TargetInfo' }
>>> +{ 'command': 'query-target', 'returns': 'BinaryTargetInfo' }
>>>    
>>>    ##
>>>    # @UuidInfo:
>>> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
>>> index 3130c5cd456..408994b67d7 100644
>>> --- a/hw/core/machine-qmp-cmds.c
>>> +++ b/hw/core/machine-qmp-cmds.c
>>> @@ -132,9 +132,9 @@ CurrentMachineParams *qmp_query_current_machine(Error **errp)
>>>        return params;
>>>    }
>>>    
>>> -TargetInfo *qmp_query_target(Error **errp)
>>> +BinaryTargetInfo *qmp_query_target(Error **errp)
>>>    {
>>> -    TargetInfo *info = g_malloc0(sizeof(*info));
>>> +    BinaryTargetInfo *info = g_malloc0(sizeof(*info));
>>>    
>>>        info->arch = qapi_enum_parse(&SysEmuTarget_lookup, target_name(), -1,
>>>                                     &error_abort);
>>
>