| Message ID | 20190605231735.26581-1-palmer@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISC-V: Break load reservations during switch_to | expand |
Ah that’s sneaky!! > On Jun 6, 2019, at 12:17 AM, Palmer Dabbelt <palmer@sifive.com> wrote: > > The comment describes why in detail. This was found because QEMU never > gives up load reservations, the issue is unlikely to manifest on real > hardware. > > Thanks to Carlos Eduardo for finding the bug! > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > --- > arch/riscv/kernel/entry.S | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S > index 1c1ecc238cfa..e9fc3480e6b4 100644 > --- a/arch/riscv/kernel/entry.S > +++ b/arch/riscv/kernel/entry.S > @@ -330,6 +330,24 @@ ENTRY(__switch_to) > add a3, a0, a4 > add a4, a1, a4 > REG_S ra, TASK_THREAD_RA_RA(a3) > + /* > + * The Linux ABI allows programs to depend on load reservations being > + * broken on context switches, but the ISA doesn't require that the > + * hardware ever breaks a load reservation. The only way to break a > + * load reservation is with a store conditional, so we emit one here. > + * Since nothing ever takes a load reservation on TASK_THREAD_RA_RA we > + * know this will always fail, but just to be on the safe side this > + * writes the same value that was unconditionally written by the > + * previous instruction. > + */ > +#if (TASK_THREAD_RA_RA != 0) > +# error "The offset between ra and ra is non-zero" > +#endif > +#if (__riscv_xlen == 64) > + sc.d x0, ra, 0(a3) > +#else > + sc.w x0, ra, 0(a3) > +#endif > REG_S sp, TASK_THREAD_SP_RA(a3) > REG_S s0, TASK_THREAD_S0_RA(a3) > REG_S s1, TASK_THREAD_S1_RA(a3) > -- > 2.21.0 >
On Wed, Jun 05, 2019 at 04:17:35PM -0700, Palmer Dabbelt wrote: > REG_S ra, TASK_THREAD_RA_RA(a3) > + /* > + * The Linux ABI allows programs to depend on load reservations being > + * broken on context switches, but the ISA doesn't require that the > + * hardware ever breaks a load reservation. The only way to break a > + * load reservation is with a store conditional, so we emit one here. > + * Since nothing ever takes a load reservation on TASK_THREAD_RA_RA we > + * know this will always fail, but just to be on the safe side this > + * writes the same value that was unconditionally written by the > + * previous instruction. > + */ > +#if (TASK_THREAD_RA_RA != 0) I don't think this check works as intended. TASK_THREAD_RA_RA is a parameterized macro, thus the above would never evaluate to 0. The error message also is rather odd while we're at it. > +#if (__riscv_xlen == 64) > + sc.d x0, ra, 0(a3) > +#else > + sc.w x0, ra, 0(a3) > +#endif I'd rather add an macro ala REG_S to asm.h and distinguish between the xlen variants there: #define REG_SC __REG_SEL(sc.d, sc.w)
On Thu, 06 Jun 2019 02:05:18 PDT (-0700), Christoph Hellwig wrote: > On Wed, Jun 05, 2019 at 04:17:35PM -0700, Palmer Dabbelt wrote: >> REG_S ra, TASK_THREAD_RA_RA(a3) >> + /* >> + * The Linux ABI allows programs to depend on load reservations being >> + * broken on context switches, but the ISA doesn't require that the >> + * hardware ever breaks a load reservation. The only way to break a >> + * load reservation is with a store conditional, so we emit one here. >> + * Since nothing ever takes a load reservation on TASK_THREAD_RA_RA we >> + * know this will always fail, but just to be on the safe side this >> + * writes the same value that was unconditionally written by the >> + * previous instruction. >> + */ >> +#if (TASK_THREAD_RA_RA != 0) > > I don't think this check works as intended. TASK_THREAD_RA_RA is a > parameterized macro, thus the above would never evaluate to 0. The > error message also is rather odd while we're at it. OK, I didn't try it. The resulting number can never be non-zero, which is why I couldn't come up with an error message that made sense. I'm not opposed to dropping the check. >> +#if (__riscv_xlen == 64) >> + sc.d x0, ra, 0(a3) >> +#else >> + sc.w x0, ra, 0(a3) >> +#endif > > I'd rather add an macro ala REG_S to asm.h and distinguish between the > xlen variants there: > > #define REG_SC __REG_SEL(sc.d, sc.w) Ya, I guess I was just being lazy. I'll put that in a v2.
On Jun 06 2019, Christoph Hellwig <hch@infradead.org> wrote: > On Wed, Jun 05, 2019 at 04:17:35PM -0700, Palmer Dabbelt wrote: >> REG_S ra, TASK_THREAD_RA_RA(a3) >> + /* >> + * The Linux ABI allows programs to depend on load reservations being >> + * broken on context switches, but the ISA doesn't require that the >> + * hardware ever breaks a load reservation. The only way to break a >> + * load reservation is with a store conditional, so we emit one here. >> + * Since nothing ever takes a load reservation on TASK_THREAD_RA_RA we >> + * know this will always fail, but just to be on the safe side this >> + * writes the same value that was unconditionally written by the >> + * previous instruction. >> + */ >> +#if (TASK_THREAD_RA_RA != 0) > > I don't think this check works as intended. TASK_THREAD_RA_RA is a > parameterized macro, Is it? Just because it is used before an open paren doesn't mean that the macro takes a parameter. Andreas.
On Thu, 06 Jun 2019 12:32:01 PDT (-0700), schwab@linux-m68k.org wrote: > On Jun 06 2019, Christoph Hellwig <hch@infradead.org> wrote: > >> On Wed, Jun 05, 2019 at 04:17:35PM -0700, Palmer Dabbelt wrote: >>> REG_S ra, TASK_THREAD_RA_RA(a3) >>> + /* >>> + * The Linux ABI allows programs to depend on load reservations being >>> + * broken on context switches, but the ISA doesn't require that the >>> + * hardware ever breaks a load reservation. The only way to break a >>> + * load reservation is with a store conditional, so we emit one here. >>> + * Since nothing ever takes a load reservation on TASK_THREAD_RA_RA we >>> + * know this will always fail, but just to be on the safe side this >>> + * writes the same value that was unconditionally written by the >>> + * previous instruction. >>> + */ >>> +#if (TASK_THREAD_RA_RA != 0) >> >> I don't think this check works as intended. TASK_THREAD_RA_RA is a >> parameterized macro, > > Is it? Just because it is used before an open paren doesn't mean that > the macro takes a parameter. Yes, you're right -- the parens there aren't a macro parameter, they're the assembly syntax for constant-offset loads. I guess I'd just assumed Christoph was referring to some magic in how asm-offsets gets generated, as I've never actually looked inside that stuff. I went ahead and just tested this $ git diff | cat diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c index 578bb5efc085..e3f06495dbf8 100644 --- a/arch/riscv/kernel/asm-offsets.c +++ b/arch/riscv/kernel/asm-offsets.c @@ -125,6 +125,7 @@ void asm_offsets(void) DEFINE(TASK_THREAD_RA_RA, offsetof(struct task_struct, thread.ra) - offsetof(struct task_struct, thread.ra) + + 1 ); DEFINE(TASK_THREAD_SP_RA, offsetof(struct task_struct, thread.sp) and it causes the expected failure $ make.cross ARCH=riscv -j1 make CROSS_COMPILE=/home/palmer/.local/opt/gcc-8.1.0-nolibc/riscv64-linux/bin/riscv64-linux- ARCH=riscv -j1 CALL scripts/checksyscalls.sh CALL scripts/atomic/check-atomics.sh CHK include/generated/compile.h AS arch/riscv/kernel/entry.o arch/riscv/kernel/entry.S:344:3: error: #error "The offset between ra and ra is non-zero" # error "The offset between ra and ra is non-zero" ^~~~~ make[1]: *** [scripts/Makefile.build:369: arch/riscv/kernel/entry.o] Error 1 make: *** [Makefile:1071: arch/riscv/kernel] Error 2 so I'm going to leave it alone. I'll submit a v2 with a better error message and a cleaner sc.w/sc.d.
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S index 1c1ecc238cfa..e9fc3480e6b4 100644 --- a/arch/riscv/kernel/entry.S +++ b/arch/riscv/kernel/entry.S @@ -330,6 +330,24 @@ ENTRY(__switch_to) add a3, a0, a4 add a4, a1, a4 REG_S ra, TASK_THREAD_RA_RA(a3) + /* + * The Linux ABI allows programs to depend on load reservations being + * broken on context switches, but the ISA doesn't require that the + * hardware ever breaks a load reservation. The only way to break a + * load reservation is with a store conditional, so we emit one here. + * Since nothing ever takes a load reservation on TASK_THREAD_RA_RA we + * know this will always fail, but just to be on the safe side this + * writes the same value that was unconditionally written by the + * previous instruction. + */ +#if (TASK_THREAD_RA_RA != 0) +# error "The offset between ra and ra is non-zero" +#endif +#if (__riscv_xlen == 64) + sc.d x0, ra, 0(a3) +#else + sc.w x0, ra, 0(a3) +#endif REG_S sp, TASK_THREAD_SP_RA(a3) REG_S s0, TASK_THREAD_S0_RA(a3) REG_S s1, TASK_THREAD_S1_RA(a3)
The comment describes why in detail. This was found because QEMU never gives up load reservations, the issue is unlikely to manifest on real hardware. Thanks to Carlos Eduardo for finding the bug! Signed-off-by: Palmer Dabbelt <palmer@sifive.com> --- arch/riscv/kernel/entry.S | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)