Re: [PATCH RESEND v7 6/6] arm64: futex: support futex with FEAT_LSUI

Yeoreum Yun
2025-09-15T08:25:08Z

Hi Catalin,

> On Sat, Aug 16, 2025 at 04:19:29PM +0100, Yeoreum Yun wrote:
> > @@ -115,11 +117,137 @@ __llsc_futex_cmpxchg(u32 __user *uaddr, u32 oldval, u32 newval, u32 *oval)
> > return ret;
> > }
> >
> > +#ifdef CONFIG_AS_HAS_LSUI
> > +
> > +#define __LSUI_PREAMBLE ".arch_extension lsui\n"
> > +
> > +#define LSUI_FUTEX_ATOMIC_OP(op, asm_op, mb) \
> > +static __always_inline int \
> > +__lsui_futex_atomic_##op(int oparg, u32 __user *uaddr, int *oval) \
> > +{ \
> > + int ret = 0; \
> > + int oldval; \
> > + \
> > + uaccess_ttbr0_enable(); \
>
> I think we can drop uaccess_ttbr0_*() from these functions. At the
> kconfig level, TTBR0_PAN selects PAN. Hardware with LSUI will also
> have PAN (since 8.1), so the above is an unnecessary branch or nop,
> depending on how the alternatives play out. But add a comment instead.

Thanks to point out this.
I'll change it.

>
> > +static __always_inline int
> > +__lsui_futex_atomic_eor(int oparg, u32 __user *uaddr, int *oval)
> > +{
> > + unsigned int loops = LLSC_MAX_LOOPS;
> > + int ret, oldval, tmp;
> > +
> > + uaccess_ttbr0_enable();
> > + /*
> > + * there are no ldteor/stteor instructions...
> > + */
> > + asm volatile("// __lsui_futex_atomic_eor\n"
> > + __LSUI_PREAMBLE
> > +" prfm pstl1strm, %2\n"
> > +"1: ldtxr %w1, %2\n"
> > +" eor %w3, %w1, %w5\n"
> > +"2: stltxr %w0, %w3, %2\n"
> > +" cbz %w0, 3f\n"
> > +" sub %w4, %w4, %w0\n"
> > +" cbnz %w4, 1b\n"
> > +" mov %w0, %w6\n"
> > +"3:\n"
> > +" dmb ish\n"
> > + _ASM_EXTABLE_UACCESS_ERR(1b, 3b, %w0)
> > + _ASM_EXTABLE_UACCESS_ERR(2b, 3b, %w0)
> > + : "=&r" (ret), "=&r" (oldval), "+Q" (*uaddr), "=&r" (tmp),
> > + "+r" (loops)
> > + : "r" (oparg), "Ir" (-EAGAIN)
> > + : "memory");
> > + uaccess_ttbr0_disable();
> > +
> > + if (!ret)
> > + *oval = oldval;
> > +
> > + return ret;
> > +}
>
> That's an unfortunate omission from the architecture.
>
> > +#define __lsui_llsc_body(op, ...) \
> > +({ \
> > + alternative_has_cap_likely(ARM64_HAS_LSUI) ? \
> > + __lsui_##op(__VA_ARGS__) : __llsc_##op(__VA_ARGS__); \
> > +})
> > +
> > +#else /* CONFIG_AS_HAS_LSUI */
> > +
> > +#define __lsui_llsc_body(op, ...) __llsc_##op(__VA_ARGS__)
> > +
> > +#endif /* CONFIG_AS_HAS_LSUI */
> > +
> > +
> > #define FUTEX_ATOMIC_OP(op) \
> > static __always_inline int \
> > __futex_atomic_##op(int oparg, u32 __user *uaddr, int *oval) \
> > { \
> > - return __llsc_futex_atomic_##op(oparg, uaddr, oval); \
> > + return __lsui_llsc_body(futex_atomic_##op, oparg, uaddr, oval); \
> > }
>
> That's what I got confused about. It looks fine:
>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks!
--
Sincerely,
Yeoreum Yun