Re: [PATCH 08/13] hw/xtensa/xtfpga: Replace memcpy()+tswap32() by stl_endian_p()

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 3/10/24 23:34, Pierrick Bouvier wrote:
> On 10/3/24 14:31, Pierrick Bouvier wrote:
>> On 10/3/24 13:48, Philippe Mathieu-Daudé wrote:
>>> On 3/10/24 18:04, Pierrick Bouvier wrote:
>>>> On 10/3/24 09:02, Philippe Mathieu-Daudé wrote:
>>>>> On 30/9/24 16:32, Thomas Huth wrote:
>>>>>> On 30/09/2024 09.34, Philippe Mathieu-Daudé wrote:
>>>>>>> Replace a pair of memcpy() + tswap32() by stl_endian_p(),
>>>>>>> which also swap the value using target endianness.
>>>>>>>
>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>> ---
>>>>>>>      hw/xtensa/xtfpga.c | 6 ++----
>>>>>>>      1 file changed, 2 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
>>>>>>> index 228f00b045..521fe84b01 100644
>>>>>>> --- a/hw/xtensa/xtfpga.c
>>>>>>> +++ b/hw/xtensa/xtfpga.c
>>>>>>> @@ -438,11 +438,9 @@ static void xtfpga_init(const XtfpgaBoardDesc
>>>>>>> *board, MachineState *machine)
>>>>>>>                  const size_t boot_sz = TARGET_BIG_ENDIAN ?
>>>>>>> sizeof(boot_be)
>>>>>>>                                                           :
>>>>>>> sizeof(boot_le);
>>>>>>>                  uint8_t *boot = TARGET_BIG_ENDIAN ? boot_be : 
>>>>>>> boot_le;
>>>>>>> -            uint32_t entry_pc = tswap32(entry_point);
>>>>>>> -            uint32_t entry_a2 = tswap32(tagptr);
>>>>>>> -            memcpy(boot + 4, &entry_pc, sizeof(entry_pc));
>>>>>>> -            memcpy(boot + 8, &entry_a2, sizeof(entry_a2));
>>>>>>> +            stl_endian_p(TARGET_BIG_ENDIAN, boot + 4, entry_point);
>>>>>>> +            stl_endian_p(TARGET_BIG_ENDIAN, boot + 8, tagptr);
>>>>>>
>>>>>> Why don't you simply use stl_p() here?
>>>>>
>>>>> We want to remove the tswap32() calls...
>>>>>
>>>>
>>>> I think is point is that you could directly use stl_be_p, instead of
>>>> stl_endian_p(TARGET_BIT_ENDIAN, ...).
>>>
>>> TARGET_BIG_ENDIAN is defined as 0 on little endian, and 1 on big one.
>>>
>>> The following change isn't worth it:
>>>
>>>      if (TARGET_BIG_ENDIAN) {
>>>        stl_be_p(boot + 8, tagptr);
>>>      } else {
>>>        stl_le_p(boot + 8, tagptr);
>>>      }
>>>
>>> Maybe I'm missing Thomas point, as the xtfpga machines are available
>>> for both xtensa-softmmu (LE) and xtensaeb-softmmu (BE).
>>>
>>>> I don't know if your intent is to make be/le variant "private" and
>>>> relies only on endian_p though.
>>>
>>> My intent is to enforce endian agnostic API uses when possible, and
>>> use LE/BE specific variant when it is known at build time.
>>
>> Oh ok, it's me who missed your point then.
>> For some reason, I thought we were always calling big endian variant.
>>
>> Thus, your implementation makes totally sense.
>>
>> Let's see if Thomas meant something different.
>> Else,
>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> 
> Looking more closely,
> stl_p is already correctly defined when you know at compile time your 
> target endianness. So Thomas was referring to this.
> 
> https://gitlab.com/qemu-project/qemu/-/blame/master/include/exec/cpu-all.h?ref_type=heads#L49

OK I guess I'm seeing Thomas point now; this series cover was not clear
enough. The goal is to remove TARGET_BIG_ENDIAN so we can build half
objects and do a little step toward the single binary.

Maybe I need to kill the stl_p & Co functions first, or do it while
suggesting this target agnostic replacement API.