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(-)
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
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)
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.
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.
>
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!
© 2016 - 2024 Red Hat, Inc.