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

[Catalin Marinas]

On Wed, Nov 12, 2014 at 11:44:50AM +0000, Punit Agrawal wrote:
>  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.

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

> +/*
> + * 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.

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

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

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.

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

Same here.

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

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

> +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);
	}

--
Catalin