Re: [PATCH RESEND v7 4/6] arm64: futex: refactor futex atomic operation

[Yeoreum Yun <yeoreum.yun@arm.com>]

Hi Mark,

[...]
> > > > diff --git a/arch/arm64/include/asm/futex.h b/arch/arm64/include/asm/futex.h
> > > > index 1d6d9f856ac5..0aeda7ced2c0 100644
> > > > --- a/arch/arm64/include/asm/futex.h
> > > > +++ b/arch/arm64/include/asm/futex.h
> > > > @@ -126,6 +126,60 @@ LSUI_FUTEX_ATOMIC_OP(or, ldtset, al)
> > > >  LSUI_FUTEX_ATOMIC_OP(andnot, ldtclr, al)
> > > >  LSUI_FUTEX_ATOMIC_OP(set, swpt, al)
> > > >
> > > > +
> > > > +#define LSUI_CMPXCHG_HELPER(suffix, start_bit)                                 \
> > > > +static __always_inline int                                                     \
> > > > +__lsui_cmpxchg_helper_##suffix(u64 __user *uaddr, u32 oldval, u32 newval)      \
> > > > +{                                                                              \
> > > > +       int ret = 0;                                                            \
> > > > +       u64 oval, nval, tmp;                                                    \
> > > > +                                                                               \
> > > > +       asm volatile("//__lsui_cmpxchg_helper_" #suffix "\n"                    \
> > > > +       __LSUI_PREAMBLE                                                         \
> > > > +"      prfm    pstl1strm, %2\n"                                                \
> > > > +"1:    ldtr    %x1, %2\n"                                                      \
> > > > +"      mov     %x3, %x1\n"                                                     \
> > > > +"      bfi     %x1, %x5, #" #start_bit ", #32\n"                               \
> > > > +"      bfi     %x3, %x6, #" #start_bit ", #32\n"                               \
> > > > +"      mov     %x4, %x1\n"                                                     \
> > > > +"2:    caslt   %x1, %x3, %2\n"                                                 \
> > > > +"      sub     %x1, %x1, %x4\n"                                                \
> > > > +"      cbz     %x1, 3f\n"                                                      \
> > > > +"      mov     %w0, %w7\n"                                                     \
> > > > +"3:\n"                                                                         \
> > > > +"      dmb     ish\n"                                                          \
> > > > +"4:\n"                                                                         \
> > > > +       _ASM_EXTABLE_UACCESS_ERR(1b, 4b, %w0)                                   \
> > > > +       _ASM_EXTABLE_UACCESS_ERR(2b, 4b, %w0)                                   \
> > > > +       : "+r" (ret), "=&r" (oval), "+Q" (*uaddr), "=&r" (nval), "=&r" (tmp)    \
> > > > +       : "r" (oldval), "r" (newval), "Ir" (-EAGAIN)                            \
> > > > +       : "memory");                                                            \
> > > > +                                                                               \
> > > > +       return ret;                                                             \
> > > > +}
> > > > +
> > > > +LSUI_CMPXCHG_HELPER(lo, 0)
> > > > +LSUI_CMPXCHG_HELPER(hi, 32)
> > > > +
> > > > +static __always_inline int
> > > > +__lsui_cmpxchg_helper(u32 __user *uaddr, u32 oldval, u32 newval, u32 *oval)
> > > > +{
> > > > +       int ret;
> > > > +       unsigned long uaddr_al;
> > > > +
> > > > +       uaddr_al = ALIGN_DOWN((unsigned long)uaddr, sizeof(u64));
> > > > +
> > > > +       if (uaddr_al != (unsigned long)uaddr)
> > > > +               ret = __lsui_cmpxchg_helper_hi((u64 __user *)uaddr_al, oldval, newval);
> > > > +       else
> > > > +               ret = __lsui_cmpxchg_helper_lo((u64 __user *)uaddr_al, oldval, newval);
> > > > +
> > > > +       if (!ret)
> > > > +               *oval = oldval;
> > > > +
> > > > +       return ret;
> > > > +}
> > >
> > > I think Will expects that you do more of this in C, e.g. have a basic
> > > user cmpxchg on a 64-bit type, e.g.
> > >
> > > /*
> > >  * NOTE: *oldp is NOT updated if a fault is taken.
> > >  */
> > > static __always_inline int
> > > user_cmpxchg64_release(u64 __usr *addr, u64 *oldp, u64 new)
> > > {
> > > 	int err = 0;
> > >
> > > 	asm volatile(
> > > 	__LSUI_PREAMBLE
> > > 	"1:	caslt	%x[old], %x[new], %[addr]\n"
> > > 	"2:\n"
> > > 	_ASM_EXTABLE_UACCESS_ERR(1b, 4b, %w0)
> > > 	: [addr] "+Q" (addr),
> > > 	  [old] "+r" (*oldp)
> > > 	: [new] "r" (new)
> > > 	: "memory"
> > > 	);
> > >
> > > 	return err;
> > > }
> > >
> > > That should be the *only* assembly you need to implement.
> > >
> > > Atop that, have a wrapper that uses get_user() and that helper above to
> > > implement the 32-bit user cmpxchg, with all the bit manipulation in C:
> >
> > Thanks for your suggestion. But small question.
> > I think it's enough to use usafe_get_user() instead of get_user() in here
> > since when FEAT_LSUI enabled, it doeesn't need to call
> > uaccess_ttbr0_enable()/disable().
>
> Regardless of uaccess_ttbr0_enable() and uaccess_ttbr0_disable()
> specifically, API-wise unsafe_get_user() is only supposed to be called
> between user_access_begin() and user_access_end(), and there's some
> stuff we probably want to add there (e.g. might_fault(), which
> unsafe_get_user() lacks today).
>
> Do we call those?

Yes when you're available.
As you mention, the difference seems might_fault(),
But I'm not sure whether that would be a reason to validate to use
get_user() instead of unsafe_get_user() taking a increase of instruction
of "nop" -- uaccess_ttbr0_enable()/disable() in LSUI
except the reason for DEUBG purpose.

--
Sincerely,
Yeoreum Yun