lib: fix GET_FXX_REG assembly

[Jessica Clarke]

On 15 May 2021, at 13:06, Charles Papon <charles.papon.90 at gmail.com> wrote:
> 
> The previous implementation was producing some broken assembly. See
> github #212 for more details
> 
> PR : https://github.com/riscv/opensbi/pull/214
> 
> ---
> include/sbi/riscv_fp.h | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/include/sbi/riscv_fp.h b/include/sbi/riscv_fp.h
> index a685884..b2c0636 100644
> --- a/include/sbi/riscv_fp.h
> +++ b/include/sbi/riscv_fp.h
> @@ -21,14 +21,14 @@
> 
> #ifdef __riscv_flen
> 
> -#define GET_F32_REG(insn, pos, regs)
>                                  \
> - ({
>                           \
> - register s32 value asm("a0") =
>                   \
> - SHIFT_RIGHT(insn, (pos)-3) & 0xf8;
>           \
> - ulong tmp;
>                   \
> - asm("1: auipc %0, %%pcrel_hi(get_f32_reg); add %0, %0, %1; jalr t0,
> %0, %%pcrel_lo(1b)" \
> -    : "=&r"(tmp), "+&r"(value)::"t0");
>                  \
> - value;
>                   \
> +#define GET_F32_REG(insn, pos, regs)
>                                   \
> + ({
>                                \
> + register ulong rf_address asm("a0") = SHIFT_RIGHT(insn, (pos)-3) &
> 0xf8;                         \
> + ulong tmp;
>                            \
> + volatile u32 value;
>                            \
> + asm volatile("1: auipc %0, %%pcrel_hi(get_f32_reg); add %0, %0, %2;
> jalr t0, %0, %%pcrel_lo(1b)" \
> + : "=&r"(tmp) : "r"(&value), "r"(rf_address)  :"t0");
>                        \
> + value;
>                            \
>  })
> #define SET_F32_REG(insn, pos, regs, val)
>                                      \
>  ({
>                               \
> @@ -42,14 +42,14 @@
>  : "t0");
>               \
>  })
> #define init_fp_reg(i) SET_F32_REG((i) << 3, 3, 0, 0)
> -#define GET_F64_REG(insn, pos, regs)
>                                  \
> - ({
>                           \
> - register ulong value asm("a0") =
>                   \
> - SHIFT_RIGHT(insn, (pos)-3) & 0xf8;
>           \
> - ulong tmp;
>                   \
> - asm("1: auipc %0, %%pcrel_hi(get_f64_reg); add %0, %0, %1; jalr t0,
> %0, %%pcrel_lo(1b)" \
> -    : "=&r"(tmp), "+&r"(value)::"t0");
>                  \
> - sizeof(ulong) == 4 ? *(int64_t *)value : (int64_t)value;
>                   \
> +#define GET_F64_REG(insn, pos, regs)
>                                    \
> + ({
>                                 \
> + register ulong rf_address asm("a0") = SHIFT_RIGHT(insn, (pos)-3) &
> 0xf8;                          \
> + ulong tmp;
>                             \
> + volatile u64 value;
>                             \
> + asm volatile("1: auipc %0, %%pcrel_hi(get_f64_reg); add %0, %0, %2;
> jalr t0, %0, %%pcrel_lo(1b)"  \
> + : "=&r"(tmp) : "r"(&value), "r"(rf_address)  :"t0");
>                         \
> + value;
>                             \
>  })
> #define SET_F64_REG(insn, pos, regs, val)
>                                      \
>  ({

This patch is wrong for numerous reasons:

1. The volatile is unnecessary as loads have side-effects modelled by the
   constraints; only stores need volatile.

2. Similarly, value does not need to be volatile.

3. But value doesn’t even need to exist.

4. You’ve lost the clobber on a0 through the old value.

5. Your formatting of the constraints is wrong.

6. GET_F64_REG is now even more broken on RV32. It won’t crash, but you’re
   reading a completely random uninitialised value from value.

On RV32, get_f64_reg expects a0 to be a pointer to a u64 and will store the
64-bit value there. The only bug in the existing code is that it needs to grow
separate offset and __val variables like SET_F64_REG in order to handle RV32
correctly. You have not fixed that bug, but you have also introduced many
additional problems.

Jess