Re: [PATCH v2 2/3] oslat: Add aarch64 support

[Punit Agrawal <punitagrawal@gmail.com>]

Hi Nicolas,

Apologies for the late comment. I just caught up with the thread.

Nicolas Saenz Julienne <nsaenzju@redhat.com> writes:

> The callbacks are based on Linux's implementation:
>  - CNTVCT_EL0 provides direct access to the system virtual timer[1].
>  - 'yield' serves as a CPU hint with similar semantics as x86's
>    'pause'[2].
>
> In contrast with the kernel's implementation, there isn't a need for
> isb() after reading CNTVCT_EL0,  this is only needed in-kernel as the
> register read has to be ordered with a subsequent locking operation.
>
> [1] See Linux's '__arch_get_hw_counter()' in arch/arm64/include/asm/vdso/gettimeofday.h
> [2] See Linux's 1baa82f4803 ("arm64: Implement cpu_relax as yield").
> Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
> --
>
> Changes since v1:
>  - Code cleanup
>  - Add compiler barriers
>
>  src/oslat/oslat.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
> index 33cccd3..c90ec1a 100644
> --- a/src/oslat/oslat.c
> +++ b/src/oslat/oslat.c
> @@ -71,6 +71,19 @@ static inline void frc(uint64_t *pval)
>  {
>  	__asm__ __volatile__("mfspr %0, 268\n" : "=r" (*pval));
>  }
> +# elif defined(__aarch64__)
> +#  define relax()          __asm__ __volatile("yield" : : : "memory")
> +
> +static inline void frc(uint64_t *pval)
> +{
> +	/*
> +	 * This isb() is required to prevent that the counter value
> +	 * is speculated.
> +	 */
> +	__asm__ __volatile__("isb" : : : "memory");
> +	__asm__ __volatile__("mrs %0, cntvct_el0" : "=r" (*pval) :: "memory");
> +

Although the isb() ensures completion of instructions before the counter
is read, I think there is still the problem of speculative execution of
instructions after the counter read being moved forward. See the
examples in Arm ARM DDI 0487F.b Section D11.2.2 "The Virtual counter"

So from my understanding the problem would be something like below -

isb()
...  <-  speculatively executed instructions from after the counter read
mrs %0, cntvct_el0

This would skew the counter value to a later point than what is intended
- a following isb() would address the issue.

> +}
>  # else
>  #  define relax()          do { } while (0)
>  #  define frc(x)