From: Heiko Stuebner <heiko@sntech.de> To: palmer@dabbelt.com, paul.walmsley@sifive.com Cc: linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, wefu@redhat.com, guoren@kernel.org, mick@ics.forth.gr, samuel@sholland.org, cmuellner@linux.com, philipp.tomsich@vrull.eu, hch@lst.de, Heiko Stuebner <heiko@sntech.de> Subject: [PATCH v2 5/5] riscv: remove usage of function-pointers from cpufeatures and t-head errata Date: Thu, 26 May 2022 22:56:46 +0200 [thread overview] Message-ID: <20220526205646.258337-6-heiko@sntech.de> (raw) In-Reply-To: <20220526205646.258337-1-heiko@sntech.de> Having a list of alternatives to check with a per-entry function pointer to a check function is nice style-wise. But in case of early-alternatives it can clash with the non-relocated kernel and the function pointer in the list pointing to a completely wrong location. This isn't an issue with one or two list entries, as in that case the compiler seems to unroll the loop and even usage of the list structure and then only does relative jumps into the check functions based on this. When adding a third entry to either list though, the issue that was hiding there from the beginning is triggered resulting a jump to a memory address that isn't part of the kernel at all. The list of features/erratas only contained an unused name and the pointer to the check function, so an easy solution for the problem is to just unroll the loop in code, dismantle the whole list structure and just call the relevant check functions one by one ourself. For the T-Head errata this includes moving the stage-check inside the check functions. The issue is only relevant for things that might be called for early- alternatives (T-Head and possible future main extensions), so the SiFive erratas were not affected from the beginning, as they got an early return for early-alternatives in the original patchset. Signed-off-by: Heiko Stuebner <heiko@sntech.de> --- arch/riscv/errata/thead/errata.c | 38 ++++++++++---------------------- arch/riscv/kernel/cpufeature.c | 32 +++++++++------------------ 2 files changed, 22 insertions(+), 48 deletions(-) diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c index e5d75270b99c..cc155228247d 100644 --- a/arch/riscv/errata/thead/errata.c +++ b/arch/riscv/errata/thead/errata.c @@ -14,40 +14,26 @@ #include <asm/patch.h> #include <asm/vendorid_list.h> -struct errata_info { - char name[ERRATA_STRING_LENGTH_MAX]; - bool (*check_func)(unsigned long arch_id, unsigned long impid); - unsigned int stage; -}; - -static bool errata_mt_check_func(unsigned long arch_id, unsigned long impid) +static bool errata_probe_pbmt(unsigned int stage, + unsigned long arch_id, unsigned long impid) { if (arch_id != 0 || impid != 0) return false; - return true; -} -static const struct errata_info errata_list[ERRATA_THEAD_NUMBER] = { - { - .name = "memory-types", - .stage = RISCV_ALTERNATIVES_EARLY_BOOT, - .check_func = errata_mt_check_func - }, -}; + if (stage == RISCV_ALTERNATIVES_EARLY_BOOT || + stage == RISCV_ALTERNATIVES_MODULE) + return true; + + return false; +} -static u32 thead_errata_probe(unsigned int stage, unsigned long archid, unsigned long impid) +static u32 thead_errata_probe(unsigned int stage, + unsigned long archid, unsigned long impid) { - const struct errata_info *info; u32 cpu_req_errata = 0; - int idx; - - for (idx = 0; idx < ERRATA_THEAD_NUMBER; idx++) { - info = &errata_list[idx]; - if ((stage == RISCV_ALTERNATIVES_MODULE || - info->stage == stage) && info->check_func(archid, impid)) - cpu_req_errata |= (1U << idx); - } + if (errata_probe_pbmt(stage, archid, impid)) + cpu_req_errata |= (1U << ERRATA_THEAD_PBMT); return cpu_req_errata; } diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index b33564df81e1..b63c25c55bf1 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -246,12 +246,7 @@ void __init riscv_fill_hwcap(void) } #ifdef CONFIG_RISCV_ALTERNATIVE -struct cpufeature_info { - char name[ERRATA_STRING_LENGTH_MAX]; - bool (*check_func)(unsigned int stage); -}; - -static bool __init_or_module cpufeature_svpbmt_check_func(unsigned int stage) +static bool __init_or_module cpufeature_probe_svpbmt(unsigned int stage) { #ifdef CONFIG_RISCV_ISA_SVPBMT switch (stage) { @@ -265,26 +260,19 @@ static bool __init_or_module cpufeature_svpbmt_check_func(unsigned int stage) return false; } -static const struct cpufeature_info __initdata_or_module -cpufeature_list[CPUFEATURE_NUMBER] = { - { - .name = "svpbmt", - .check_func = cpufeature_svpbmt_check_func - }, -}; - +/* + * Probe presence of individual extensions. + * + * This code may also be executed before kernel relocation, so we cannot use + * addresses generated by the address-of operator as they won't be valid in + * this context. + */ static u32 __init_or_module cpufeature_probe(unsigned int stage) { - const struct cpufeature_info *info; u32 cpu_req_feature = 0; - int idx; - - for (idx = 0; idx < CPUFEATURE_NUMBER; idx++) { - info = &cpufeature_list[idx]; - if (info->check_func(stage)) - cpu_req_feature |= (1U << idx); - } + if (cpufeature_probe_svpbmt(stage)) + cpu_req_feature |= (1U << CPUFEATURE_SVPBMT); return cpu_req_feature; } -- 2.35.1
WARNING: multiple messages have this Message-ID (diff)
From: Heiko Stuebner <heiko@sntech.de> To: palmer@dabbelt.com, paul.walmsley@sifive.com Cc: linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org, wefu@redhat.com, guoren@kernel.org, mick@ics.forth.gr, samuel@sholland.org, cmuellner@linux.com, philipp.tomsich@vrull.eu, hch@lst.de, Heiko Stuebner <heiko@sntech.de> Subject: [PATCH v2 5/5] riscv: remove usage of function-pointers from cpufeatures and t-head errata Date: Thu, 26 May 2022 22:56:46 +0200 [thread overview] Message-ID: <20220526205646.258337-6-heiko@sntech.de> (raw) In-Reply-To: <20220526205646.258337-1-heiko@sntech.de> Having a list of alternatives to check with a per-entry function pointer to a check function is nice style-wise. But in case of early-alternatives it can clash with the non-relocated kernel and the function pointer in the list pointing to a completely wrong location. This isn't an issue with one or two list entries, as in that case the compiler seems to unroll the loop and even usage of the list structure and then only does relative jumps into the check functions based on this. When adding a third entry to either list though, the issue that was hiding there from the beginning is triggered resulting a jump to a memory address that isn't part of the kernel at all. The list of features/erratas only contained an unused name and the pointer to the check function, so an easy solution for the problem is to just unroll the loop in code, dismantle the whole list structure and just call the relevant check functions one by one ourself. For the T-Head errata this includes moving the stage-check inside the check functions. The issue is only relevant for things that might be called for early- alternatives (T-Head and possible future main extensions), so the SiFive erratas were not affected from the beginning, as they got an early return for early-alternatives in the original patchset. Signed-off-by: Heiko Stuebner <heiko@sntech.de> --- arch/riscv/errata/thead/errata.c | 38 ++++++++++---------------------- arch/riscv/kernel/cpufeature.c | 32 +++++++++------------------ 2 files changed, 22 insertions(+), 48 deletions(-) diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c index e5d75270b99c..cc155228247d 100644 --- a/arch/riscv/errata/thead/errata.c +++ b/arch/riscv/errata/thead/errata.c @@ -14,40 +14,26 @@ #include <asm/patch.h> #include <asm/vendorid_list.h> -struct errata_info { - char name[ERRATA_STRING_LENGTH_MAX]; - bool (*check_func)(unsigned long arch_id, unsigned long impid); - unsigned int stage; -}; - -static bool errata_mt_check_func(unsigned long arch_id, unsigned long impid) +static bool errata_probe_pbmt(unsigned int stage, + unsigned long arch_id, unsigned long impid) { if (arch_id != 0 || impid != 0) return false; - return true; -} -static const struct errata_info errata_list[ERRATA_THEAD_NUMBER] = { - { - .name = "memory-types", - .stage = RISCV_ALTERNATIVES_EARLY_BOOT, - .check_func = errata_mt_check_func - }, -}; + if (stage == RISCV_ALTERNATIVES_EARLY_BOOT || + stage == RISCV_ALTERNATIVES_MODULE) + return true; + + return false; +} -static u32 thead_errata_probe(unsigned int stage, unsigned long archid, unsigned long impid) +static u32 thead_errata_probe(unsigned int stage, + unsigned long archid, unsigned long impid) { - const struct errata_info *info; u32 cpu_req_errata = 0; - int idx; - - for (idx = 0; idx < ERRATA_THEAD_NUMBER; idx++) { - info = &errata_list[idx]; - if ((stage == RISCV_ALTERNATIVES_MODULE || - info->stage == stage) && info->check_func(archid, impid)) - cpu_req_errata |= (1U << idx); - } + if (errata_probe_pbmt(stage, archid, impid)) + cpu_req_errata |= (1U << ERRATA_THEAD_PBMT); return cpu_req_errata; } diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c index b33564df81e1..b63c25c55bf1 100644 --- a/arch/riscv/kernel/cpufeature.c +++ b/arch/riscv/kernel/cpufeature.c @@ -246,12 +246,7 @@ void __init riscv_fill_hwcap(void) } #ifdef CONFIG_RISCV_ALTERNATIVE -struct cpufeature_info { - char name[ERRATA_STRING_LENGTH_MAX]; - bool (*check_func)(unsigned int stage); -}; - -static bool __init_or_module cpufeature_svpbmt_check_func(unsigned int stage) +static bool __init_or_module cpufeature_probe_svpbmt(unsigned int stage) { #ifdef CONFIG_RISCV_ISA_SVPBMT switch (stage) { @@ -265,26 +260,19 @@ static bool __init_or_module cpufeature_svpbmt_check_func(unsigned int stage) return false; } -static const struct cpufeature_info __initdata_or_module -cpufeature_list[CPUFEATURE_NUMBER] = { - { - .name = "svpbmt", - .check_func = cpufeature_svpbmt_check_func - }, -}; - +/* + * Probe presence of individual extensions. + * + * This code may also be executed before kernel relocation, so we cannot use + * addresses generated by the address-of operator as they won't be valid in + * this context. + */ static u32 __init_or_module cpufeature_probe(unsigned int stage) { - const struct cpufeature_info *info; u32 cpu_req_feature = 0; - int idx; - - for (idx = 0; idx < CPUFEATURE_NUMBER; idx++) { - info = &cpufeature_list[idx]; - if (info->check_func(stage)) - cpu_req_feature |= (1U << idx); - } + if (cpufeature_probe_svpbmt(stage)) + cpu_req_feature |= (1U << CPUFEATURE_SVPBMT); return cpu_req_feature; } -- 2.35.1 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2022-05-26 20:57 UTC|newest] Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-05-26 20:56 [PATCH v2 0/5] riscv: some svpbmt fixes Heiko Stuebner 2022-05-26 20:56 ` Heiko Stuebner 2022-05-26 20:56 ` [PATCH v2 1/5] riscv: drop cpufeature_apply_feature tracking variable Heiko Stuebner 2022-05-26 20:56 ` Heiko Stuebner 2022-05-27 2:48 ` Guo Ren 2022-05-27 2:48 ` Guo Ren 2022-05-26 20:56 ` [PATCH v2 2/5] riscv: Improve description for RISCV_ISA_SVPBMT Kconfig symbol Heiko Stuebner 2022-05-26 20:56 ` Heiko Stuebner 2022-05-27 2:51 ` Guo Ren 2022-05-27 2:51 ` Guo Ren 2022-05-26 20:56 ` [PATCH v2 3/5] riscv: make patch-function pointer more generic in cpu_manufacturer_info struct Heiko Stuebner 2022-05-26 20:56 ` Heiko Stuebner 2022-05-26 20:56 ` [PATCH v2 4/5] riscv: fix dependency for t-head errata Heiko Stuebner 2022-05-26 20:56 ` Heiko Stuebner 2022-05-27 2:50 ` Guo Ren 2022-05-27 2:50 ` Guo Ren 2022-05-26 20:56 ` Heiko Stuebner [this message] 2022-05-26 20:56 ` [PATCH v2 5/5] riscv: remove usage of function-pointers from cpufeatures and " Heiko Stuebner 2022-05-29 1:27 ` Samuel Holland 2022-05-29 1:27 ` Samuel Holland 2022-05-29 17:49 ` Heiko Stübner 2022-05-29 17:49 ` Heiko Stübner 2022-06-16 23:03 ` [PATCH v2 0/5] riscv: some svpbmt fixes Palmer Dabbelt 2022-06-16 23:03 ` Palmer Dabbelt 2022-06-17 7:13 ` Heiko Stübner 2022-06-17 7:13 ` Heiko Stübner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220526205646.258337-6-heiko@sntech.de \ --to=heiko@sntech.de \ --cc=cmuellner@linux.com \ --cc=guoren@kernel.org \ --cc=hch@lst.de \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=mick@ics.forth.gr \ --cc=palmer@dabbelt.com \ --cc=paul.walmsley@sifive.com \ --cc=philipp.tomsich@vrull.eu \ --cc=samuel@sholland.org \ --cc=wefu@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.