Re: [PATCH 3/6] arm64/vdso: Add time napespace page

[Mark Rutland]

Hi Andrei,

On Wed, Apr 15, 2020 at 10:26:15PM -0700, Andrei Vagin wrote:
> diff --git a/arch/arm64/include/asm/vdso.h b/arch/arm64/include/asm/vdso.h
> index 07468428fd29..351c145d3808 100644
> --- a/arch/arm64/include/asm/vdso.h
> +++ b/arch/arm64/include/asm/vdso.h
> @@ -12,6 +12,12 @@
>   */
>  #define VDSO_LBASE	0x0
>  
> +#ifdef CONFIG_TIME_NS
> +#define __VVAR_PAGES    2
> +#else
> +#define __VVAR_PAGES    1
> +#endif
> +
>  #ifndef __ASSEMBLY__
  
> +#ifdef CONFIG_TIME_NS
> +static __always_inline const struct vdso_data *__arch_get_timens_vdso_data(void)
> +{
> +	const struct vdso_data *ret;
> +
> +	ret = _timens_data;
> +	OPTIMIZER_HIDE_VAR(ret);
> +
> +	return ret;
> +}
> +#endif

Sorry for the confusion here, but please either:

* Add a preparatory patch making __arch_get_vdso_data() use
  OPTIMIZER_HIDE_VAR(), and use OPTIMIZER_HIDE_VAR() here.

* Use the same assembly as __arch_get_vdso_data() currently does.

... and either way add a comment here:

	/* See __arch_get_vdso_data() */

... so taht the rationale is obvious.

[...]

> +enum vvar_pages {
> +	VVAR_DATA_PAGE_OFFSET = 0,
> +#ifdef CONFIG_TIME_NS
> +	VVAR_TIMENS_PAGE_OFFSET = 1,
> +#endif /* CONFIG_TIME_NS */
> +	VVAR_NR_PAGES = __VVAR_PAGES,
> +};

Pet peeve, but we don't need the initializers here, as enums start from
zero. The last element shouldn't have a trailing comma as we don't
expect to add elements after it in future.

Rather than assigning to VVAR_NR_PAGES, it'd be better to use a
BUILD_BUG_ON() to verify that it is the number we expect:

enum vvar_pages {
	VVAR_DATA_PAGE,
#ifdef CONFIG_TIME_NS
	VVAR_TIMENS_PAGE,
#endif
	VVAR_NR_PAGES
};

BUILD_BUG_ON(VVAR_NR_PAGES != __VVAR_PAGES);

Thanks,
Mark.