diff mbox series

[RFC,4/9] drivers/perf: riscv: Read upper bits of a firmware counter

Message ID 20231205024310.1593100-5-atishp@rivosinc.com
State Superseded
Headers show
Series RISC-V SBI v2.0 PMU improvements and Perf sampling in KVM guest | expand

Commit Message

Atish Kumar Patra Dec. 5, 2023, 2:43 a.m. UTC 
SBI v2.0 introduced a explicit function to read the upper bits
for any firmwar counter width that is longer than XLEN. Currently,
this is only applicable for RV32 where firmware counter can be
64 bit.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 drivers/perf/riscv_pmu_sbi.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Conor Dooley Dec. 7, 2023, 12:32 p.m. UTC | #1 
On Mon, Dec 04, 2023 at 06:43:05PM -0800, Atish Patra wrote:
> SBI v2.0 introduced a explicit function to read the upper bits
> for any firmwar counter width that is longer than XLEN. Currently,
> this is only applicable for RV32 where firmware counter can be
> 64 bit.

The v2.0 spec explicitly says that this function returns the upper
32 bits of the counter for rv32 and will always return 0 for rv64
or higher. The commit message here seems overly generic compared to
the actual definition in the spec, and makes it seem like it could
be used with a 128 bit counter on rv64 to get the upper 64 bits.

I tried to think about what "generic" situation the commit message
had been written for, but the things I came up with would all require
changes to the spec to define behaviour for FID #5 and/or FID #1, so
in the end I couldn't figure out the rationale behind the non-committal
wording used here.

> 
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  drivers/perf/riscv_pmu_sbi.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 40a335350d08..1c9049e6b574 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -490,16 +490,23 @@ 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 defined(CONFIG_32BIT)

Why is this not IS_ENABLED()? The code below uses one. You could then
fold it into the if statement below.

> +		if (sbi_v2_available && info.width >= 32) {

 >= 32? I know it is from the spec, but why does the spec define it as
 "One less than number of bits in CSR"? Saving bits in the structure I
 guess?

> +			ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_FW_READ_HI,
> +					hwc->idx, 0, 0, 0, 0, 0);

> +			if (!ret.error)
> +				val = val | ((u64)ret.value << 32);

If the first ecall fails but the second one doesn't won't we corrupt
val by only setting the upper bits? If returning val == 0 is the thing
to do in the error case (which it is in the existing code) should the
first `if (!ret.error)` become `if (ret.error)` -> `return 0`?


> +				val = val | ((u64)ret.value << 32);

Also, |= ?

Cheers,
Conor.

> +		}
> +#endif
>  	} 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;
> -- 
> 2.34.1
>
Anup Patel Dec. 14, 2023, 12:30 p.m. UTC | #2 
On Thu, Dec 7, 2023 at 6:03 PM Conor Dooley <conor.dooley@microchip.com> wrote:
>
> On Mon, Dec 04, 2023 at 06:43:05PM -0800, Atish Patra wrote:
> > SBI v2.0 introduced a explicit function to read the upper bits
> > for any firmwar counter width that is longer than XLEN. Currently,
> > this is only applicable for RV32 where firmware counter can be
> > 64 bit.
>
> The v2.0 spec explicitly says that this function returns the upper
> 32 bits of the counter for rv32 and will always return 0 for rv64
> or higher. The commit message here seems overly generic compared to
> the actual definition in the spec, and makes it seem like it could
> be used with a 128 bit counter on rv64 to get the upper 64 bits.
>
> I tried to think about what "generic" situation the commit message
> had been written for, but the things I came up with would all require
> changes to the spec to define behaviour for FID #5 and/or FID #1, so
> in the end I couldn't figure out the rationale behind the non-committal
> wording used here.
>
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >  drivers/perf/riscv_pmu_sbi.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> > index 40a335350d08..1c9049e6b574 100644
> > --- a/drivers/perf/riscv_pmu_sbi.c
> > +++ b/drivers/perf/riscv_pmu_sbi.c
> > @@ -490,16 +490,23 @@ 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 defined(CONFIG_32BIT)
>
> Why is this not IS_ENABLED()? The code below uses one. You could then
> fold it into the if statement below.
>
> > +             if (sbi_v2_available && info.width >= 32) {
>
>  >= 32? I know it is from the spec, but why does the spec define it as
>  "One less than number of bits in CSR"? Saving bits in the structure I
>  guess?

Yes, it is for using fewer bits in counter_info.

The maximum width of a HW counter is 64 bits. The absolute value 64
requires 7 bits in counter_info whereas absolute value 63 requires 6 bits
in counter_info. Also, a HW counter if it exists will have at least 1 bit
implemented otherwise the HW counter does not exist.

Regards,
Anup

>
> > +                     ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_FW_READ_HI,
> > +                                     hwc->idx, 0, 0, 0, 0, 0);
>
> > +                     if (!ret.error)
> > +                             val = val | ((u64)ret.value << 32);
>
> If the first ecall fails but the second one doesn't won't we corrupt
> val by only setting the upper bits? If returning val == 0 is the thing
> to do in the error case (which it is in the existing code) should the
> first `if (!ret.error)` become `if (ret.error)` -> `return 0`?
>
>
> > +                             val = val | ((u64)ret.value << 32);
>
> Also, |= ?
>
> Cheers,
> Conor.
>
> > +             }
> > +#endif
> >       } 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;
> > --
> > 2.34.1
> >
Atish Kumar Patra Dec. 16, 2023, 11:54 p.m. UTC | #3 
On Thu, Dec 14, 2023 at 4:30 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Thu, Dec 7, 2023 at 6:03 PM Conor Dooley <conor.dooley@microchip.com> wrote:
> >
> > On Mon, Dec 04, 2023 at 06:43:05PM -0800, Atish Patra wrote:
> > > SBI v2.0 introduced a explicit function to read the upper bits
> > > for any firmwar counter width that is longer than XLEN. Currently,
> > > this is only applicable for RV32 where firmware counter can be
> > > 64 bit.
> >
> > The v2.0 spec explicitly says that this function returns the upper
> > 32 bits of the counter for rv32 and will always return 0 for rv64
> > or higher. The commit message here seems overly generic compared to
> > the actual definition in the spec, and makes it seem like it could
> > be used with a 128 bit counter on rv64 to get the upper 64 bits.
> >
> > I tried to think about what "generic" situation the commit message
> > had been written for, but the things I came up with would all require
> > changes to the spec to define behaviour for FID #5 and/or FID #1, so
> > in the end I couldn't figure out the rationale behind the non-committal
> > wording used here.
> >
The intention was to show that this can be extended in the future for
other XLEN systems
(obviously with spec modification). But I got your point. We can
update it whenever we have
such systems and the spec. Modified the commit text to match what is
in the spec .

> > >
> > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > ---
> > >  drivers/perf/riscv_pmu_sbi.c | 11 +++++++++--
> > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> > > index 40a335350d08..1c9049e6b574 100644
> > > --- a/drivers/perf/riscv_pmu_sbi.c
> > > +++ b/drivers/perf/riscv_pmu_sbi.c
> > > @@ -490,16 +490,23 @@ 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 defined(CONFIG_32BIT)
> >
> > Why is this not IS_ENABLED()? The code below uses one. You could then
> > fold it into the if statement below.
> >

Done.

> > > +             if (sbi_v2_available && info.width >= 32) {
> >
> >  >= 32? I know it is from the spec, but why does the spec define it as
> >  "One less than number of bits in CSR"? Saving bits in the structure I
> >  guess?
>
> Yes, it is for using fewer bits in counter_info.
>
> The maximum width of a HW counter is 64 bits. The absolute value 64
> requires 7 bits in counter_info whereas absolute value 63 requires 6 bits
> in counter_info. Also, a HW counter if it exists will have at least 1 bit
> implemented otherwise the HW counter does not exist.
>
> Regards,
> Anup
>
> >
> > > +                     ret = sbi_ecall(SBI_EXT_PMU, SBI_EXT_PMU_COUNTER_FW_READ_HI,
> > > +                                     hwc->idx, 0, 0, 0, 0, 0);
> >
> > > +                     if (!ret.error)
> > > +                             val = val | ((u64)ret.value << 32);
> >
> > If the first ecall fails but the second one doesn't won't we corrupt
> > val by only setting the upper bits? If returning val == 0 is the thing
> > to do in the error case (which it is in the existing code) should the
> > first `if (!ret.error)` become `if (ret.error)` -> `return 0`?
> >

Sure. Fixed it.

> >
> > > +                             val = val | ((u64)ret.value << 32);
> >
> > Also, |= ?
> >

Done.


> > Cheers,
> > Conor.
> >
> > > +             }
> > > +#endif
> > >       } 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;
> > > --
> > > 2.34.1
> > >
diff mbox series

Patch

diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
index 40a335350d08..1c9049e6b574 100644
--- a/drivers/perf/riscv_pmu_sbi.c
+++ b/drivers/perf/riscv_pmu_sbi.c
@@ -490,16 +490,23 @@  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 defined(CONFIG_32BIT)
+		if (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 = val | ((u64)ret.value << 32);
+		}
+#endif
 	} 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;