Re: [PATCH v9 11/12] riscv: don't use global static vars to store alternative data

[Guo Ren]

Reviewed-by: Guo Ren <guoren@xxxxxxxxxx>

On Wed, Apr 13, 2022 at 11:05 AM Heiko Stuebner <heiko@xxxxxxxxx> wrote:
>
> Right now the code uses a global struct to store vendor-ids
> and another global variable to store the vendor-patch-function.
>
> There exist specific cases where we'll need to patch the kernel
> at an even earlier stage, where trying to write to a static
> variable might actually result in hangs.
>
> Also collecting the vendor-information consists of 3 sbi-ecalls
> (or csr-reads) which is pretty negligible in the context of
> booting a kernel.
>
> So rework the code to not rely on static variables and instead
> collect the vendor-information when a round of alternatives is
> to be applied.
>
> Signed-off-by: Heiko Stuebner <heiko@xxxxxxxxx>
> ---
>  arch/riscv/kernel/alternative.c | 51 ++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 27 deletions(-)
>
> diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c
> index e6c9de9f9ba6..27f722ae452b 100644
> --- a/arch/riscv/kernel/alternative.c
> +++ b/arch/riscv/kernel/alternative.c
> @@ -16,41 +16,35 @@
>  #include <asm/sbi.h>
>  #include <asm/csr.h>
>
> -static struct cpu_manufacturer_info_t {
> +struct cpu_manufacturer_info_t {
>         unsigned long vendor_id;
>         unsigned long arch_id;
>         unsigned long imp_id;
> -} cpu_mfr_info;
> +       void (*vendor_patch_func)(struct alt_entry *begin, struct alt_entry *end,
> +                                 unsigned long archid, unsigned long impid,
> +                                 unsigned int stage);
> +};
>
> -static void (*vendor_patch_func)(struct alt_entry *begin, struct alt_entry *end,
> -                                unsigned long archid, unsigned long impid,
> -                                unsigned int stage) __initdata_or_module;
> -
> -static inline void __init riscv_fill_cpu_mfr_info(void)
> +static void __init_or_module riscv_fill_cpu_mfr_info(struct cpu_manufacturer_info_t *cpu_mfr_info)
>  {
>  #ifdef CONFIG_RISCV_M_MODE
> -       cpu_mfr_info.vendor_id = csr_read(CSR_MVENDORID);
> -       cpu_mfr_info.arch_id = csr_read(CSR_MARCHID);
> -       cpu_mfr_info.imp_id = csr_read(CSR_MIMPID);
> +       cpu_mfr_info->vendor_id = csr_read(CSR_MVENDORID);
> +       cpu_mfr_info->arch_id = csr_read(CSR_MARCHID);
> +       cpu_mfr_info->imp_id = csr_read(CSR_MIMPID);
>  #else
> -       cpu_mfr_info.vendor_id = sbi_get_mvendorid();
> -       cpu_mfr_info.arch_id = sbi_get_marchid();
> -       cpu_mfr_info.imp_id = sbi_get_mimpid();
> +       cpu_mfr_info->vendor_id = sbi_get_mvendorid();
> +       cpu_mfr_info->arch_id = sbi_get_marchid();
> +       cpu_mfr_info->imp_id = sbi_get_mimpid();
>  #endif
> -}
> -
> -static void __init init_alternative(void)
> -{
> -       riscv_fill_cpu_mfr_info();
>
> -       switch (cpu_mfr_info.vendor_id) {
> +       switch (cpu_mfr_info->vendor_id) {
>  #ifdef CONFIG_ERRATA_SIFIVE
>         case SIFIVE_VENDOR_ID:
> -               vendor_patch_func = sifive_errata_patch_func;
> +               cpu_mfr_info->vendor_patch_func = sifive_errata_patch_func;
>                 break;
>  #endif
>         default:
> -               vendor_patch_func = NULL;
> +               cpu_mfr_info->vendor_patch_func = NULL;
>         }
>  }
>
> @@ -63,14 +57,19 @@ static void __init_or_module _apply_alternatives(struct alt_entry *begin,
>                                                  struct alt_entry *end,
>                                                  unsigned int stage)
>  {
> +       struct cpu_manufacturer_info_t cpu_mfr_info;
> +
> +       riscv_fill_cpu_mfr_info(&cpu_mfr_info);
> +
>         riscv_cpufeature_patch_func(begin, end, stage);
>
> -       if (!vendor_patch_func)
> +       if (!cpu_mfr_info.vendor_patch_func)
>                 return;
>
> -       vendor_patch_func(begin, end,
> -                         cpu_mfr_info.arch_id, cpu_mfr_info.imp_id,
> -                         stage);
> +       cpu_mfr_info.vendor_patch_func(begin, end,
> +                                  cpu_mfr_info.arch_id,
> +                                  cpu_mfr_info.imp_id,
> +                                  stage);
>  }
>
>  void __init apply_boot_alternatives(void)
> @@ -78,8 +77,6 @@ void __init apply_boot_alternatives(void)
>         /* If called on non-boot cpu things could go wrong */
>         WARN_ON(smp_processor_id() != 0);
>
> -       init_alternative();
> -
>         _apply_alternatives((struct alt_entry *)__alt_start,
>                             (struct alt_entry *)__alt_end,
>                             RISCV_ALTERNATIVES_BOOT);
> --
> 2.35.1
>


-- 
Best Regards
 Guo Ren

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