[PATCH v2 0/5] riscv: some svpbmt fixes

[PATCH v2 0/5] riscv: some svpbmt fixes

[Heiko Stuebner]

Some additionals comments and notes from autobuilders received
after the series got applied, warranted some changes.

So this is a small collection of cleanups for the svpbmt v10 series.

changes in v2:
- add Guo's Review
- add patch dropping the use of function pointers in code
  that can run before relocation

Heiko Stuebner (5):
  riscv: drop cpufeature_apply_feature tracking variable
  riscv: Improve description for RISCV_ISA_SVPBMT Kconfig symbol
  riscv: make patch-function pointer more generic in
    cpu_manufacturer_info struct
  riscv: fix dependency for t-head errata
  riscv: remove usage of function-pointers from cpufeatures and t-head
    errata

 arch/riscv/Kconfig               |  9 ++++++--
 arch/riscv/Kconfig.erratas       |  1 +
 arch/riscv/errata/thead/errata.c | 38 ++++++++++----------------------
 arch/riscv/kernel/alternative.c  | 18 +++++++--------
 arch/riscv/kernel/cpufeature.c   | 37 +++++++++----------------------
 5 files changed, 40 insertions(+), 63 deletions(-)

-- 
2.35.1

[PATCH v2 1/5] riscv: drop cpufeature_apply_feature tracking variable

[Heiko Stuebner]

The variable was tracking which feature patches got applied
but that information was never actually used - and thus resulted
in a warning as well.

Drop the variable.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/riscv/kernel/cpufeature.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index dea3ea19deee..b33564df81e1 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -294,7 +294,6 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,
 						  unsigned int stage)
 {
 	u32 cpu_req_feature = cpufeature_probe(stage);
-	u32 cpu_apply_feature = 0;
 	struct alt_entry *alt;
 	u32 tmp;
 
@@ -308,10 +307,8 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,
 		}
 
 		tmp = (1U << alt->errata_id);
-		if (cpu_req_feature & tmp) {
+		if (cpu_req_feature & tmp)
 			patch_text_nosync(alt->old_ptr, alt->alt_ptr, alt->alt_len);
-			cpu_apply_feature |= tmp;
-		}
 	}
 }
 #endif
-- 
2.35.1

Re: [PATCH v2 1/5] riscv: drop cpufeature_apply_feature tracking variable

[Guo Ren]

Safe modification.

Reviewed-by: Guo Ren <guoren@kernel.org>

On Fri, May 27, 2022 at 4:57 AM Heiko Stuebner <heiko@sntech.de> wrote:
>
> The variable was tracking which feature patches got applied
> but that information was never actually used - and thus resulted
> in a warning as well.
>
> Drop the variable.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  arch/riscv/kernel/cpufeature.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index dea3ea19deee..b33564df81e1 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -294,7 +294,6 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,
>                                                   unsigned int stage)
>  {
>         u32 cpu_req_feature = cpufeature_probe(stage);
> -       u32 cpu_apply_feature = 0;
>         struct alt_entry *alt;
>         u32 tmp;
>
> @@ -308,10 +307,8 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,
>                 }
>
>                 tmp = (1U << alt->errata_id);
> -               if (cpu_req_feature & tmp) {
> +               if (cpu_req_feature & tmp)
>                         patch_text_nosync(alt->old_ptr, alt->alt_ptr, alt->alt_len);
> -                       cpu_apply_feature |= tmp;
> -               }
>         }
>  }
>  #endif
> --
> 2.35.1
>


-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/

[PATCH v2 2/5] riscv: Improve description for RISCV_ISA_SVPBMT Kconfig symbol

[Heiko Stuebner]

This improves the symbol's description to make it easier for
people to understand what it is about.

Suggested-by: Christoph Hellwig <hch@lst.de>
Suggested-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/riscv/Kconfig | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 65285b980134..a4b299ad4473 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -356,8 +356,13 @@ config RISCV_ISA_SVPBMT
 	select RISCV_ALTERNATIVE
 	default y
 	help
-	   Adds support to dynamically detect the presence of the SVPBMT extension
-	   (Supervisor-mode: page-based memory types) and enable its usage.
+	   Adds support to dynamically detect the presence of the SVPBMT
+	   ISA-extension (Supervisor-mode: page-based memory types) and
+	   enable its usage.
+
+	   The memory type for a page contains a combination of attributes
+	   that indicate the cacheability, idempotency, and ordering
+	   properties for access to that page.
 
 	   The SVPBMT extension is only available on 64Bit cpus.
 
-- 
2.35.1

Re: [PATCH v2 2/5] riscv: Improve description for RISCV_ISA_SVPBMT Kconfig symbol

[Guo Ren]

Reviewed-by: Guo Ren <guoren@kernel.org>

On Fri, May 27, 2022 at 4:57 AM Heiko Stuebner <heiko@sntech.de> wrote:
>
> This improves the symbol's description to make it easier for
> people to understand what it is about.
>
> Suggested-by: Christoph Hellwig <hch@lst.de>
> Suggested-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  arch/riscv/Kconfig | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 65285b980134..a4b299ad4473 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -356,8 +356,13 @@ config RISCV_ISA_SVPBMT
>         select RISCV_ALTERNATIVE
>         default y
>         help
> -          Adds support to dynamically detect the presence of the SVPBMT extension
> -          (Supervisor-mode: page-based memory types) and enable its usage.
> +          Adds support to dynamically detect the presence of the SVPBMT
> +          ISA-extension (Supervisor-mode: page-based memory types) and
> +          enable its usage.
> +
> +          The memory type for a page contains a combination of attributes
> +          that indicate the cacheability, idempotency, and ordering
> +          properties for access to that page.
>
>            The SVPBMT extension is only available on 64Bit cpus.
>
> --
> 2.35.1
>


-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/

[PATCH v2 3/5] riscv: make patch-function pointer more generic in cpu_manufacturer_info struct

[Heiko Stuebner]

During review the naming of the function-pointer was called
confusing as the vendor id is just one of three inputs for
the patching and indeed it serves no real purpose, as with
recent changes the function pointer is not a static
global element anymore, so drop the "vendor_" prefix.

Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/riscv/kernel/alternative.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c
index c9d0d3c53223..a7d26a00beea 100644
--- a/arch/riscv/kernel/alternative.c
+++ b/arch/riscv/kernel/alternative.c
@@ -20,7 +20,7 @@ struct cpu_manufacturer_info_t {
 	unsigned long vendor_id;
 	unsigned long arch_id;
 	unsigned long imp_id;
-	void (*vendor_patch_func)(struct alt_entry *begin, struct alt_entry *end,
+	void (*patch_func)(struct alt_entry *begin, struct alt_entry *end,
 				  unsigned long archid, unsigned long impid,
 				  unsigned int stage);
 };
@@ -40,16 +40,16 @@ static void __init_or_module riscv_fill_cpu_mfr_info(struct cpu_manufacturer_inf
 	switch (cpu_mfr_info->vendor_id) {
 #ifdef CONFIG_ERRATA_SIFIVE
 	case SIFIVE_VENDOR_ID:
-		cpu_mfr_info->vendor_patch_func = sifive_errata_patch_func;
+		cpu_mfr_info->patch_func = sifive_errata_patch_func;
 		break;
 #endif
 #ifdef CONFIG_ERRATA_THEAD
 	case THEAD_VENDOR_ID:
-		cpu_mfr_info->vendor_patch_func = thead_errata_patch_func;
+		cpu_mfr_info->patch_func = thead_errata_patch_func;
 		break;
 #endif
 	default:
-		cpu_mfr_info->vendor_patch_func = NULL;
+		cpu_mfr_info->patch_func = NULL;
 	}
 }
 
@@ -68,13 +68,13 @@ static void __init_or_module _apply_alternatives(struct alt_entry *begin,
 
 	riscv_cpufeature_patch_func(begin, end, stage);
 
-	if (!cpu_mfr_info.vendor_patch_func)
+	if (!cpu_mfr_info.patch_func)
 		return;
 
-	cpu_mfr_info.vendor_patch_func(begin, end,
-				   cpu_mfr_info.arch_id,
-				   cpu_mfr_info.imp_id,
-				   stage);
+	cpu_mfr_info.patch_func(begin, end,
+				cpu_mfr_info.arch_id,
+				cpu_mfr_info.imp_id,
+				stage);
 }
 
 void __init apply_boot_alternatives(void)
-- 
2.35.1

[PATCH v2 4/5] riscv: fix dependency for t-head errata

[Heiko Stuebner]

alternatives only work correctly on non-xip-kernels and while the
selected alternative-symbol has the correct dependency the symbol
selecting it also needs that dependency.

So add the missing dependency to the T-Head errata Kconfig symbol.

Reported-by: kernel test robot <yujie.liu@intel.com>
Reviewed-by: Guo Ren <guoren@kernel.org>
Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 arch/riscv/Kconfig.erratas | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/riscv/Kconfig.erratas b/arch/riscv/Kconfig.erratas
index ebfcd5cc6eaf..457ac72c9b36 100644
--- a/arch/riscv/Kconfig.erratas
+++ b/arch/riscv/Kconfig.erratas
@@ -35,6 +35,7 @@ config ERRATA_SIFIVE_CIP_1200
 
 config ERRATA_THEAD
 	bool "T-HEAD errata"
+	depends on !XIP_KERNEL
 	select RISCV_ALTERNATIVE
 	help
 	  All T-HEAD errata Kconfig depend on this Kconfig. Disabling
-- 
2.35.1

Re: [PATCH v2 4/5] riscv: fix dependency for t-head errata

[Guo Ren]

Em... This is a limitation to our scenario but seems it okay for
current t-thead SoC products.

Reviewed-by: Guo Ren <guoren@kernel.org>

On Fri, May 27, 2022 at 4:57 AM Heiko Stuebner <heiko@sntech.de> wrote:
>
> alternatives only work correctly on non-xip-kernels and while the
> selected alternative-symbol has the correct dependency the symbol
> selecting it also needs that dependency.
>
> So add the missing dependency to the T-Head errata Kconfig symbol.
>
> Reported-by: kernel test robot <yujie.liu@intel.com>
> Reviewed-by: Guo Ren <guoren@kernel.org>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  arch/riscv/Kconfig.erratas | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/riscv/Kconfig.erratas b/arch/riscv/Kconfig.erratas
> index ebfcd5cc6eaf..457ac72c9b36 100644
> --- a/arch/riscv/Kconfig.erratas
> +++ b/arch/riscv/Kconfig.erratas
> @@ -35,6 +35,7 @@ config ERRATA_SIFIVE_CIP_1200
>
>  config ERRATA_THEAD
>         bool "T-HEAD errata"
> +       depends on !XIP_KERNEL
>         select RISCV_ALTERNATIVE
>         help
>           All T-HEAD errata Kconfig depend on this Kconfig. Disabling
> --
> 2.35.1
>


-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/

[PATCH v2 5/5] riscv: remove usage of function-pointers from cpufeatures and t-head errata

[Heiko Stuebner]

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

Re: [PATCH v2 5/5] riscv: remove usage of function-pointers from cpufeatures and t-head errata

[Samuel Holland]

On 5/26/22 3:56 PM, Heiko Stuebner wrote:
> 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)

Now that this function isn't used as a function pointer anymore, it doesn't need
to be specific to SVPBMT. I think the logic here is the same for ZICBOM. Does it
make sense to move it to the calling function?

With the conflicts between this and the CMO series manually fixed:

Tested-by: Samuel Holland <samuel@sholland.org>

Regards,
Samuel

>  {
>  #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;
>  }
> 

Re: [PATCH v2 5/5] riscv: remove usage of function-pointers from cpufeatures and t-head errata

[Heiko Stübner]

Hi,

Am Sonntag, 29. Mai 2022, 03:27:48 CEST schrieb Samuel Holland:
> On 5/26/22 3:56 PM, Heiko Stuebner wrote:
> > 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>

[...]

> > -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)
> 
> Now that this function isn't used as a function pointer anymore, it doesn't need
> to be specific to SVPBMT. I think the logic here is the same for ZICBOM. Does it
> make sense to move it to the calling function?

we don't know how other extensions need to probe though, so for example
in my yet-to-send zicbom-v3 the probe function itself looks like

static u32 __init_or_module cpufeature_probe(unsigned int stage)
{
        u32 cpu_req_feature = 0;

        if (cpufeature_probe_svpbmt(stage))
                cpu_req_feature |= (1U << CPUFEATURE_SVPBMT);

        if (cpufeature_probe_cmo(stage))
                cpu_req_feature |= (1U << CPUFEATURE_CMO);

        return cpu_req_feature;
}

As this might get longer in the future, I actually do like the actual
checks being separate for readability.

But I'll just yield to the majority opinion ;-)

Heiko


> 
> With the conflicts between this and the CMO series manually fixed:
> 
> Tested-by: Samuel Holland <samuel@sholland.org>
> 
> Regards,
> Samuel
> 
> >  {
> >  #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;
> >  }
> > 
> 
> 

Re: [PATCH v2 0/5] riscv: some svpbmt fixes

[Palmer Dabbelt]

On Thu, 26 May 2022 13:56:41 PDT (-0700), heiko@sntech.de wrote:
> Some additionals comments and notes from autobuilders received
> after the series got applied, warranted some changes.
>
> So this is a small collection of cleanups for the svpbmt v10 series.
>
> changes in v2:
> - add Guo's Review
> - add patch dropping the use of function pointers in code
>   that can run before relocation
>
> Heiko Stuebner (5):
>   riscv: drop cpufeature_apply_feature tracking variable
>   riscv: Improve description for RISCV_ISA_SVPBMT Kconfig symbol
>   riscv: make patch-function pointer more generic in
>     cpu_manufacturer_info struct
>   riscv: fix dependency for t-head errata
>   riscv: remove usage of function-pointers from cpufeatures and t-head
>     errata
>
>  arch/riscv/Kconfig               |  9 ++++++--
>  arch/riscv/Kconfig.erratas       |  1 +
>  arch/riscv/errata/thead/errata.c | 38 ++++++++++----------------------
>  arch/riscv/kernel/alternative.c  | 18 +++++++--------
>  arch/riscv/kernel/cpufeature.c   | 37 +++++++++----------------------
>  5 files changed, 40 insertions(+), 63 deletions(-)

IMO only three of these are actually fixes, the rest are cleanups.  I've 
got ahead and put everything on a branch, with

    riscv: Improve description for RISCV_ISA_SVPBMT Kconfig symbol
    riscv: drop cpufeature_apply_feature tracking variable
    riscv: fix dependency for t-head errata

first.  Those are on fixes, the whole thing is in for-next.

Thanks!

Re: [PATCH v2 0/5] riscv: some svpbmt fixes

[Heiko Stübner]

Hi Palmer,

Am Freitag, 17. Juni 2022, 01:03:32 CEST schrieb Palmer Dabbelt:
> On Thu, 26 May 2022 13:56:41 PDT (-0700), heiko@sntech.de wrote:
> > Some additionals comments and notes from autobuilders received
> > after the series got applied, warranted some changes.
> >
> > So this is a small collection of cleanups for the svpbmt v10 series.
> >
> > changes in v2:
> > - add Guo's Review
> > - add patch dropping the use of function pointers in code
> >   that can run before relocation
> >
> > Heiko Stuebner (5):
> >   riscv: drop cpufeature_apply_feature tracking variable
> >   riscv: Improve description for RISCV_ISA_SVPBMT Kconfig symbol
> >   riscv: make patch-function pointer more generic in
> >     cpu_manufacturer_info struct
> >   riscv: fix dependency for t-head errata
> >   riscv: remove usage of function-pointers from cpufeatures and t-head
> >     errata
> >
> >  arch/riscv/Kconfig               |  9 ++++++--
> >  arch/riscv/Kconfig.erratas       |  1 +
> >  arch/riscv/errata/thead/errata.c | 38 ++++++++++----------------------
> >  arch/riscv/kernel/alternative.c  | 18 +++++++--------
> >  arch/riscv/kernel/cpufeature.c   | 37 +++++++++----------------------
> >  5 files changed, 40 insertions(+), 63 deletions(-)
> 
> IMO only three of these are actually fixes, the rest are cleanups.  I've 
> got ahead and put everything on a branch, with
> 
>     riscv: Improve description for RISCV_ISA_SVPBMT Kconfig symbol
>     riscv: drop cpufeature_apply_feature tracking variable
>     riscv: fix dependency for t-head errata
> 
> first.  Those are on fixes, the whole thing is in for-next.

thanks a lot.

For the fixes I think Nathan's "riscv: Fix ALT_THEAD_PMA's asm parameters" [0]
would also be quite important.

Heiko

[0] https://lore.kernel.org/r/20220518184529.454008-1-nathan@kernel.org