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

Philippe Mathieu-Daudé posted 1 patch 1 week, 1 day ago
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(-)
[PATCH] hw/riscv/spike: Replace tswap64() by ldq_endian_p()
Posted by Philippe Mathieu-Daudé 1 week, 1 day ago
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]),
+                                             &ch, 1);
                     qemu_chr_fe_write(&s->chr, &ch, 1);
                     resp = 0x100 | (uint8_t)payload;
                 } else {
@@ -320,7 +323,8 @@ static const MemoryRegionOps htif_mm_ops = {
 };
 
 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)
 {
     uint64_t base, size, tohost_offset, fromhost_offset;
 
@@ -345,6 +349,7 @@ HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
     s->pending_read = 0;
     s->allow_tohost = 0;
     s->fromhost_inprogress = 0;
+    s->target_is_bigendian = target_is_bigendian;
     qemu_chr_fe_init(&s->chr, chr, &error_abort);
     qemu_chr_fe_set_handlers(&s->chr, htif_can_recv, htif_recv, htif_event,
         htif_be_change, s, NULL, true);
diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
index 64074395bc..0989cd4a41 100644
--- a/hw/riscv/spike.c
+++ b/hw/riscv/spike.c
@@ -327,7 +327,7 @@ static void spike_board_init(MachineState *machine)
 
     /* initialize HTIF using symbols found in load_kernel */
     htif_mm_init(system_memory, serial_hd(0), memmap[SPIKE_HTIF].base,
-                 htif_custom_base);
+                 htif_custom_base, TARGET_BIG_ENDIAN);
 }
 
 static void spike_set_signature(Object *obj, const char *val, Error **errp)
-- 
2.45.2


Re: [PATCH] hw/riscv/spike: Replace tswap64() by ldq_endian_p()
Posted by Daniel Henrique Barboza 6 days, 19 hours ago
Phil, this patch breaks 'make check-avocado' in my env:


   AVOCADO tests/avocado
JOB ID     : 2e98b3ea8b63d22f092ff73bdadfd975cbc27026
JOB LOG    : /home/danielhb/work/qemu/build/tests/results/job-2024-10-02T10.48-2e98b3e/job.log
  (01/15) tests/avocado/riscv_opensbi.py:RiscvOpenSBI.test_riscv32_spike: STARTED
  (01/15) tests/avocado/riscv_opensbi.py:RiscvOpenSBI.test_riscv32_spike: INTERRUPTED: Test interrupted: Timeout reached (5.07 s)
Interrupting job (failfast).
RESULTS    : PASS 0 | ERROR 0 | FAIL 0 | SKIP 14 | WARN 0 | INTERRUPT 1 | CANCEL 0
JOB TIME   : 7.57 s

Test summary:
01-tests/avocado/riscv_opensbi.py:RiscvOpenSBI.test_riscv32_spike: INTERRUPTED
make[1]: *** [/home/danielhb/work/qemu/tests/Makefile.include:141: check-avocado] Error 8


In fact, if you try to execute the 'spike' machine with --nographics, you're
supposed to see the OpenSBI banner and boot. With this patch I don't see
anything:


$ ./build/qemu-system-riscv32 -M spike --nographic
(---nothing---)


For reference this is what I applied on top of master to test it:


9de01b7f1c (HEAD -> review) hw/riscv/spike: Replace tswap64() by ldq_endian_p()
202986f968 hw/net/tulip: Use ld/st_endian_pci_dma() API
30aacb872e hw/pci/pci_device: Introduce ld/st_endian_pci_dma() API
8ee9a2310b hw/pci/pci_device: Add PCI_DMA_DEFINE_LDST_END() macro
54271f92e5 hw/virtio/virtio-access: Use ld/st_endian_phys() API
54ff063593 exec/memory_ldst_phys: Introduce ld/st_endian_phys() API
5749a411cc hw/xtensa/xtfpga: Replace memcpy()+tswap32() by stl_endian_p()
75b7a99a5d hw/xtensa/xtfpga: Remove TARGET_BIG_ENDIAN #ifdef'ry
652016da1e tests/tcg/plugins: Use the ld/st_endian_p() API
d1e4d2544a hw/mips: Add cpu_is_bigendian field to BlCpuCfg structure
6a7b3e09bb hw/mips: Pass BlCpuCfg argument to bootloader API
0466217d0e target/arm/ptw: Use the ld/st_endian_p() API
920a241f00 hw/virtio/virtio-access: Use the ld/st_endian_p() API
e5fc1a2224 qemu/bswap: Introduce ld/st_endian_p() API
062cfce8d4 (origin/master, origin/HEAD, master) Merge tag 'pull-target-arm-20241001' of https://git.linaro.org/people/pmaydell/qemu-arm into staging


Let me know if I did something wrong. Thanks,


Daniel

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]),
> +                                             &ch, 1);
>                       qemu_chr_fe_write(&s->chr, &ch, 1);
>                       resp = 0x100 | (uint8_t)payload;
>                   } else {
> @@ -320,7 +323,8 @@ static const MemoryRegionOps htif_mm_ops = {
>   };
>   
>   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)
>   {
>       uint64_t base, size, tohost_offset, fromhost_offset;
>   
> @@ -345,6 +349,7 @@ HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
>       s->pending_read = 0;
>       s->allow_tohost = 0;
>       s->fromhost_inprogress = 0;
> +    s->target_is_bigendian = target_is_bigendian;
>       qemu_chr_fe_init(&s->chr, chr, &error_abort);
>       qemu_chr_fe_set_handlers(&s->chr, htif_can_recv, htif_recv, htif_event,
>           htif_be_change, s, NULL, true);
> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
> index 64074395bc..0989cd4a41 100644
> --- a/hw/riscv/spike.c
> +++ b/hw/riscv/spike.c
> @@ -327,7 +327,7 @@ static void spike_board_init(MachineState *machine)
>   
>       /* initialize HTIF using symbols found in load_kernel */
>       htif_mm_init(system_memory, serial_hd(0), memmap[SPIKE_HTIF].base,
> -                 htif_custom_base);
> +                 htif_custom_base, TARGET_BIG_ENDIAN);
>   }
>   
>   static void spike_set_signature(Object *obj, const char *val, Error **errp)

Re: [PATCH] hw/riscv/spike: Replace tswap64() by ldq_endian_p()
Posted by Mark Cave-Ayland 6 days, 18 hours ago
On 02/10/2024 15:17, Daniel Henrique Barboza wrote:

> Phil, this patch breaks 'make check-avocado' in my env:
> 
> 
>    AVOCADO tests/avocado
> JOB ID     : 2e98b3ea8b63d22f092ff73bdadfd975cbc27026
> JOB LOG    : 
> /home/danielhb/work/qemu/build/tests/results/job-2024-10-02T10.48-2e98b3e/job.log
>   (01/15) tests/avocado/riscv_opensbi.py:RiscvOpenSBI.test_riscv32_spike: STARTED
>   (01/15) tests/avocado/riscv_opensbi.py:RiscvOpenSBI.test_riscv32_spike: 
> INTERRUPTED: Test interrupted: Timeout reached (5.07 s)
> Interrupting job (failfast).
> RESULTS    : PASS 0 | ERROR 0 | FAIL 0 | SKIP 14 | WARN 0 | INTERRUPT 1 | CANCEL 0
> JOB TIME   : 7.57 s
> 
> Test summary:
> 01-tests/avocado/riscv_opensbi.py:RiscvOpenSBI.test_riscv32_spike: INTERRUPTED
> make[1]: *** [/home/danielhb/work/qemu/tests/Makefile.include:141: check-avocado] 
> Error 8
> 
> 
> In fact, if you try to execute the 'spike' machine with --nographics, you're
> supposed to see the OpenSBI banner and boot. With this patch I don't see
> anything:
> 
> 
> $ ./build/qemu-system-riscv32 -M spike --nographic
> (---nothing---)
> 
> 
> For reference this is what I applied on top of master to test it:
> 
> 
> 9de01b7f1c (HEAD -> review) hw/riscv/spike: Replace tswap64() by ldq_endian_p()
> 202986f968 hw/net/tulip: Use ld/st_endian_pci_dma() API
> 30aacb872e hw/pci/pci_device: Introduce ld/st_endian_pci_dma() API
> 8ee9a2310b hw/pci/pci_device: Add PCI_DMA_DEFINE_LDST_END() macro
> 54271f92e5 hw/virtio/virtio-access: Use ld/st_endian_phys() API
> 54ff063593 exec/memory_ldst_phys: Introduce ld/st_endian_phys() API
> 5749a411cc hw/xtensa/xtfpga: Replace memcpy()+tswap32() by stl_endian_p()
> 75b7a99a5d hw/xtensa/xtfpga: Remove TARGET_BIG_ENDIAN #ifdef'ry
> 652016da1e tests/tcg/plugins: Use the ld/st_endian_p() API
> d1e4d2544a hw/mips: Add cpu_is_bigendian field to BlCpuCfg structure
> 6a7b3e09bb hw/mips: Pass BlCpuCfg argument to bootloader API
> 0466217d0e target/arm/ptw: Use the ld/st_endian_p() API
> 920a241f00 hw/virtio/virtio-access: Use the ld/st_endian_p() API
> e5fc1a2224 qemu/bswap: Introduce ld/st_endian_p() API
> 062cfce8d4 (origin/master, origin/HEAD, master) Merge tag 'pull-target-arm-20241001' 
> of https://git.linaro.org/people/pmaydell/qemu-arm into staging
> 
> 
> Let me know if I did something wrong. Thanks,
> 
> 
> Daniel
> 
> 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?

>> +                                             &ch, 1);




>>                       qemu_chr_fe_write(&s->chr, &ch, 1);
>>                       resp = 0x100 | (uint8_t)payload;
>>                   } else {
>> @@ -320,7 +323,8 @@ static const MemoryRegionOps htif_mm_ops = {
>>   };
>>   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)
>>   {
>>       uint64_t base, size, tohost_offset, fromhost_offset;
>> @@ -345,6 +349,7 @@ HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
>>       s->pending_read = 0;
>>       s->allow_tohost = 0;
>>       s->fromhost_inprogress = 0;
>> +    s->target_is_bigendian = target_is_bigendian;
>>       qemu_chr_fe_init(&s->chr, chr, &error_abort);
>>       qemu_chr_fe_set_handlers(&s->chr, htif_can_recv, htif_recv, htif_event,
>>           htif_be_change, s, NULL, true);
>> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>> index 64074395bc..0989cd4a41 100644
>> --- a/hw/riscv/spike.c
>> +++ b/hw/riscv/spike.c
>> @@ -327,7 +327,7 @@ static void spike_board_init(MachineState *machine)
>>       /* initialize HTIF using symbols found in load_kernel */
>>       htif_mm_init(system_memory, serial_hd(0), memmap[SPIKE_HTIF].base,
>> -                 htif_custom_base);
>> +                 htif_custom_base, TARGET_BIG_ENDIAN);
>>   }
>>   static void spike_set_signature(Object *obj, const char *val, Error **errp)


ATB,

Mark.


Re: [PATCH] hw/riscv/spike: Replace tswap64() by ldq_endian_p()
Posted by Daniel Henrique Barboza 6 days, 18 hours ago

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:
>>
>>
>>    AVOCADO tests/avocado
>> JOB ID     : 2e98b3ea8b63d22f092ff73bdadfd975cbc27026
>> JOB LOG    : /home/danielhb/work/qemu/build/tests/results/job-2024-10-02T10.48-2e98b3e/job.log
>>   (01/15) tests/avocado/riscv_opensbi.py:RiscvOpenSBI.test_riscv32_spike: STARTED
>>   (01/15) tests/avocado/riscv_opensbi.py:RiscvOpenSBI.test_riscv32_spike: INTERRUPTED: Test interrupted: Timeout reached (5.07 s)
>> Interrupting job (failfast).
>> RESULTS    : PASS 0 | ERROR 0 | FAIL 0 | SKIP 14 | WARN 0 | INTERRUPT 1 | CANCEL 0
>> JOB TIME   : 7.57 s
>>
>> Test summary:
>> 01-tests/avocado/riscv_opensbi.py:RiscvOpenSBI.test_riscv32_spike: INTERRUPTED
>> make[1]: *** [/home/danielhb/work/qemu/tests/Makefile.include:141: check-avocado] Error 8
>>
>>
>> In fact, if you try to execute the 'spike' machine with --nographics, you're
>> supposed to see the OpenSBI banner and boot. With this patch I don't see
>> anything:
>>
>>
>> $ ./build/qemu-system-riscv32 -M spike --nographic
>> (---nothing---)
>>
>>
>> For reference this is what I applied on top of master to test it:
>>
>>
>> 9de01b7f1c (HEAD -> review) hw/riscv/spike: Replace tswap64() by ldq_endian_p()
>> 202986f968 hw/net/tulip: Use ld/st_endian_pci_dma() API
>> 30aacb872e hw/pci/pci_device: Introduce ld/st_endian_pci_dma() API
>> 8ee9a2310b hw/pci/pci_device: Add PCI_DMA_DEFINE_LDST_END() macro
>> 54271f92e5 hw/virtio/virtio-access: Use ld/st_endian_phys() API
>> 54ff063593 exec/memory_ldst_phys: Introduce ld/st_endian_phys() API
>> 5749a411cc hw/xtensa/xtfpga: Replace memcpy()+tswap32() by stl_endian_p()
>> 75b7a99a5d hw/xtensa/xtfpga: Remove TARGET_BIG_ENDIAN #ifdef'ry
>> 652016da1e tests/tcg/plugins: Use the ld/st_endian_p() API
>> d1e4d2544a hw/mips: Add cpu_is_bigendian field to BlCpuCfg structure
>> 6a7b3e09bb hw/mips: Pass BlCpuCfg argument to bootloader API
>> 0466217d0e target/arm/ptw: Use the ld/st_endian_p() API
>> 920a241f00 hw/virtio/virtio-access: Use the ld/st_endian_p() API
>> e5fc1a2224 qemu/bswap: Introduce ld/st_endian_p() API
>> 062cfce8d4 (origin/master, origin/HEAD, master) Merge tag 'pull-target-arm-20241001' of https://git.linaro.org/people/pmaydell/qemu-arm into staging
>>
>>
>> Let me know if I did something wrong. Thanks,
>>
>>
>> Daniel
>>
>> 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?

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
    ____                    _____ ____ _____
   / __ \                  / ____|  _ \_   _|
  | |  | |_ __   ___ _ __ | (___ | |_) || |
  | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
  | |__| | |_) |  __/ | | |____) | |_) || |_
   \____/| .__/ \___|_| |_|_____/|____/_____|
         | |
         |_|
(...)



Thanks,

Daniel

> 
>>> +                                             &ch, 1);
> 
> 
> 
> 
>>>                       qemu_chr_fe_write(&s->chr, &ch, 1);
>>>                       resp = 0x100 | (uint8_t)payload;
>>>                   } else {
>>> @@ -320,7 +323,8 @@ static const MemoryRegionOps htif_mm_ops = {
>>>   };
>>>   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)
>>>   {
>>>       uint64_t base, size, tohost_offset, fromhost_offset;
>>> @@ -345,6 +349,7 @@ HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
>>>       s->pending_read = 0;
>>>       s->allow_tohost = 0;
>>>       s->fromhost_inprogress = 0;
>>> +    s->target_is_bigendian = target_is_bigendian;
>>>       qemu_chr_fe_init(&s->chr, chr, &error_abort);
>>>       qemu_chr_fe_set_handlers(&s->chr, htif_can_recv, htif_recv, htif_event,
>>>           htif_be_change, s, NULL, true);
>>> diff --git a/hw/riscv/spike.c b/hw/riscv/spike.c
>>> index 64074395bc..0989cd4a41 100644
>>> --- a/hw/riscv/spike.c
>>> +++ b/hw/riscv/spike.c
>>> @@ -327,7 +327,7 @@ static void spike_board_init(MachineState *machine)
>>>       /* initialize HTIF using symbols found in load_kernel */
>>>       htif_mm_init(system_memory, serial_hd(0), memmap[SPIKE_HTIF].base,
>>> -                 htif_custom_base);
>>> +                 htif_custom_base, TARGET_BIG_ENDIAN);
>>>   }
>>>   static void spike_set_signature(Object *obj, const char *val, Error **errp)
> 
> 
> ATB,
> 
> Mark.
> 

Re: [PATCH] hw/riscv/spike: Replace tswap64() by ldq_endian_p()
Posted by Philippe Mathieu-Daudé 5 days, 12 hours ago
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!