| Message ID | 20240930124831.54232-1-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/riscv/spike: Replace tswap64() by ldq_endian_p() | expand |
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!
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)
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(-)