diff mbox series

[v5,1/2] RISC-V: Cache SBI vendor values

Message ID 20221010122726.2405153-2-heiko@sntech.de (mailing list archive)
State Superseded
Headers show
Series riscv_pmu_sbi: add support for PMU variant on T-Head C9xx cores | expand

Commit Message

Heiko Stübner Oct. 10, 2022, 12:27 p.m. UTC 
sbi_get_mvendorid(), sbi_get_marchid() and sbi_get_mimpid() might get
called multiple times, though the values of these CSRs should not change
during the runtime of a specific machine.

So cache the values in the functions and prevent multiple ecalls
to read these values.

As Andrew Jones noted, at least marchid and mimpid may be negative
values when viewed as a long, so we use a separate static bool to
indiciate the cached status.

Suggested-by: Atish Patra <atishp@atishpatra.org>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/riscv/kernel/sbi.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

Comments

Anup Patel Oct. 10, 2022, 12:45 p.m. UTC | #1 
On Mon, Oct 10, 2022 at 5:57 PM Heiko Stuebner <heiko@sntech.de> wrote:
>
> sbi_get_mvendorid(), sbi_get_marchid() and sbi_get_mimpid() might get
> called multiple times, though the values of these CSRs should not change
> during the runtime of a specific machine.
>
> So cache the values in the functions and prevent multiple ecalls
> to read these values.
>
> As Andrew Jones noted, at least marchid and mimpid may be negative
> values when viewed as a long, so we use a separate static bool to
> indiciate the cached status.
>
> Suggested-by: Atish Patra <atishp@atishpatra.org>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  arch/riscv/kernel/sbi.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index 775d3322b422..cc618aaa9d11 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -625,17 +625,41 @@ static inline long sbi_get_firmware_version(void)
>
>  long sbi_get_mvendorid(void)
>  {
> -       return __sbi_base_ecall(SBI_EXT_BASE_GET_MVENDORID);
> +       static long id;
> +       static bool cached;
> +
> +       if (!cached) {
> +               id = __sbi_base_ecall(SBI_EXT_BASE_GET_MVENDORID);
> +               cached = true;
> +       }
> +
> +       return id;
>  }
>
>  long sbi_get_marchid(void)
>  {
> -       return __sbi_base_ecall(SBI_EXT_BASE_GET_MARCHID);
> +       static long id;
> +       static bool cached;

This breaks for heterogeneous SMP systems (similar to big.LITTLE)
where HARTs will have different marchid even though they belong to
same CPU Vendor.

Due to the above rationale, the patch adding marchid, mvendorid, and
mimpid in /proc/cpuinfo caches these values on a per-CPU basis.

Regards,
Anup

> +
> +       if (!cached) {
> +               id = __sbi_base_ecall(SBI_EXT_BASE_GET_MARCHID);
> +               cached = true;
> +       }
> +
> +       return id;
>  }
>
>  long sbi_get_mimpid(void)
>  {
> -       return __sbi_base_ecall(SBI_EXT_BASE_GET_MIMPID);
> +       static long id;
> +       static bool cached;
> +
> +       if (!cached) {
> +               id = __sbi_base_ecall(SBI_EXT_BASE_GET_MIMPID);
> +               cached = true;
> +       }
> +
> +       return id;
>  }
>
>  static void sbi_send_cpumask_ipi(const struct cpumask *target)
> --
> 2.35.1
>
Heiko Stübner Oct. 10, 2022, 12:54 p.m. UTC | #2 
Am Montag, 10. Oktober 2022, 14:45:45 CEST schrieb Anup Patel:
> On Mon, Oct 10, 2022 at 5:57 PM Heiko Stuebner <heiko@sntech.de> wrote:
> >
> > sbi_get_mvendorid(), sbi_get_marchid() and sbi_get_mimpid() might get
> > called multiple times, though the values of these CSRs should not change
> > during the runtime of a specific machine.
> >
> > So cache the values in the functions and prevent multiple ecalls
> > to read these values.
> >
> > As Andrew Jones noted, at least marchid and mimpid may be negative
> > values when viewed as a long, so we use a separate static bool to
> > indiciate the cached status.
> >
> > Suggested-by: Atish Patra <atishp@atishpatra.org>
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> >  arch/riscv/kernel/sbi.c | 30 +++++++++++++++++++++++++++---
> >  1 file changed, 27 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > index 775d3322b422..cc618aaa9d11 100644
> > --- a/arch/riscv/kernel/sbi.c
> > +++ b/arch/riscv/kernel/sbi.c
> > @@ -625,17 +625,41 @@ static inline long sbi_get_firmware_version(void)
> >
> >  long sbi_get_mvendorid(void)
> >  {
> > -       return __sbi_base_ecall(SBI_EXT_BASE_GET_MVENDORID);
> > +       static long id;
> > +       static bool cached;
> > +
> > +       if (!cached) {
> > +               id = __sbi_base_ecall(SBI_EXT_BASE_GET_MVENDORID);
> > +               cached = true;
> > +       }
> > +
> > +       return id;
> >  }
> >
> >  long sbi_get_marchid(void)
> >  {
> > -       return __sbi_base_ecall(SBI_EXT_BASE_GET_MARCHID);
> > +       static long id;
> > +       static bool cached;
> 
> This breaks for heterogeneous SMP systems (similar to big.LITTLE)
> where HARTs will have different marchid even though they belong to
> same CPU Vendor.
> 
> Due to the above rationale, the patch adding marchid, mvendorid, and
> mimpid in /proc/cpuinfo caches these values on a per-CPU basis.

For people reading along, I think you mean
https://lore.kernel.org/r/20220727043829.151794-1-apatel@ventanamicro.com

For my understanding, was there a reason in the past for doing the caching
only for cpuinfo and not for every invocation of the ecalls?


Thanks
Heiko
Anup Patel Oct. 10, 2022, 1:14 p.m. UTC | #3 
On Mon, Oct 10, 2022 at 6:25 PM Heiko Stuebner <heiko@sntech.de> wrote:
>
> Am Montag, 10. Oktober 2022, 14:45:45 CEST schrieb Anup Patel:
> > On Mon, Oct 10, 2022 at 5:57 PM Heiko Stuebner <heiko@sntech.de> wrote:
> > >
> > > sbi_get_mvendorid(), sbi_get_marchid() and sbi_get_mimpid() might get
> > > called multiple times, though the values of these CSRs should not change
> > > during the runtime of a specific machine.
> > >
> > > So cache the values in the functions and prevent multiple ecalls
> > > to read these values.
> > >
> > > As Andrew Jones noted, at least marchid and mimpid may be negative
> > > values when viewed as a long, so we use a separate static bool to
> > > indiciate the cached status.
> > >
> > > Suggested-by: Atish Patra <atishp@atishpatra.org>
> > > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > > ---
> > >  arch/riscv/kernel/sbi.c | 30 +++++++++++++++++++++++++++---
> > >  1 file changed, 27 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > > index 775d3322b422..cc618aaa9d11 100644
> > > --- a/arch/riscv/kernel/sbi.c
> > > +++ b/arch/riscv/kernel/sbi.c
> > > @@ -625,17 +625,41 @@ static inline long sbi_get_firmware_version(void)
> > >
> > >  long sbi_get_mvendorid(void)
> > >  {
> > > -       return __sbi_base_ecall(SBI_EXT_BASE_GET_MVENDORID);
> > > +       static long id;
> > > +       static bool cached;
> > > +
> > > +       if (!cached) {
> > > +               id = __sbi_base_ecall(SBI_EXT_BASE_GET_MVENDORID);
> > > +               cached = true;
> > > +       }
> > > +
> > > +       return id;
> > >  }
> > >
> > >  long sbi_get_marchid(void)
> > >  {
> > > -       return __sbi_base_ecall(SBI_EXT_BASE_GET_MARCHID);
> > > +       static long id;
> > > +       static bool cached;
> >
> > This breaks for heterogeneous SMP systems (similar to big.LITTLE)
> > where HARTs will have different marchid even though they belong to
> > same CPU Vendor.
> >
> > Due to the above rationale, the patch adding marchid, mvendorid, and
> > mimpid in /proc/cpuinfo caches these values on a per-CPU basis.
>
> For people reading along, I think you mean
> https://lore.kernel.org/r/20220727043829.151794-1-apatel@ventanamicro.com

Yes, this is the patch I am referring.

>
> For my understanding, was there a reason in the past for doing the caching
> only for cpuinfo and not for every invocation of the ecalls?

The caching was done only for /proc/cpuinfo because at that point
time only "cat /proc/cpuinfo" would need these value at runtime.

Now that we have more uses of marchid, mvendorid, and mimpid,
at runtime, we should definitely have the cached values available
to other parts of kernel.

My suggestion is to keep sbi_get_xyz() functions unmodified and
instead add new functions in arch/riscv/kernel/cpu.c (on-top-of
/proc/cpuinfo patch) which allows users to read cached values of
any CPU.

For example, we could export following functions from
arch/riscv/kernel/cpu.c:
unsigned long riscv_cached_mvendorid(unsigned int cpu);
unsigned long riscv_cached_marchid(unsigned int cpu);
unsigned long riscv_cached_mimpid(unsigned int cpu);

Regards,
Anup
Heiko Stübner Oct. 11, 2022, 7:58 a.m. UTC | #4 
Hi Anup,

Am Montag, 10. Oktober 2022, 15:14:21 CEST schrieb Anup Patel:
> On Mon, Oct 10, 2022 at 6:25 PM Heiko Stuebner <heiko@sntech.de> wrote:
> >
> > Am Montag, 10. Oktober 2022, 14:45:45 CEST schrieb Anup Patel:
> > > On Mon, Oct 10, 2022 at 5:57 PM Heiko Stuebner <heiko@sntech.de> wrote:
> > > >
> > > > sbi_get_mvendorid(), sbi_get_marchid() and sbi_get_mimpid() might get
> > > > called multiple times, though the values of these CSRs should not change
> > > > during the runtime of a specific machine.
> > > >
> > > > So cache the values in the functions and prevent multiple ecalls
> > > > to read these values.
> > > >
> > > > As Andrew Jones noted, at least marchid and mimpid may be negative
> > > > values when viewed as a long, so we use a separate static bool to
> > > > indiciate the cached status.
> > > >
> > > > Suggested-by: Atish Patra <atishp@atishpatra.org>
> > > > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > > > ---
> > > >  arch/riscv/kernel/sbi.c | 30 +++++++++++++++++++++++++++---
> > > >  1 file changed, 27 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > > > index 775d3322b422..cc618aaa9d11 100644
> > > > --- a/arch/riscv/kernel/sbi.c
> > > > +++ b/arch/riscv/kernel/sbi.c
> > > > @@ -625,17 +625,41 @@ static inline long sbi_get_firmware_version(void)
> > > >
> > > >  long sbi_get_mvendorid(void)
> > > >  {
> > > > -       return __sbi_base_ecall(SBI_EXT_BASE_GET_MVENDORID);
> > > > +       static long id;
> > > > +       static bool cached;
> > > > +
> > > > +       if (!cached) {
> > > > +               id = __sbi_base_ecall(SBI_EXT_BASE_GET_MVENDORID);
> > > > +               cached = true;
> > > > +       }
> > > > +
> > > > +       return id;
> > > >  }
> > > >
> > > >  long sbi_get_marchid(void)
> > > >  {
> > > > -       return __sbi_base_ecall(SBI_EXT_BASE_GET_MARCHID);
> > > > +       static long id;
> > > > +       static bool cached;
> > >
> > > This breaks for heterogeneous SMP systems (similar to big.LITTLE)
> > > where HARTs will have different marchid even though they belong to
> > > same CPU Vendor.
> > >
> > > Due to the above rationale, the patch adding marchid, mvendorid, and
> > > mimpid in /proc/cpuinfo caches these values on a per-CPU basis.
> >
> > For people reading along, I think you mean
> > https://lore.kernel.org/r/20220727043829.151794-1-apatel@ventanamicro.com
> 
> Yes, this is the patch I am referring.
> 
> >
> > For my understanding, was there a reason in the past for doing the caching
> > only for cpuinfo and not for every invocation of the ecalls?
> 
> The caching was done only for /proc/cpuinfo because at that point
> time only "cat /proc/cpuinfo" would need these value at runtime.
> 
> Now that we have more uses of marchid, mvendorid, and mimpid,
> at runtime, we should definitely have the cached values available
> to other parts of kernel.
> 
> My suggestion is to keep sbi_get_xyz() functions unmodified and
> instead add new functions in arch/riscv/kernel/cpu.c (on-top-of
> /proc/cpuinfo patch) which allows users to read cached values of
> any CPU.
> 
> For example, we could export following functions from
> arch/riscv/kernel/cpu.c:
> unsigned long riscv_cached_mvendorid(unsigned int cpu);
> unsigned long riscv_cached_marchid(unsigned int cpu);
> unsigned long riscv_cached_mimpid(unsigned int cpu);

sounds sensible, so I'll go that way you suggested :-)


Thanks
Heiko
diff mbox series

Patch

diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
index 775d3322b422..cc618aaa9d11 100644
--- a/arch/riscv/kernel/sbi.c
+++ b/arch/riscv/kernel/sbi.c
@@ -625,17 +625,41 @@  static inline long sbi_get_firmware_version(void)
 
 long sbi_get_mvendorid(void)
 {
-	return __sbi_base_ecall(SBI_EXT_BASE_GET_MVENDORID);
+	static long id;
+	static bool cached;
+
+	if (!cached) {
+		id = __sbi_base_ecall(SBI_EXT_BASE_GET_MVENDORID);
+		cached = true;
+	}
+
+	return id;
 }
 
 long sbi_get_marchid(void)
 {
-	return __sbi_base_ecall(SBI_EXT_BASE_GET_MARCHID);
+	static long id;
+	static bool cached;
+
+	if (!cached) {
+		id = __sbi_base_ecall(SBI_EXT_BASE_GET_MARCHID);
+		cached = true;
+	}
+
+	return id;
 }
 
 long sbi_get_mimpid(void)
 {
-	return __sbi_base_ecall(SBI_EXT_BASE_GET_MIMPID);
+	static long id;
+	static bool cached;
+
+	if (!cached) {
+		id = __sbi_base_ecall(SBI_EXT_BASE_GET_MIMPID);
+		cached = true;
+	}
+
+	return id;
 }
 
 static void sbi_send_cpumask_ipi(const struct cpumask *target)