| Message ID | 20220511201107.2311757-1-atishp@rivosinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] RISC-V: Fix counter restart during overflow for RV32 | expand |
On Thu, May 12, 2022 at 1:41 AM Atish Patra <atishp@rivosinc.com> wrote: > > Pass the upper half of the initial value of the counter correctly > for RV32. > > Fixes: 4905ec2fb7e6 ("RISC-V: Add sscofpmf extension support") > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > --- > drivers/perf/riscv_pmu_sbi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > index a1317a483512..24cea59612be 100644 > --- a/drivers/perf/riscv_pmu_sbi.c > +++ b/drivers/perf/riscv_pmu_sbi.c > @@ -526,7 +526,7 @@ static inline void pmu_sbi_start_overflow_mask(struct riscv_pmu *pmu, > max_period = riscv_pmu_ctr_get_width_mask(event); > init_val = local64_read(&hwc->prev_count) & max_period; > sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_START, idx, 1, > - flag, init_val, 0, 0); > + flag, init_val, init_val >> 32, 0); This should be under "#if __riscv_xlen == 32". > } > ctr_ovf_mask = ctr_ovf_mask >> 1; > idx++; > -- > 2.25.1 > Apart from above, this looks good to me. Reviewed-by: Anup Patel <anup@brainfault.org> Regards, Anup
Am Donnerstag, 12. Mai 2022, 06:44:12 CEST schrieb Anup Patel: > On Thu, May 12, 2022 at 1:41 AM Atish Patra <atishp@rivosinc.com> wrote: > > > > Pass the upper half of the initial value of the counter correctly > > for RV32. > > > > Fixes: 4905ec2fb7e6 ("RISC-V: Add sscofpmf extension support") > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > > --- > > drivers/perf/riscv_pmu_sbi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > > index a1317a483512..24cea59612be 100644 > > --- a/drivers/perf/riscv_pmu_sbi.c > > +++ b/drivers/perf/riscv_pmu_sbi.c > > @@ -526,7 +526,7 @@ static inline void pmu_sbi_start_overflow_mask(struct riscv_pmu *pmu, > > max_period = riscv_pmu_ctr_get_width_mask(event); > > init_val = local64_read(&hwc->prev_count) & max_period; > > sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_START, idx, 1, > > - flag, init_val, 0, 0); > > + flag, init_val, init_val >> 32, 0); > > This should be under "#if __riscv_xlen == 32". What's the difference between using CONFIG_32BIT and checking the __riscv_xlen flag value? CONFIG_32BIT seems to be a bit the more kernel'ish way to do this, but it looks like most SBI parts check the __riscv_xlen instead. In any case, looking at the opensbi-side of the call, this fix is abviously correct, so Reviewed-by: Heiko Stuebner <heiko@sntech.de>
On Thu, May 12, 2022 at 6:12 PM Heiko Stübner <heiko@sntech.de> wrote: > > Am Donnerstag, 12. Mai 2022, 06:44:12 CEST schrieb Anup Patel: > > On Thu, May 12, 2022 at 1:41 AM Atish Patra <atishp@rivosinc.com> wrote: > > > > > > Pass the upper half of the initial value of the counter correctly > > > for RV32. > > > > > > Fixes: 4905ec2fb7e6 ("RISC-V: Add sscofpmf extension support") > > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > > > --- > > > drivers/perf/riscv_pmu_sbi.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > > > index a1317a483512..24cea59612be 100644 > > > --- a/drivers/perf/riscv_pmu_sbi.c > > > +++ b/drivers/perf/riscv_pmu_sbi.c > > > @@ -526,7 +526,7 @@ static inline void pmu_sbi_start_overflow_mask(struct riscv_pmu *pmu, > > > max_period = riscv_pmu_ctr_get_width_mask(event); > > > init_val = local64_read(&hwc->prev_count) & max_period; > > > sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_START, idx, 1, > > > - flag, init_val, 0, 0); > > > + flag, init_val, init_val >> 32, 0); > > > > This should be under "#if __riscv_xlen == 32". > > What's the difference between using CONFIG_32BIT > and checking the __riscv_xlen flag value? It's one and the same. > > CONFIG_32BIT seems to be a bit the more kernel'ish > way to do this, but it looks like most SBI parts check the > __riscv_xlen instead. I agree with you. We should prefer "#ifdef CONFIG_32BIT" in this case to match the kernel coding style. Currently, OpenSBI does not have CONFIG_xyz defines so over there we use "#if __riscv_xlen == 32". Regards, Anup > > > In any case, looking at the opensbi-side of the call, > this fix is abviously correct, so > > Reviewed-by: Heiko Stuebner <heiko@sntech.de> > > >
On Thu, May 12, 2022 at 8:36 AM Anup Patel <anup@brainfault.org> wrote: > > On Thu, May 12, 2022 at 6:12 PM Heiko Stübner <heiko@sntech.de> wrote: > > > > Am Donnerstag, 12. Mai 2022, 06:44:12 CEST schrieb Anup Patel: > > > On Thu, May 12, 2022 at 1:41 AM Atish Patra <atishp@rivosinc.com> wrote: > > > > > > > > Pass the upper half of the initial value of the counter correctly > > > > for RV32. > > > > > > > > Fixes: 4905ec2fb7e6 ("RISC-V: Add sscofpmf extension support") > > > > > > > > Signed-off-by: Atish Patra <atishp@rivosinc.com> > > > > --- > > > > drivers/perf/riscv_pmu_sbi.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > > > > index a1317a483512..24cea59612be 100644 > > > > --- a/drivers/perf/riscv_pmu_sbi.c > > > > +++ b/drivers/perf/riscv_pmu_sbi.c > > > > @@ -526,7 +526,7 @@ static inline void pmu_sbi_start_overflow_mask(struct riscv_pmu *pmu, > > > > max_period = riscv_pmu_ctr_get_width_mask(event); > > > > init_val = local64_read(&hwc->prev_count) & max_period; > > > > sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_START, idx, 1, > > > > - flag, init_val, 0, 0); > > > > + flag, init_val, init_val >> 32, 0); > > > > > > This should be under "#if __riscv_xlen == 32". > > > > What's the difference between using CONFIG_32BIT > > and checking the __riscv_xlen flag value? > > It's one and the same. > > > > > CONFIG_32BIT seems to be a bit the more kernel'ish > > way to do this, but it looks like most SBI parts check the > > __riscv_xlen instead. > Not only SBI parts, there are more users of __riscv_xlen compared CONFIG_32BIT in arch/riscv. > I agree with you. We should prefer "#ifdef CONFIG_32BIT" > in this case to match the kernel coding style. > Sure. I will change it to CONFIG_32BIT. > Currently, OpenSBI does not have CONFIG_xyz defines so > over there we use "#if __riscv_xlen == 32". > > Regards, > Anup > > > > > > > In any case, looking at the opensbi-side of the call, > > this fix is abviously correct, so > > > > Reviewed-by: Heiko Stuebner <heiko@sntech.de> > > > > > >
diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c index a1317a483512..24cea59612be 100644 --- a/drivers/perf/riscv_pmu_sbi.c +++ b/drivers/perf/riscv_pmu_sbi.c @@ -526,7 +526,7 @@ static inline void pmu_sbi_start_overflow_mask(struct riscv_pmu *pmu, max_period = riscv_pmu_ctr_get_width_mask(event); init_val = local64_read(&hwc->prev_count) & max_period; sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_START, idx, 1, - flag, init_val, 0, 0); + flag, init_val, init_val >> 32, 0); } ctr_ovf_mask = ctr_ovf_mask >> 1; idx++;
Pass the upper half of the initial value of the counter correctly for RV32. Fixes: 4905ec2fb7e6 ("RISC-V: Add sscofpmf extension support") Signed-off-by: Atish Patra <atishp@rivosinc.com> --- drivers/perf/riscv_pmu_sbi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)