Re: [PATCH] hw/char/riscv_htif: Fix 64-bit var write order in 32-bit system

[Alistair Francis]

On Wed, Jul 31, 2024 at 12:51 AM Nikita Novikov <n.novikov@syntacore.com> wrote:
>
> When we are trying to write 64-bit value in 32-bit system, the value in HTIF 
> device divides
> on 2 separate 32-bit parts. So device expects write to xHOST_OFFSET1 first, 
> then to xHOST_OFFSET2.
> But some compilers (ex. CLANG) can change the writing queue (xHOST_OFFSET2 
> first, xHOST_OFFSET1 second)
> because of machine instructions order swapping. That causes wrong behaviour 
> of HTIF device.

I don't follow, isn't this MMIO why is the compiler swapping the order?

> This patch is removing dependency of writing order, so the device will work 
> correctly regardless of compilers.

Urgh, the HTIF is undocumented, so there isn't really a way to know if
this is right or not.

It kind of seems like the QEMU implementation is the best
documentation at the moment [1], so I'm reluctant to make this change.

>
> Signed-off-by: Nikita Novikov <n.novikov@syntacore.com>
> ---
>  hw/char/riscv_htif.c         | 43 +++++++++++++++++++-----------------
>  include/hw/char/riscv_htif.h | 11 +++++++--
>  2 files changed, 32 insertions(+), 22 deletions(-)
>
> diff --git a/hw/char/riscv_htif.c b/hw/char/riscv_htif.c
> index 9bef60def1..d914f158ea 100644
> --- a/hw/char/riscv_htif.c
> +++ b/hw/char/riscv_htif.c
> @@ -233,7 +233,8 @@ static void htif_handle_tohost_write(HTIFState *s, 
> uint64_t val_written)
>          if (cmd == HTIF_CONSOLE_CMD_GETC) {
>              /* this should be a queue, but not yet implemented as such */
>              s->pending_read = val_written;
> -            s->tohost = 0; /* clear to indicate we read */
> +            s->tohost = 0;
> +            s->tohost_status = 0x0;     /* clear to indicate we read */

This seems like a separate change?

>              return;
>          } else if (cmd == HTIF_CONSOLE_CMD_PUTC) {
>              uint8_t ch = (uint8_t)payload;
> @@ -258,7 +259,8 @@ static void htif_handle_tohost_write(HTIFState *s, 
> uint64_t val_written)
>       *  }
>       */
>      s->fromhost = (val_written >> 48 << 48) | (resp << 16 >> 16);
> -    s->tohost = 0; /* clear to indicate we read */
> +    s->tohost = 0;
> +    s->tohost_status = 0x0; /* clear to indicate we read */
>  }
>
>  #define TOHOST_OFFSET1      (s->tohost_offset)
> @@ -291,32 +293,33 @@ static void htif_mm_write(void *opaque, hwaddr addr,
>  {
>      HTIFState *s = opaque;
>      if (addr == TOHOST_OFFSET1) {
> -        if (s->tohost == 0x0) {
> -            s->allow_tohost = 1;
> -            s->tohost = value & 0xFFFFFFFF;
> -        } else {
> -            s->allow_tohost = 0;
> -        }
> +        s->tohost = deposit64(s->tohost, 0, 32, value);
> +        s->tohost_status |= 0x1;
>      } else if (addr == TOHOST_OFFSET2) {
> -        if (s->allow_tohost) {
> -            s->tohost |= value << 32;
> -            htif_handle_tohost_write(s, s->tohost);
> -        }
> +        s->tohost = deposit64(s->tohost, 32, 32, value);
> +        s->tohost_status |= 0x2;

Won't this break existing users?

>      } else if (addr == FROMHOST_OFFSET1) {
> -        s->fromhost_inprogress = 1;
> -        s->fromhost = value & 0xFFFFFFFF;
> +        s->fromhost = deposit64(s->fromhost, 0, 32, value);
> +        s->fromhost_status |= 0x1;
>      } else if (addr == FROMHOST_OFFSET2) {
> -        s->fromhost |= value << 32;
> -        s->fromhost_inprogress = 0;
> +        s->fromhost = deposit64(s->fromhost, 32, 32, value);
> +        s->fromhost_status |= 0x2;
>      } else {
> -        qemu_log("Invalid htif write: address %016" PRIx64 "\n",
> -            (uint64_t)addr);
> +        qemu_log("Invalid htif write: address %016" HWADDR_PRIu "\n", addr);
> +    }
> +    if (s->tohost_status == 0x3) {
> +        htif_handle_tohost_write(s, s->tohost);
> +        s->tohost_status = 0x0;
>      }
>  }

1: https://github.com/riscv-software-src/opensbi/issues/238

Alistair

>
>  static const MemoryRegionOps htif_mm_ops = {
>      .read = htif_mm_read,
>      .write = htif_mm_write,
> +    .valid.min_access_size = 4,
> +    .valid.max_access_size = 8,
> +    .impl.min_access_size = 4,
> +    .impl.max_access_size = 4
>  };
>
>  HTIFState *htif_mm_init(MemoryRegion *address_space, Chardev *chr,
> @@ -343,8 +346,8 @@ HTIFState *htif_mm_init(MemoryRegion *address_space, 
> Chardev *chr,
>      s->tohost_offset = tohost_offset;
>      s->fromhost_offset = fromhost_offset;
>      s->pending_read = 0;
> -    s->allow_tohost = 0;
> -    s->fromhost_inprogress = 0;
> +    s->tohost_status = 0;
> +    s->fromhost_status = 0;
>      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/include/hw/char/riscv_htif.h b/include/hw/char/riscv_htif.h
> index df493fdf6b..2b6d50dc60 100644
> --- a/include/hw/char/riscv_htif.h
> +++ b/include/hw/char/riscv_htif.h
> @@ -27,8 +27,15 @@
>  #define TYPE_HTIF_UART "riscv.htif.uart"
>
>  typedef struct HTIFState {
> -    int allow_tohost;
> -    int fromhost_inprogress;
> +    /*
> +     * Vars below can be in 4 states:
> +     * 0x0 - nothing written
> +     * 0x1 - xHOST_OFFEST1 written
> +     * 0x2 - xHOST_OFFSET2 written
> +     * 0x3 - Both written
> +     */
> +    int tohost_status;
> +    int fromhost_status;
>
>      uint64_t tohost;
>      uint64_t fromhost;
> --
> 2.34.1
>
>