| Message ID | 20240403080452.1007601-4-atishp@rivosinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V SBI v2.0 PMU improvements and Perf sampling in KVM guest | expand |
On Wed, Apr 03, 2024 at 01:04:32AM -0700, Atish Patra wrote: > SBI v2.0 introduced a explicit function to read the upper 32 bits > for any firmware counter width that is longer than 32bits. > This is only applicable for RV32 where firmware counter can be > 64 bit. > > Reviewed-by: Andrew Jones <ajones@ventanamicro.com> > Acked-by: Palmer Dabbelt <palmer@rivosinc.com> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> > Reviewed-by: Anup Patel <anup@brainfault.org> > Signed-off-by: Atish Patra <atishp@rivosinc.com> > --- > drivers/perf/riscv_pmu_sbi.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c > index 3e44d2fb8bf8..babf1b9a4dbe 100644 > --- a/drivers/perf/riscv_pmu_sbi.c > +++ b/drivers/perf/riscv_pmu_sbi.c > @@ -57,6 +57,8 @@ asm volatile(ALTERNATIVE( \ > PMU_FORMAT_ATTR(event, "config:0-47"); > PMU_FORMAT_ATTR(firmware, "config:63"); > > +static bool sbi_v2_available; > + > static struct attribute *riscv_arch_formats_attr[] = { > &format_attr_event.attr, > &format_attr_firmware.attr, > @@ -511,19 +513,29 @@ static u64 pmu_sbi_ctr_read(struct perf_event *event) > struct hw_perf_event *hwc = &event->hw; > int idx = hwc->idx; > struct sbiret ret; > - union sbi_pmu_ctr_info info; > u64 val = 0; > + union sbi_pmu_ctr_info info = pmu_ctr_list[idx]; > > if (pmu_sbi_is_fw_event(event)) { > ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_FW_READ, > hwc->idx, 0, 0, 0, 0, 0); > - if (!ret.error) > - val = ret.value; > + if (ret.error) > + return 0; > + > + val = ret.value; > + if (IS_ENABLED(CONFIG_32BIT) && sbi_v2_available && info.width >= 32) { > + ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_FW_READ_HI, > + hwc->idx, 0, 0, 0, 0, 0); > + if (!ret.error) > + val |= ((u64)ret.value << 32); > + else > + WARN_ONCE(1, "Unable to read upper 32 bits of firmware counter error: %d\n", > + sbi_err_map_linux_errno(ret.error)); I don't think we should use sbi_err_map_linux_errno() in this case since we don't have a 1:1 mapping of SBI errors to Linux errors and we don't propagate the error as a Linux error. For warnings, it's better to output the exact SBI error. > + } > } else { > - info = pmu_ctr_list[idx]; > val = riscv_pmu_ctr_read_csr(info.csr); > if (IS_ENABLED(CONFIG_32BIT)) > - val = ((u64)riscv_pmu_ctr_read_csr(info.csr + 0x80)) << 31 | val; > + val |= ((u64)riscv_pmu_ctr_read_csr(info.csr + 0x80)) << 32; > } > > return val; > @@ -1135,6 +1147,9 @@ static int __init pmu_sbi_devinit(void) > return 0; > } > > + if (sbi_spec_version >= sbi_mk_version(2, 0)) > + sbi_v2_available = true; > + > ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_RISCV_STARTING, > "perf/riscv/pmu:starting", > pmu_sbi_starting_cpu, pmu_sbi_dying_cpu); > -- > 2.34.1 > Thanks, drew
On 4/4/24 04:02, Andrew Jones wrote: > On Wed, Apr 03, 2024 at 01:04:32AM -0700, Atish Patra wrote: >> SBI v2.0 introduced a explicit function to read the upper 32 bits >> for any firmware counter width that is longer than 32bits. >> This is only applicable for RV32 where firmware counter can be >> 64 bit. >> >> Reviewed-by: Andrew Jones <ajones@ventanamicro.com> >> Acked-by: Palmer Dabbelt <palmer@rivosinc.com> >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com> >> Reviewed-by: Anup Patel <anup@brainfault.org> >> Signed-off-by: Atish Patra <atishp@rivosinc.com> >> --- >> drivers/perf/riscv_pmu_sbi.c | 25 ++++++++++++++++++++----- >> 1 file changed, 20 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c >> index 3e44d2fb8bf8..babf1b9a4dbe 100644 >> --- a/drivers/perf/riscv_pmu_sbi.c >> +++ b/drivers/perf/riscv_pmu_sbi.c >> @@ -57,6 +57,8 @@ asm volatile(ALTERNATIVE( \ >> PMU_FORMAT_ATTR(event, "config:0-47"); >> PMU_FORMAT_ATTR(firmware, "config:63"); >> >> +static bool sbi_v2_available; >> + >> static struct attribute *riscv_arch_formats_attr[] = { >> &format_attr_event.attr, >> &format_attr_firmware.attr, >> @@ -511,19 +513,29 @@ static u64 pmu_sbi_ctr_read(struct perf_event *event) >> struct hw_perf_event *hwc = &event->hw; >> int idx = hwc->idx; >> struct sbiret ret; >> - union sbi_pmu_ctr_info info; >> u64 val = 0; >> + union sbi_pmu_ctr_info info = pmu_ctr_list[idx]; >> >> if (pmu_sbi_is_fw_event(event)) { >> ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_FW_READ, >> hwc->idx, 0, 0, 0, 0, 0); >> - if (!ret.error) >> - val = ret.value; >> + if (ret.error) >> + return 0; >> + >> + val = ret.value; >> + if (IS_ENABLED(CONFIG_32BIT) && sbi_v2_available && info.width >= 32) { >> + ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_FW_READ_HI, >> + hwc->idx, 0, 0, 0, 0, 0); >> + if (!ret.error) >> + val |= ((u64)ret.value << 32); >> + else >> + WARN_ONCE(1, "Unable to read upper 32 bits of firmware counter error: %d\n", >> + sbi_err_map_linux_errno(ret.error)); > > I don't think we should use sbi_err_map_linux_errno() in this case since > we don't have a 1:1 mapping of SBI errors to Linux errors and we don't > propagate the error as a Linux error. For warnings, it's better to output > the exact SBI error. > Sure. Fixed it. >> + } >> } else { >> - info = pmu_ctr_list[idx]; >> val = riscv_pmu_ctr_read_csr(info.csr); >> if (IS_ENABLED(CONFIG_32BIT)) >> - val = ((u64)riscv_pmu_ctr_read_csr(info.csr + 0x80)) << 31 | val; >> + val |= ((u64)riscv_pmu_ctr_read_csr(info.csr + 0x80)) << 32; >> } >> >> return val; >> @@ -1135,6 +1147,9 @@ static int __init pmu_sbi_devinit(void) >> return 0; >> } >> >> + if (sbi_spec_version >= sbi_mk_version(2, 0)) >> + sbi_v2_available = true; >> + >> ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_RISCV_STARTING, >> "perf/riscv/pmu:starting", >> pmu_sbi_starting_cpu, pmu_sbi_dying_cpu); >> -- >> 2.34.1 >> > > Thanks, > drew
diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c index 3e44d2fb8bf8..babf1b9a4dbe 100644 --- a/drivers/perf/riscv_pmu_sbi.c +++ b/drivers/perf/riscv_pmu_sbi.c @@ -57,6 +57,8 @@ asm volatile(ALTERNATIVE( \ PMU_FORMAT_ATTR(event, "config:0-47"); PMU_FORMAT_ATTR(firmware, "config:63"); +static bool sbi_v2_available; + static struct attribute *riscv_arch_formats_attr[] = { &format_attr_event.attr, &format_attr_firmware.attr, @@ -511,19 +513,29 @@ static u64 pmu_sbi_ctr_read(struct perf_event *event) struct hw_perf_event *hwc = &event->hw; int idx = hwc->idx; struct sbiret ret; - union sbi_pmu_ctr_info info; u64 val = 0; + union sbi_pmu_ctr_info info = pmu_ctr_list[idx]; if (pmu_sbi_is_fw_event(event)) { ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_FW_READ, hwc->idx, 0, 0, 0, 0, 0); - if (!ret.error) - val = ret.value; + if (ret.error) + return 0; + + val = ret.value; + if (IS_ENABLED(CONFIG_32BIT) && sbi_v2_available && info.width >= 32) { + ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_FW_READ_HI, + hwc->idx, 0, 0, 0, 0, 0); + if (!ret.error) + val |= ((u64)ret.value << 32); + else + WARN_ONCE(1, "Unable to read upper 32 bits of firmware counter error: %d\n", + sbi_err_map_linux_errno(ret.error)); + } } else { - info = pmu_ctr_list[idx]; val = riscv_pmu_ctr_read_csr(info.csr); if (IS_ENABLED(CONFIG_32BIT)) - val = ((u64)riscv_pmu_ctr_read_csr(info.csr + 0x80)) << 31 | val; + val |= ((u64)riscv_pmu_ctr_read_csr(info.csr + 0x80)) << 32; } return val; @@ -1135,6 +1147,9 @@ static int __init pmu_sbi_devinit(void) return 0; } + if (sbi_spec_version >= sbi_mk_version(2, 0)) + sbi_v2_available = true; + ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_RISCV_STARTING, "perf/riscv/pmu:starting", pmu_sbi_starting_cpu, pmu_sbi_dying_cpu);