Re: [PATCH v8 2/4] arm64: vdso: Implement __vdso_futex_robust_try_unlock()

[André Almeida]

Em 16/09/2026 11:27, Mark Rutland escreveu:
> Hi André,
>
> I have a few comments below. I'm not sure what the plan is for 32-bit
> robust lists, and I think there are a few problems with the assembly.
>
> On Fri, Aug 21, 2026 at 06:50:43PM -0300, André Almeida wrote:

[...]

> > +	register __u64 *pop_reg asm("x2") = pop;
> > +	register __u32 result_reg asm("w3") = 0;
> > +	__u32 val;
> > +
> > +	asm volatile (
> > +		".globl						  "
> > +		"__futex_list64_try_unlock_cs_start,		  "
> > +		"__futex_list64_try_unlock_cs_success,		  "
> > +		"__futex_list64_try_unlock_cs_end		\n"
>
> Sorry, when I mentioned putting these within the assembly, I meant doing
> something like:
>
> | #define LABEL(l)						\
> | 	"	.globl " #l "\n"				\
> | 	#l ":\n"
>
> ... and within the asssembly, having:
>
> | 	"	insn1					\n"
> | 	"	insn2					\n"
> | 	LABEL(__futex_list64_try_unlock_cs_start)
> | 	"	insn2					\n"
> | 	LABEL(__futex_list64_try_unlock_cs_start)
> | 	"	insn4					\n"
> | 	LABEL(__futex_list64_try_unlock_cs_success)
> | 	"	insn5					\n"
>
> That way we use the full strings, which are easy to grep for, and we
> only have to define each label string once.

Oh, I see, thanks! I will change that for the next version.

> > +
> > +		"	prfm pstl1strm, %[lock]			\n"
> > +		"retry:						\n"
> > +		"	ldxr %w[val], %[lock]			\n"
> > +		"	cmp %w[tid], %w[val]			\n"
> > +		"	bne __futex_list64_try_unlock_cs_end	\n"
>
> The 'bne' here should be 'b.ne'. IIUC assemblers accept the former by
> historical accident, and 'b.<cond>' is the architecturally defined
> mnemonic.
>
> As general thing, we format assembly with a tab between the instruction
> and first operand, so the above should be:
>
> | 	"	prfm	pstl1strm, %[lock]			\n"
> | 	"retry:							\n"
> | 	"	ldxr	%w[val], %[lock]			\n"
> | 	"	cmp	%w[tid], %w[val]			\n"
> | 	"	b.ne	__futex_list64_try_unlock_cs_end	\n"
>
> > +		"	stlxr %w[result], wzr, %[lock]		\n"
> > +		"__futex_list64_try_unlock_cs_start:		\n"
> > +		"	cbnz %w[result], retry			\n"
> > +		"__futex_list64_try_unlock_cs_success:		\n"
> > +		"	str xzr, %[pop_reg]			\n"
> > +		"__futex_list64_try_unlock_cs_end:		\n"
> > +
> > +		: [val] "=&r" (val), [result] "=&r" (result_reg), [pop_reg] "+Q" (*pop_reg)
> > +		: [tid] "r" (tid), [lock] "Q" (*lock)
> > +		: "cc", "memory"
> > +	);
>
> As a general note, for assembly constaints, please put each constraint
> on its own line, e.g.
>
> 	: [val] "=&r" (val),
> 	  [result] "=&r" (result_reg),
> 	  [pop_reg] "+Q" (*pop_reg)
> 	: [tid] "r" (tid),
> 	  [lock] "Q" (*lock)
> 	: "cc", "memory"
>
> That formatting makes it much easier to read each constraint
> individually, and it makes it easier to review changes to individual
> constaints.
>
> Generally, for asm we have a preferred style:
>
> 	asm volatile(
> 	"	// string starts aligned with 'asm'	\n"
> 	"	// with 1 tab indend within that.	\n"
> 	"						\n"
> 	"	insn	operand1, operand2		\n"
> 	"label:						\n"
> 	"	insn	operand1, operand2		\n"
> 	: [output1] "=r" (...),
> 	  [output2] "=r" (...)
> 	: [input1] "r" (...).
> 	  [input2] "r" (...)
> 	: "clobbers"
> 	);
>
> I see a couple of problems with the constraints as-is.
>
> Firstly, the constraints for [pop_reg] doesn't guarantee that x2 is
> used. The "+Q" constraint takes a memory operand (in this case the
> location pointed to by 'pop_reg', and limits the addressing mode to a
> single base register with no offset. The operand is the memroy location,
> not the register holding the memory location, so this constraint won't
> necessarily use x2. Unless 'pop_reg' itself is passed into a register
> constraint, the compiler might not ensure that 'x2' is populated.
>
> For example:
>
> | [mark@gravadlaks:~/tests/asm-operands-q-reg]% cat test.c
> | unsigned long foo(unsigned long *ptr)
> | {
> |         register unsigned long *ptr_reg asm("x2") = ptr;
> |         unsigned long val;
> |
> |         asm volatile(
> |         "       ldr     %[val], %[qptr]\n"
> |         : [val] "=&r" (val)
> |         : [qptr] "Q" (*ptr_reg)
> |         );
> |
> |         return val;
> | }
> | [mark@gravadlaks:~/tests/asm-operands-q-reg]% usekorg 16.2.0 aarch64-linux-gcc -c test.c -O2
> | [mark@gravadlaks:~/tests/asm-operands-q-reg]% usekorg 16.2.0 aarch64-linux-objdump -d test.o
> |
> | test.o:     file format elf64-littleaarch64
> |
> |
> | Disassembly of section .text:
> |
> | 0000000000000000 <foo>:
> |    0:   f9400001        ldr     x1, [x0]
> |    4:   aa0103e0        mov     x0, x1
> |    8:   d65f03c0        ret
>
> We will need an "r" input constraint for 'pop_reg' to ensure that x2 is
> allocated. Either in addition to an "=Q" output constraint for the
> actual memory write, or we rely on the memory clobber to hazaard against
> other memory accesses.
>
> Secondly, the "Q" input constaint for 'lock' looks inaccurate, given
> that we both read and write to 'lock'. Either that should be a "+Q"
> output constraint, or we rely on the memory clobber to hazard against
> other memory accesses.

Thank you very much Mark, that is a lot of good information about 
writing arm64 asm!

If I understood correctly, the correct approach here would be to use 
pop_reg as an input operand, making sure that we store the address hold 
by *pop on x2 register right? So something along the lines:

__u32 __vdso_futex_robust_list64_try_unlock(__u32 *lock, __u32 tid, 
__u64 *pop)
{
	register __u64 pop_reg asm("x2") = (__u64) pop;

...

		"	str 	xzr, [%x[pop_reg]]		\n"
...

		: [val] "=&r" (val),
		  [result] "=&r" (result_reg),
		  [lock] "+Q" (*lock)
		: [tid] "r" (tid),
		  [pop_reg] "r" (pop_reg)
		: "cc", "memory"

Does that looks right?

Thanks again for the help


> Mark.
>
> > +
> > +	return val;
> > +}
> >
> > --
> > 2.55.0