[PATCHv4 3/5] arm64: Port SWP/SWPB emulation support from arm

Hi Catalin,

Thanks for reviewing the series.

Catalin Marinas [off-list ref] writes:

On Wed, Nov 12, 2014 at 11:44:50AM +0000, Punit Agrawal wrote:

quoted

 Documentation/arm64/legacy_instructions.txt |  40 +++
 arch/arm64/Kconfig                          |  39 +++
 arch/arm64/include/asm/insn.h               |   6 +
 arch/arm64/kernel/Makefile                  |   1 +
 arch/arm64/kernel/armv8_deprecated.c        | 419 ++++++++++++++++++++++++++++
 arch/arm64/kernel/insn.c                    |   8 +

I would have split this in two patches, one adding the framework and
another the emulation. I'm not too bothered (can leave them as a single
patch) but I have some comments below for grouping the code a bit as it
looks like it's a mix of API for emulation, swp emulation, the API again
making it hard to follow.

I will split them out. It's easier to follow.

quoted

--- /dev/null
+++ b/arch/arm64/kernel/armv8_deprecated.c

@@ -0,0 +1,419 @@
+/*
+ *  Copyright (C) 2014 ARM Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/perf_event.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/sysctl.h>
+
+#include <asm/insn.h>
+#include <asm/opcodes.h>
+#include <asm/system_misc.h>
+#include <asm/traps.h>
+#include <asm/uaccess.h>
+
+/*
+ * The runtime support for deprecated instruction support can be in one of
+ * following three states -
+ *
+ * 0 = undef
+ * 1 = emulate (software emulation)
+ * 2 = hw (supported in hardware)
+ */
+enum insn_emulation_mode {
+       INSN_UNDEF,
+       INSN_EMULATE,
+       INSN_HW,
+};
+
+enum legacy_insn_status {
+       INSN_DEPRECATED,
+       INSN_OBSOLETE,
+};
+
+struct insn_emulation_ops {
+       const char              *name;
+       enum legacy_insn_status status;
+       struct undef_hook       *hooks;
+       int                     (*set_hw_mode)(bool enable);
+};
+
+struct insn_emulation {
+       struct list_head node;
+       struct insn_emulation_ops *ops;
+       int current_mode;
+       int min;
+       int max;
+};

Please move the register_* etc. functions here and let the swp emulation
grouped together.

Done.

quoted

+/*
+ * Only emulate SWP/SWPB executed in ARM state/User mode.
+ * The kernel must be SWP free and SWP{B} does not exist in Thumb.
+ */
+static struct undef_hook swp_hooks[] = {
+               {
+                       .instr_mask     = 0x0fb00ff0,
+                       .instr_val      = 0x01000090,
+                       .pstate_mask    = COMPAT_PSR_MODE_MASK,
+                       .pstate_val     = COMPAT_PSR_MODE_USR,
+                       .fn             = swp_handler
+               },
+               { }
+};

It may be the email client but it looks like too much indentation
here.

Fixed.

quoted

+
+static struct insn_emulation_ops swp_ops = {
+       .name = "swp",
+       .status = INSN_OBSOLETE,
+       .hooks = swp_hooks,
+       .set_hw_mode = NULL,
+};
+

Code below up to armv8_deprecated_init should be moved earlier.

quoted

+static LIST_HEAD(insn_emulation);
+static int nr_insn_emulated;
+static DEFINE_RAW_SPINLOCK(insn_emulation_lock);
+
+static bool register_emulation_hooks(struct insn_emulation_ops *ops)
+{
+       bool success = true;
+       struct undef_hook *hook;
+
+       if (!ops->hooks) {
+               success = false;
+               goto ret;
+       }
+
+       for (hook = ops->hooks; hook->instr_mask; hook++)
+               if (register_undef_hook(hook)) {
+                       success = false;
+                       break;
+               }
+
+ret:
+       return success;
+}

I don't really think this code needs so much checking and error
returning. As I already commented, register_undef_hook() should not
return anything and for the ops->hooks check I think we could just use
BUG_ON(!ops->hooks).

I've simplified both the above function and the one below with the BUG_ON.

Do we need to add support for instructions that can't be emulated? I
wouldn't go there yet until we see some real use-case.

Thanks! Some of the checks were put in anticipation of exactly such
features.

quoted

+static void remove_emulation_hooks(struct insn_emulation_ops *ops)
+{
+       struct undef_hook *hook;
+
+       if (!ops->hooks)
+               return;

Same here.

quoted

+
+       for (hook = ops->hooks; hook->instr_mask; hook++)
+               unregister_undef_hook(hook);
+
+       pr_notice("Removed %s emulation handler\n", ops->name);
+}
+
+static int update_insn_emulation_mode(struct insn_emulation *insn,
+                                      enum insn_emulation_mode prev)
+{
+       int ret = 0;
+
+       switch (prev) {
+       case INSN_UNDEF: /* Nothing to be done */
+               break;
+       case INSN_EMULATE:
+               remove_emulation_hooks(insn->ops);
+               break;
+       case INSN_HW:
+               if (insn->ops->set_hw_mode) {
+                       insn->ops->set_hw_mode(false);
+                       pr_notice("Disabled %s support\n", insn->ops->name);
+               }
+               break;
+       }
+
+       switch (insn->current_mode) {
+       case INSN_UNDEF:
+               break;
+       case INSN_EMULATE:
+               if (!register_emulation_hooks(insn->ops)) {
+                       insn->current_mode = INSN_UNDEF;
+                       ret = -EINVAL;
+               } else
+                       pr_notice("Registered %s emulation handler\n",
+                                 insn->ops->name);
+               break;

If we assume that register_emulation_hooks() doesn't fail, we could
simplify slightly here.

Done.

quoted

+       case INSN_HW:
+               if (insn->ops->set_hw_mode && insn->ops->set_hw_mode(true)) {
+                       pr_notice("Enabled %s support\n", insn->ops->name);
+               } else {
+                       insn->current_mode = INSN_UNDEF;
+                       ret = -EINVAL;
+               }
+               break;

I would rather let it fall back to the previous. So in the caller, we
could re-instate the previous mode rather than undef.

Ok.

quoted

+static void register_insn_emulation(struct insn_emulation_ops *ops)
+{
+       unsigned long flags;
+       struct insn_emulation *insn;
+
+       insn = kzalloc(sizeof(*insn), GFP_KERNEL);
+       insn->ops = ops;
+       insn->min = INSN_UNDEF;
+
+       switch (ops->status) {
+       case INSN_DEPRECATED:
+               insn->current_mode = INSN_EMULATE;
+               insn->max = INSN_HW;
+               break;
+       case INSN_OBSOLETE:
+               insn->current_mode = INSN_UNDEF;
+               insn->max = INSN_EMULATE;
+               break;
+       }
+
+       raw_spin_lock_irqsave(&insn_emulation_lock, flags);
+       list_add(&insn->node, &insn_emulation);
+       nr_insn_emulated++;
+       raw_spin_unlock_irqrestore(&insn_emulation_lock, flags);
+
+       /* Register any handlers if required */
+       update_insn_emulation_mode(insn, INSN_UNDEF);
+}
+
+static int emulation_proc_handler(struct ctl_table *table, int write,
+                                 void __user *buffer, size_t *lenp,
+                                 loff_t *ppos)
+{
+       int ret = 0;
+       struct insn_emulation *insn = (struct insn_emulation *) table->data;
+       enum insn_emulation_mode prev_mode = insn->current_mode;
+
+       table->data = &insn->current_mode;
+       ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
+
+       if (ret || !write || prev_mode == insn->current_mode)
+               goto ret;
+
+       ret = update_insn_emulation_mode(insn, prev_mode);

After this instruction, something like below to restore the previous
mode:

	if (!ret) {
		enum insn_emulation mode = insn_mode->current_mode;
		insn->current_mode = prev_mode;
		update_insn_emulation_mode(insn, prev_mode);
	}

I've added code similar to your suggestion to revert to previous mode.

I'll post a new version with your's (and Steven's) comments addressed
soon.

Thanks,
Punit

--
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel