linux-kernel - Re: [PATCH v3 1/3] arm64: wrap and generalise counter read functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201105132705.GF82102@C02TD0UTHF1T.local>
Date:   Thu, 5 Nov 2020 13:27:05 +0000
From:   Mark Rutland <mark.rutland@....com>
To:     Ionela Voinescu <ionela.voinescu@....com>
Cc:     catalin.marinas@....com, sudeep.holla@....com, will@...nel.org,
        souvik.chakravarty@....com, viresh.kumar@...aro.org,
        valentin.schneider@....com, dietmar.eggemann@....com,
        morten.rasmussen@....com, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/3] arm64: wrap and generalise counter read functions

Hi Ionela,

On Thu, Nov 05, 2020 at 12:27:00PM +0000, Ionela Voinescu wrote:
> -#ifdef CONFIG_ARM64_AMU_EXTN
> +#define COUNTER_READ_STORE(NAME, VAL) \
> +static inline u64 read_##NAME(void) \
> +{ \
> +	return VAL; \
> +} \
> +static inline void store_##NAME(void *val) \
> +{ \
> +	*(u64 *)val = read_##NAME(); \
> +}
>  
> -#undef pr_fmt
> -#define pr_fmt(fmt) "AMU: " fmt
> +#ifdef CONFIG_ARM64_AMU_EXTN
> +COUNTER_READ_STORE(corecnt, read_sysreg_s(SYS_AMEVCNTR0_CORE_EL0));
> +COUNTER_READ_STORE(constcnt, read_sysreg_s(SYS_AMEVCNTR0_CONST_EL0));
> +#else
> +COUNTER_READ_STORE(corecnt, 0);
> +COUNTER_READ_STORE(constcnt, 0);
> +#endif

I think this level of indirection is confusing (e.g. since you can't
grep for `read_constcnt` to find the implementation). I also don't think
we need to store_* functions here -- they're only needed by patch 3, and
they can be wrapped in helper functions there (which should also solve
the warning you mention in the cover letter).

I think if we need these wrappers, it'd be better to have:

#ifdef CONFIG_ARM64_AMU_EXTN
#define read_corecnt()	read_sysreg_s(SYS_AMEVCNTR0_CORE_EL0)
#define read_constcnt()	read_sysreg_s(SYS_AMEVCNTR0_CONST_EL0)
#else
#define read_corecnt()	(0UL)
#define read_constcnt()	(0UL)
#endif

Thanks,
Mark.