linux-kernel - Re: [PATCH v2 07/12] clocksource: mips-gic-timer: Always use cluster 0 counter as clocksource

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: <5ff65346-ba7b-c440-a7e7-73c84fe13165@syrmia.com>
Date:   Mon, 27 Jun 2022 16:17:43 +0200
From:   Dragan Mladjenovic <Dragan.Mladjenovic@...mia.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Chao-ying Fu <cfu@...ecomp.com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Greg Ungerer <gerg@...nel.org>,
        Hauke Mehrtens <hauke@...ke-m.de>,
        Ilya Lipnitskiy <ilya.lipnitskiy@...il.com>,
        Jiaxun Yang <jiaxun.yang@...goat.com>,
        linux-kernel@...r.kernel.org, linux-mips@...r.kernel.org,
        Marc Zyngier <maz@...nel.org>,
        Paul Burton <paulburton@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Serge Semin <fancer.lancer@...il.com>,
        Tiezhu Yang <yangtiezhu@...ngson.cn>,
        Thomas Bogendoerfer <tsbogend@...ha.franken.de>
Subject: Re: [PATCH v2 07/12] clocksource: mips-gic-timer: Always use cluster
 0 counter as clocksource

On 25-May-22 14:10, Dragan Mladjenovic wrote:
> From: Paul Burton <paulburton@...nel.org>
> 
> In a multi-cluster MIPS system we have multiple GICs - one in each
> cluster - each of which has its own independent counter. The counters in
> each GIC are not synchronised in any way, so they can drift relative to
> one another through the lifetime of the system. This is problematic for
> a clocksource which ought to be global.
> 
> Avoid problems by always accessing cluster 0's counter, using
> cross-cluster register access. This adds overhead so we only do so on
> systems where we actually have CPUs present in multiple clusters.
> For now, be extra conservative and don't use gic counter for vdso or
> sched_clock in this case.
> 
> Signed-off-by: Paul Burton <paulburton@...nel.org>
> Signed-off-by: Chao-ying Fu <cfu@...ecomp.com>
> Signed-off-by: Dragan Mladjenovic <dragan.mladjenovic@...mia.com>
> 
> diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
> index be4175f415ba..6632d314a2c0 100644
> --- a/drivers/clocksource/mips-gic-timer.c
> +++ b/drivers/clocksource/mips-gic-timer.c
> @@ -170,6 +170,37 @@ static u64 gic_hpt_read(struct clocksource *cs)
>   	return gic_read_count();
>   }
>   
> +static u64 gic_hpt_read_multicluster(struct clocksource *cs)
> +{
> +	unsigned int hi, hi2, lo;
> +	u64 count;
> +
> +	mips_cm_lock_other(0, 0, 0, CM_GCR_Cx_OTHER_BLOCK_GLOBAL);
> +
> +	if (mips_cm_is64) {
> +		count = read_gic_redir_counter();
> +		goto out;
> +	}
> +
> +	hi = read_gic_redir_counter_32h();
> +	while (true) {
> +		lo = read_gic_redir_counter_32l();
> +
> +		/* If hi didn't change then lo didn't wrap & we're done */
> +		hi2 = read_gic_redir_counter_32h();
> +		if (hi2 == hi)
> +			break;
> +
> +		/* Otherwise, repeat with the latest hi value */
> +		hi = hi2;
> +	}
> +
> +	count = (((u64)hi) << 32) + lo;
> +out:
> +	mips_cm_unlock_other();
> +	return count;
> +}
> +
>   static struct clocksource gic_clocksource = {
>   	.name			= "GIC",
>   	.read			= gic_hpt_read,
> @@ -204,6 +235,11 @@ static int __init __gic_clocksource_init(void)
>   	/* Calculate a somewhat reasonable rating value. */
>   	gic_clocksource.rating = 200 + gic_frequency / 10000000;
>   
> +	if (mips_cps_multicluster_cpus()) {
> +		gic_clocksource.read = &gic_hpt_read_multicluster;
> +		gic_clocksource.vdso_clock_mode = VDSO_CLOCKMODE_NONE;
> +	}
> +
>   	ret = clocksource_register_hz(&gic_clocksource, gic_frequency);
>   	if (ret < 0)
>   		pr_warn("Unable to register clocksource\n");
> @@ -262,7 +298,8 @@ static int __init gic_clocksource_of_init(struct device_node *node)
>   	 * stable CPU frequency or on the platforms with CM3 and CPU frequency
>   	 * change performed by the CPC core clocks divider.
>   	 */
> -	if (mips_cm_revision() >= CM_REV_CM3 || !IS_ENABLED(CONFIG_CPU_FREQ)) {
> +	if ((mips_cm_revision() >= CM_REV_CM3 || !IS_ENABLED(CONFIG_CPU_FREQ)) &&
> +	     !mips_cps_multicluster_cpus()) {
>   		sched_clock_register(mips_cm_is64 ?
>   				     gic_read_count_64 : gic_read_count_2x32,
>   				     64, gic_frequency);

Hi,

I was expecting some comments on this, but I'll ask first. We now taking 
a conservative approach of not using gic as sched_clock in multicluster 
case. Is this necessary or can sched_clock tolerate a fixed delta 
between clocks on different cpu clusters?

Best regards,

Dragan