Re: [Qemu-devel] [PATCH v1 23/30] RISC-V: Fix CLINT timecmp low 32-bit writes

[Alistair Francis]

On Tue, May 22, 2018 at 5:15 PM, Michael Clark <address@hidden> wrote:
> A missing shift made updates to the low order bits
> of timecmp erroneously copy the old low order bits
> into the high order bits of the 64-bit timecmp
> register. Add the missing shift and rename timecmp
> local variables to timecmp_hi and timecmp_lo.
>
> This bug didn't show up as the low order bits are
> usually written first followed by the high order
> bits meaning the high order bits contained an invalid
> value between the timecmp_lo and timecmp_hi update.
>
> Cc: Palmer Dabbelt <address@hidden>
> Cc: Sagar Karandikar <address@hidden>
> Cc: Bastian Koppelmann <address@hidden>
> Cc: Alistair Francis <address@hidden>
> Co-Authored-by: Johannes Haring <address@hidden>
> Signed-off-by: Michael Clark <address@hidden>

Reviewed-by: Alistair Francis <address@hidden>

Alistair

> ---
>  hw/riscv/sifive_clint.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/riscv/sifive_clint.c b/hw/riscv/sifive_clint.c
> index 0d2fd52487e6..d4c159e93736 100644
> --- a/hw/riscv/sifive_clint.c
> +++ b/hw/riscv/sifive_clint.c
> @@ -146,15 +146,15 @@ static void sifive_clint_write(void *opaque, hwaddr 
> addr, uint64_t value,
>              error_report("clint: invalid timecmp hartid: %zu", hartid);
>          } else if ((addr & 0x7) == 0) {
>              /* timecmp_lo */
> -            uint64_t timecmp = env->timecmp;
> +            uint64_t timecmp_hi = env->timecmp >> 32;
>              sifive_clint_write_timecmp(RISCV_CPU(cpu),
> -                timecmp << 32 | (value & 0xFFFFFFFF));
> +                timecmp_hi << 32 | (value & 0xFFFFFFFF));
>              return;
>          } else if ((addr & 0x7) == 4) {
>              /* timecmp_hi */
> -            uint64_t timecmp = env->timecmp;
> +            uint64_t timecmp_lo = env->timecmp;
>              sifive_clint_write_timecmp(RISCV_CPU(cpu),
> -                value << 32 | (timecmp & 0xFFFFFFFF));
> +                value << 32 | (timecmp_lo & 0xFFFFFFFF));
>          } else {
>              error_report("clint: invalid timecmp write: %08x", 
> (uint32_t)addr);
>          }
> --
> 2.7.0
>
>