Received-SPF: pass (google.com: domain of linux-kernel+bounces-159116-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <4fa7ce5b-3ee3-4b04-a950-220eef6a633e@sifive.com>
Date: Thu, 25 Apr 2024 15:18:04 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [kvm-riscv/for-next 2/2] drivers/perf: riscv: Fix RV32 snapshot
 overflow use case
To: Atish Patra <atishp@rivosinc.com>
Cc: Alexandre Ghiti <alexghiti@rivosinc.com>,
 Andrew Jones <ajones@ventanamicro.com>, Anup Patel <anup@brainfault.org>,
 Atish Patra <atishp@atishpatra.org>,
 Conor Dooley <conor.dooley@microchip.com>, linux-riscv@lists.infradead.org,
 kvm-riscv@lists.infradead.org, Mark Rutland <mark.rutland@arm.com>,
 Palmer Dabbelt <palmer@dabbelt.com>, Paul Walmsley
 <paul.walmsley@sifive.com>, Will Deacon <will@kernel.org>,
 linux-kernel@vger.kernel.org
References: <20240425232933.4111680-1-atishp@rivosinc.com>
 <20240425232933.4111680-3-atishp@rivosinc.com>
Content-Language: en-US
From: Samuel Holland <samuel.holland@sifive.com>
In-Reply-To: <20240425232933.4111680-3-atishp@rivosinc.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 2024-04-25 6:29 PM, Atish Patra wrote:
> The shadow copy alogirthm is implemented incorrectly. This patch fixes
> the behavior by keeping a per cpu shadow copy of the counter values to
> avoid clobbering for the cases where system more than XLEN counters and
> the overflown counter index are beyond XLEN. This issue can only be
> observed only in RV32 if an SBI implementation assigns logical counters
> ids greater than XLEN or firmware counter overflow is supported in the
> future.
> 
> Fixes : commit 22f5dac41004d ("drivers/perf: riscv: Implement SBI PMU snapshot function")

Same comment as for patch 1. The logic looks correct as far as I can tell, so:

Reviewed-by: Samuel Holland <samuel.holland@sifive.com>

One minor comment below.

> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  drivers/perf/riscv_pmu_sbi.c   | 49 +++++++++++++++++++---------------
>  include/linux/perf/riscv_pmu.h |  2 ++
>  2 files changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 2694110f1cff..98aaeb13e9db 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -588,6 +588,7 @@ static int pmu_sbi_snapshot_setup(struct riscv_pmu *pmu, int cpu)
>  		return sbi_err_map_linux_errno(ret.error);
>  	}
>  
> +	memset(cpu_hw_evt->snapshot_cval_shcopy, 0, sizeof(u64) * RISCV_MAX_COUNTERS);
>  	cpu_hw_evt->snapshot_set_done = true;
>  
>  	return 0;
> @@ -605,7 +606,7 @@ static u64 pmu_sbi_ctr_read(struct perf_event *event)
>  	union sbi_pmu_ctr_info info = pmu_ctr_list[idx];
>  
>  	/* Read the value from the shared memory directly only if counter is stopped */
> -	if (sbi_pmu_snapshot_available() & (hwc->state & PERF_HES_STOPPED)) {
> +	if (sbi_pmu_snapshot_available() && (hwc->state & PERF_HES_STOPPED)) {
>  		val = sdata->ctr_values[idx];
>  		return val;
>  	}
> @@ -769,36 +770,38 @@ static inline void pmu_sbi_stop_hw_ctrs(struct riscv_pmu *pmu)
>  	struct cpu_hw_events *cpu_hw_evt = this_cpu_ptr(pmu->hw_events);
>  	struct riscv_pmu_snapshot_data *sdata = cpu_hw_evt->snapshot_addr;
>  	unsigned long flag = 0;
> -	int i;
> +	int i, idx;
>  	struct sbiret ret;
> -	unsigned long temp_ctr_values[64] = {0};
> -	unsigned long ctr_val, temp_ctr_overflow_mask = 0;
> +	u64 temp_ctr_overflow_mask = 0;
>  
>  	if (sbi_pmu_snapshot_available())
>  		flag = SBI_PMU_STOP_FLAG_TAKE_SNAPSHOT;
>  
> +	/* Reset the shadow copy to avoid save/restore any value from previous overflow */
> +	memset(cpu_hw_evt->snapshot_cval_shcopy, 0, sizeof(u64) * RISCV_MAX_COUNTERS);
> +
>  	for (i = 0; i < BITS_TO_LONGS(RISCV_MAX_COUNTERS); i++) {
>  		/* No need to check the error here as we can't do anything about the error */
>  		ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_STOP, i * BITS_PER_LONG,
>  				cpu_hw_evt->used_hw_ctrs[i], flag, 0, 0, 0);
>  		if (!ret.error && sbi_pmu_snapshot_available()) {
>  			/* Save the counter values to avoid clobbering */
> -			temp_ctr_values[i * BITS_PER_LONG + i] = sdata->ctr_values[i];
> -			/* Save the overflow mask to avoid clobbering */
> -			if (BIT(i) & sdata->ctr_overflow_mask)
> -				temp_ctr_overflow_mask |= BIT(i + i * BITS_PER_LONG);
> +			for_each_set_bit(idx, &cpu_hw_evt->used_hw_ctrs[i], BITS_PER_LONG) {
> +				cpu_hw_evt->snapshot_cval_shcopy[i * BITS_PER_LONG + idx] =
> +							sdata->ctr_values[idx];
> +				/* Save the overflow mask to avoid clobbering */
> +				if (BIT(idx) & sdata->ctr_overflow_mask)
> +					temp_ctr_overflow_mask |= BIT(idx + i * BITS_PER_LONG);

This is equivalent to doing

  temp_ctr_overflow_mask |= sdata->ctr_overflow_mask << (i * BITS_PER_LONG);

outside the for_each_set_bit() loop.

> +			}
>  		}
>  	}
>  
> -	/* Restore the counter values to the shared memory */
> +	/* Restore the counter values to the shared memory for used hw counters */
>  	if (sbi_pmu_snapshot_available()) {
> -		for (i = 0; i < 64; i++) {
> -			ctr_val = temp_ctr_values[i];
> -			if (ctr_val)
> -				sdata->ctr_values[i] = ctr_val;
> -			if (temp_ctr_overflow_mask)
> -				sdata->ctr_overflow_mask = temp_ctr_overflow_mask;
> -		}
> +		for_each_set_bit(idx, cpu_hw_evt->used_hw_ctrs, RISCV_MAX_COUNTERS)
> +			sdata->ctr_values[idx] = cpu_hw_evt->snapshot_cval_shcopy[idx];
> +		if (temp_ctr_overflow_mask)
> +			sdata->ctr_overflow_mask = temp_ctr_overflow_mask;
>  	}
>  }
>  
> @@ -850,7 +853,7 @@ static inline void pmu_sbi_start_ovf_ctrs_sbi(struct cpu_hw_events *cpu_hw_evt,
>  static inline void pmu_sbi_start_ovf_ctrs_snapshot(struct cpu_hw_events *cpu_hw_evt,
>  						   u64 ctr_ovf_mask)
>  {
> -	int idx = 0;
> +	int i, idx = 0;
>  	struct perf_event *event;
>  	unsigned long flag = SBI_PMU_START_FLAG_INIT_SNAPSHOT;
>  	u64 max_period, init_val = 0;
> @@ -863,7 +866,7 @@ static inline void pmu_sbi_start_ovf_ctrs_snapshot(struct cpu_hw_events *cpu_hw_
>  			hwc = &event->hw;
>  			max_period = riscv_pmu_ctr_get_width_mask(event);
>  			init_val = local64_read(&hwc->prev_count) & max_period;
> -			sdata->ctr_values[idx] = init_val;
> +			cpu_hw_evt->snapshot_cval_shcopy[idx] = init_val;
>  		}
>  		/*
>  		 * We do not need to update the non-overflow counters the previous
> @@ -871,10 +874,14 @@ static inline void pmu_sbi_start_ovf_ctrs_snapshot(struct cpu_hw_events *cpu_hw_
>  		 */
>  	}
>  
> -	for (idx = 0; idx < BITS_TO_LONGS(RISCV_MAX_COUNTERS); idx++) {
> +	for (i = 0; i < BITS_TO_LONGS(RISCV_MAX_COUNTERS); i++) {
> +		/* Restore the counter values to relative indices for used hw counters */
> +		for_each_set_bit(idx, &cpu_hw_evt->used_hw_ctrs[i], BITS_PER_LONG)
> +			sdata->ctr_values[idx] =
> +					cpu_hw_evt->snapshot_cval_shcopy[idx + i * BITS_PER_LONG];
>  		/* Start all the counters in a single shot */
>  		sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_START, idx * BITS_PER_LONG,
> -			  cpu_hw_evt->used_hw_ctrs[idx], flag, 0, 0, 0);
> +			  cpu_hw_evt->used_hw_ctrs[i], flag, 0, 0, 0);
>  	}
>  }
>  
> @@ -898,7 +905,7 @@ static irqreturn_t pmu_sbi_ovf_handler(int irq, void *dev)
>  	int lidx, hidx, fidx;
>  	struct riscv_pmu *pmu;
>  	struct perf_event *event;
> -	unsigned long overflow;
> +	u64 overflow;
>  	u64 overflowed_ctrs = 0;
>  	struct cpu_hw_events *cpu_hw_evt = dev;
>  	u64 start_clock = sched_clock();
> diff --git a/include/linux/perf/riscv_pmu.h b/include/linux/perf/riscv_pmu.h
> index c3fa90970042..701974639ff2 100644
> --- a/include/linux/perf/riscv_pmu.h
> +++ b/include/linux/perf/riscv_pmu.h
> @@ -45,6 +45,8 @@ struct cpu_hw_events {
>  	phys_addr_t snapshot_addr_phys;
>  	/* Boolean flag to indicate setup is already done */
>  	bool snapshot_set_done;
> +	/* A shadow copy of the counter values to avoid clobbering during multiple SBI calls */
> +	u64 snapshot_cval_shcopy[RISCV_MAX_COUNTERS];
>  };
>  
>  struct riscv_pmu {