| Message ID | 20190617030947.32175-1-palmer@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] RISC-V: Break load reservations during switch_to | expand |
Hi Palmer, On Sun, 16 Jun 2019, Palmer Dabbelt 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! > > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> This breaks the rv32_defconfig build: AS arch/riscv/kernel/entry.o arch/riscv/kernel/entry.S: Assembler messages: arch/riscv/kernel/entry.S:343: Error: unrecognized opcode `sc.d x0,ra,0(a3)' make[1]: *** [scripts/Makefile.build:369: arch/riscv/kernel/entry.o] Error 1 > --- > Changes since v2 <20190607222222.15300-1-palmer@sifive.com>: > > * REG_SC has the arguments the right way around. > > Changes since v1 <20190605231735.26581-1-palmer@sifive.com>: > > * REG_SC is now defined as a helper macro, for any code that wants to SC > a register-sized value. > * The explicit #ifdef to check that TASK_THREAD_RA_RA is 0 has been > removed. Instead we rely on the assembler to catch non-zero SC > offsets. I've tested this does actually work. > > arch/riscv/include/asm/asm.h | 1 + > arch/riscv/kernel/entry.S | 11 +++++++++++ > 2 files changed, 12 insertions(+) > > diff --git a/arch/riscv/include/asm/asm.h b/arch/riscv/include/asm/asm.h > index 5ad4cb622bed..946b671f996c 100644 > --- a/arch/riscv/include/asm/asm.h > +++ b/arch/riscv/include/asm/asm.h > @@ -30,6 +30,7 @@ > > #define REG_L __REG_SEL(ld, lw) > #define REG_S __REG_SEL(sd, sw) > +#define REG_SC __REG_SEL(sc.w, sc.d) I guess this should be __REG_SEL(sc.d, sc.w) ? - Paul
On Mon, 17 Jun 2019 06:12:57 PDT (-0700), Paul Walmsley wrote: > Hi Palmer, > > On Sun, 16 Jun 2019, Palmer Dabbelt 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! >> >> Reviewed-by: Christoph Hellwig <hch@lst.de> >> Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > > This breaks the rv32_defconfig build: > > AS arch/riscv/kernel/entry.o > arch/riscv/kernel/entry.S: Assembler messages: > arch/riscv/kernel/entry.S:343: Error: unrecognized opcode `sc.d x0,ra,0(a3)' > make[1]: *** [scripts/Makefile.build:369: arch/riscv/kernel/entry.o] Error 1 > >> --- >> Changes since v2 <20190607222222.15300-1-palmer@sifive.com>: >> >> * REG_SC has the arguments the right way around. >> >> Changes since v1 <20190605231735.26581-1-palmer@sifive.com>: >> >> * REG_SC is now defined as a helper macro, for any code that wants to SC >> a register-sized value. >> * The explicit #ifdef to check that TASK_THREAD_RA_RA is 0 has been >> removed. Instead we rely on the assembler to catch non-zero SC >> offsets. I've tested this does actually work. >> >> arch/riscv/include/asm/asm.h | 1 + >> arch/riscv/kernel/entry.S | 11 +++++++++++ >> 2 files changed, 12 insertions(+) >> >> diff --git a/arch/riscv/include/asm/asm.h b/arch/riscv/include/asm/asm.h >> index 5ad4cb622bed..946b671f996c 100644 >> --- a/arch/riscv/include/asm/asm.h >> +++ b/arch/riscv/include/asm/asm.h >> @@ -30,6 +30,7 @@ >> >> #define REG_L __REG_SEL(ld, lw) >> #define REG_S __REG_SEL(sd, sw) >> +#define REG_SC __REG_SEL(sc.w, sc.d) > > I guess this should be __REG_SEL(sc.d, sc.w) ? Sorry about that, I forget to actually include the fixup in the patch. There's also some debate about the comment's correctness, so I'll submit a v4.
diff --git a/arch/riscv/include/asm/asm.h b/arch/riscv/include/asm/asm.h index 5ad4cb622bed..946b671f996c 100644 --- a/arch/riscv/include/asm/asm.h +++ b/arch/riscv/include/asm/asm.h @@ -30,6 +30,7 @@ #define REG_L __REG_SEL(ld, lw) #define REG_S __REG_SEL(sd, sw) +#define REG_SC __REG_SEL(sc.w, sc.d) #define SZREG __REG_SEL(8, 4) #define LGREG __REG_SEL(3, 2) diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S index fd9b57c8b4ce..7e5d6e035b51 100644 --- a/arch/riscv/kernel/entry.S +++ b/arch/riscv/kernel/entry.S @@ -330,6 +330,17 @@ 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. + */ + REG_SC x0, ra, TASK_THREAD_RA_RA(a3) REG_S sp, TASK_THREAD_SP_RA(a3) REG_S s0, TASK_THREAD_S0_RA(a3) REG_S s1, TASK_THREAD_S1_RA(a3)