Re: [PATCH 05/16] qemu/bswap: implement {ld,st}.*_p as functions

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

On 3/10/25 13:17, BALATON Zoltan wrote:
> On Mon, 10 Mar 2025, Pierrick Bouvier wrote:
>> On 3/10/25 09:53, Richard Henderson wrote:
>>> On 3/10/25 09:43, Pierrick Bouvier wrote:
>>>> On 3/10/25 09:37, Richard Henderson wrote:
>>>>> On 3/10/25 09:14, Pierrick Bouvier wrote:
>>>>>> On 3/10/25 09:08, Richard Henderson wrote:
>>>>>>> On 3/9/25 21:58, Pierrick Bouvier wrote:
>>>>>>>> For now, they are duplicate of the same macros in cpu-all.h that we
>>>>>>>> eliminate in next commit.
>>>>>>>>
>>>>>>>> Keep code readable by not defining them with macros, but simply their
>>>>>>>> implementation.
>>>>>>>>
>>>>>>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>>>>>>
>>>>>>> Why do you want these in bswap.h, rather than tswap.h?
>>>>>>> They're target swaps, after all.
>>>>>>>
>>>>>>>
>>>>>>> r~
>>>>>>
>>>>>> No preference on that, I simply added them to the same file than their
>>>>>> explicit endianness
>>>>>> variant. Would you prefer the endianness agnostic variant to be in
>>>>>> tswap.h instead?
>>>>>
>>>>> I think I would.
>>>>
>>>> Ok, I will move it.
>>>>
>>>>>
>>>>> In addition, I think we want
>>>>>
>>>>> #ifdef COMPILING_PER_TARGET
>>>>> #define target_words_bigendian()  TARGET_BIG_ENDIAN
>>>>> #else
>>>>> bool target_words_bigendian(void);
>>>>> #endif
>>>>>
>>>>> moving the conditional from around target_needs_bswap just below.
>>>>>
>>>>> With that, we eliminate the extra branch that you're otherwise
>>>>> adding to target-specific code with this patch.
>>>>>
>>>>
>>>> I understand the change requested, but should we really aim in that
>>>> direction? In the end,
>>>> if we pursue the compilation units deduplication, the branch will be
>>>> present anyway.
>>>>
>>>> I'm ok with your change, just asking if we really want to preserve target
>>>> specific code
>>>> until the "end".
>>>
>>> All of target/ is target specific.  De-duplication will not eliminate that.
>>>
>>
>> My vocabulary was wrong here. I meant "if we want to preserve target specific
>> macros" until the end.
>> Sure, there will always be compilation units (devices, cpus, helpers, ...)
>> specific to a target. I just wonder if sticking to ifdef paradigm for this
>> kind of code is worth the "optimization" we are supposed to get.
> 
> I've already tried to say that in the previous reply but maybe I can
> explain it better here. I think keeping per target binaries would be
> desired so single binary would not replace it just become an additional
> option. For example when I want to play with old stuff I compile with
> --target-list=ppc-softmmu and don't want to wait compiling all the other
> targets I don't use and not even interested in PPC64. A distro may want to
> ship a single qemu-system binary instead but other distros may prefer per
> target packages not one huge package so users can decide which ones to
> install. All of these are valid use cases, therefore this single binary
> should be an additional option not the only true way from now on
> replacing existing per target builds.
> 

This valid use case is and *will* stay the default. There is no secret 
evil plan to break people habits here :).

For now, we don't even introduce a "single binary" configure option, 
simply because the resulting code could not link, so there is no point.

If you're curious about this, it is the command we use to check the work 
left before being able to do it:
# configure with any target list, we use all in our case
$ ./configure ...
$ jq --raw-output < build/compile_commands.json '.[].file' | sort |
   uniq -c | sort -rn | grep -v '^\s*1 '

Once this list is empty, we'll be able to link and execute a single 
binary (and adding a additional new main() for that). But first things 
first.

> Regards,
> BALATON Zoltan
> 
>> I'll add the change requested.
>>
>>>
>>> r~
>>
>>