diff mbox

[2/3] arm64: Consolidate hotplug notifier for instruction emulation

Message ID 1421325366-13263-3-git-send-email-suzuki.poulose@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Suzuki K Poulose Jan. 15, 2015, 12:36 p.m. UTC
From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>

As of now each insn_emulation has a cpu hotplug notifier that
enables/disables the CPU feature bit for the functionality. This
patch re-arranges the code, such that there is only one notifier
that runs through the list of registered emulation hooks and runs
their corresponding set_hw_mode.

We do nothing when a CPU is dying as we will set the appropriate bits
as it comes back online based on the state of the hooks.

Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 Documentation/arm64/legacy_instructions.txt |    4 +
 arch/arm64/include/asm/cpufeature.h         |    2 +
 arch/arm64/kernel/armv8_deprecated.c        |  113 +++++++++++++++------------
 3 files changed, 69 insertions(+), 50 deletions(-)

Comments

Will Deacon Jan. 16, 2015, 4:07 p.m. UTC | #1
On Thu, Jan 15, 2015 at 12:36:05PM +0000, Suzuki K. Poulose wrote:
> From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>
> 
> As of now each insn_emulation has a cpu hotplug notifier that
> enables/disables the CPU feature bit for the functionality. This
> patch re-arranges the code, such that there is only one notifier
> that runs through the list of registered emulation hooks and runs
> their corresponding set_hw_mode.
> 
> We do nothing when a CPU is dying as we will set the appropriate bits
> as it comes back online based on the state of the hooks.
> 
> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
>  Documentation/arm64/legacy_instructions.txt |    4 +
>  arch/arm64/include/asm/cpufeature.h         |    2 +
>  arch/arm64/kernel/armv8_deprecated.c        |  113 +++++++++++++++------------
>  3 files changed, 69 insertions(+), 50 deletions(-)
> 
> diff --git a/Documentation/arm64/legacy_instructions.txt b/Documentation/arm64/legacy_instructions.txt
> index a3b3da2..0a4dc26 100644
> --- a/Documentation/arm64/legacy_instructions.txt
> +++ b/Documentation/arm64/legacy_instructions.txt
> @@ -27,6 +27,10 @@ behaviours and the corresponding values of the sysctl nodes -
>    instructions. Using hardware execution generally provides better
>    performance, but at the loss of ability to gather runtime statistics
>    about the use of the deprecated instructions.
> +  Note: Emulation of a deprecated instruction depends on the availability
> +  of the feature on all the active CPUs. In case of CPU hotplug, if a new
> +  CPU doesn't support a feature, it could result in the abortion of the
> +  hotplug operation.

Is this true? We should be able to *emulate* the instruction anywhere,
it's the "hardware execution" setting that needs CPU support.

>  The default mode depends on the status of the instruction in the
>  architecture. Deprecated instructions should default to emulation
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index c7f68d1..a56b45f 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -29,6 +29,8 @@
>  #define ID_AA64MMFR0_EL1_BigEndEL0	(0x1UL << 16)
>  #define ID_AA64MMFR0_EL1_BigEnd		(0x1UL << 8)
>  
> +#define SCTLR_EL1_CP15BEN 	(1 << 5)
> +
>  #ifndef __ASSEMBLY__
>  
>  extern DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
> diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
> index c363671..be64218 100644
> --- a/arch/arm64/kernel/armv8_deprecated.c
> +++ b/arch/arm64/kernel/armv8_deprecated.c
> @@ -19,6 +19,7 @@
>  #include <asm/system_misc.h>
>  #include <asm/traps.h>
>  #include <asm/uaccess.h>
> +#include <asm/cpufeature.h>
>  
>  #define CREATE_TRACE_POINTS
>  #include "trace-events-emulation.h"
> @@ -46,7 +47,7 @@ struct insn_emulation_ops {
>  	const char		*name;
>  	enum legacy_insn_status	status;
>  	struct undef_hook	*hooks;
> -	int			(*set_hw_mode)(bool enable);
> +	int			(*set_hw_mode)(void *enable);

I think it would be cleaner to have a wrapper for the on_each_cpu
variant of this, otherwise we lose the type information altogether.

>  };
>  
>  struct insn_emulation {
> @@ -85,6 +86,40 @@ static void remove_emulation_hooks(struct insn_emulation_ops *ops)
>  	pr_notice("Removed %s emulation handler\n", ops->name);
>  }
>  
> +/* Run set_hw_mode(mode) on all active CPUs */
> +static int run_all_cpu_set_hw_mode(struct insn_emulation *insn, bool enable)
> +{
> +	void (*set_hw_mode)(void *) = (void (*) (void *))insn->ops->set_hw_mode;
> +	if (!set_hw_mode)
> +		return -EINVAL;
> +	on_each_cpu(set_hw_mode, (void *)enable, true);
> +	return 0;
> +}
> +
> +/*
> + * Run set_hw_mode for all insns on a starting CPU.
> + * Returns:
> + *  0 		- If all the hooks ran successfully.
> + * -EINVAL	- At least one hook is not supported by the CPU.
> + *		  abort the hotplug.
> + */
> +static int run_all_insn_set_hw_mode(void)
> +{
> +	int rc = 0;
> +	unsigned long flags;
> +	struct insn_emulation *insn;
> +
> +	raw_spin_lock_irqsave(&insn_emulation_lock, flags);
> +	list_for_each_entry(insn, &insn_emulation, node) {
> +		bool hw_mode = (insn->current_mode == INSN_HW);
> +		if (insn->ops->set_hw_mode &&
> +			insn->ops->set_hw_mode((void*)hw_mode))
> +			rc = -EINVAL;
> +	}
> +	raw_spin_unlock_irqrestore(&insn_emulation_lock, flags);
> +	return rc;
> +}
> +
>  static int update_insn_emulation_mode(struct insn_emulation *insn,
>  				       enum insn_emulation_mode prev)
>  {
> @@ -97,10 +132,8 @@ static int update_insn_emulation_mode(struct insn_emulation *insn,
>  		remove_emulation_hooks(insn->ops);
>  		break;
>  	case INSN_HW:
> -		if (insn->ops->set_hw_mode) {
> -			insn->ops->set_hw_mode(false);
> +		if (!run_all_cpu_set_hw_mode(insn, false))
>  			pr_notice("Disabled %s support\n", insn->ops->name);
> -		}
>  		break;
>  	}
>  
> @@ -111,10 +144,9 @@ static int update_insn_emulation_mode(struct insn_emulation *insn,
>  		register_emulation_hooks(insn->ops);
>  		break;
>  	case INSN_HW:
> -		if (insn->ops->set_hw_mode && insn->ops->set_hw_mode(true))
> +		ret = run_all_cpu_set_hw_mode(insn, true);
> +		if (!ret)
>  			pr_notice("Enabled %s support\n", insn->ops->name);
> -		else
> -			ret = -EINVAL;
>  		break;
>  	}
>  
> @@ -133,6 +165,8 @@ static void register_insn_emulation(struct insn_emulation_ops *ops)
>  	switch (ops->status) {
>  	case INSN_DEPRECATED:
>  		insn->current_mode = INSN_EMULATE;
> +		/* Disable the HW mode if it was turned on at early boot time */
> +		run_all_cpu_set_hw_mode(insn, false);
>  		insn->max = INSN_HW;
>  		break;
>  	case INSN_OBSOLETE:
> @@ -453,8 +487,6 @@ ret:
>  	return 0;
>  }
>  
> -#define SCTLR_EL1_CP15BEN (1 << 5)
> -
>  static inline void config_sctlr_el1(u32 clear, u32 set)
>  {
>  	u32 val;
> @@ -465,48 +497,13 @@ static inline void config_sctlr_el1(u32 clear, u32 set)
>  	asm volatile("msr sctlr_el1, %0" : : "r" (val));
>  }
>  
> -static void enable_cp15_ben(void *info)
> -{
> -	config_sctlr_el1(0, SCTLR_EL1_CP15BEN);
> -}
> -
> -static void disable_cp15_ben(void *info)
> +static int cp15_barrier_set_hw_mode(void *enable)
>  {
> -	config_sctlr_el1(SCTLR_EL1_CP15BEN, 0);
> -}
> -
> -static int cpu_hotplug_notify(struct notifier_block *b,
> -			      unsigned long action, void *hcpu)
> -{
> -	switch (action) {
> -	case CPU_STARTING:
> -	case CPU_STARTING_FROZEN:
> -		enable_cp15_ben(NULL);
> -		return NOTIFY_DONE;
> -	case CPU_DYING:
> -	case CPU_DYING_FROZEN:
> -		disable_cp15_ben(NULL);
> -		return NOTIFY_DONE;
> -	}
> -
> -	return NOTIFY_OK;
> -}
> -
> -static struct notifier_block cpu_hotplug_notifier = {
> -	.notifier_call = cpu_hotplug_notify,
> -};
> -
> -static int cp15_barrier_set_hw_mode(bool enable)
> -{
> -	if (enable) {
> -		register_cpu_notifier(&cpu_hotplug_notifier);
> -		on_each_cpu(enable_cp15_ben, NULL, true);
> -	} else {
> -		unregister_cpu_notifier(&cpu_hotplug_notifier);
> -		on_each_cpu(disable_cp15_ben, NULL, true);
> -	}
> -
> -	return true;
> +	if (enable)
> +		config_sctlr_el1(0, SCTLR_EL1_CP15BEN);
> +	else
> +		config_sctlr_el1(SCTLR_EL1_CP15BEN, 0);
> +	return 0;
>  }
>  
>  static struct undef_hook cp15_barrier_hooks[] = {
> @@ -534,6 +531,21 @@ static struct insn_emulation_ops cp15_barrier_ops = {
>  	.set_hw_mode = cp15_barrier_set_hw_mode,
>  };
>  
> +static int insn_cpu_hotplug_notify(struct notifier_block *b,
> +			      unsigned long action, void *hcpu)
> +{
> +	int rc = 0;
> +	if ((action & ~CPU_TASKS_FROZEN) == CPU_STARTING)
> +		rc = run_all_insn_set_hw_mode();
> +
> +	/* Abort the CPU hotplug if there is an incompatibility */
> +	return notifier_from_errno(rc);

Could we restrict the emulation options instead and just disable hardware
support for the instruction?

Will
Mark Rutland Jan. 16, 2015, 4:32 p.m. UTC | #2
On Fri, Jan 16, 2015 at 04:07:30PM +0000, Will Deacon wrote:
> On Thu, Jan 15, 2015 at 12:36:05PM +0000, Suzuki K. Poulose wrote:
> > From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>
> > 
> > As of now each insn_emulation has a cpu hotplug notifier that
> > enables/disables the CPU feature bit for the functionality. This
> > patch re-arranges the code, such that there is only one notifier
> > that runs through the list of registered emulation hooks and runs
> > their corresponding set_hw_mode.
> > 
> > We do nothing when a CPU is dying as we will set the appropriate bits
> > as it comes back online based on the state of the hooks.
> > 
> > Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > ---
> >  Documentation/arm64/legacy_instructions.txt |    4 +
> >  arch/arm64/include/asm/cpufeature.h         |    2 +
> >  arch/arm64/kernel/armv8_deprecated.c        |  113 +++++++++++++++------------
> >  3 files changed, 69 insertions(+), 50 deletions(-)
> > 
> > diff --git a/Documentation/arm64/legacy_instructions.txt b/Documentation/arm64/legacy_instructions.txt
> > index a3b3da2..0a4dc26 100644
> > --- a/Documentation/arm64/legacy_instructions.txt
> > +++ b/Documentation/arm64/legacy_instructions.txt
> > @@ -27,6 +27,10 @@ behaviours and the corresponding values of the sysctl nodes -
> >    instructions. Using hardware execution generally provides better
> >    performance, but at the loss of ability to gather runtime statistics
> >    about the use of the deprecated instructions.
> > +  Note: Emulation of a deprecated instruction depends on the availability
> > +  of the feature on all the active CPUs. In case of CPU hotplug, if a new
> > +  CPU doesn't support a feature, it could result in the abortion of the
> > +  hotplug operation.
> 
> Is this true? We should be able to *emulate* the instruction anywhere,
> it's the "hardware execution" setting that needs CPU support.

Not quite. In ARMv8 there are three possible cases for endianness
support:

(a) Mixed endian support at all levels
    ID_AA64MMFR0_EL1.BigEnd == 0b0001
    ID_AA64MMFR0_EL1.BigEnd0 == 0b0000 (RES0, invalid)

(b) Mixed endian support at EL0 only
    ID_AA64MMFR0_EL1.BigEnd == 0b0000
    ID_AA64MMFR0_EL1.BigEnd0 == 0b0001
    SCTLR_EL1.EE has a fixed value

(c) No mixed endian support at all
    ID_AA64MMFR0_EL1.BigEnd == 0b0000
    ID_AA64MMFR0_EL1.BigEnd0 == 0b0000
    SCTLR_EL1.{EE,E0E} have fixed values

We can emulate setend in cases (a) and (b), but not (c) unless we
trapped every single instruction that could access memory. I don't think
that's feasible.

Also fun in case (c) is that the kernel may not be able to run on all
CPUs (consider a BE kernel where some CPUs are fixed to LE at EL1).

I hope no-one builds a system where CPUs are mismatched w.r.t.
endianness support.

Thanks,
Mark.
Will Deacon Jan. 16, 2015, 4:44 p.m. UTC | #3
On Fri, Jan 16, 2015 at 04:32:54PM +0000, Mark Rutland wrote:
> On Fri, Jan 16, 2015 at 04:07:30PM +0000, Will Deacon wrote:
> > On Thu, Jan 15, 2015 at 12:36:05PM +0000, Suzuki K. Poulose wrote:
> > > From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>
> > > 
> > > As of now each insn_emulation has a cpu hotplug notifier that
> > > enables/disables the CPU feature bit for the functionality. This
> > > patch re-arranges the code, such that there is only one notifier
> > > that runs through the list of registered emulation hooks and runs
> > > their corresponding set_hw_mode.
> > > 
> > > We do nothing when a CPU is dying as we will set the appropriate bits
> > > as it comes back online based on the state of the hooks.
> > > 
> > > Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > ---
> > >  Documentation/arm64/legacy_instructions.txt |    4 +
> > >  arch/arm64/include/asm/cpufeature.h         |    2 +
> > >  arch/arm64/kernel/armv8_deprecated.c        |  113 +++++++++++++++------------
> > >  3 files changed, 69 insertions(+), 50 deletions(-)
> > > 
> > > diff --git a/Documentation/arm64/legacy_instructions.txt b/Documentation/arm64/legacy_instructions.txt
> > > index a3b3da2..0a4dc26 100644
> > > --- a/Documentation/arm64/legacy_instructions.txt
> > > +++ b/Documentation/arm64/legacy_instructions.txt
> > > @@ -27,6 +27,10 @@ behaviours and the corresponding values of the sysctl nodes -
> > >    instructions. Using hardware execution generally provides better
> > >    performance, but at the loss of ability to gather runtime statistics
> > >    about the use of the deprecated instructions.
> > > +  Note: Emulation of a deprecated instruction depends on the availability
> > > +  of the feature on all the active CPUs. In case of CPU hotplug, if a new
> > > +  CPU doesn't support a feature, it could result in the abortion of the
> > > +  hotplug operation.
> > 
> > Is this true? We should be able to *emulate* the instruction anywhere,
> > it's the "hardware execution" setting that needs CPU support.
> 
> Not quite. In ARMv8 there are three possible cases for endianness
> support:
> 
> (a) Mixed endian support at all levels
>     ID_AA64MMFR0_EL1.BigEnd == 0b0001
>     ID_AA64MMFR0_EL1.BigEnd0 == 0b0000 (RES0, invalid)
> 
> (b) Mixed endian support at EL0 only
>     ID_AA64MMFR0_EL1.BigEnd == 0b0000
>     ID_AA64MMFR0_EL1.BigEnd0 == 0b0001
>     SCTLR_EL1.EE has a fixed value
> 
> (c) No mixed endian support at all
>     ID_AA64MMFR0_EL1.BigEnd == 0b0000
>     ID_AA64MMFR0_EL1.BigEnd0 == 0b0000
>     SCTLR_EL1.{EE,E0E} have fixed values
> 
> We can emulate setend in cases (a) and (b), but not (c) unless we
> trapped every single instruction that could access memory. I don't think
> that's feasible.
> 
> Also fun in case (c) is that the kernel may not be able to run on all
> CPUs (consider a BE kernel where some CPUs are fixed to LE at EL1).
> 
> I hope no-one builds a system where CPUs are mismatched w.r.t.
> endianness support.

Agreed, the problem here is all down to the wording.
Documentation/arm64/legacy_instructions.txt isn't specific to SETEND
emulation and saying "Emulation of a deprecated instruction depends on the
availability of the feature on all the active CPUs." as a blanket statement
doesn't make a lot of sense in isolation. It's certainly not the case for
SWP and CP15 barriers, for example.

With SETEND, we have requirements on mixed-endian support for the emulation.
Fine, but let's not muddy the waters around the general CPU requirements
for deprecated instruction emulation.

Will
Suzuki K Poulose Jan. 16, 2015, 4:47 p.m. UTC | #4
On 16/01/15 16:07, Will Deacon wrote:
> On Thu, Jan 15, 2015 at 12:36:05PM +0000, Suzuki K. Poulose wrote:
>> From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>
>>
>> As of now each insn_emulation has a cpu hotplug notifier that
>> enables/disables the CPU feature bit for the functionality. This
>> patch re-arranges the code, such that there is only one notifier
>> that runs through the list of registered emulation hooks and runs
>> their corresponding set_hw_mode.
>>
>> We do nothing when a CPU is dying as we will set the appropriate bits
>> as it comes back online based on the state of the hooks.
>>
>> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
>> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
>> ---
>>   Documentation/arm64/legacy_instructions.txt |    4 +
>>   arch/arm64/include/asm/cpufeature.h         |    2 +
>>   arch/arm64/kernel/armv8_deprecated.c        |  113 +++++++++++++++------------
>>   3 files changed, 69 insertions(+), 50 deletions(-)
>>
>> diff --git a/Documentation/arm64/legacy_instructions.txt b/Documentation/arm64/legacy_instructions.txt
>> index a3b3da2..0a4dc26 100644
>> --- a/Documentation/arm64/legacy_instructions.txt
>> +++ b/Documentation/arm64/legacy_instructions.txt
>> @@ -27,6 +27,10 @@ behaviours and the corresponding values of the sysctl nodes -
>>     instructions. Using hardware execution generally provides better
>>     performance, but at the loss of ability to gather runtime statistics
>>     about the use of the deprecated instructions.
>> +  Note: Emulation of a deprecated instruction depends on the availability
>> +  of the feature on all the active CPUs. In case of CPU hotplug, if a new
>> +  CPU doesn't support a feature, it could result in the abortion of the
>> +  hotplug operation.
>
> Is this true? We should be able to *emulate* the instruction anywhere,
> it's the "hardware execution" setting that needs CPU support.
Yes. Particularly for SETEND, if the CPU doesn't support mixed endian 
data access at EL0, we shouldn't emulate setend and cause unexpected 
results at EL0. So it is upto the hook to raise a flag if it can emulate 
without the hardware capability.

>
>>   The default mode depends on the status of the instruction in the
>>   architecture. Deprecated instructions should default to emulation
>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>> index c7f68d1..a56b45f 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -29,6 +29,8 @@
>>   #define ID_AA64MMFR0_EL1_BigEndEL0	(0x1UL << 16)
>>   #define ID_AA64MMFR0_EL1_BigEnd		(0x1UL << 8)
>>
>> +#define SCTLR_EL1_CP15BEN 	(1 << 5)
>> +
>>   #ifndef __ASSEMBLY__
>>
>>   extern DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
>> diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
>> index c363671..be64218 100644
>> --- a/arch/arm64/kernel/armv8_deprecated.c
>> +++ b/arch/arm64/kernel/armv8_deprecated.c
>> @@ -19,6 +19,7 @@
>>   #include <asm/system_misc.h>
>>   #include <asm/traps.h>
>>   #include <asm/uaccess.h>
>> +#include <asm/cpufeature.h>
>>
>>   #define CREATE_TRACE_POINTS
>>   #include "trace-events-emulation.h"
>> @@ -46,7 +47,7 @@ struct insn_emulation_ops {
>>   	const char		*name;
>>   	enum legacy_insn_status	status;
>>   	struct undef_hook	*hooks;
>> -	int			(*set_hw_mode)(bool enable);
>> +	int			(*set_hw_mode)(void *enable);
>
> I think it would be cleaner to have a wrapper for the on_each_cpu
> variant of this, otherwise we lose the type information altogether.
>
OK.
>>   };
>>
>>   struct insn_emulation {
>> @@ -85,6 +86,40 @@ static void remove_emulation_hooks(struct insn_emulation_ops *ops)
>>   	pr_notice("Removed %s emulation handler\n", ops->name);
>>   }
>>

>>
>> +static int insn_cpu_hotplug_notify(struct notifier_block *b,
>> +			      unsigned long action, void *hcpu)
>> +{
>> +	int rc = 0;
>> +	if ((action & ~CPU_TASKS_FROZEN) == CPU_STARTING)
>> +		rc = run_all_insn_set_hw_mode();
>> +
>> +	/* Abort the CPU hotplug if there is an incompatibility */
>> +	return notifier_from_errno(rc);
>
> Could we restrict the emulation options instead and just disable hardware
> support for the instruction?

As explained earlier, each hook could decide if missing a feature could 
result in an abort.

Thanks
Suzuki
Mark Rutland Jan. 16, 2015, 4:48 p.m. UTC | #5
On Fri, Jan 16, 2015 at 04:44:34PM +0000, Will Deacon wrote:
> On Fri, Jan 16, 2015 at 04:32:54PM +0000, Mark Rutland wrote:
> > On Fri, Jan 16, 2015 at 04:07:30PM +0000, Will Deacon wrote:
> > > On Thu, Jan 15, 2015 at 12:36:05PM +0000, Suzuki K. Poulose wrote:
> > > > From: "Suzuki K. Poulose" <suzuki.poulose@arm.com>
> > > > 
> > > > As of now each insn_emulation has a cpu hotplug notifier that
> > > > enables/disables the CPU feature bit for the functionality. This
> > > > patch re-arranges the code, such that there is only one notifier
> > > > that runs through the list of registered emulation hooks and runs
> > > > their corresponding set_hw_mode.
> > > > 
> > > > We do nothing when a CPU is dying as we will set the appropriate bits
> > > > as it comes back online based on the state of the hooks.
> > > > 
> > > > Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> > > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > > ---
> > > >  Documentation/arm64/legacy_instructions.txt |    4 +
> > > >  arch/arm64/include/asm/cpufeature.h         |    2 +
> > > >  arch/arm64/kernel/armv8_deprecated.c        |  113 +++++++++++++++------------
> > > >  3 files changed, 69 insertions(+), 50 deletions(-)
> > > > 
> > > > diff --git a/Documentation/arm64/legacy_instructions.txt b/Documentation/arm64/legacy_instructions.txt
> > > > index a3b3da2..0a4dc26 100644
> > > > --- a/Documentation/arm64/legacy_instructions.txt
> > > > +++ b/Documentation/arm64/legacy_instructions.txt
> > > > @@ -27,6 +27,10 @@ behaviours and the corresponding values of the sysctl nodes -
> > > >    instructions. Using hardware execution generally provides better
> > > >    performance, but at the loss of ability to gather runtime statistics
> > > >    about the use of the deprecated instructions.
> > > > +  Note: Emulation of a deprecated instruction depends on the availability
> > > > +  of the feature on all the active CPUs. In case of CPU hotplug, if a new
> > > > +  CPU doesn't support a feature, it could result in the abortion of the
> > > > +  hotplug operation.
> > > 
> > > Is this true? We should be able to *emulate* the instruction anywhere,
> > > it's the "hardware execution" setting that needs CPU support.
> > 
> > Not quite. In ARMv8 there are three possible cases for endianness
> > support:
> > 
> > (a) Mixed endian support at all levels
> >     ID_AA64MMFR0_EL1.BigEnd == 0b0001
> >     ID_AA64MMFR0_EL1.BigEnd0 == 0b0000 (RES0, invalid)
> > 
> > (b) Mixed endian support at EL0 only
> >     ID_AA64MMFR0_EL1.BigEnd == 0b0000
> >     ID_AA64MMFR0_EL1.BigEnd0 == 0b0001
> >     SCTLR_EL1.EE has a fixed value
> > 
> > (c) No mixed endian support at all
> >     ID_AA64MMFR0_EL1.BigEnd == 0b0000
> >     ID_AA64MMFR0_EL1.BigEnd0 == 0b0000
> >     SCTLR_EL1.{EE,E0E} have fixed values
> > 
> > We can emulate setend in cases (a) and (b), but not (c) unless we
> > trapped every single instruction that could access memory. I don't think
> > that's feasible.
> > 
> > Also fun in case (c) is that the kernel may not be able to run on all
> > CPUs (consider a BE kernel where some CPUs are fixed to LE at EL1).
> > 
> > I hope no-one builds a system where CPUs are mismatched w.r.t.
> > endianness support.
> 
> Agreed, the problem here is all down to the wording.
> Documentation/arm64/legacy_instructions.txt isn't specific to SETEND
> emulation and saying "Emulation of a deprecated instruction depends on the
> availability of the feature on all the active CPUs." as a blanket statement
> doesn't make a lot of sense in isolation. It's certainly not the case for
> SWP and CP15 barriers, for example.
> 
> With SETEND, we have requirements on mixed-endian support for the emulation.
> Fine, but let's not muddy the waters around the general CPU requirements
> for deprecated instruction emulation.

Sure. I was only contending the "We should be able to *emulate* the
instruction anywhere" part of your statement.

We should probably add _a_ note as part of the patch introducing SETEND
emulation, but not the one above, and we can drop it from this patch.

Mark.
diff mbox

Patch

diff --git a/Documentation/arm64/legacy_instructions.txt b/Documentation/arm64/legacy_instructions.txt
index a3b3da2..0a4dc26 100644
--- a/Documentation/arm64/legacy_instructions.txt
+++ b/Documentation/arm64/legacy_instructions.txt
@@ -27,6 +27,10 @@  behaviours and the corresponding values of the sysctl nodes -
   instructions. Using hardware execution generally provides better
   performance, but at the loss of ability to gather runtime statistics
   about the use of the deprecated instructions.
+  Note: Emulation of a deprecated instruction depends on the availability
+  of the feature on all the active CPUs. In case of CPU hotplug, if a new
+  CPU doesn't support a feature, it could result in the abortion of the
+  hotplug operation.
 
 The default mode depends on the status of the instruction in the
 architecture. Deprecated instructions should default to emulation
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index c7f68d1..a56b45f 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -29,6 +29,8 @@ 
 #define ID_AA64MMFR0_EL1_BigEndEL0	(0x1UL << 16)
 #define ID_AA64MMFR0_EL1_BigEnd		(0x1UL << 8)
 
+#define SCTLR_EL1_CP15BEN 	(1 << 5)
+
 #ifndef __ASSEMBLY__
 
 extern DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
index c363671..be64218 100644
--- a/arch/arm64/kernel/armv8_deprecated.c
+++ b/arch/arm64/kernel/armv8_deprecated.c
@@ -19,6 +19,7 @@ 
 #include <asm/system_misc.h>
 #include <asm/traps.h>
 #include <asm/uaccess.h>
+#include <asm/cpufeature.h>
 
 #define CREATE_TRACE_POINTS
 #include "trace-events-emulation.h"
@@ -46,7 +47,7 @@  struct insn_emulation_ops {
 	const char		*name;
 	enum legacy_insn_status	status;
 	struct undef_hook	*hooks;
-	int			(*set_hw_mode)(bool enable);
+	int			(*set_hw_mode)(void *enable);
 };
 
 struct insn_emulation {
@@ -85,6 +86,40 @@  static void remove_emulation_hooks(struct insn_emulation_ops *ops)
 	pr_notice("Removed %s emulation handler\n", ops->name);
 }
 
+/* Run set_hw_mode(mode) on all active CPUs */
+static int run_all_cpu_set_hw_mode(struct insn_emulation *insn, bool enable)
+{
+	void (*set_hw_mode)(void *) = (void (*) (void *))insn->ops->set_hw_mode;
+	if (!set_hw_mode)
+		return -EINVAL;
+	on_each_cpu(set_hw_mode, (void *)enable, true);
+	return 0;
+}
+
+/*
+ * Run set_hw_mode for all insns on a starting CPU.
+ * Returns:
+ *  0 		- If all the hooks ran successfully.
+ * -EINVAL	- At least one hook is not supported by the CPU.
+ *		  abort the hotplug.
+ */
+static int run_all_insn_set_hw_mode(void)
+{
+	int rc = 0;
+	unsigned long flags;
+	struct insn_emulation *insn;
+
+	raw_spin_lock_irqsave(&insn_emulation_lock, flags);
+	list_for_each_entry(insn, &insn_emulation, node) {
+		bool hw_mode = (insn->current_mode == INSN_HW);
+		if (insn->ops->set_hw_mode &&
+			insn->ops->set_hw_mode((void*)hw_mode))
+			rc = -EINVAL;
+	}
+	raw_spin_unlock_irqrestore(&insn_emulation_lock, flags);
+	return rc;
+}
+
 static int update_insn_emulation_mode(struct insn_emulation *insn,
 				       enum insn_emulation_mode prev)
 {
@@ -97,10 +132,8 @@  static int update_insn_emulation_mode(struct insn_emulation *insn,
 		remove_emulation_hooks(insn->ops);
 		break;
 	case INSN_HW:
-		if (insn->ops->set_hw_mode) {
-			insn->ops->set_hw_mode(false);
+		if (!run_all_cpu_set_hw_mode(insn, false))
 			pr_notice("Disabled %s support\n", insn->ops->name);
-		}
 		break;
 	}
 
@@ -111,10 +144,9 @@  static int update_insn_emulation_mode(struct insn_emulation *insn,
 		register_emulation_hooks(insn->ops);
 		break;
 	case INSN_HW:
-		if (insn->ops->set_hw_mode && insn->ops->set_hw_mode(true))
+		ret = run_all_cpu_set_hw_mode(insn, true);
+		if (!ret)
 			pr_notice("Enabled %s support\n", insn->ops->name);
-		else
-			ret = -EINVAL;
 		break;
 	}
 
@@ -133,6 +165,8 @@  static void register_insn_emulation(struct insn_emulation_ops *ops)
 	switch (ops->status) {
 	case INSN_DEPRECATED:
 		insn->current_mode = INSN_EMULATE;
+		/* Disable the HW mode if it was turned on at early boot time */
+		run_all_cpu_set_hw_mode(insn, false);
 		insn->max = INSN_HW;
 		break;
 	case INSN_OBSOLETE:
@@ -453,8 +487,6 @@  ret:
 	return 0;
 }
 
-#define SCTLR_EL1_CP15BEN (1 << 5)
-
 static inline void config_sctlr_el1(u32 clear, u32 set)
 {
 	u32 val;
@@ -465,48 +497,13 @@  static inline void config_sctlr_el1(u32 clear, u32 set)
 	asm volatile("msr sctlr_el1, %0" : : "r" (val));
 }
 
-static void enable_cp15_ben(void *info)
-{
-	config_sctlr_el1(0, SCTLR_EL1_CP15BEN);
-}
-
-static void disable_cp15_ben(void *info)
+static int cp15_barrier_set_hw_mode(void *enable)
 {
-	config_sctlr_el1(SCTLR_EL1_CP15BEN, 0);
-}
-
-static int cpu_hotplug_notify(struct notifier_block *b,
-			      unsigned long action, void *hcpu)
-{
-	switch (action) {
-	case CPU_STARTING:
-	case CPU_STARTING_FROZEN:
-		enable_cp15_ben(NULL);
-		return NOTIFY_DONE;
-	case CPU_DYING:
-	case CPU_DYING_FROZEN:
-		disable_cp15_ben(NULL);
-		return NOTIFY_DONE;
-	}
-
-	return NOTIFY_OK;
-}
-
-static struct notifier_block cpu_hotplug_notifier = {
-	.notifier_call = cpu_hotplug_notify,
-};
-
-static int cp15_barrier_set_hw_mode(bool enable)
-{
-	if (enable) {
-		register_cpu_notifier(&cpu_hotplug_notifier);
-		on_each_cpu(enable_cp15_ben, NULL, true);
-	} else {
-		unregister_cpu_notifier(&cpu_hotplug_notifier);
-		on_each_cpu(disable_cp15_ben, NULL, true);
-	}
-
-	return true;
+	if (enable)
+		config_sctlr_el1(0, SCTLR_EL1_CP15BEN);
+	else
+		config_sctlr_el1(SCTLR_EL1_CP15BEN, 0);
+	return 0;
 }
 
 static struct undef_hook cp15_barrier_hooks[] = {
@@ -534,6 +531,21 @@  static struct insn_emulation_ops cp15_barrier_ops = {
 	.set_hw_mode = cp15_barrier_set_hw_mode,
 };
 
+static int insn_cpu_hotplug_notify(struct notifier_block *b,
+			      unsigned long action, void *hcpu)
+{
+	int rc = 0;
+	if ((action & ~CPU_TASKS_FROZEN) == CPU_STARTING)
+		rc = run_all_insn_set_hw_mode();
+
+	/* Abort the CPU hotplug if there is an incompatibility */
+	return notifier_from_errno(rc);
+}
+
+static struct notifier_block insn_cpu_hotplug_notifier = {
+	.notifier_call = insn_cpu_hotplug_notify,
+};
+
 /*
  * Invoked as late_initcall, since not needed before init spawned.
  */
@@ -545,6 +557,7 @@  static int __init armv8_deprecated_init(void)
 	if (IS_ENABLED(CONFIG_CP15_BARRIER_EMULATION))
 		register_insn_emulation(&cp15_barrier_ops);
 
+	register_cpu_notifier(&insn_cpu_hotplug_notifier);
 	register_insn_emulation_sysctl(ctl_abi);
 
 	return 0;