On Thu, Jan 15, 2015 at 12:36:05PM +0000, Suzuki K. Poulose wrote:
> From: "Suzuki K. Poulose" <suzuki.poul...@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.poul...@arm.com>
> Signed-off-by: Mark Rutland <mark.rutl...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
