[PATCH-for-10.0 0/3] hw/char/riscv_htif: Remove tswap64() calls

[PATCH-for-10.0 0/3] hw/char/riscv_htif: Remove tswap64() calls

[Philippe Mathieu-Daudé]

Trying to make sense of these tswap64 calls I
figured this device could be simplified.

Tested using 'make check-{qtest,functional}'
on both big/little endian hosts, no failure but
I'm not sure the code path is covered.

Philippe Mathieu-Daudé (3):
  MAINTAINERS: Cover RISC-V HTIF interface
  hw/char/riscv_htif: Explicit little-endian implementation
  hw/char/riscv_htif: Clarify MemoryRegionOps expect 32-bit accesses

 MAINTAINERS          |  2 ++
 hw/char/riscv_htif.c | 15 ++++++++++-----
 2 files changed, 12 insertions(+), 5 deletions(-)

-- 
2.45.2

[PATCH-for-10.0 1/3] MAINTAINERS: Cover RISC-V HTIF interface

[Philippe Mathieu-Daudé]

The HTIF interface is RISC-V specific, add
it within the MAINTAINERS section covering
hw/riscv/.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
IMHO 'RISC-V TCG CPUs' should cover target/riscv/ which are
the accelerator-facing implementations, and each machine or
device in hw/riscv/ should have its own section. Not going
to clean that in this patch.
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2b1c4abed65..046e05dd28d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -324,8 +324,10 @@ S: Supported
 F: configs/targets/riscv*
 F: docs/system/target-riscv.rst
 F: target/riscv/
+F: hw/char/riscv_htif.c
 F: hw/riscv/
 F: hw/intc/riscv*
+F: include/hw/char/riscv_htif.h
 F: include/hw/riscv/
 F: linux-user/host/riscv32/
 F: linux-user/host/riscv64/
-- 
2.45.2

Re: [PATCH-for-10.0 1/3] MAINTAINERS: Cover RISC-V HTIF interface

[Daniel Henrique Barboza]

On 11/29/24 12:43 PM, Philippe Mathieu-Daudé wrote:
> The HTIF interface is RISC-V specific, add
> it within the MAINTAINERS section covering
> hw/riscv/.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

> IMHO 'RISC-V TCG CPUs' should cover target/riscv/ which are
> the accelerator-facing implementations, and each machine or
> device in hw/riscv/ should have its own section. Not going
> to clean that in this patch.
> ---
>   MAINTAINERS | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2b1c4abed65..046e05dd28d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -324,8 +324,10 @@ S: Supported
>   F: configs/targets/riscv*
>   F: docs/system/target-riscv.rst
>   F: target/riscv/
> +F: hw/char/riscv_htif.c
>   F: hw/riscv/
>   F: hw/intc/riscv*
> +F: include/hw/char/riscv_htif.h
>   F: include/hw/riscv/
>   F: linux-user/host/riscv32/
>   F: linux-user/host/riscv64/

Re: [PATCH-for-10.0 1/3] MAINTAINERS: Cover RISC-V HTIF interface

[Alistair Francis]

On Sat, Nov 30, 2024 at 12:44 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> The HTIF interface is RISC-V specific, add
> it within the MAINTAINERS section covering
> hw/riscv/.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> IMHO 'RISC-V TCG CPUs' should cover target/riscv/ which are
> the accelerator-facing implementations, and each machine or
> device in hw/riscv/ should have its own section. Not going
> to clean that in this patch.
> ---
>  MAINTAINERS | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2b1c4abed65..046e05dd28d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -324,8 +324,10 @@ S: Supported
>  F: configs/targets/riscv*
>  F: docs/system/target-riscv.rst
>  F: target/riscv/
> +F: hw/char/riscv_htif.c
>  F: hw/riscv/
>  F: hw/intc/riscv*
> +F: include/hw/char/riscv_htif.h
>  F: include/hw/riscv/
>  F: linux-user/host/riscv32/
>  F: linux-user/host/riscv64/
> --
> 2.45.2
>
>

[PATCH-for-10.0 2/3] hw/char/riscv_htif: Explicit little-endian implementation

[Philippe Mathieu-Daudé]

Since our RISC-V system emulation is only built for little
endian, the HTIF device aims to interface with little endian
memory accesses, thus we can explicit htif_mm_ops:endianness
being DEVICE_LITTLE_ENDIAN.

In that case tswap64() is equivalent to le64_to_cpu(), as in
"convert this 64-bit little-endian value into host cpu order".
Replace to simplify.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/char/riscv_htif.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
index 0345088e8b3..3f84d8d6738 100644
--- a/hw/char/riscv_htif.c
+++ b/hw/char/riscv_htif.c
@@ -29,7 +29,7 @@
 #include "qemu/timer.h"
 #include "qemu/error-report.h"
 #include "exec/address-spaces.h"
-#include "exec/tswap.h"
+#include "qemu/bswap.h"
 #include "sysemu/dma.h"
 #include "sysemu/runstate.h"
 
@@ -212,11 +212,11 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
             } else {
                 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 (le64_to_cpu(syscall[0]) == PK_SYS_WRITE &&
+                    le64_to_cpu(syscall[1]) == HTIF_DEV_CONSOLE &&
+                    le64_to_cpu(syscall[3]) == HTIF_CONSOLE_CMD_PUTC) {
                     uint8_t ch;
-                    cpu_physical_memory_read(tswap64(syscall[2]), &ch, 1);
+                    cpu_physical_memory_read(le64_to_cpu(syscall[2]), &ch, 1);
                     /*
                      * XXX this blocks entire thread. Rewrite to use
                      * qemu_chr_fe_write and background I/O callbacks
@@ -324,6 +324,7 @@ static void htif_mm_write(void *opaque, hwaddr addr,
 static const MemoryRegionOps htif_mm_ops = {
     .read = htif_mm_read,
     .write = htif_mm_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
 HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
-- 
2.45.2

Re: [PATCH-for-10.0 2/3] hw/char/riscv_htif: Explicit little-endian implementation

[Daniel Henrique Barboza]

On 11/29/24 12:43 PM, Philippe Mathieu-Daudé wrote:
> Since our RISC-V system emulation is only built for little
> endian, the HTIF device aims to interface with little endian
> memory accesses, thus we can explicit htif_mm_ops:endianness
> being DEVICE_LITTLE_ENDIAN.
> 
> In that case tswap64() is equivalent to le64_to_cpu(), as in
> "convert this 64-bit little-endian value into host cpu order".
> Replace to simplify.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

>   hw/char/riscv_htif.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index 0345088e8b3..3f84d8d6738 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -29,7 +29,7 @@
>   #include "qemu/timer.h"
>   #include "qemu/error-report.h"
>   #include "exec/address-spaces.h"
> -#include "exec/tswap.h"
> +#include "qemu/bswap.h"
>   #include "sysemu/dma.h"
>   #include "sysemu/runstate.h"
>   
> @@ -212,11 +212,11 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
>               } else {
>                   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 (le64_to_cpu(syscall[0]) == PK_SYS_WRITE &&
> +                    le64_to_cpu(syscall[1]) == HTIF_DEV_CONSOLE &&
> +                    le64_to_cpu(syscall[3]) == HTIF_CONSOLE_CMD_PUTC) {
>                       uint8_t ch;
> -                    cpu_physical_memory_read(tswap64(syscall[2]), &ch, 1);
> +                    cpu_physical_memory_read(le64_to_cpu(syscall[2]), &ch, 1);
>                       /*
>                        * XXX this blocks entire thread. Rewrite to use
>                        * qemu_chr_fe_write and background I/O callbacks
> @@ -324,6 +324,7 @@ static void htif_mm_write(void *opaque, hwaddr addr,
>   static const MemoryRegionOps htif_mm_ops = {
>       .read = htif_mm_read,
>       .write = htif_mm_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>   };
>   
>   HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,

Re: [PATCH-for-10.0 2/3] hw/char/riscv_htif: Explicit little-endian implementation

[Alistair Francis]

On Sat, Nov 30, 2024 at 12:43 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Since our RISC-V system emulation is only built for little
> endian, the HTIF device aims to interface with little endian
> memory accesses, thus we can explicit htif_mm_ops:endianness
> being DEVICE_LITTLE_ENDIAN.
>
> In that case tswap64() is equivalent to le64_to_cpu(), as in
> "convert this 64-bit little-endian value into host cpu order".
> Replace to simplify.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/char/riscv_htif.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index 0345088e8b3..3f84d8d6738 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -29,7 +29,7 @@
>  #include "qemu/timer.h"
>  #include "qemu/error-report.h"
>  #include "exec/address-spaces.h"
> -#include "exec/tswap.h"
> +#include "qemu/bswap.h"
>  #include "sysemu/dma.h"
>  #include "sysemu/runstate.h"
>
> @@ -212,11 +212,11 @@ static void htif_handle_tohost_write(HTIFState *s, uint64_t val_written)
>              } else {
>                  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 (le64_to_cpu(syscall[0]) == PK_SYS_WRITE &&
> +                    le64_to_cpu(syscall[1]) == HTIF_DEV_CONSOLE &&
> +                    le64_to_cpu(syscall[3]) == HTIF_CONSOLE_CMD_PUTC) {
>                      uint8_t ch;
> -                    cpu_physical_memory_read(tswap64(syscall[2]), &ch, 1);
> +                    cpu_physical_memory_read(le64_to_cpu(syscall[2]), &ch, 1);
>                      /*
>                       * XXX this blocks entire thread. Rewrite to use
>                       * qemu_chr_fe_write and background I/O callbacks
> @@ -324,6 +324,7 @@ static void htif_mm_write(void *opaque, hwaddr addr,
>  static const MemoryRegionOps htif_mm_ops = {
>      .read = htif_mm_read,
>      .write = htif_mm_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>
>  HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
> --
> 2.45.2
>
>

[PATCH-for-10.0 3/3] hw/char/riscv_htif: Clarify MemoryRegionOps expect 32-bit accesses

[Philippe Mathieu-Daudé]

Looking at htif_mm_ops[] read/write handlers, we notice they
expect 32-bit values to accumulate into to the 'fromhost' and
'tohost' 64-bit variables. Explicit by setting the .impl
min/max fields.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
Notes

1/ these variables belong to HTIFState but are declared statically!

  static uint64_t fromhost_addr, tohost_addr, begin_sig_addr, end_sig_addr;

2/ I believe a 64-bit implementation would simplify the logic.

3/ This is a non-QOM device model!

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/char/riscv_htif.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
index 3f84d8d6738..db69b5e3ca7 100644
--- a/hw/char/riscv_htif.c
+++ b/hw/char/riscv_htif.c
@@ -325,6 +325,10 @@ static const MemoryRegionOps htif_mm_ops = {
     .read = htif_mm_read,
     .write = htif_mm_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
 };
 
 HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
-- 
2.45.2

Re: [PATCH-for-10.0 3/3] hw/char/riscv_htif: Clarify MemoryRegionOps expect 32-bit accesses

[Daniel Henrique Barboza]

On 11/29/24 12:43 PM, Philippe Mathieu-Daudé wrote:
> Looking at htif_mm_ops[] read/write handlers, we notice they
> expect 32-bit values to accumulate into to the 'fromhost' and
> 'tohost' 64-bit variables. Explicit by setting the .impl
> min/max fields.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> Notes
> 
> 1/ these variables belong to HTIFState but are declared statically!
> 
>    static uint64_t fromhost_addr, tohost_addr, begin_sig_addr, end_sig_addr;
> 
> 2/ I believe a 64-bit implementation would simplify the logic.


Perhaps. We would need to check with the protocol first. There might be
a reason why we're using 64 bit read/writes internally while doing 32 bits
r/w in the device.


For now:

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>

> 
> 3/ This is a non-QOM device model!
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/char/riscv_htif.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index 3f84d8d6738..db69b5e3ca7 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -325,6 +325,10 @@ static const MemoryRegionOps htif_mm_ops = {
>       .read = htif_mm_read,
>       .write = htif_mm_write,
>       .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
>   };
>   
>   HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,

Re: [PATCH-for-10.0 3/3] hw/char/riscv_htif: Clarify MemoryRegionOps expect 32-bit accesses

[Alistair Francis]

On Sat, Nov 30, 2024 at 12:45 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Looking at htif_mm_ops[] read/write handlers, we notice they
> expect 32-bit values to accumulate into to the 'fromhost' and
> 'tohost' 64-bit variables. Explicit by setting the .impl
> min/max fields.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> Notes
>
> 1/ these variables belong to HTIFState but are declared statically!
>
>   static uint64_t fromhost_addr, tohost_addr, begin_sig_addr, end_sig_addr;
>
> 2/ I believe a 64-bit implementation would simplify the logic.
>
> 3/ This is a non-QOM device model!
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/char/riscv_htif.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index 3f84d8d6738..db69b5e3ca7 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -325,6 +325,10 @@ static const MemoryRegionOps htif_mm_ops = {
>      .read = htif_mm_read,
>      .write = htif_mm_write,
>      .endianness = DEVICE_LITTLE_ENDIAN,
> +    .impl = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
>  };
>
>  HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
> --
> 2.45.2
>
>

Re: [PATCH-for-10.0 0/3] hw/char/riscv_htif: Remove tswap64() calls

[Alistair Francis]

On Sat, Nov 30, 2024 at 12:44 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Trying to make sense of these tswap64 calls I
> figured this device could be simplified.
>
> Tested using 'make check-{qtest,functional}'
> on both big/little endian hosts, no failure but
> I'm not sure the code path is covered.
>
> Philippe Mathieu-Daudé (3):
>   MAINTAINERS: Cover RISC-V HTIF interface
>   hw/char/riscv_htif: Explicit little-endian implementation
>   hw/char/riscv_htif: Clarify MemoryRegionOps expect 32-bit accesses

Thanks!

Applied to riscv-to-apply.next

Alistair

>
>  MAINTAINERS          |  2 ++
>  hw/char/riscv_htif.c | 15 ++++++++++-----
>  2 files changed, 12 insertions(+), 5 deletions(-)
>
> --
> 2.45.2
>
>