linux-kernel - Re: [PATCH v2] drivers/perf: riscv_pmu_sbi: add support for PMU variant on T-Head C9xx cores

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:   Fri, 26 Aug 2022 17:57:33 +0000
From:   <Conor.Dooley@...rochip.com>
To:     <heiko@...ech.de>, <atishp@...shpatra.org>, <anup@...infault.org>,
        <will@...nel.org>, <mark.rutland@....com>,
        <paul.walmsley@...ive.com>, <palmer@...belt.com>,
        <aou@...s.berkeley.edu>
CC:     <linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <philipp.tomsich@...ll.eu>, <cmuellner@...ux.com>,
        <samuel@...lland.org>, <guoren@...nel.org>
Subject: Re: [PATCH v2] drivers/perf: riscv_pmu_sbi: add support for PMU
 variant on T-Head C9xx cores

On 26/08/2022 17:35, Heiko Stuebner wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> With the T-HEAD C9XX cores being designed before or during the ratification
> to the SSCOFPMF extension, they implement functionality very similar but
> not equal to it. So add some adaptions to allow the C9XX to still handle
> its PMU through the regular SBI PMU interface instead of defining new
> interfaces or drivers.
> 
> To work properly, this requires a matching change in SBI, though the actual
> interface between kernel and SBI does not change.
> 
> The main differences are a the overflow CSR and irq number.
> 
> As the reading of the overflow-csr is in the hot-path during irq handling

Hey Heiko,

Very nitpicky, but I had to read this twice to get it.. If you respin,
it'd be worth adding a comma after "handling".

> use an errata and alternatives to not introduce new conditionals there.
> 
> Signed-off-by: Heiko Stuebner <heiko@...ech.de>
> ---
> changes in v2:
> - use alternatives for the CSR access
> - make the irq num selection a bit nicer
> 
> There is of course a matching opensbi-part whose current implementation can
> be found on [0], but as comments show, this needs some more work still.
> 
> 
> [0] https://patchwork.ozlabs.org/project/opensbi/cover/20220817112004.745776-1-heiko@sntech.de/
> 
>  arch/riscv/Kconfig.erratas           | 14 ++++++++++++
>  arch/riscv/errata/thead/errata.c     | 19 +++++++++++++++++
>  arch/riscv/include/asm/errata_list.h | 16 +++++++++++++-
>  drivers/perf/riscv_pmu_sbi.c         | 32 +++++++++++++++++++---------
>  4 files changed, 70 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/riscv/Kconfig.erratas b/arch/riscv/Kconfig.erratas
> index 6850e9389930..f1eaac4c0073 100644
> --- a/arch/riscv/Kconfig.erratas
> +++ b/arch/riscv/Kconfig.erratas
> @@ -66,4 +66,18 @@ config ERRATA_THEAD_CMO
> 
>           If you don't know what to do here, say "Y".
> 
> +config ERRATA_THEAD_PMU
> +       bool "Apply T-Head PMU errata"
> +       depends on ERRATA_THEAD
> +       depends on RISCV_PMU_SBI
> +       default y
> +       help
> +         The T-Head C9xx cores implement a PMU overflow extension very
> +         similar to the core SSCOFPMF extension.
> +
> +         This will apply the overflow errata to handle the non-standard
> +         behaviour via the regular SBI PMU driver and interface.
> +
> +         If you don't know what to do here, say "Y".
> +
>  endmenu # "CPU errata selection"
> diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
> index 202c83f677b2..e6101eab25c8 100644
> --- a/arch/riscv/errata/thead/errata.c
> +++ b/arch/riscv/errata/thead/errata.c
> @@ -44,6 +44,22 @@ static bool errata_probe_cmo(unsigned int stage,
>  #endif
>  }
> 
> +static bool errata_probe_pmu(unsigned int stage,
> +                            unsigned long arch_id, unsigned long impid)
> +{
> +#ifdef CONFIG_ERRATA_THEAD_PMU

Is there a reason that all the alternatives use ifdef
rather than if(IS_ENABLED())?

> +       if (arch_id != 0 || impid != 0)
> +               return false;
> +
> +       if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
> +               return false;
> +
> +       return true;
> +#else
> +       return false;
> +#endif
> +}
> +
>  static u32 thead_errata_probe(unsigned int stage,
>                               unsigned long archid, unsigned long impid)
>  {
> @@ -55,6 +71,9 @@ static u32 thead_errata_probe(unsigned int stage,
>         if (errata_probe_cmo(stage, archid, impid))
>                 cpu_req_errata |= (1U << ERRATA_THEAD_CMO);
> 
> +       if (errata_probe_pmu(stage, archid, impid))
> +               cpu_req_errata |= (1U << ERRATA_THEAD_PMU);

BIT(ERRATA_THEAD_PMU), no? Ditto for the CMO I guess..

> +
>         return cpu_req_errata;
>  }
> 

Thanks,
Conor.