Re: [PATCH] hw/riscv/spike: Replace tswap64() by ldq_endian_p()

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

On 2/10/24 17:01, Daniel Henrique Barboza wrote:
> On 10/2/24 11:44 AM, Mark Cave-Ayland wrote:
>> On 02/10/2024 15:17, Daniel Henrique Barboza wrote:
>>
>>> Phil, this patch breaks 'make check-avocado' in my env:


>>> On 9/30/24 9:48 AM, Philippe Mathieu-Daudé wrote:
>>>> Hold the target endianness in HTIFState::target_is_bigendian.
>>>> Pass the target endianness as argument to htif_mm_init().
>>>> Replace tswap64() calls by ldq_endian_p() ones.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>> Based-on: <20240930073450.33195-2-philmd@linaro.org>
>>>>            "qemu/bswap: Introduce ld/st_endian_p() API"
>>>>
>>>> Note: this is a non-QOM device!
>>>> ---
>>>>   include/hw/char/riscv_htif.h |  4 +++-
>>>>   hw/char/riscv_htif.c         | 17 +++++++++++------
>>>>   hw/riscv/spike.c             |  2 +-
>>>>   3 files changed, 15 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/include/hw/char/riscv_htif.h 
>>>> b/include/hw/char/riscv_htif.h
>>>> index df493fdf6b..24868ddfe1 100644
>>>> --- a/include/hw/char/riscv_htif.h
>>>> +++ b/include/hw/char/riscv_htif.h
>>>> @@ -35,6 +35,7 @@ typedef struct HTIFState {
>>>>       hwaddr tohost_offset;
>>>>       hwaddr fromhost_offset;
>>>>       MemoryRegion mmio;
>>>> +    bool target_is_bigendian;
>>>>       CharBackend chr;
>>>>       uint64_t pending_read;
>>>> @@ -49,6 +50,7 @@ void htif_symbol_callback(const char *st_name, int 
>>>> st_info, uint64_t st_value,
>>>>   /* legacy pre qom */
>>>>   HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
>>>> -                        uint64_t nonelf_base, bool custom_base);
>>>> +                        uint64_t nonelf_base, bool custom_base,
>>>> +                        bool target_is_bigendian);
>>>>   #endif
>>>> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
>>>> index 9bef60def1..77951f3c76 100644
>>>> --- a/hw/char/riscv_htif.c
>>>> +++ b/hw/char/riscv_htif.c
>>>> @@ -30,7 +30,6 @@
>>>>   #include "qemu/timer.h"
>>>>   #include "qemu/error-report.h"
>>>>   #include "exec/address-spaces.h"
>>>> -#include "exec/tswap.h"
>>>>   #include "sysemu/dma.h"
>>>>   #include "sysemu/runstate.h"
>>>> @@ -211,13 +210,17 @@ static void htif_handle_tohost_write(HTIFState 
>>>> *s, uint64_t val_written)
>>>>                       SHUTDOWN_CAUSE_GUEST_SHUTDOWN, exit_code);
>>>>                   return;
>>>>               } else {
>>>> +                bool be = s->target_is_bigendian;
>>>>                   uint64_t syscall[8];
>>>> +
>>>>                   cpu_physical_memory_read(payload, syscall, 
>>>> sizeof(syscall));
>>>> -                if (tswap64(syscall[0]) == PK_SYS_WRITE &&
>>>> -                    tswap64(syscall[1]) == HTIF_DEV_CONSOLE &&
>>>> -                    tswap64(syscall[3]) == HTIF_CONSOLE_CMD_PUTC) {
>>>> +                if (ldq_endian_p(be, &syscall[0]) == PK_SYS_WRITE &&
>>>> +                    ldq_endian_p(be, &syscall[1]) == 
>>>> HTIF_DEV_CONSOLE &&
>>>> +                    ldq_endian_p(be, &syscall[3]) == 
>>>> HTIF_CONSOLE_CMD_PUTC) {
>>>>                       uint8_t ch;
>>>> -                    cpu_physical_memory_read(tswap64(syscall[2]), 
>>>> &ch, 1);
>>>> +
>>>> +                    cpu_physical_memory_read(ldl_endian_p(be, 
>>>> &syscall[2]),
>>
>>                                                   ^^^^^^^^^^^^
>>
>> Shouldn't this be ldq_endian_p() for a 64-bit value?

Oops, thanks Mark, stupid c/p mistake.

> Bingo! This change fixes make check-avocado and the OpenSBI boot:
> 
> $ git diff
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index 77951f3c76..0ed038a70c 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -219,7 +219,7 @@ static void htif_handle_tohost_write(HTIFState *s, 
> uint64_t val_written)
>                       ldq_endian_p(be, &syscall[3]) == 
> HTIF_CONSOLE_CMD_PUTC) {
>                       uint8_t ch;
> 
> -                    cpu_physical_memory_read(ldl_endian_p(be, 
> &syscall[2]),
> +                    cpu_physical_memory_read(ldq_endian_p(be, 
> &syscall[2]),
>                                                &ch, 1);
>                       qemu_chr_fe_write(&s->chr, &ch, 1);
>                       resp = 0x100 | (uint8_t)payload;
> 
> $ ./build/qemu-system-riscv32 -M spike --nographic
> 
> OpenSBI v1.5.1
>     ____                    _____ ____ _____
>    / __ \                  / ____|  _ \_   _|
>   | |  | |_ __   ___ _ __ | (___ | |_) || |
>   | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
>   | |__| | |_) |  __/ | | |____) | |_) || |_
>    \____/| .__/ \___|_| |_|_____/|____/_____|
>          | |
>          |_|
> (...)

I'll take that as a T-b tag :P Thanks Daniel!