Re: [PATCH v4 12/21] arm64: cpufeature: Add an early command-line cpufeature override facility

2021-01-24 Thread Marc Zyngier
On Sat, 23 Jan 2021 13:43:52 +,
Catalin Marinas  wrote:
> 
> On Mon, Jan 18, 2021 at 09:45:24AM +, Marc Zyngier wrote:
> > +struct reg_desc {
> > +   const char * const  name;
> > +   u64 * const val;
> > +   u64 * const mask;
> > +   struct {
> > +   const char * const  name;
> > +   u8   shift;
> > +   }   fields[];
> > +};
> 
> Sorry, I didn't see this earlier. Do we need to add all these consts
> here? So you want the pointers to be const but why is 'shift' special
> and not a const then? Is it modified later?
> 
> Would this not work:
> 
> struct reg_desc {
>   const char  *name;
>   u64 *val;
>   u64 *mask;
>   struct {
>   const char  *name;
>   u8  shift;
>   } fields[];
> };
> 
> > +static const struct reg_desc * const regs[] __initdata = {
> 
> as we already declare the whole struct reg_desc pointers here as const.
> I may have confused myself...

It definitely is better. Specially given that we throw all of this
away right after boot, there is no harm in keeping it simple.

I've also renamed "reg_desc" to "ftr_set_desc", because we don't
always describe a register (like for kaslr).

Thanks,

M.

-- 
Without deviation from the norm, progress is not possible.


Re: [PATCH v4 12/21] arm64: cpufeature: Add an early command-line cpufeature override facility

2021-01-23 Thread Catalin Marinas
On Mon, Jan 18, 2021 at 09:45:24AM +, Marc Zyngier wrote:
> +struct reg_desc {
> + const char * const  name;
> + u64 * const val;
> + u64 * const mask;
> + struct {
> + const char * const  name;
> + u8   shift;
> + }   fields[];
> +};

Sorry, I didn't see this earlier. Do we need to add all these consts
here? So you want the pointers to be const but why is 'shift' special
and not a const then? Is it modified later?

Would this not work:

struct reg_desc {
const char  *name;
u64 *val;
u64 *mask;
struct {
const char  *name;
u8  shift;
} fields[];
};

> +static const struct reg_desc * const regs[] __initdata = {

as we already declare the whole struct reg_desc pointers here as const.
I may have confused myself...

-- 
Catalin


Re: [PATCH v4 12/21] arm64: cpufeature: Add an early command-line cpufeature override facility

2021-01-23 Thread Catalin Marinas
On Mon, Jan 18, 2021 at 09:45:24AM +, Marc Zyngier wrote:
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index d74e5f84042e..b3c4dd04f74b 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -435,6 +435,7 @@ SYM_FUNC_START_LOCAL(__primary_switched)
>  
>   mov x0, x21 // pass FDT address in x0
>   bl  early_fdt_map   // Try mapping the FDT early
> + bl  init_shadow_regs
>   bl  switch_to_vhe
>  #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)
>   bl  kasan_early_init
[...]
> +void __init init_shadow_regs(void)

Do we need an asmlinkage here? And a declaration somewhere to silence
sparse (we did this for entry-common.c function even if the .S files
don't consume the prototypes).

-- 
Catalin


Re: [PATCH v4 12/21] arm64: cpufeature: Add an early command-line cpufeature override facility

2021-01-18 Thread David Brazdil
On Mon, Jan 18, 2021 at 09:45:24AM +, Marc Zyngier wrote:
> In order to be able to override CPU features at boot time,
> let's add a command line parser that matches options of the
> form "cpureg.feature=value", and store the corresponding
> value into the override val/mask pair.
> 
> No features are currently defined, so no expected change in
> functionality.
> 
> Signed-off-by: Marc Zyngier 

Acked-by: David Brazdil 

> ---
>  arch/arm64/kernel/Makefile |   2 +-
>  arch/arm64/kernel/head.S   |   1 +
>  arch/arm64/kernel/idreg-override.c | 119 +
>  3 files changed, 121 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm64/kernel/idreg-override.c
> 
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 86364ab6f13f..2262f0392857 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -17,7 +17,7 @@ obj-y   := debug-monitors.o entry.o 
> irq.o fpsimd.o  \
>  return_address.o cpuinfo.o cpu_errata.o  
> \
>  cpufeature.o alternative.o cacheinfo.o   
> \
>  smp.o smp_spin_table.o topology.o smccc-call.o   
> \
> -syscall.o proton-pack.o
> +syscall.o proton-pack.o idreg-override.o
>  
>  targets  += efi-entry.o
>  
> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
> index d74e5f84042e..b3c4dd04f74b 100644
> --- a/arch/arm64/kernel/head.S
> +++ b/arch/arm64/kernel/head.S
> @@ -435,6 +435,7 @@ SYM_FUNC_START_LOCAL(__primary_switched)
>  
>   mov x0, x21 // pass FDT address in x0
>   bl  early_fdt_map   // Try mapping the FDT early
> + bl  init_shadow_regs
>   bl  switch_to_vhe
>  #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)
>   bl  kasan_early_init
> diff --git a/arch/arm64/kernel/idreg-override.c 
> b/arch/arm64/kernel/idreg-override.c
> new file mode 100644
> index ..392f93b67103
> --- /dev/null
> +++ b/arch/arm64/kernel/idreg-override.c
> @@ -0,0 +1,119 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Early cpufeature override framework
> + *
> + * Copyright (C) 2020 Google LLC
> + * Author: Marc Zyngier 
> + */
> +
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +
> +struct reg_desc {
> + const char * const  name;
> + u64 * const val;
> + u64 * const mask;
> + struct {
> + const char * const  name;
> + u8   shift;
nit: There's an extra space before `shift`.

> + }   fields[];
> +};
> +
> +static const struct reg_desc * const regs[] __initdata = {
> +};
> +
> +static int __init find_field(const char *cmdline, const struct reg_desc *reg,
> +  int f, u64 *v)
> +{
> + char buf[256], *str;
> + size_t len;
> +
> + snprintf(buf, ARRAY_SIZE(buf), "%s.%s=", reg->name, 
> reg->fields[f].name);
> +
> + str = strstr(cmdline, buf);
> + if (!(str == cmdline || (str > cmdline && *(str - 1) == ' ')))
> + return -1;
> +
> + str += strlen(buf);
> + len = strcspn(str, " ");
> + len = min(len, ARRAY_SIZE(buf) - 1);
> + strncpy(buf, str, len);
> + buf[len] = 0;
> +
> + return kstrtou64(buf, 0, v);
> +}
> +
> +static void __init match_options(const char *cmdline)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(regs); i++) {
> + int f;
> +
> + if (!regs[i]->val || !regs[i]->mask)
> + continue;
> +
> + for (f = 0; regs[i]->fields[f].name; f++) {
> + u64 v;
> +
> + if (find_field(cmdline, regs[i], f, ))
> + continue;
> +
> + *regs[i]->val  |= (v & 0xf) << regs[i]->fields[f].shift;
> + *regs[i]->mask |= 0xfUL << regs[i]->fields[f].shift;
> + }
> + }
> +}
> +
> +static __init void parse_cmdline(void)
> +{
> + if (!IS_ENABLED(CONFIG_CMDLINE_FORCE)) {
> + const u8 *prop;
> + void *fdt;
> + int node;
> +
> + fdt = get_early_fdt_ptr();
> + if (!fdt)
> + goto out;
> +
> + node = fdt_path_offset(fdt, "/chosen");
> + if (node < 0)
> + goto out;
> +
> + prop = fdt_getprop(fdt, node, "bootargs", NULL);
> + if (!prop)
> + goto out;
> +
> + match_options(prop);
> +
> + if (!IS_ENABLED(CONFIG_CMDLINE_EXTEND))
> + return;
> + }
> +
> +out:
> + match_options(CONFIG_CMDLINE);
> +}
> +
> +void __init init_shadow_regs(void)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(regs); i++) {
> +