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 <[email protected]>
---
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
Ah that’s sneaky!!
> On Jun 6, 2019, at 12:17 AM, Palmer Dabbelt <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]> 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.
--
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
On Thu, 06 Jun 2019 12:32:01 PDT (-0700), [email protected] wrote:
> On Jun 06 2019, Christoph Hellwig <[email protected]> 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.