[PATCH 0/6] Legacy instruction emulation for arm64

[PATCH 0/6] Legacy instruction emulation for arm64

[Punit Agrawal]

Hi,

This patchset adds support for the emulation of certain legacy
instructions from ARMv7 to the v8 port of Linux. More specifically the
patches add support for SWP and CP15 barrier instructions emulation.

Patch 1/6 fixes a bug in the SWP emulation in v7 where it incorrectly
assumes that SWP instruction implies barriers.

Patch 2/6 creates arm64 debugfs directory followed by 3-4/6 which add
infrastructure code to add support for undefined instruction hooks and
decoding condition codes.

Patch 5/6 adds support for SWP while 6/6 introduces CP15 barrier
instructions emulation. The emulation can be individually enabled as
well as there is support to disable this at runtime via debugfs.

All feedback welcome.

Thanks,
Punit

Punit Agrawal (6):
  arm: Fix in-correct barrier usage in SWP{B} emulation
  arm64: Create arch debugfs directory
  arm64: Add support for hooks to handle undefined instructions
  arm64: Add AArch32 instruction set condition code checks
  arm64: Port SWP/SWPB emulation support from arm
  arm64: Emulate CP15 Barrier instructions

 arch/arm/kernel/swp_emulate.c    |   15 --
 arch/arm64/Kconfig               |   56 +++++++
 arch/arm64/include/asm/insn.h    |   11 ++
 arch/arm64/include/asm/opcodes.h |    1 +
 arch/arm64/include/asm/traps.h   |   16 ++
 arch/arm64/kernel/Makefile       |    6 +-
 arch/arm64/kernel/insn.c         |   23 +++
 arch/arm64/kernel/setup.c        |   14 ++
 arch/arm64/kernel/traps.c        |   66 ++++++++
 arch/arm64/kernel/v7_obsolete.c  |  332 ++++++++++++++++++++++++++++++++++++++
 10 files changed, 524 insertions(+), 16 deletions(-)
 create mode 100644 arch/arm64/include/asm/opcodes.h
 create mode 100644 arch/arm64/kernel/v7_obsolete.c

-- 
1.7.10.4

[PATCH 1/6] arm: Fix in-correct barrier usage in SWP{B} emulation

[Punit Agrawal]

According to the ARM ARMv7, explicit barriers are necessary when using
synchronisation primitives such as SWP{B}. The use of these
instructions does not automatically imply a barrier and any ordering
requirements by the software must be explicitly expressed with the use
of suitable barriers.

Based on this, remove the barriers from SWP{B} emulation.

Cc: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
---
 arch/arm/kernel/swp_emulate.c |   15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/arch/arm/kernel/swp_emulate.c b/arch/arm/kernel/swp_emulate.c
index b1b8988..74117b5 100644
--- a/arch/arm/kernel/swp_emulate.c
+++ b/arch/arm/kernel/swp_emulate.c
@@ -141,14 +141,6 @@ static int emulate_swpX(unsigned int address, unsigned int *data,
 	while (1) {
 		unsigned long temp;
 
-		/*
-		 * Barrier required between accessing protected resource and
-		 * releasing a lock for it. Legacy code might not have done
-		 * this, and we cannot determine that this is not the case
-		 * being emulated, so insert always.
-		 */
-		smp_mb();
-
 		if (type == TYPE_SWPB)
 			__user_swpb_asm(*data, address, res, temp);
 		else
@@ -161,13 +153,6 @@ static int emulate_swpX(unsigned int address, unsigned int *data,
 	}
 
 	if (res == 0) {
-		/*
-		 * Barrier also required between acquiring a lock for a
-		 * protected resource and accessing the resource. Inserted for
-		 * same reason as above.
-		 */
-		smp_mb();
-
 		if (type == TYPE_SWPB)
 			swpbcounter++;
 		else
-- 
1.7.10.4

[PATCH 1/6] arm: Fix in-correct barrier usage in SWP{B} emulation

[Will Deacon]

On Tue, Aug 26, 2014 at 11:28:45AM +0100, Punit Agrawal wrote:
> According to the ARM ARMv7, explicit barriers are necessary when using
> synchronisation primitives such as SWP{B}. The use of these
> instructions does not automatically imply a barrier and any ordering
> requirements by the software must be explicitly expressed with the use
> of suitable barriers.
> 
> Based on this, remove the barriers from SWP{B} emulation.
> 
> Cc: Russell King <linux@arm.linux.org.uk>
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> ---
>  arch/arm/kernel/swp_emulate.c |   15 ---------------
>  1 file changed, 15 deletions(-)

Acked-by: Will Deacon <will.deacon@arm.com>

Will

[PATCH 1/6] arm: Fix in-correct barrier usage in SWP{B} emulation

[Catalin Marinas]

On Tue, Aug 26, 2014 at 11:28:45AM +0100, Punit Agrawal wrote:
> According to the ARM ARMv7, explicit barriers are necessary when using
> synchronisation primitives such as SWP{B}. The use of these
> instructions does not automatically imply a barrier and any ordering
> requirements by the software must be explicitly expressed with the use
> of suitable barriers.
> 
> Based on this, remove the barriers from SWP{B} emulation.
> 
> Cc: Russell King <linux@arm.linux.org.uk>
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>

This patch is independent to the SWP emulation for arm64 series. Unless
Russell has any objections, you should send it to his patch system (git
send-email to patches at arm.linux.org.uk with "KernelVersion: v3..." after
the commit log; I hope Russell drops the KernelVersion requirement some
day ;)).

-- 
Catalin

[PATCH 1/6] arm: Fix in-correct barrier usage in SWP{B} emulation

[Punit Agrawal]

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

> On Tue, Aug 26, 2014 at 11:28:45AM +0100, Punit Agrawal wrote:
>> According to the ARM ARMv7, explicit barriers are necessary when using
>> synchronisation primitives such as SWP{B}. The use of these
>> instructions does not automatically imply a barrier and any ordering
>> requirements by the software must be explicitly expressed with the use
>> of suitable barriers.
>> 
>> Based on this, remove the barriers from SWP{B} emulation.
>> 
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
>
> This patch is independent to the SWP emulation for arm64 series. Unless
> Russell has any objections, you should send it to his patch system (git
> send-email to patches at arm.linux.org.uk with "KernelVersion: v3..." after
> the commit log; I hope Russell drops the KernelVersion requirement some
> day ;)).

OK. Unless I hear any objections, I'll send the patch to Russell's
system tomorrow.

[PATCH 2/6] arm64: Create arch debugfs directory

[Punit Agrawal]

Create an arm64 directory in debugfs as a container for the
arch related debug entries. The patch provides the arch_debugfs_dir
prototype expected by core layers.

Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
---
 arch/arm64/kernel/setup.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 7ec7846..cf4cec0 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -17,6 +17,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <linux/debugfs.h>
 #include <linux/export.h>
 #include <linux/kernel.h>
 #include <linux/stddef.h>
@@ -398,6 +399,19 @@ static int __init arm64_device_init(void)
 }
 arch_initcall_sync(arm64_device_init);
 
+struct dentry *arch_debugfs_dir;
+EXPORT_SYMBOL(arch_debugfs_dir);
+
+static int __init arch_kdebugfs_init(void)
+{
+	arch_debugfs_dir = debugfs_create_dir("arm64", NULL);
+	if (IS_ERR_OR_NULL(arch_debugfs_dir))
+		return -ENOMEM;
+
+	return 0;
+}
+arch_initcall(arch_kdebugfs_init);
+
 static DEFINE_PER_CPU(struct cpu, cpu_data);
 
 static int __init topology_init(void)
-- 
1.7.10.4

[PATCH 3/6] arm64: Add support for hooks to handle undefined instructions

[Punit Agrawal]

Add support to register hooks for undefined instructions. The handlers
will be called when the undefined instruction and the processor state
(as contained in pstate) match criteria used at registration.

Note: The patch only deals with ARM instruction encodings and needs
fixing to handle thumb instructions as well.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
---
 arch/arm64/include/asm/traps.h |   16 ++++++++++
 arch/arm64/kernel/traps.c      |   66 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 82 insertions(+)

diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h
index 10ca8ff..4faaf03 100644
--- a/arch/arm64/include/asm/traps.h
+++ b/arch/arm64/include/asm/traps.h
@@ -18,6 +18,22 @@
 #ifndef __ASM_TRAP_H
 #define __ASM_TRAP_H
 
+#include <linux/list.h>
+
+struct pt_regs;
+
+struct undef_hook {
+	struct list_head node;
+	u32 instr_mask;
+	u32 instr_val;
+	u64 pstate_mask;
+	u64 pstate_val;
+	int (*fn)(struct pt_regs *regs, u32 instr);
+};
+
+int register_undef_hook(struct undef_hook *hook);
+void unregister_undef_hook(struct undef_hook *hook);
+
 static inline int in_exception_text(unsigned long ptr)
 {
 	extern char __exception_text_start[];
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 7ffaddd..6cc8fce 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -257,6 +257,69 @@ void arm64_notify_die(const char *str, struct pt_regs *regs,
 		die(str, regs, err);
 }
 
+static LIST_HEAD(undef_hook);
+static DEFINE_RAW_SPINLOCK(undef_lock);
+
+int register_undef_hook(struct undef_hook *hook)
+{
+	unsigned long flags;
+
+	/*
+	 * We currently don't support handling undefined Thumb mode
+	 * instructions.
+	 */
+	if (hook->pstate_mask & COMPAT_PSR_T_BIT) {
+		pr_warn("Thumb mode not supported for undefined instruction hooks\n");
+		return -EINVAL;
+	}
+
+	raw_spin_lock_irqsave(&undef_lock, flags);
+	list_add(&hook->node, &undef_hook);
+	raw_spin_unlock_irqrestore(&undef_lock, flags);
+
+	return 0;
+}
+
+void unregister_undef_hook(struct undef_hook *hook)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&undef_lock, flags);
+	list_del(&hook->node);
+	raw_spin_unlock_irqrestore(&undef_lock, flags);
+}
+
+static int call_undef_hook(struct pt_regs *regs)
+{
+	struct undef_hook *hook;
+	unsigned long flags;
+	u32 instr;
+	int (*fn)(struct pt_regs *regs, u32 instr) = NULL;
+	void __user *pc = (void __user *)instruction_pointer(regs);
+
+	/*
+	 * Currently, undefined instruction patching is only supported
+	 * for user mode. Also, as we're not emulating any thumb
+	 * instructions lets not add thumb instruction decoding until
+	 * it is needed.
+	 */
+	if (!compat_user_mode(regs) || compat_thumb_mode(regs))
+		return 1;
+
+	get_user(instr, (u32 __user *)pc);
+	instr = le32_to_cpu(instr);
+
+	raw_spin_lock_irqsave(&undef_lock, flags);
+	list_for_each_entry(hook, &undef_hook, node)
+		if ((instr & hook->instr_mask) == hook->instr_val &&
+			(regs->pstate & hook->pstate_mask) == hook->pstate_val)
+			fn = hook->fn;
+
+	raw_spin_unlock_irqrestore(&undef_lock, flags);
+
+	return fn ? fn(regs, instr) : 1;
+}
+
 asmlinkage void __exception do_undefinstr(struct pt_regs *regs)
 {
 	siginfo_t info;
@@ -266,6 +329,9 @@ asmlinkage void __exception do_undefinstr(struct pt_regs *regs)
 	if (!aarch32_break_handler(regs))
 		return;
 
+	if (call_undef_hook(regs) == 0)
+		return;
+
 	if (show_unhandled_signals && unhandled_signal(current, SIGILL) &&
 	    printk_ratelimit()) {
 		pr_info("%s[%d]: undefined instruction: pc=%p\n",
-- 
1.7.10.4

[PATCH 3/6] arm64: Add support for hooks to handle undefined instructions

[Will Deacon]

Hi Punit,

On Tue, Aug 26, 2014 at 11:28:47AM +0100, Punit Agrawal wrote:
> Add support to register hooks for undefined instructions. The handlers
> will be called when the undefined instruction and the processor state
> (as contained in pstate) match criteria used at registration.
> 
> Note: The patch only deals with ARM instruction encodings and needs
> fixing to handle thumb instructions as well.

[...]

> +static int call_undef_hook(struct pt_regs *regs)
> +{
> +	struct undef_hook *hook;
> +	unsigned long flags;
> +	u32 instr;
> +	int (*fn)(struct pt_regs *regs, u32 instr) = NULL;
> +	void __user *pc = (void __user *)instruction_pointer(regs);
> +
> +	/*
> +	 * Currently, undefined instruction patching is only supported
> +	 * for user mode. Also, as we're not emulating any thumb
> +	 * instructions lets not add thumb instruction decoding until
> +	 * it is needed.
> +	 */
> +	if (!compat_user_mode(regs) || compat_thumb_mode(regs))
> +		return 1;

What do you mean by `undefined instruction patching'? I don't see anything
in the mechanism that means this can't be reused for kernel code, then we
just register the SWP emulation hook for userspace only using the mode (like
we do for kgdb).

> +	get_user(instr, (u32 __user *)pc);
> +	instr = le32_to_cpu(instr);
> +
> +	raw_spin_lock_irqsave(&undef_lock, flags);
> +	list_for_each_entry(hook, &undef_hook, node)
> +		if ((instr & hook->instr_mask) == hook->instr_val &&
> +			(regs->pstate & hook->pstate_mask) == hook->pstate_val)
> +			fn = hook->fn;
> +
> +	raw_spin_unlock_irqrestore(&undef_lock, flags);
> +
> +	return fn ? fn(regs, instr) : 1;
> +}
> +
>  asmlinkage void __exception do_undefinstr(struct pt_regs *regs)
>  {
>  	siginfo_t info;
> @@ -266,6 +329,9 @@ asmlinkage void __exception do_undefinstr(struct pt_regs *regs)
>  	if (!aarch32_break_handler(regs))
>  		return;
>  
> +	if (call_undef_hook(regs) == 0)
> +		return;

I'd like to reuse this hook for the aarch32 break hooks (you can see the
direct call in the context above). That means adding support for thumb
after all. Is there a reason you've been avoiding that?

Will

[PATCH 3/6] arm64: Add support for hooks to handle undefined instructions

[Ard Biesheuvel]

On 26 August 2014 15:13, Will Deacon <will.deacon@arm.com> wrote:
> Hi Punit,
>
> On Tue, Aug 26, 2014 at 11:28:47AM +0100, Punit Agrawal wrote:
>> Add support to register hooks for undefined instructions. The handlers
>> will be called when the undefined instruction and the processor state
>> (as contained in pstate) match criteria used at registration.
>>
>> Note: The patch only deals with ARM instruction encodings and needs
>> fixing to handle thumb instructions as well.
>
> [...]
>
>> +static int call_undef_hook(struct pt_regs *regs)
>> +{
>> +     struct undef_hook *hook;
>> +     unsigned long flags;
>> +     u32 instr;
>> +     int (*fn)(struct pt_regs *regs, u32 instr) = NULL;
>> +     void __user *pc = (void __user *)instruction_pointer(regs);
>> +
>> +     /*
>> +      * Currently, undefined instruction patching is only supported
>> +      * for user mode. Also, as we're not emulating any thumb
>> +      * instructions lets not add thumb instruction decoding until
>> +      * it is needed.
>> +      */
>> +     if (!compat_user_mode(regs) || compat_thumb_mode(regs))
>> +             return 1;
>
> What do you mean by `undefined instruction patching'? I don't see anything
> in the mechanism that means this can't be reused for kernel code, then we
> just register the SWP emulation hook for userspace only using the mode (like
> we do for kgdb).
>

You need this patch in order to be able to return from an undef
exception taken in EL1:

--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -287,7 +287,9 @@ el1_undef:
         */
        enable_dbg
        mov     x0, sp
-       b       do_undefinstr
+       bl      do_undefinstr
+
+       kernel_exit 1
 el1_dbg:
        /*
         * Debug exception handling

-- 
Ard.


>> +     get_user(instr, (u32 __user *)pc);
>> +     instr = le32_to_cpu(instr);
>> +
>> +     raw_spin_lock_irqsave(&undef_lock, flags);
>> +     list_for_each_entry(hook, &undef_hook, node)
>> +             if ((instr & hook->instr_mask) == hook->instr_val &&
>> +                     (regs->pstate & hook->pstate_mask) == hook->pstate_val)
>> +                     fn = hook->fn;
>> +
>> +     raw_spin_unlock_irqrestore(&undef_lock, flags);
>> +
>> +     return fn ? fn(regs, instr) : 1;
>> +}
>> +
>>  asmlinkage void __exception do_undefinstr(struct pt_regs *regs)
>>  {
>>       siginfo_t info;
>> @@ -266,6 +329,9 @@ asmlinkage void __exception do_undefinstr(struct pt_regs *regs)
>>       if (!aarch32_break_handler(regs))
>>               return;
>>
>> +     if (call_undef_hook(regs) == 0)
>> +             return;
>
> I'd like to reuse this hook for the aarch32 break hooks (you can see the
> direct call in the context above). That means adding support for thumb
> after all. Is there a reason you've been avoiding that?
>
> Will
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/6] arm64: Add support for hooks to handle undefined instructions

[Will Deacon]

On Tue, Aug 26, 2014 at 03:21:09PM +0100, Ard Biesheuvel wrote:
> On 26 August 2014 15:13, Will Deacon <will.deacon@arm.com> wrote:
> > On Tue, Aug 26, 2014 at 11:28:47AM +0100, Punit Agrawal wrote:
> >> Add support to register hooks for undefined instructions. The handlers
> >> will be called when the undefined instruction and the processor state
> >> (as contained in pstate) match criteria used at registration.
> >>
> >> Note: The patch only deals with ARM instruction encodings and needs
> >> fixing to handle thumb instructions as well.
> >
> > [...]
> >
> >> +static int call_undef_hook(struct pt_regs *regs)
> >> +{
> >> +     struct undef_hook *hook;
> >> +     unsigned long flags;
> >> +     u32 instr;
> >> +     int (*fn)(struct pt_regs *regs, u32 instr) = NULL;
> >> +     void __user *pc = (void __user *)instruction_pointer(regs);
> >> +
> >> +     /*
> >> +      * Currently, undefined instruction patching is only supported
> >> +      * for user mode. Also, as we're not emulating any thumb
> >> +      * instructions lets not add thumb instruction decoding until
> >> +      * it is needed.
> >> +      */
> >> +     if (!compat_user_mode(regs) || compat_thumb_mode(regs))
> >> +             return 1;
> >
> > What do you mean by `undefined instruction patching'? I don't see anything
> > in the mechanism that means this can't be reused for kernel code, then we
> > just register the SWP emulation hook for userspace only using the mode (like
> > we do for kgdb).
> >
> 
> You need this patch in order to be able to return from an undef
> exception taken in EL1:
> 
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -287,7 +287,9 @@ el1_undef:
>          */
>         enable_dbg
>         mov     x0, sp
> -       b       do_undefinstr
> +       bl      do_undefinstr
> +
> +       kernel_exit 1
>  el1_dbg:
>         /*
>          * Debug exception handling

Hmm, I'm surprised we don't already need something like this for KGDB...

Will

[PATCH 3/6] arm64: Add support for hooks to handle undefined instructions

[Catalin Marinas]

On Tue, Aug 26, 2014 at 03:30:11PM +0100, Will Deacon wrote:
> On Tue, Aug 26, 2014 at 03:21:09PM +0100, Ard Biesheuvel wrote:
> > On 26 August 2014 15:13, Will Deacon <will.deacon@arm.com> wrote:
> > > On Tue, Aug 26, 2014 at 11:28:47AM +0100, Punit Agrawal wrote:
> > >> Add support to register hooks for undefined instructions. The handlers
> > >> will be called when the undefined instruction and the processor state
> > >> (as contained in pstate) match criteria used at registration.
> > >>
> > >> Note: The patch only deals with ARM instruction encodings and needs
> > >> fixing to handle thumb instructions as well.
> > >
> > > [...]
> > >
> > >> +static int call_undef_hook(struct pt_regs *regs)
> > >> +{
> > >> +     struct undef_hook *hook;
> > >> +     unsigned long flags;
> > >> +     u32 instr;
> > >> +     int (*fn)(struct pt_regs *regs, u32 instr) = NULL;
> > >> +     void __user *pc = (void __user *)instruction_pointer(regs);
> > >> +
> > >> +     /*
> > >> +      * Currently, undefined instruction patching is only supported
> > >> +      * for user mode. Also, as we're not emulating any thumb
> > >> +      * instructions lets not add thumb instruction decoding until
> > >> +      * it is needed.
> > >> +      */
> > >> +     if (!compat_user_mode(regs) || compat_thumb_mode(regs))
> > >> +             return 1;
> > >
> > > What do you mean by `undefined instruction patching'? I don't see anything
> > > in the mechanism that means this can't be reused for kernel code, then we
> > > just register the SWP emulation hook for userspace only using the mode (like
> > > we do for kgdb).
> > 
> > You need this patch in order to be able to return from an undef
> > exception taken in EL1:
> > 
> > --- a/arch/arm64/kernel/entry.S
> > +++ b/arch/arm64/kernel/entry.S
> > @@ -287,7 +287,9 @@ el1_undef:
> >          */
> >         enable_dbg
> >         mov     x0, sp
> > -       b       do_undefinstr
> > +       bl      do_undefinstr
> > +
> > +       kernel_exit 1
> >  el1_dbg:
> >         /*
> >          * Debug exception handling
> 
> Hmm, I'm surprised we don't already need something like this for KGDB...

We don't expect undef exceptions at EL1, so far they are fatal as we
don't have any hooks for them. Doesn't KGDB use dedicated breakpoint
instructions?

-- 
Catalin

[PATCH 3/6] arm64: Add support for hooks to handle undefined instructions

[Will Deacon]

On Wed, Aug 27, 2014 at 05:47:14PM +0100, Catalin Marinas wrote:
> On Tue, Aug 26, 2014 at 03:30:11PM +0100, Will Deacon wrote:
> > On Tue, Aug 26, 2014 at 03:21:09PM +0100, Ard Biesheuvel wrote:
> > > You need this patch in order to be able to return from an undef
> > > exception taken in EL1:
> > > 
> > > --- a/arch/arm64/kernel/entry.S
> > > +++ b/arch/arm64/kernel/entry.S
> > > @@ -287,7 +287,9 @@ el1_undef:
> > >          */
> > >         enable_dbg
> > >         mov     x0, sp
> > > -       b       do_undefinstr
> > > +       bl      do_undefinstr
> > > +
> > > +       kernel_exit 1
> > >  el1_dbg:
> > >         /*
> > >          * Debug exception handling
> > 
> > Hmm, I'm surprised we don't already need something like this for KGDB...
> 
> We don't expect undef exceptions at EL1, so far they are fatal as we
> don't have any hooks for them. Doesn't KGDB use dedicated breakpoint
> instructions?

Ah yeah, we use magic immediates in the BRK instruction. I was getting
confused with arch/arm/, where we actually use an undefined encoding for
the same thing. That explains why things appear to be working!

Will

[PATCH 3/6] arm64: Add support for hooks to handle undefined instructions

[Punit Agrawal]

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

> Hi Punit,
>
> On Tue, Aug 26, 2014 at 11:28:47AM +0100, Punit Agrawal wrote:
>> Add support to register hooks for undefined instructions. The handlers
>> will be called when the undefined instruction and the processor state
>> (as contained in pstate) match criteria used at registration.
>> 
>> Note: The patch only deals with ARM instruction encodings and needs
>> fixing to handle thumb instructions as well.
>
> [...]
>
>> +static int call_undef_hook(struct pt_regs *regs)
>> +{
>> +	struct undef_hook *hook;
>> +	unsigned long flags;
>> +	u32 instr;
>> +	int (*fn)(struct pt_regs *regs, u32 instr) = NULL;
>> +	void __user *pc = (void __user *)instruction_pointer(regs);
>> +
>> +	/*
>> +	 * Currently, undefined instruction patching is only supported
>> +	 * for user mode. Also, as we're not emulating any thumb
>> +	 * instructions lets not add thumb instruction decoding until
>> +	 * it is needed.
>> +	 */
>> +	if (!compat_user_mode(regs) || compat_thumb_mode(regs))
>> +		return 1;
>
> What do you mean by `undefined instruction patching'? 

That's poorly worded. I'll improve upon it.

> I don't see anything
> in the mechanism that means this can't be reused for kernel code, then we
> just register the SWP emulation hook for userspace only using the mode (like
> we do for kgdb).

There's nothing in the mechanism to prevent it's use for kernel code. I
was erring on the side of caution as I hadn't tested it.

>
>> +	get_user(instr, (u32 __user *)pc);
>> +	instr = le32_to_cpu(instr);
>> +
>> +	raw_spin_lock_irqsave(&undef_lock, flags);
>> +	list_for_each_entry(hook, &undef_hook, node)
>> +		if ((instr & hook->instr_mask) == hook->instr_val &&
>> +			(regs->pstate & hook->pstate_mask) == hook->pstate_val)
>> +			fn = hook->fn;
>> +
>> +	raw_spin_unlock_irqrestore(&undef_lock, flags);
>> +
>> +	return fn ? fn(regs, instr) : 1;
>> +}
>> +
>>  asmlinkage void __exception do_undefinstr(struct pt_regs *regs)
>>  {
>>  	siginfo_t info;
>> @@ -266,6 +329,9 @@ asmlinkage void __exception do_undefinstr(struct pt_regs *regs)
>>  	if (!aarch32_break_handler(regs))
>>  		return;
>>  
>> +	if (call_undef_hook(regs) == 0)
>> +		return;
>
> I'd like to reuse this hook for the aarch32 break hooks (you can see the
> direct call in the context above). That means adding support for thumb
> after all. Is there a reason you've been avoiding that?

None, other than to not add code before it's needed. I've just had a
quick look at break handler and it looks pretty straight forward to fold
that change in this set. Are you OK with that?

The code started out when it wasn't clear if the code would target
upstream kernels. I tried to not enable too many, what was by some
considered, hacks.

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

[PATCH 3/6] arm64: Add support for hooks to handle undefined instructions

[Will Deacon]

On Tue, Aug 26, 2014 at 03:56:56PM +0100, Punit Agrawal wrote:
> Will Deacon <will.deacon@arm.com> writes:
> > On Tue, Aug 26, 2014 at 11:28:47AM +0100, Punit Agrawal wrote:
> >>  asmlinkage void __exception do_undefinstr(struct pt_regs *regs)
> >>  {
> >>  	siginfo_t info;
> >> @@ -266,6 +329,9 @@ asmlinkage void __exception do_undefinstr(struct pt_regs *regs)
> >>  	if (!aarch32_break_handler(regs))
> >>  		return;
> >>  
> >> +	if (call_undef_hook(regs) == 0)
> >> +		return;
> >
> > I'd like to reuse this hook for the aarch32 break hooks (you can see the
> > direct call in the context above). That means adding support for thumb
> > after all. Is there a reason you've been avoiding that?
> 
> None, other than to not add code before it's needed. I've just had a
> quick look at break handler and it looks pretty straight forward to fold
> that change in this set. Are you OK with that?

Just do it as a separate patch on top of this one.

Will

[PATCH 3/6] arm64: Add support for hooks to handle undefined instructions

[Catalin Marinas]

On Tue, Aug 26, 2014 at 03:56:56PM +0100, Punit Agrawal wrote:
> Will Deacon <will.deacon@arm.com> writes:
> > On Tue, Aug 26, 2014 at 11:28:47AM +0100, Punit Agrawal wrote:
> >> Add support to register hooks for undefined instructions. The handlers
> >> will be called when the undefined instruction and the processor state
> >> (as contained in pstate) match criteria used at registration.
> >> 
> >> Note: The patch only deals with ARM instruction encodings and needs
> >> fixing to handle thumb instructions as well.
> >
> > [...]
> >
> >> +static int call_undef_hook(struct pt_regs *regs)
> >> +{
> >> +	struct undef_hook *hook;
> >> +	unsigned long flags;
> >> +	u32 instr;
> >> +	int (*fn)(struct pt_regs *regs, u32 instr) = NULL;
> >> +	void __user *pc = (void __user *)instruction_pointer(regs);
> >> +
> >> +	/*
> >> +	 * Currently, undefined instruction patching is only supported
> >> +	 * for user mode. Also, as we're not emulating any thumb
> >> +	 * instructions lets not add thumb instruction decoding until
> >> +	 * it is needed.
> >> +	 */
> >> +	if (!compat_user_mode(regs) || compat_thumb_mode(regs))
> >> +		return 1;

[...]

> > I don't see anything
> > in the mechanism that means this can't be reused for kernel code, then we
> > just register the SWP emulation hook for userspace only using the mode (like
> > we do for kgdb).
> 
> There's nothing in the mechanism to prevent it's use for kernel code. I
> was erring on the side of caution as I hadn't tested it.

I don't think we even have a guaranteed undefined instruction for
AArch64, so I don't see the point of undef hook for kernel. But we could
implement different entry paths rather than just one do_undefinstr:
do_aarch64_undef, do_aarch32_undef (or maybe kernel/user split where the
kernel one just panics).

-- 
Catalin

[PATCH 4/6] arm64: Add AArch32 instruction set condition code checks

[Punit Agrawal]

Port support for AArch32 instruction condition code checking from arm
to arm64.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
---
 arch/arm64/include/asm/opcodes.h |    1 +
 arch/arm64/kernel/Makefile       |    5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/include/asm/opcodes.h

diff --git a/arch/arm64/include/asm/opcodes.h b/arch/arm64/include/asm/opcodes.h
new file mode 100644
index 0000000..4e603ea
--- /dev/null
+++ b/arch/arm64/include/asm/opcodes.h
@@ -0,0 +1 @@
+#include <../../arm/include/asm/opcodes.h>
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 1f4d891..eee33a0 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -16,7 +16,10 @@ arm64-obj-y		:= cputable.o debug-monitors.o entry.o irq.o fpsimd.o	\
 			   hyp-stub.o psci.o cpu_ops.o insn.o return_address.o
 
 arm64-obj-$(CONFIG_COMPAT)		+= sys32.o kuser32.o signal32.o 	\
-					   sys_compat.o
+					   sys_compat.o 			\
+					   $(addprefix ../../arm/kernel/,	\
+						opcodes.o)
+
 arm64-obj-$(CONFIG_FUNCTION_TRACER)	+= ftrace.o entry-ftrace.o
 arm64-obj-$(CONFIG_MODULES)		+= arm64ksyms.o module.o
 arm64-obj-$(CONFIG_SMP)			+= smp.o smp_spin_table.o topology.o
-- 
1.7.10.4

[PATCH 5/6] arm64: Port SWP/SWPB emulation support from arm

[Punit Agrawal]

The SWP instruction was deprecated in the ARMv6 architecture,
superseded by the LDREX/STREX family of instructions for
load-linked/store-conditional operations. The ARMv7 multiprocessing
extensions mandate that SWP/SWPB instructions are treated as undefined
from reset, with the ability to enable them through the System Control
Register SW bit. With ARMv8, the option to enable these instructions
through System Control Register was dropped as well.

This patch ports the alternative solution to emulate the SWP and SWPB
instructions using LDXR/STXR sequences from the arm port to
arm64. Additionaly, the patch also proivdes support to log the
emulation statistics via debugfs.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
---
 arch/arm64/Kconfig              |   41 +++++++
 arch/arm64/include/asm/insn.h   |    9 ++
 arch/arm64/kernel/Makefile      |    1 +
 arch/arm64/kernel/insn.c        |   11 ++
 arch/arm64/kernel/v7_obsolete.c |  241 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 303 insertions(+)
 create mode 100644 arch/arm64/kernel/v7_obsolete.c

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index c52894e..ba780b1 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -142,6 +142,47 @@ config ARCH_XGENE
 	help
 	  This enables support for AppliedMicro X-Gene SOC Family
 
+comment "Processor Features"
+
+config V7_OBSOLETE
+	bool "Emulate obsolete ARMv7 instructions"
+	depends on COMPAT
+	help
+	  AArch32 legacy software support may require certain
+	  instructions that have been deprecated or obsoleted in the
+	  architecture.
+
+	  Enable this config to enable selective emulation of these
+	  features.
+
+	  If unsure, say N
+
+if V7_OBSOLETE
+
+config SWP_EMULATION
+	bool "Emulate SWP/SWPB instructions"
+	help
+	  ARMv8 obsoletes the use of SWP/SWPB instructions such that
+	  they are always undefined. Say Y here to enable software
+	  emulation of these instructions for userspace (not kernel)
+	  using LDXR/STXR.
+
+	  In some older versions of glibc [<=2.8] SWP is used during futex
+	  trylock() operations with the assumption that the code will not
+	  be preempted. This invalid assumption may be more likely to fail
+	  with SWP emulation enabled, leading to deadlock of the user
+	  application.
+
+	  NOTE: when accessing uncached shared regions, LDXR/STXR rely
+	  on an external transaction monitoring block called a global
+	  monitor to maintain update atomicity. If your system does not
+	  implement a global monitor, this option can cause programs that
+	  perform SWP operations to uncached memory to deadlock.
+
+	  If unsure, say N
+
+endif
+
 endmenu
 
 menu "Bus support"
diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index dc1f73b..2861cc6 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -105,6 +105,15 @@ bool aarch64_insn_hotpatch_safe(u32 old_insn, u32 new_insn);
 int aarch64_insn_patch_text_nosync(void *addr, u32 insn);
 int aarch64_insn_patch_text_sync(void *addrs[], u32 insns[], int cnt);
 int aarch64_insn_patch_text(void *addrs[], u32 insns[], int cnt);
+
+#ifdef CONFIG_V7_OBSOLETE
+#define RN_OFFSET	16
+#define RT_OFFSET	12
+#define RT2_OFFSET	 0
+
+u32 aarch32_insn_extract_reg_num(u32 insn, int offset);
+#endif /* CONFIG_V7_OBSOLETE */
+
 #endif /* __ASSEMBLY__ */
 
 #endif	/* __ASM_INSN_H */
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index eee33a0..2a35a73 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -30,6 +30,7 @@ arm64-obj-$(CONFIG_EARLY_PRINTK)	+= early_printk.o
 arm64-obj-$(CONFIG_ARM64_CPU_SUSPEND)	+= sleep.o suspend.o
 arm64-obj-$(CONFIG_JUMP_LABEL)		+= jump_label.o
 arm64-obj-$(CONFIG_KGDB)		+= kgdb.o
+arm64-obj-$(CONFIG_V7_OBSOLETE)		+= v7_obsolete.o
 
 obj-y					+= $(arm64-obj-y) vdso/
 obj-m					+= $(arm64-obj-m)
diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index 92f3683..5db9d35 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -302,3 +302,14 @@ u32 __kprobes aarch64_insn_gen_nop(void)
 {
 	return aarch64_insn_gen_hint(AARCH64_INSN_HINT_NOP);
 }
+
+#ifdef CONFIG_V7_OBSOLETE
+/*
+ * Macros/defines for extracting register numbers from instruction.
+ */
+u32 aarch32_insn_extract_reg_num(u32 insn, int offset)
+{
+	return (insn & (0xf << offset)) >> offset;
+}
+
+#endif /* CONFIG_V7_OBSOLETE */
diff --git a/arch/arm64/kernel/v7_obsolete.c b/arch/arm64/kernel/v7_obsolete.c
new file mode 100644
index 0000000..e9427cb
--- /dev/null
+++ b/arch/arm64/kernel/v7_obsolete.c
@@ -0,0 +1,241 @@
+/*
+ *  arch/arm64/kernel/v7_obsolete.c
+ *
+ *  Copied from arch/arm/kernel/swp_emulate.c and modified for ARMv8
+ *
+ *  Copyright (C) 2009,2012,2014 ARM Limited
+ *  __user_* functions adapted from include/asm/uaccess.h
+ *
+ * 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/debugfs.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/sched.h>
+#include <linux/perf_event.h>
+
+#include <asm/insn.h>
+#include <asm/opcodes.h>
+#include <asm/system_misc.h>
+#include <asm/traps.h>
+#include <asm/uaccess.h>
+
+/*
+ *  Implements emulation of the SWP/SWPB instructions using load-exclusive and
+ *  store-exclusive.
+ *
+ *  Syntax of SWP{B} instruction: SWP{B}<c> <Rt>, <Rt2>, [<Rn>]
+ *  Where: Rt  = destination
+ *	   Rt2 = source
+ *	   Rn  = address
+ */
+
+
+/*
+ * Error-checking SWP macros implemented using ldxr{b}/stxr{b}
+ */
+#define __user_swpX_asm(data, addr, res, temp, B)		\
+	__asm__ __volatile__(					\
+	"	mov		%w2, %w1\n"			\
+	"0:	ldxr"B"		%w1, [%3]\n"			\
+	"1:	stxr"B"		%w0, %w2, [%3]\n"		\
+	"	cbz		%w0, 2f\n"			\
+	"	mov		%w0, %w4\n"			\
+	"2:\n"							\
+	"	.pushsection	 .fixup,\"ax\"\n"		\
+	"	.align		2\n"				\
+	"3:	mov		%w0, %w5\n"			\
+	"	b		2b\n"				\
+	"	.popsection"					\
+	"	.pushsection	 __ex_table,\"a\"\n"		\
+	"	.align		3\n"				\
+	"	.quad		0b, 3b\n"			\
+	"	.quad		1b, 3b\n"			\
+	"	.popsection"					\
+	: "=&r" (res), "+r" (data), "=&r" (temp)		\
+	: "r" (addr), "i" (-EAGAIN), "i" (-EFAULT)		\
+	: "memory")
+
+#define __user_swp_asm(data, addr, res, temp) \
+	__user_swpX_asm(data, addr, res, temp, "")
+#define __user_swpb_asm(data, addr, res, temp) \
+	__user_swpX_asm(data, addr, res, temp, "b")
+
+/*
+ * Bit 22 of the instruction encoding distinguishes between
+ * the SWP and SWPB variants (bit set means SWPB).
+ */
+#define TYPE_SWPB (1 << 22)
+
+static atomic_t swp_counter;
+static atomic_t swpb_counter;
+static u32 swp_enabled = 1;
+
+/*
+ * Set up process info to signal segmentation fault - called on access error.
+ */
+static void set_segfault(struct pt_regs *regs, unsigned long addr)
+{
+	siginfo_t info;
+
+	down_read(&current->mm->mmap_sem);
+	if (find_vma(current->mm, addr) == NULL)
+		info.si_code = SEGV_MAPERR;
+	else
+		info.si_code = SEGV_ACCERR;
+	up_read(&current->mm->mmap_sem);
+
+	info.si_signo = SIGSEGV;
+	info.si_errno = 0;
+	info.si_addr  = (void *) instruction_pointer(regs);
+
+	pr_debug("SWP{B} emulation: access caused memory abort!\n");
+	arm64_notify_die("Illegal memory access", regs, &info, 0);
+}
+
+static int emulate_swpX(unsigned int address, unsigned int *data,
+			unsigned int type)
+{
+	unsigned int res = 0;
+
+	if ((type != TYPE_SWPB) && (address & 0x3)) {
+		/* SWP to unaligned address not permitted */
+		pr_debug("SWP instruction on unaligned pointer!\n");
+		return -EFAULT;
+	}
+
+	while (1) {
+		unsigned long temp;
+
+		if (type == TYPE_SWPB)
+			__user_swpb_asm(*data, address, res, temp);
+		else
+			__user_swp_asm(*data, address, res, temp);
+
+		if (likely(res != -EAGAIN) || signal_pending(current))
+			break;
+
+		cond_resched();
+	}
+
+	return res;
+}
+
+/*
+ * swp_handler logs the id of calling process, dissects the instruction, sanity
+ * checks the memory location, calls emulate_swpX for the actual operation and
+ * deals with fixup/error handling before returning
+ */
+static int swp_handler(struct pt_regs *regs, u32 instr)
+{
+	u32 address, destreg, data, type;
+	int rn, rt2, res = 0;
+
+	if (!swp_enabled)
+		return -EFAULT;
+
+	perf_sw_event(PERF_COUNT_SW_EMULATION_FAULTS, 1, regs, regs->pc);
+
+	type = instr & TYPE_SWPB;
+
+	switch (arm_check_condition(instr, regs->pstate)) {
+	case ARM_OPCODE_CONDTEST_PASS:
+		break;
+	case ARM_OPCODE_CONDTEST_FAIL:
+		/* Condition failed - return to next instruction */
+		goto ret;
+	case ARM_OPCODE_CONDTEST_UNCOND:
+		/* If unconditional encoding - not a SWP, undef */
+		return -EFAULT;
+	default:
+		return -EINVAL;
+	}
+
+	rn = aarch32_insn_extract_reg_num(instr, RN_OFFSET);
+	rt2 = aarch32_insn_extract_reg_num(instr, RT2_OFFSET);
+
+	address = (u32)regs->user_regs.regs[rn];
+	data	= (u32)regs->user_regs.regs[rt2];
+	destreg = aarch32_insn_extract_reg_num(instr, RT_OFFSET);
+
+	pr_debug("addr in r%d->0x%08x, dest is r%d, source in r%d->0x%08x)\n",
+		rn, address, destreg,
+		aarch32_insn_extract_reg_num(instr, RT2_OFFSET), data);
+
+	/* Check access in reasonable access range for both SWP and SWPB */
+	if (!access_ok(VERIFY_WRITE, (address & ~3), 4)) {
+		pr_debug("SWP{B} emulation: access to 0x%08x not allowed!\n",
+			address);
+		goto fault;
+	}
+
+	res = emulate_swpX(address, &data, type);
+	if (res == -EFAULT)
+		goto fault;
+	else if (res == 0)
+		regs->user_regs.regs[destreg] = data;
+
+ret:
+	if (type == TYPE_SWPB)
+		atomic_inc(&swpb_counter);
+	else
+		atomic_inc(&swp_counter);
+
+	pr_warn_ratelimited("\"%s\" (%ld) uses obsolete SWP{B} instruction at 0x%llx\n",
+			current->comm, (unsigned long)current->pid, regs->pc);
+
+	regs->pc += 4;
+	return 0;
+
+fault:
+	set_segfault(regs, address);
+
+	return 0;
+}
+
+/*
+ * 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_hook = {
+	.instr_mask	= 0x0fb00ff0,
+	.instr_val	= 0x01000090,
+	.pstate_mask	= COMPAT_PSR_MODE_MASK,
+	.pstate_val	= COMPAT_PSR_MODE_USR,
+	.fn		= swp_handler
+};
+
+static void __init swp_emulation_init(void)
+{
+	struct dentry *swp_d;
+
+	atomic_set(&swp_counter, 0);
+	atomic_set(&swpb_counter, 0);
+
+	swp_d = debugfs_create_dir("swp_emulation", arch_debugfs_dir);
+	if (!IS_ERR_OR_NULL(swp_d)) {
+		debugfs_create_atomic_t("swp_count", S_IRUGO, swp_d, &swp_counter);
+		debugfs_create_atomic_t("swpb_count", S_IRUGO, swp_d, &swpb_counter);
+		debugfs_create_bool("enabled", S_IRUGO | S_IWUSR, swp_d,
+				&swp_enabled);
+	}
+
+	if (register_undef_hook(&swp_hook) == 0)
+		pr_notice("Registered SWP/SWPB emulation handler\n");
+}
+
+/*
+ * Invoked as late_initcall, since not needed before init spawned.
+ */
+static int __init v7_obsolete_init(void)
+{
+	if (IS_ENABLED(CONFIG_SWP_EMULATION))
+		swp_emulation_init();
+
+	return 0;
+}
+
+late_initcall(v7_obsolete_init);
-- 
1.7.10.4

[PATCH 5/6] arm64: Port SWP/SWPB emulation support from arm

[Arnd Bergmann]

On Tuesday 26 August 2014 11:28:49 Punit Agrawal wrote:
> 
> This patch ports the alternative solution to emulate the SWP and SWPB
> instructions using LDXR/STXR sequences from the arm port to
> arm64. Additionaly, the patch also proivdes support to log the
> emulation statistics via debugfs.

I'm not sure that putting this into debugfs is a good idea in this
case: while in general that is considered a good place for this
kind of debugging information, we already have a precedent on arm32
for using procfs, and I see no reason to introduce an incompatible
interface for arm64.

You also add an interface for disabling the feature at runtime,
which we don't have on arm32, but that interface is not available
if debugfs is disabled or not mounted. Maybe a sysctl would be
more appropriate? That one could also be shared with arm32.

> +       swp_d = debugfs_create_dir("swp_emulation", arch_debugfs_dir);
> +       if (!IS_ERR_OR_NULL(swp_d)) {

Never use IS_ERR_OR_NULL(). It's enough to check for !swp_d here,
since debugfs_create_dir returns NULL on error. You don't need to
check for debugfs being disabled here because you don't do anything
in the conditional code block if it is disabled. 

	Arnd

[PATCH 5/6] arm64: Port SWP/SWPB emulation support from arm

[Will Deacon]

On Tue, Aug 26, 2014 at 12:32:23PM +0100, Arnd Bergmann wrote:
> On Tuesday 26 August 2014 11:28:49 Punit Agrawal wrote:
> > 
> > This patch ports the alternative solution to emulate the SWP and SWPB
> > instructions using LDXR/STXR sequences from the arm port to
> > arm64. Additionaly, the patch also proivdes support to log the
> > emulation statistics via debugfs.
> 
> I'm not sure that putting this into debugfs is a good idea in this
> case: while in general that is considered a good place for this
> kind of debugging information, we already have a precedent on arm32
> for using procfs, and I see no reason to introduce an incompatible
> interface for arm64.
> 
> You also add an interface for disabling the feature at runtime,
> which we don't have on arm32, but that interface is not available
> if debugfs is disabled or not mounted. Maybe a sysctl would be
> more appropriate? That one could also be shared with arm32.

One advantage of using debugfs is that it provides a place to keep
controls/statistics for any emulations that we add in the future, as opposed
to littering them around in /proc or (worse) having a mixture of the two.

Will

[PATCH 5/6] arm64: Port SWP/SWPB emulation support from arm

[Arnd Bergmann]

On Tuesday 26 August 2014 13:25:43 Will Deacon wrote:
> On Tue, Aug 26, 2014 at 12:32:23PM +0100, Arnd Bergmann wrote:
> > On Tuesday 26 August 2014 11:28:49 Punit Agrawal wrote:
> > > 
> > > This patch ports the alternative solution to emulate the SWP and SWPB
> > > instructions using LDXR/STXR sequences from the arm port to
> > > arm64. Additionaly, the patch also proivdes support to log the
> > > emulation statistics via debugfs.
> > 
> > I'm not sure that putting this into debugfs is a good idea in this
> > case: while in general that is considered a good place for this
> > kind of debugging information, we already have a precedent on arm32
> > for using procfs, and I see no reason to introduce an incompatible
> > interface for arm64.
> > 
> > You also add an interface for disabling the feature at runtime,
> > which we don't have on arm32, but that interface is not available
> > if debugfs is disabled or not mounted. Maybe a sysctl would be
> > more appropriate? That one could also be shared with arm32.
> 
> One advantage of using debugfs is that it provides a place to keep
> controls/statistics for any emulations that we add in the future, as opposed
> to littering them around in /proc or (worse) having a mixture of the two.

Yes, I understood that. I just had another idea: would it make sense to
use a tracepoint rather than a simple counter? That way you can actually
see who is using those instructions with ftrace.

You still wouldn't get the files in the same place as the enable switch
though. The easiest way to implement that switch btw would be a
module_param: It can be passed on the command line (using
swp_emulate.enable=0) or at runtime by writing to
/sys/module/swp_emulate/parameters/enable.

If we do both, there is no longer a need to have any debugfs file logic,
which is also a plus.

	Arnd

[PATCH 5/6] arm64: Port SWP/SWPB emulation support from arm

[Will Deacon]

On Tue, Aug 26, 2014 at 02:26:58PM +0100, Arnd Bergmann wrote:
> On Tuesday 26 August 2014 13:25:43 Will Deacon wrote:
> > On Tue, Aug 26, 2014 at 12:32:23PM +0100, Arnd Bergmann wrote:
> > > On Tuesday 26 August 2014 11:28:49 Punit Agrawal wrote:
> > > > 
> > > > This patch ports the alternative solution to emulate the SWP and SWPB
> > > > instructions using LDXR/STXR sequences from the arm port to
> > > > arm64. Additionaly, the patch also proivdes support to log the
> > > > emulation statistics via debugfs.
> > > 
> > > I'm not sure that putting this into debugfs is a good idea in this
> > > case: while in general that is considered a good place for this
> > > kind of debugging information, we already have a precedent on arm32
> > > for using procfs, and I see no reason to introduce an incompatible
> > > interface for arm64.
> > > 
> > > You also add an interface for disabling the feature at runtime,
> > > which we don't have on arm32, but that interface is not available
> > > if debugfs is disabled or not mounted. Maybe a sysctl would be
> > > more appropriate? That one could also be shared with arm32.
> > 
> > One advantage of using debugfs is that it provides a place to keep
> > controls/statistics for any emulations that we add in the future, as opposed
> > to littering them around in /proc or (worse) having a mixture of the two.
> 
> Yes, I understood that. I just had another idea: would it make sense to
> use a tracepoint rather than a simple counter? That way you can actually
> see who is using those instructions with ftrace.

That would also be useful for perf, where the plain `emulation fault' event
can be a little too broad.

> You still wouldn't get the files in the same place as the enable switch
> though. The easiest way to implement that switch btw would be a
> module_param: It can be passed on the command line (using
> swp_emulate.enable=0) or at runtime by writing to
> /sys/module/swp_emulate/parameters/enable.
> 
> If we do both, there is no longer a need to have any debugfs file logic,
> which is also a plus.

Sounds good to me.

Will

[PATCH 5/6] arm64: Port SWP/SWPB emulation support from arm

[Punit Agrawal]

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

> On Tue, Aug 26, 2014 at 02:26:58PM +0100, Arnd Bergmann wrote:
>> On Tuesday 26 August 2014 13:25:43 Will Deacon wrote:
>> > On Tue, Aug 26, 2014 at 12:32:23PM +0100, Arnd Bergmann wrote:
>> > > On Tuesday 26 August 2014 11:28:49 Punit Agrawal wrote:
>> > > > 
>> > > > This patch ports the alternative solution to emulate the SWP and SWPB
>> > > > instructions using LDXR/STXR sequences from the arm port to
>> > > > arm64. Additionaly, the patch also proivdes support to log the
>> > > > emulation statistics via debugfs.
>> > > 
>> > > I'm not sure that putting this into debugfs is a good idea in this
>> > > case: while in general that is considered a good place for this
>> > > kind of debugging information, we already have a precedent on arm32
>> > > for using procfs, and I see no reason to introduce an incompatible
>> > > interface for arm64.
>> > > 
>> > > You also add an interface for disabling the feature at runtime,
>> > > which we don't have on arm32, but that interface is not available
>> > > if debugfs is disabled or not mounted. Maybe a sysctl would be
>> > > more appropriate? That one could also be shared with arm32.
>> > 
>> > One advantage of using debugfs is that it provides a place to keep
>> > controls/statistics for any emulations that we add in the future, as opposed
>> > to littering them around in /proc or (worse) having a mixture of the two.
>> 
>> Yes, I understood that. I just had another idea: would it make sense to
>> use a tracepoint rather than a simple counter? That way you can actually
>> see who is using those instructions with ftrace.
>
> That would also be useful for perf, where the plain `emulation fault' event
> can be a little too broad.

I'll replace the counters with trace points.

There is still the pr_warn which informs the user about applications
using legacy instructions. Hopefully, this should encourage updating the
software.

>
>> You still wouldn't get the files in the same place as the enable switch
>> though. The easiest way to implement that switch btw would be a
>> module_param: It can be passed on the command line (using
>> swp_emulate.enable=0) or at runtime by writing to
>> /sys/module/swp_emulate/parameters/enable.
>> 
>> If we do both, there is no longer a need to have any debugfs file logic,
>> which is also a plus.
>
> Sounds good to me.

Just a note: instead of being 'swp_emulate.enable=0' this'll be
'v7_obsolete.swp_emulate=0' and correspondingly for the other features.

I'll include these changes in the next version.

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

[PATCH 5/6] arm64: Port SWP/SWPB emulation support from arm

[Arnd Bergmann]

On Wednesday 27 August 2014, Punit Agrawal wrote:
> I'll replace the counters with trace points.
> 
> There is still the pr_warn which informs the user about applications
> using legacy instructions. Hopefully, this should encourage updating the
> software.

pr_warn_ratelimit() please. There is no point printing this all the time
if nobody reads the messages.

> >> If we do both, there is no longer a need to have any debugfs file logic,
> >> which is also a plus.
> >
> > Sounds good to me.
> 
> Just a note: instead of being 'swp_emulate.enable=0' this'll be
> 'v7_obsolete.swp_emulate=0' and correspondingly for the other features.

It would be nice if the module name could be the same for arm32 and arm64,
and I don't know if we want to rename swp_emulate.c to v7_obsolete.c
on arm32.

Other than that, I have no opinion on the specific name of the module
or the option.

	Arnd

[PATCH 5/6] arm64: Port SWP/SWPB emulation support from arm

[Punit Agrawal]

Arnd Bergmann <arnd@arndb.de> writes:

> On Wednesday 27 August 2014, Punit Agrawal wrote:
>> I'll replace the counters with trace points.
>> 
>> There is still the pr_warn which informs the user about applications
>> using legacy instructions. Hopefully, this should encourage updating the
>> software.
>
> pr_warn_ratelimit() please. There is no point printing this all the time
> if nobody reads the messages.

Agreed. The patch indeed uses pr_warn_ratelimit().

>
>> >> If we do both, there is no longer a need to have any debugfs file logic,
>> >> which is also a plus.
>> >
>> > Sounds good to me.
>> 
>> Just a note: instead of being 'swp_emulate.enable=0' this'll be
>> 'v7_obsolete.swp_emulate=0' and correspondingly for the other features.
>
> It would be nice if the module name could be the same for arm32 and arm64,
> and I don't know if we want to rename swp_emulate.c to v7_obsolete.c
> on arm32.

Catalin was suggesting renaming v7_obsolete.c, so I'll wait to see what
is the outcome there. Calling it swp_emulate.c feels wrong as in the
subsequent patch we add CP15 barrier emulation to it.

>
> Other than that, I have no opinion on the specific name of the module
> or the option.
>
> 	Arnd
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/6] arm64: Port SWP/SWPB emulation support from arm

[Catalin Marinas]

On Tue, Aug 26, 2014 at 11:28:49AM +0100, Punit Agrawal wrote:
> The SWP instruction was deprecated in the ARMv6 architecture,
> superseded by the LDREX/STREX family of instructions for
> load-linked/store-conditional operations. The ARMv7 multiprocessing
> extensions mandate that SWP/SWPB instructions are treated as undefined
> from reset, with the ability to enable them through the System Control
> Register SW bit. With ARMv8, the option to enable these instructions
> through System Control Register was dropped as well.
> 
> This patch ports the alternative solution to emulate the SWP and SWPB
> instructions using LDXR/STXR sequences from the arm port to
> arm64. Additionaly, the patch also proivdes support to log the

s/proivdes/provides/

> emulation statistics via debugfs.
> 
> Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
> ---
>  arch/arm64/Kconfig              |   41 +++++++
>  arch/arm64/include/asm/insn.h   |    9 ++
>  arch/arm64/kernel/Makefile      |    1 +
>  arch/arm64/kernel/insn.c        |   11 ++
>  arch/arm64/kernel/v7_obsolete.c |  241 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 303 insertions(+)
>  create mode 100644 arch/arm64/kernel/v7_obsolete.c
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index c52894e..ba780b1 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -142,6 +142,47 @@ config ARCH_XGENE
>  	help
>  	  This enables support for AppliedMicro X-Gene SOC Family
>  
> +comment "Processor Features"
> +
> +config V7_OBSOLETE

We could place this under a sub-menu (I haven't checked how it would
look like).

> +	bool "Emulate obsolete ARMv7 instructions"
> +	depends on COMPAT
> +	help
> +	  AArch32 legacy software support may require certain
> +	  instructions that have been deprecated or obsoleted in the
> +	  architecture.
> +
> +	  Enable this config to enable selective emulation of these
> +	  features.
> +
> +	  If unsure, say N

I think we need to get some terminology right:

deprecated:	no longer recommended, usually with a configuration
		option to be enabled/disabled
obsolete:	no longer present

So what we are adding here is deprecated ARMv7 features that have been
obsolete in ARMv8. You could as well say "Emulate deprecated/obsolete
ARMv8 A32 instructions". "Deprecated ARMv7..." could work but we may
want to add options for features that haven't been deprecated in ARMv7,
just ARMv8 A32.

You could also change the config option (and file/function names) for
consistency to something like a32_obsolete (for config option, maybe
CONFIG_ARM64_A32_OBSOLETE, it gets even more confusing ;)).

> +config SWP_EMULATION
> +	bool "Emulate SWP/SWPB instructions"
> +	help
> +	  ARMv8 obsoletes the use of SWP/SWPB instructions such that

"... the use of A32 SWP/SWPB" to be clearer.

> +	  they are always undefined. Say Y here to enable software
> +	  emulation of these instructions for userspace (not kernel)
> +	  using LDXR/STXR.

No need to say "not kernel" since the kernel uses the A64 ISA.

> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index dc1f73b..2861cc6 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -105,6 +105,15 @@ bool aarch64_insn_hotpatch_safe(u32 old_insn, u32 new_insn);
>  int aarch64_insn_patch_text_nosync(void *addr, u32 insn);
>  int aarch64_insn_patch_text_sync(void *addrs[], u32 insns[], int cnt);
>  int aarch64_insn_patch_text(void *addrs[], u32 insns[], int cnt);
> +
> +#ifdef CONFIG_V7_OBSOLETE
> +#define RN_OFFSET	16
> +#define RT_OFFSET	12
> +#define RT2_OFFSET	 0
> +
> +u32 aarch32_insn_extract_reg_num(u32 insn, int offset);
> +#endif /* CONFIG_V7_OBSOLETE */
> +
>  #endif /* __ASSEMBLY__ */
>  
>  #endif	/* __ASM_INSN_H */

[...]

> --- a/arch/arm64/kernel/insn.c
> +++ b/arch/arm64/kernel/insn.c
> @@ -302,3 +302,14 @@ u32 __kprobes aarch64_insn_gen_nop(void)
>  {
>  	return aarch64_insn_gen_hint(AARCH64_INSN_HINT_NOP);
>  }
> +
> +#ifdef CONFIG_V7_OBSOLETE
> +/*
> + * Macros/defines for extracting register numbers from instruction.
> + */
> +u32 aarch32_insn_extract_reg_num(u32 insn, int offset)
> +{
> +	return (insn & (0xf << offset)) >> offset;
> +}
> +
> +#endif /* CONFIG_V7_OBSOLETE */

I don't think we need to bother with an #ifdef here and in the header.
The code is small enough to affect the compilation time and the linker
should get rid of it if not used.

-- 
Catalin

[PATCH 6/6] arm64: Emulate CP15 Barrier instructions

[Punit Agrawal]

The CP15 barrier instructions (CP15ISB, CP15DSB and CP15DMB) are
deprecated in the ARMv7 architecture, superseded by ISB, DSB and DMB
instructions respectively. Some implementations may provide support
for these instructions which can then be enabled by setting the
CP15BEN bit in the SCTLR in ARMv7 or SCTLR_EL1 in ARMv8. If not
enabled, the encodings for these instructions become undefined.

To support legacy software that uses these instructions, this patch
emulates the barrier instructions by installing an undefined
instruction handler. The patch also adds a debugfs entry to track
the occurrence of the deprecated barrier instructions. It is possible
to runtime disable them from debugfs.

Signed-off-by: Punit Agrawal <punit.agrawal@arm.com>
---
 arch/arm64/Kconfig              |   15 +++++++
 arch/arm64/include/asm/insn.h   |    2 +
 arch/arm64/kernel/insn.c        |   12 ++++++
 arch/arm64/kernel/v7_obsolete.c |   91 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 120 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index ba780b1..135c15f 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -181,6 +181,21 @@ config SWP_EMULATION
 
 	  If unsure, say N
 
+config CP15_BARRIER_EMULATION
+	bool "Emulate CP15 Barrier instructions"
+	help
+	  ARMv7 architecture deprecates the use of CP15 barrier
+	  instructions - CP15ISB, CP15DSB and CP15DMB. It is strongly
+	  recommended to use the ISB, DSB and DMB instructions
+	  instead.
+
+	  Say Y here to enable software emulation of these
+	  instructions for AArch32 userspace code. When emulation
+	  is enabled, statistics related to the occurrence of these
+	  instructions are also made available via debugfs.
+
+	  If unsure, say N
+
 endif
 
 endmenu
diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 2861cc6..165a8dd 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -112,6 +112,8 @@ int aarch64_insn_patch_text(void *addrs[], u32 insns[], int cnt);
 #define RT2_OFFSET	 0
 
 u32 aarch32_insn_extract_reg_num(u32 insn, int offset);
+u32 aarch32_insn_mcr_extract_opc2(u32 insn);
+u32 aarch32_insn_mcr_extract_crm(u32 insn);
 #endif /* CONFIG_V7_OBSOLETE */
 
 #endif /* __ASSEMBLY__ */
diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
index 5db9d35..10c6104 100644
--- a/arch/arm64/kernel/insn.c
+++ b/arch/arm64/kernel/insn.c
@@ -312,4 +312,16 @@ u32 aarch32_insn_extract_reg_num(u32 insn, int offset)
 	return (insn & (0xf << offset)) >> offset;
 }
 
+#define OPC2_MASK	0x7
+#define OPC2_OFFSET	5
+u32 aarch32_insn_mcr_extract_opc2(u32 insn)
+{
+	return (insn & (OPC2_MASK << OPC2_OFFSET)) >> OPC2_OFFSET;
+}
+
+#define CRM_MASK	0xf
+u32 aarch32_insn_mcr_extract_crm(u32 insn)
+{
+	return insn & CRM_MASK;
+}
 #endif /* CONFIG_V7_OBSOLETE */
diff --git a/arch/arm64/kernel/v7_obsolete.c b/arch/arm64/kernel/v7_obsolete.c
index e9427cb..ed77889 100644
--- a/arch/arm64/kernel/v7_obsolete.c
+++ b/arch/arm64/kernel/v7_obsolete.c
@@ -227,6 +227,94 @@ static void __init swp_emulation_init(void)
 		pr_notice("Registered SWP/SWPB emulation handler\n");
 }
 
+static atomic_t cp15_barrier_count;
+static u32 cp15_barrier_enabled = 1;
+
+static int cp15barrier_handler(struct pt_regs *regs, u32 instr)
+{
+	if (!cp15_barrier_enabled)
+		return -EFAULT;
+
+	perf_sw_event(PERF_COUNT_SW_EMULATION_FAULTS, 1, regs, regs->pc);
+
+	switch (arm_check_condition(instr, regs->pstate)) {
+	case ARM_OPCODE_CONDTEST_PASS:
+		break;
+	case ARM_OPCODE_CONDTEST_FAIL:
+		/* Condition failed - return to next instruction */
+		goto ret;
+	case ARM_OPCODE_CONDTEST_UNCOND:
+		/* If unconditional encoding - not a barrier instruction */
+		return -EFAULT;
+	default:
+		return -EINVAL;
+	}
+
+	switch(aarch32_insn_mcr_extract_crm(instr)) {
+	case 10:
+		/*
+		 * dmb - mcr p15, 0, Rt, c7, c10, 5
+		 * dsb - mcr p15, 0, Rt, c7, c10, 4
+		 */
+		if (aarch32_insn_mcr_extract_opc2(instr) == 5)
+			dmb();
+		else
+			dsb();
+		break;
+	case 5:
+		/*
+		 * isb - mcr p15, 0, Rt, c7, c5, 4
+		 */
+		isb();
+		break;
+	}
+
+ret:
+	atomic_inc(&cp15_barrier_count);
+	pr_warn_ratelimited("\"%s\" (%ld) uses deprecated CP15 Barrier instruction at 0x%llx\n",
+			current->comm, (unsigned long)current->pid, regs->pc);
+
+	regs->pc += 4;
+	return 0;
+}
+
+/* data barrier */
+static struct undef_hook cp15db_hook = {
+	.instr_mask	= 0x0fff0fdf,
+	.instr_val	= 0x0e070f9a,
+	.pstate_mask	= COMPAT_PSR_MODE_MASK,
+	.pstate_val	= COMPAT_PSR_MODE_USR,
+	.fn		= cp15barrier_handler,
+};
+
+/* instruction barrier */
+static struct undef_hook cp15isb_hook = {
+	.instr_mask	= 0x0fff0fff,
+	.instr_val	= 0x0e070f95,
+	.pstate_mask	= COMPAT_PSR_MODE_MASK,
+	.pstate_val	= COMPAT_PSR_MODE_USR,
+	.fn		= cp15barrier_handler,
+};
+
+static void __init cp15_barrier_emulation_init(void)
+{
+	struct dentry *cp15_d;
+
+	atomic_set(&cp15_barrier_count, 0);
+
+	cp15_d = debugfs_create_dir("cp15_barrier_emulation", arch_debugfs_dir);
+	if (!IS_ERR_OR_NULL(cp15_d)) {
+		debugfs_create_atomic_t("cp15_barrier_count", S_IRUGO, cp15_d,
+				&cp15_barrier_count);
+		debugfs_create_bool("enabled", S_IRUGO | S_IWUSR, cp15_d,
+				&cp15_barrier_enabled);
+	}
+
+	if (register_undef_hook(&cp15db_hook) == 0 &&
+		register_undef_hook(&cp15isb_hook) == 0)
+		pr_notice("Registered CP15 Barrier emulation handler\n");
+}
+
 /*
  * Invoked as late_initcall, since not needed before init spawned.
  */
@@ -235,6 +323,9 @@ static int __init v7_obsolete_init(void)
 	if (IS_ENABLED(CONFIG_SWP_EMULATION))
 		swp_emulation_init();
 
+	if (IS_ENABLED(CONFIG_CP15_BARRIER_EMULATION))
+		cp15_barrier_emulation_init();
+
 	return 0;
 }
 
-- 
1.7.10.4

[PATCH 6/6] arm64: Emulate CP15 Barrier instructions

[Will Deacon]

On Tue, Aug 26, 2014 at 11:28:50AM +0100, Punit Agrawal wrote:
> The CP15 barrier instructions (CP15ISB, CP15DSB and CP15DMB) are
> deprecated in the ARMv7 architecture, superseded by ISB, DSB and DMB
> instructions respectively. Some implementations may provide support
> for these instructions which can then be enabled by setting the
> CP15BEN bit in the SCTLR in ARMv7 or SCTLR_EL1 in ARMv8. If not
> enabled, the encodings for these instructions become undefined.
> 
> To support legacy software that uses these instructions, this patch
> emulates the barrier instructions by installing an undefined
> instruction handler. The patch also adds a debugfs entry to track
> the occurrence of the deprecated barrier instructions. It is possible
> to runtime disable them from debugfs.

[...]

> diff --git a/arch/arm64/kernel/v7_obsolete.c b/arch/arm64/kernel/v7_obsolete.c
> index e9427cb..ed77889 100644
> --- a/arch/arm64/kernel/v7_obsolete.c
> +++ b/arch/arm64/kernel/v7_obsolete.c
> @@ -227,6 +227,94 @@ static void __init swp_emulation_init(void)
>  		pr_notice("Registered SWP/SWPB emulation handler\n");
>  }
>  
> +static atomic_t cp15_barrier_count;
> +static u32 cp15_barrier_enabled = 1;

Stupid question, but how this variable get updated?

> +
> +static int cp15barrier_handler(struct pt_regs *regs, u32 instr)
> +{
> +	if (!cp15_barrier_enabled)
> +		return -EFAULT;
> +
> +	perf_sw_event(PERF_COUNT_SW_EMULATION_FAULTS, 1, regs, regs->pc);
> +
> +	switch (arm_check_condition(instr, regs->pstate)) {
> +	case ARM_OPCODE_CONDTEST_PASS:
> +		break;
> +	case ARM_OPCODE_CONDTEST_FAIL:
> +		/* Condition failed - return to next instruction */
> +		goto ret;
> +	case ARM_OPCODE_CONDTEST_UNCOND:
> +		/* If unconditional encoding - not a barrier instruction */
> +		return -EFAULT;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	switch(aarch32_insn_mcr_extract_crm(instr)) {
> +	case 10:
> +		/*
> +		 * dmb - mcr p15, 0, Rt, c7, c10, 5
> +		 * dsb - mcr p15, 0, Rt, c7, c10, 4
> +		 */
> +		if (aarch32_insn_mcr_extract_opc2(instr) == 5)
> +			dmb();
> +		else
> +			dsb();

It would be cleaner to check for the value 4 here.

> +		break;
> +	case 5:
> +		/*
> +		 * isb - mcr p15, 0, Rt, c7, c5, 4
> +		 */
> +		isb();

You don't need this isb() -- it is implicit in the exception return. A
comment to that effect will suffice.

Will

[PATCH 6/6] arm64: Emulate CP15 Barrier instructions

[Catalin Marinas]

On Tue, Aug 26, 2014 at 11:28:50AM +0100, Punit Agrawal wrote:
> diff --git a/arch/arm64/kernel/v7_obsolete.c b/arch/arm64/kernel/v7_obsolete.c
> index e9427cb..ed77889 100644
> --- a/arch/arm64/kernel/v7_obsolete.c
> +++ b/arch/arm64/kernel/v7_obsolete.c
> @@ -227,6 +227,94 @@ static void __init swp_emulation_init(void)
>  		pr_notice("Registered SWP/SWPB emulation handler\n");
>  }
>  
> +static atomic_t cp15_barrier_count;

Should we add counters for each barrier type? It may be more
informative.

> +/* data barrier */
> +static struct undef_hook cp15db_hook = {
> +	.instr_mask	= 0x0fff0fdf,
> +	.instr_val	= 0x0e070f9a,
> +	.pstate_mask	= COMPAT_PSR_MODE_MASK,
> +	.pstate_val	= COMPAT_PSR_MODE_USR,
> +	.fn		= cp15barrier_handler,
> +};
> +
> +/* instruction barrier */
> +static struct undef_hook cp15isb_hook = {
> +	.instr_mask	= 0x0fff0fff,
> +	.instr_val	= 0x0e070f95,
> +	.pstate_mask	= COMPAT_PSR_MODE_MASK,
> +	.pstate_val	= COMPAT_PSR_MODE_USR,
> +	.fn		= cp15barrier_handler,
> +};

It now crossed my mind that these CP15 barriers are valid in Thumb-2 as
well, same encoding. But we need the hook and the masks here to trap
them.

-- 
Catalin

[PATCH 6/6] arm64: Emulate CP15 Barrier instructions

[Punit Agrawal]

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

> On Tue, Aug 26, 2014 at 11:28:50AM +0100, Punit Agrawal wrote:
>> diff --git a/arch/arm64/kernel/v7_obsolete.c b/arch/arm64/kernel/v7_obsolete.c
>> index e9427cb..ed77889 100644
>> --- a/arch/arm64/kernel/v7_obsolete.c
>> +++ b/arch/arm64/kernel/v7_obsolete.c
>> @@ -227,6 +227,94 @@ static void __init swp_emulation_init(void)
>>  		pr_notice("Registered SWP/SWPB emulation handler\n");
>>  }
>>  
>> +static atomic_t cp15_barrier_count;
>
> Should we add counters for each barrier type? It may be more
> informative.

Arnd proposed to use trace points instead of counters. I can emit
different trace points for the different barrier types.

I am not sure if there is any benefit in providing this extra
information. IIUC, the stats being presented (whether they be via
procfs, debugfs or ftrace) are intended to highglight the presence of
legacy instructions and encourage migration away from using these
features.

But I am happy to defer to your judgement if you see the benefit.

>
>> +/* data barrier */
>> +static struct undef_hook cp15db_hook = {
>> +	.instr_mask	= 0x0fff0fdf,
>> +	.instr_val	= 0x0e070f9a,
>> +	.pstate_mask	= COMPAT_PSR_MODE_MASK,
>> +	.pstate_val	= COMPAT_PSR_MODE_USR,
>> +	.fn		= cp15barrier_handler,
>> +};
>> +
>> +/* instruction barrier */
>> +static struct undef_hook cp15isb_hook = {
>> +	.instr_mask	= 0x0fff0fff,
>> +	.instr_val	= 0x0e070f95,
>> +	.pstate_mask	= COMPAT_PSR_MODE_MASK,
>> +	.pstate_val	= COMPAT_PSR_MODE_USR,
>> +	.fn		= cp15barrier_handler,
>> +};
>
> It now crossed my mind that these CP15 barriers are valid in Thumb-2 as
> well, same encoding. But we need the hook and the masks here to trap
> them.

Right. I'll add the masks. This coupled with the decoding of
instructions in thumb mode for undef exception, should take care of CP15
barriers in Thumb-2.

[PATCH 6/6] arm64: Emulate CP15 Barrier instructions

[Catalin Marinas]

On Thu, Aug 28, 2014 at 10:34:58AM +0100, Punit Agrawal wrote:
> Catalin Marinas <catalin.marinas@arm.com> writes:
> 
> > On Tue, Aug 26, 2014 at 11:28:50AM +0100, Punit Agrawal wrote:
> >> diff --git a/arch/arm64/kernel/v7_obsolete.c b/arch/arm64/kernel/v7_obsolete.c
> >> index e9427cb..ed77889 100644
> >> --- a/arch/arm64/kernel/v7_obsolete.c
> >> +++ b/arch/arm64/kernel/v7_obsolete.c
> >> @@ -227,6 +227,94 @@ static void __init swp_emulation_init(void)
> >>  		pr_notice("Registered SWP/SWPB emulation handler\n");
> >>  }
> >>  
> >> +static atomic_t cp15_barrier_count;
> >
> > Should we add counters for each barrier type? It may be more
> > informative.
> 
> Arnd proposed to use trace points instead of counters. I can emit
> different trace points for the different barrier types.

This would work as well.

-- 
Catalin