Re: [PATCH RFC] arm64: vdso: Use __arch_counter_get_cntvct()

[Breno Leitao <leitao@debian.org>]

Hello Marc,

On Wed, Apr 02, 2025 at 11:22:51PM +0100, Marc Zyngier wrote:
> > > -	arch_counter_enforce_ordering(res);
> > > -
> > > -	return res;
> > > +	return __arch_counter_get_cntvct();
> > 
> > I won't pretend I understand it all, but you really want to have a
> > look at the link just above the arch_counter_enforce_ordering()
> > definition, pasted below for your convenience:
> > 
> > https://lore.kernel.org/r/alpine.DEB.2.21.1902081950260.1662@nanos.tec.linutronix.de/
> > 
> > Dropping this ordering enforcement seems pretty adventurous unless you
> > have very strong guarantees about the context this executes in.
> 
> Ah, I appear to have misread this patch, and
> __arch_counter_get_cntvct() does have the same ordering requirements.

Right, I've originally ensured that this part remained unchanged, with
one notable exception. The __arch_counter_get_cntvct() function does not
mark memory as clobbered, whereas the original code did.

The original code, which is being removed, used the following assembly
construction:

	asm volatile(
	ALTERNATIVE("isb\n mrs %0, cntvct_el0",
			"nop\n" __mrs_s("%0", SYS_CNTVCTSS_EL0),
			ARM64_HAS_ECV)
	: "=r" (res)
	:
	: "memory");

This code explicitly marked memory as clobbered using the "memory"
clobber specifier.
In contrast, the __arch_counter_get_cntvct() uses a similar assembly
instruction, but without the memory clobber specifier:

	asm volatile(
	ALTERNATIVE("isb\n mrs %0, cntvct_el0",
			"nop\n" __mrs_s("%0", SYS_CNTVCTSS_EL0),
			ARM64_HAS_ECV)
	: "=r" (cnt));

From my analysis, I understand that memory clobbering is not necessary
in this case. The assembly instruction only accesses registers and does
not modify memory. The use of explicit memory variable (res/cnt) in the
assembly code ensures that memory is safe.

Other than that, nothing else changes.

> Apologies for the noise.

Since you created *all* this noise regarding instruction ordering, can
I pick your brain in the same topic? :-P

If my machine has Speculation Barrier (sb)[1] support, is it a good
replacement for `isb` ? Do you happen to know?

[1] https://developer.arm.com/documentation/ddi0602/2022-06/Base-Instructions/SB--Speculation-Barrier-

Thanks for your review!
--breno