Re: [PATCH v5 03/27] arm64: alternative: Apply alternatives early in boot process

[James Morse]

Hi Julien,

On 28/08/18 16:51, Julien Thierry wrote:
> From: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
> 
> Currently alternatives are applied very late in the boot process (and
> a long time after we enable scheduling). Some alternative sequences,
> such as those that alter the way CPU context is stored, must be applied
> much earlier in the boot sequence.
> 
> Introduce apply_boot_alternatives() to allow some alternatives to be
> applied immediately after we detect the CPU features of the boot CPU.


> diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
> index b5d6039..70c2604 100644
> --- a/arch/arm64/kernel/alternative.c
> +++ b/arch/arm64/kernel/alternative.c
> @@ -145,7 +145,8 @@ static void clean_dcache_range_nopatch(u64 start, u64 end)
>  	} while (cur += d_size, cur < end);
>  }
>  
> -static void __apply_alternatives(void *alt_region, bool is_module)
> +static void __apply_alternatives(void *alt_region,  bool is_module,
> +				 unsigned long feature_mask)

Shouldn't feature_mask be a DECLARE_BITMAP() maybe-array like cpu_hwcaps?
This means it keeps working when NR_CAPS grows over 64, which might happen
sooner than we think for backported errata...


> @@ -155,6 +156,9 @@ static void __apply_alternatives(void *alt_region, bool is_module)
>  	for (alt = region->begin; alt < region->end; alt++) {
>  		int nr_inst;
>  
> +		if ((BIT(alt->cpufeature) & feature_mask) == 0)
> +			continue;
> +
>  		/* Use ARM64_CB_PATCH as an unconditional patch */
>  		if (alt->cpufeature < ARM64_CB_PATCH &&
>  		    !cpus_have_cap(alt->cpufeature))
> @@ -213,7 +217,7 @@ static int __apply_alternatives_multi_stop(void *unused)
>  		isb();
>  	} else {
>  		BUG_ON(alternatives_applied);
> -		__apply_alternatives(&region, false);
> +		__apply_alternatives(&region, false, ~boot_capabilities);

Ah, this is tricky. There is a bitmap_complement() for the DECLARE_BITMAP()
stuff, but we'd need a second array...

We could pass the scope around, but then __apply_alternatives() would need to
lookup the struct arm64_cpu_capabilities up every time. This is only a problem
as we have one cap-number-space for errata/features, but separate sparse lists.

(I think applying the alternatives one cap at a time is a bad idea as we would
need to walk the alternative region NR_CAPS times)


> @@ -227,6 +231,24 @@ void __init apply_alternatives_all(void)
>  	stop_machine(__apply_alternatives_multi_stop, NULL, cpu_online_mask);
>  }
>  
> +/*
> + * This is called very early in the boot process (directly after we run
> + * a feature detect on the boot CPU). No need to worry about other CPUs
> + * here.
> + */
> +void __init apply_boot_alternatives(void)
> +{
> +	struct alt_region region = {
> +		.begin	= (struct alt_instr *)__alt_instructions,
> +		.end	= (struct alt_instr *)__alt_instructions_end,
> +	};
> +
> +	/* If called on non-boot cpu things could go wrong */
> +	WARN_ON(smp_processor_id() != 0);

Isn't the problem if there are multiple CPUs online?


> +	__apply_alternatives(&region, false, boot_capabilities);
> +}
> +
>  #ifdef CONFIG_MODULES
>  void apply_alternatives_module(void *start, size_t length)
>  {

> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 3bc1c8b..0d1e41e 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -52,6 +52,8 @@
>  DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
>  EXPORT_SYMBOL(cpu_hwcaps);
>  
> +unsigned long boot_capabilities;
> +
>  /*
>   * Flag to indicate if we have computed the system wide
>   * capabilities based on the boot time active CPUs. This
> @@ -1375,6 +1377,9 @@ static void __update_cpu_capabilities(const struct arm64_cpu_capabilities *caps,
>  		if (!cpus_have_cap(caps->capability) && caps->desc)
>  			pr_info("%s %s\n", info, caps->desc);
>  		cpus_set_cap(caps->capability);

Hmm, the bitmap behind cpus_set_cap() is what cpus_have_cap() in
__apply_alternatives() looks at. If you had a call to __apply_alternatives after
update_cpu_capabilities(SCOPE_BOOT_CPU), but before any others, it would only
apply those alternatives...

(I don't think there is a problem re-applying the same alternative, but I
haven't checked).


> +
> +		if (caps->type & SCOPE_BOOT_CPU)
> +			__set_bit(caps->capability, &boot_capabilities);
>  	}
>  }


Thanks,

James