Re: [PATCH v8 15/26] arm64: alternative: Apply alternatives early in boot process

From: Julien Thierry
Date: Tue Jan 08 2019 - 10:20:16 EST

Next message: Michael Mueller: "Re: [PATCH v5 13/15] KVM: s390: add function process_gib_alert_list()"
Previous message: Mimi Zohar: "Re: [RFC PATCH 1/1] KEYS, integrity: Link .platform keyring to .secondary_trusted_keys"
In reply to: Suzuki K Poulose: "Re: [PATCH v8 15/26] arm64: alternative: Apply alternatives early in boot process"
Next in thread: Suzuki K Poulose: "Re: [PATCH v8 15/26] arm64: alternative: Apply alternatives early in boot process"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Suzuki,

On 08/01/2019 14:51, Suzuki K Poulose wrote:
> Hi Julien,
>
> On 08/01/2019 14:07, 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.
>>
>> Signed-off-by: Daniel Thompson <daniel.thompson@xxxxxxxxxx>
>> [julien.thierry@xxxxxxx: rename to fit new cpufeature framework better,
>> ÂÂÂÂÂÂÂÂÂÂÂÂ apply BOOT_SCOPE feature early in boot]
>> Signed-off-by: Julien Thierry <julien.thierry@xxxxxxx>
>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>> Cc: Will Deacon <will.deacon@xxxxxxx>
>> Cc: Christoffer Dall <christoffer.dall@xxxxxxx>
>> Cc: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
>> ---
>> Â arch/arm64/include/asm/alternative.h |Â 1 +
>> Â arch/arm64/include/asm/cpufeature.hÂ |Â 4 ++++
>> Â arch/arm64/kernel/alternative.cÂÂÂÂÂ | 43
>> +++++++++++++++++++++++++++++++-----
>> Â arch/arm64/kernel/cpufeature.cÂÂÂÂÂÂ |Â 6 +++++
>> Â arch/arm64/kernel/smp.cÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 7 ++++++
>> Â 5 files changed, 56 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/alternative.h
>> b/arch/arm64/include/asm/alternative.h
>> index 9806a23..b9f8d78 100644
>> --- a/arch/arm64/include/asm/alternative.h
>> +++ b/arch/arm64/include/asm/alternative.h
>> @@ -25,6 +25,7 @@ struct alt_instr {
>> Â typedef void (*alternative_cb_t)(struct alt_instr *alt,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __le32 *origptr, __le32 *updptr, int nr_inst);
>> Â +void __init apply_boot_alternatives(void);
>> Â void __init apply_alternatives_all(void);
>> Â bool alternative_is_applied(u16 cpufeature);
>> Â diff --git a/arch/arm64/include/asm/cpufeature.h
>> b/arch/arm64/include/asm/cpufeature.h
>> index 89c3f31..e505e1f 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -391,6 +391,10 @@ static inline int cpucap_default_scope(const
>> struct arm64_cpu_capabilities *cap)
>> Â extern struct static_key_false cpu_hwcap_keys[ARM64_NCAPS];
>> Â extern struct static_key_false arm64_const_caps_ready;
>> Â +/* ARM64 CAPS + alternative_cb */
>> +#define ARM64_NPATCHABLE (ARM64_NCAPS + 1)
>> +extern DECLARE_BITMAP(boot_capabilities, ARM64_NPATCHABLE);
>> +
>> Â #define for_each_available_cap(cap)ÂÂÂÂÂÂÂ \
>> ÂÂÂÂÂ for_each_set_bit(cap, cpu_hwcaps, ARM64_NCAPS)
>> Â diff --git a/arch/arm64/kernel/alternative.c
>> b/arch/arm64/kernel/alternative.c
>> index c947d22..a9b4677 100644
>> --- a/arch/arm64/kernel/alternative.c
>> +++ b/arch/arm64/kernel/alternative.c
>> @@ -155,7 +155,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)
>> Â {
>> ÂÂÂÂÂ struct alt_instr *alt;
>> ÂÂÂÂÂ struct alt_region *region = alt_region;
>> @@ -165,6 +166,9 @@ static void __apply_alternatives(void *alt_region,
>> bool is_module)
>> ÂÂÂÂÂ for (alt = region->begin; alt < region->end; alt++) {
>> ÂÂÂÂÂÂÂÂÂ int nr_inst;
>> Â +ÂÂÂÂÂÂÂ if (!test_bit(alt->cpufeature, feature_mask))
>> +ÂÂÂÂÂÂÂÂÂÂÂ continue;
>> +
>> ÂÂÂÂÂÂÂÂÂ /* Use ARM64_CB_PATCH as an unconditional patch */
>> ÂÂÂÂÂÂÂÂÂ if (alt->cpufeature < ARM64_CB_PATCH &&
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ !cpus_have_cap(alt->cpufeature))
>> @@ -203,8 +207,11 @@ static void __apply_alternatives(void
>> *alt_region, bool is_module)
>> ÂÂÂÂÂÂÂÂÂ __flush_icache_all();
>> ÂÂÂÂÂÂÂÂÂ isb();
>> Â -ÂÂÂÂÂÂÂ /* We applied all that was available */
>> -ÂÂÂÂÂÂÂ bitmap_copy(applied_alternatives, cpu_hwcaps, ARM64_NCAPS);
>> +ÂÂÂÂÂÂÂ /* Ignore ARM64_CB bit from feature mask */
>> +ÂÂÂÂÂÂÂ bitmap_or(applied_alternatives, applied_alternatives,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂ feature_mask, ARM64_NCAPS);
>> +ÂÂÂÂÂÂÂ bitmap_and(applied_alternatives, applied_alternatives,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ cpu_hwcaps, ARM64_NCAPS);
>> ÂÂÂÂÂ }
>> Â }
>> Â @@ -225,8 +232,13 @@ static int __apply_alternatives_multi_stop(void
>> *unused)
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ cpu_relax();
>> ÂÂÂÂÂÂÂÂÂ isb();
>> ÂÂÂÂÂ } else {
>> +ÂÂÂÂÂÂÂ DECLARE_BITMAP(remaining_capabilities, ARM64_NPATCHABLE);
>> +
>> +ÂÂÂÂÂÂÂ bitmap_complement(remaining_capabilities, boot_capabilities,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ ARM64_NPATCHABLE);
>> +
>> ÂÂÂÂÂÂÂÂÂ BUG_ON(all_alternatives_applied);
>> -ÂÂÂÂÂÂÂ __apply_alternatives(&region, false);
>> +ÂÂÂÂÂÂÂ __apply_alternatives(&region, false, remaining_capabilities);
>> ÂÂÂÂÂÂÂÂÂ /* Barriers provided by the cache flushing */
>> ÂÂÂÂÂÂÂÂÂ WRITE_ONCE(all_alternatives_applied, 1);
>> ÂÂÂÂÂ }
>> @@ -240,6 +252,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);
>> +
>> +ÂÂÂ __apply_alternatives(&region, false, &boot_capabilities[0]);
>> +}
>> +
>> Â #ifdef CONFIG_MODULES
>> Â void apply_alternatives_module(void *start, size_t length)
>> Â {
>> @@ -247,7 +277,10 @@ void apply_alternatives_module(void *start,
>> size_t length)
>> ÂÂÂÂÂÂÂÂÂ .beginÂÂÂ = start,
>> ÂÂÂÂÂÂÂÂÂ .endÂÂÂ = start + length,
>> ÂÂÂÂÂ };
>> +ÂÂÂ DECLARE_BITMAP(all_capabilities, ARM64_NPATCHABLE);
>> +
>> +ÂÂÂ bitmap_fill(all_capabilities, ARM64_NPATCHABLE);
>> Â -ÂÂÂ __apply_alternatives(&region, true);
>> +ÂÂÂ __apply_alternatives(&region, true, &all_capabilities[0]);
>> Â }
>> Â #endif
>> diff --git a/arch/arm64/kernel/cpufeature.c
>> b/arch/arm64/kernel/cpufeature.c
>> index 84fa5be..71c8d4f 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -54,6 +54,9 @@
>> Â EXPORT_SYMBOL(cpu_hwcaps);
>> Â static struct arm64_cpu_capabilities const __ro_after_init
>> *cpu_hwcaps_ptrs[ARM64_NCAPS];
>> Â +/* Need also bit for ARM64_CB_PATCH */
>> +DECLARE_BITMAP(boot_capabilities, ARM64_NPATCHABLE);
>> +
>> Â /*
>> ÂÂ * Flag to indicate if we have computed the system wide
>> ÂÂ * capabilities based on the boot time active CPUs. This
>> @@ -1672,6 +1675,9 @@ static void update_cpu_capabilities(u16 scope_mask)
>> ÂÂÂÂÂÂÂÂÂ if (caps->desc)
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ pr_info("detected: %s\n", caps->desc);
>> ÂÂÂÂÂÂÂÂÂ cpus_set_cap(caps->capability);
>> +
>> +ÂÂÂÂÂÂÂ if (caps->type & SCOPE_BOOT_CPU)
>
> You may want to do :
> ÂÂÂÂÂÂÂ if (scope_mask & SCOPE_BOOT_CPU)
>
> for a tighter check to ensure this doesn't update the boot_capabilities
> after we have applied the boot_scope alternatives and miss applying the
> alternatives for those, should someone add a multi-scope (i.e
> SCOPE_BOOT_CPU and
> something else) capability (even by mistake).
>

But a multi-scope capability containing SCOPE_BOOT_CPU should already
get updated for setup_boot_cpu_capabilities. Capabilities marked with
SCOPE_BOOT_CPU need to be enabled on the boot CPU or not at all.

Shouldn't the call to caps->matches() fail for a boot feature that was
not found on the boot cpu?

Also, you made the opposite suggestion 4 version ago with a more
worrying scenario :) :
https://lkml.org/lkml/2018/5/25/208

Otherwise, if my assumption above is wrong, it means the check should
probably be:
if (caps->type & SCOPE_BOOT_CPU && scope_mask & SCOPE_BOOT_CPU)

But my current understanding is that we don't need that.

> With that:
>
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>

Let me know if I can keep your tag or if I indeed need to change the
condition.

Thanks,

--
Julien Thierry

Next message: Michael Mueller: "Re: [PATCH v5 13/15] KVM: s390: add function process_gib_alert_list()"
Previous message: Mimi Zohar: "Re: [RFC PATCH 1/1] KEYS, integrity: Link .platform keyring to .secondary_trusted_keys"
In reply to: Suzuki K Poulose: "Re: [PATCH v8 15/26] arm64: alternative: Apply alternatives early in boot process"
Next in thread: Suzuki K Poulose: "Re: [PATCH v8 15/26] arm64: alternative: Apply alternatives early in boot process"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]