diff mbox series

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

Message ID 1546956464-48825-16-git-send-email-julien.thierry@arm.com
State Superseded
Headers show
Series None | expand

Commit Message

Julien Thierry Jan. 8, 2019, 2:07 p.m. UTC
From: Daniel Thompson <daniel.thompson@linaro.org>


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@linaro.org>

[julien.thierry@arm.com: rename to fit new cpufeature framework better,
			 apply BOOT_SCOPE feature early in boot]
Signed-off-by: Julien Thierry <julien.thierry@arm.com>

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Christoffer Dall <christoffer.dall@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 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(-)

-- 
1.9.1

Comments

Suzuki K Poulose Jan. 8, 2019, 2:51 p.m. UTC | #1
Hi Julien,

On 08/01/2019 14:07, Julien Thierry wrote:
> From: Daniel Thompson <daniel.thompson@linaro.org>

> 

> 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@linaro.org>

> [julien.thierry@arm.com: rename to fit new cpufeature framework better,

> 			 apply BOOT_SCOPE feature early in boot]

> Signed-off-by: Julien Thierry <julien.thierry@arm.com>

> Cc: Catalin Marinas <catalin.marinas@arm.com>

> Cc: Will Deacon <will.deacon@arm.com>

> Cc: Christoffer Dall <christoffer.dall@arm.com>

> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>

> ---

>   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).

With that:

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Julien Thierry Jan. 8, 2019, 3:20 p.m. UTC | #2
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@linaro.org>

>>

>> 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@linaro.org>

>> [julien.thierry@arm.com: rename to fit new cpufeature framework better,

>>              apply BOOT_SCOPE feature early in boot]

>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>

>> Cc: Catalin Marinas <catalin.marinas@arm.com>

>> Cc: Will Deacon <will.deacon@arm.com>

>> Cc: Christoffer Dall <christoffer.dall@arm.com>

>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>

>> ---

>>   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@arm.com>


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

Thanks,

-- 
Julien Thierry
Suzuki K Poulose Jan. 8, 2019, 5:40 p.m. UTC | #3
On 08/01/2019 15:20, Julien Thierry wrote:
> 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@linaro.org>

>>>

>>> 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@linaro.org>

>>> [julien.thierry@arm.com: rename to fit new cpufeature framework better,

>>>               apply BOOT_SCOPE feature early in boot]

>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>

>>> Cc: Catalin Marinas <catalin.marinas@arm.com>

>>> Cc: Will Deacon <will.deacon@arm.com>

>>> Cc: Christoffer Dall <christoffer.dall@arm.com>

>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>

>>> ---

>>>    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.


Yes, you're right. It is not normal to have multiple SCOPE for a "capability".
But if someone comes with such a cap, we may miss handling this case. It is
always better to be safer.

> 

> 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


Ah, you're right. I missed that. We need the additional check as you mention
below.

> 

> 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)


Yes, this is what we want.


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

> 

>> With that:

>>

>> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>



Cheers
Suzuki
Julien Thierry Jan. 10, 2019, 10:50 a.m. UTC | #4
On 08/01/2019 17:40, Suzuki K Poulose wrote:
> 

> 

> On 08/01/2019 15:20, Julien Thierry wrote:

>> 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@linaro.org>

>>>>

>>>> 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@linaro.org>

>>>> [julien.thierry@arm.com: rename to fit new cpufeature framework better,

>>>>               apply BOOT_SCOPE feature early in boot]

>>>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>

>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>

>>>> Cc: Will Deacon <will.deacon@arm.com>

>>>> Cc: Christoffer Dall <christoffer.dall@arm.com>

>>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>

>>>> ---

>>>>    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.

> 

> Yes, you're right. It is not normal to have multiple SCOPE for a

> "capability".

> But if someone comes with such a cap, we may miss handling this case. It is

> always better to be safer.

> 

>>

>> 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

> 

> Ah, you're right. I missed that. We need the additional check as you

> mention

> below.

> 

>>

>> 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)

> 

> Yes, this is what we want.


Yes you're right, the behaviour I was thinking of were the ID register,
where there views are altered for following CPUs when system wide
features are missing on the boot CPU. But ->matches() callbacks don't
only rely on ID registers.

I'll add that for the next version.

Thanks,

-- 
Julien Thierry
diff mbox series

Patch

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)
+			set_bit(caps->capability, boot_capabilities);
 	}
 }
 
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 1598d6f..a944edd 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -419,6 +419,13 @@  void __init smp_prepare_boot_cpu(void)
 	 */
 	jump_label_init();
 	cpuinfo_store_boot_cpu();
+
+	/*
+	 * We now know enough about the boot CPU to apply the
+	 * alternatives that cannot wait until interrupt handling
+	 * and/or scheduling is enabled.
+	 */
+	apply_boot_alternatives();
 }
 
 static u64 __init of_get_cpu_mpidr(struct device_node *dn)