linux-kernel - Re: [PATCH v5 1/2] RISC-V: Cache SBI vendor values

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 10 Oct 2022 18:15:45 +0530
From:   Anup Patel <apatel@...tanamicro.com>
To:     Heiko Stuebner <heiko@...ech.de>
Cc:     atishp@...shpatra.org, anup@...infault.org, will@...nel.org,
        mark.rutland@....com, paul.walmsley@...ive.com, palmer@...belt.com,
        aou@...s.berkeley.edu, linux-riscv@...ts.infradead.org,
        linux-kernel@...r.kernel.org, Conor.Dooley@...rochip.com,
        ajones@...tanamicro.com
Subject: Re: [PATCH v5 1/2] RISC-V: Cache SBI vendor values

On Mon, Oct 10, 2022 at 5:57 PM Heiko Stuebner <heiko@...ech.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@...shpatra.org>
> Signed-off-by: Heiko Stuebner <heiko@...ech.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
>