lib: fix GET_FXX_REG assembly
Charles Papon
charles.papon.90 at gmail.com
Tue Jun 8 10:34:01 PDT 2021
And there is the github PR :
https://github.com/riscv/opensbi/pull/218/files
Charles
On Tue, Jun 8, 2021 at 7:32 PM Charles Papon <charles.papon.90 at gmail.com> wrote:
>
> There is a new patch. Basically, it keeps the 32 bits and 64 bits
> implementation separated. Never used "=m"(value) by the past, I guess
> it is the right way to specify that the given variable is written by
> the asm ?
>
> From f9ea099d8cddf0161897751b8873ee7797a711dc Mon Sep 17 00:00:00 2001
> From: Dolu1990 <charles.papon.90 at gmail.com>
> Date: Tue, 8 Jun 2021 19:21:33 +0200
> Subject: [PATCH] lib: fix GET_FXX_REG assembly
>
> The previous implementation was producing some broken assembly. See
> github #212 for more details
>
> Signed-off-by: Charles Papon <charles.papon.90 at gmail.com>
> ---
> include/sbi/riscv_fp.h | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/include/sbi/riscv_fp.h b/include/sbi/riscv_fp.h
> index a685884..e07d37b 100644
> --- a/include/sbi/riscv_fp.h
> +++ b/include/sbi/riscv_fp.h
> @@ -42,6 +42,8 @@
> : "t0");
> \
> })
> #define init_fp_reg(i) SET_F32_REG((i) << 3, 3, 0, 0)
> +
> +#if __riscv_xlen == 64
> #define GET_F64_REG(insn, pos, regs)
> \
> ({
> \
> register ulong value asm("a0") =
> \
> @@ -51,6 +53,18 @@
> : "=&r"(tmp), "+&r"(value)::"t0");
> \
> sizeof(ulong) == 4 ? *(int64_t *)value : (int64_t)value;
> \
> })
> +#else
> +#define GET_F64_REG(insn, pos, regs)
> \
> + ({
> \
> + ulong rf_address = SHIFT_RIGHT(insn, (pos)-3) & 0xf8;
> \
> + u64 value;
> \
> + register ulong ptr asm("a0") = (ulong)&value;
> \
> + asm ("1: auipc t1, %%pcrel_hi(get_f64_reg); add t1, t1, %2; jalr t0,
> t1, %%pcrel_lo(1b)" \
> + : "=m"(value) : "r"(ptr), "r"(rf_address) : "t0", "t1");
> \
> + value;
> \
> + })
> +#endif
> +
> #define SET_F64_REG(insn, pos, regs, val)
> \
> ({
> \
> uint64_t __val = (val);
> \
> --
> 2.17.1
>
>
>
> On Sat, May 15, 2021 at 5:23 PM Charles Papon
> <charles.papon.90 at gmail.com> wrote:
> >
> > Hoo right, i forced a0 on the wrong variable. this was misleading. I
> > have a patch for that, but now i see that RV64 do not use the memory
> > to get the FPU register value :
> > # define get_f64(which) fmv.x.d a0, which; jr t0
> >
> > So right, that patch isn't the good one XD
> >
> > > 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
> >
> > What do you mean by grow ? stack allocation ?
> >
> > On Sat, May 15, 2021 at 3:32 PM Jessica Clarke <jrtc27 at jrtc27.com> wrote:
> > >
> > > 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
> > >
More information about the opensbi
mailing list