Subject: Re: [PATCH v10 4/9] arm64: add conditional instruction simulation
 support
To: David Long <dave.long@linaro.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will.deacon@arm.com>,
        Sandeepa Prabhu <sandeepa.s.prabhu@gmail.com>,
        William Cohen <wcohen@redhat.com>, Pratyush Anand <panand@redhat.com>,
        Steve Capper <steve.capper@linaro.org>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
References: <1456801047-29014-1-git-send-email-dave.long@linaro.org>
 <1456801047-29014-5-git-send-email-dave.long@linaro.org>
Cc: Dave P Martin <Dave.Martin@arm.com>, Mark Rutland <mark.rutland@arm.com>,
        Robin Murphy <Robin.Murphy@arm.com>,
        Ard Biesheuvel <ard.biesheuvel@linaro.org>,
        Jens Wiklander <jens.wiklander@linaro.org>,
        Christoffer Dall <christoffer.dall@linaro.org>,
        =?UTF-8?Q?Alex_Benn=c3=a9e?= <alex.bennee@linaro.org>,
        Yang Shi <yang.shi@linaro.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        "Suzuki K. Poulose" <suzuki.poulose@arm.com>,
        Kees Cook <keescook@chromium.org>, Zi Shen Lim <zlim.lnx@gmail.com>,
        John Blackwood <john.blackwood@ccur.com>, Feng Kan <fkan@apm.com>,
        Balamurugan Shanmugam <bshanmugam@apm.com>,
        James Morse <james.morse@arm.com>,
        Vladimir Murzin <Vladimir.Murzin@arm.com>,
        Mark Salyzyn <salyzyn@android.com>, Petr Mladek <pmladek@suse.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Mark Brown <broonie@kernel.org>
From: Marc Zyngier <marc.zyngier@arm.com>
Organization: ARM Ltd
Message-ID: <56D5D4CC.8010808@arm.com>
Date: Tue, 1 Mar 2016 17:43:40 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Icedove/38.5.0
MIME-Version: 1.0
In-Reply-To: <1456801047-29014-5-git-send-email-dave.long@linaro.org>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6780
Lines: 212

On 01/03/16 02:57, David Long wrote:
> From: "David A. Long" <dave.long@linaro.org>
> 
> Cease using the arm32 arm_check_condition() function and replace it with
> a local version for use in deprecated instruction support on arm64. Also
> make the function table used by this available for future use by kprobes
> and/or uprobes.
> 
> This function is dervied from code written by Sandeepa Prabhu.
> 
> Signed-off-by: Sandeepa Prabhu <sandeepa.s.prabhu@gmail.com>
> Signed-off-by: David A. Long <dave.long@linaro.org>
> ---
>  arch/arm64/include/asm/insn.h        |  3 ++
>  arch/arm64/kernel/Makefile           |  3 +-
>  arch/arm64/kernel/armv8_deprecated.c | 19 ++++++-
>  arch/arm64/kernel/insn.c             | 96 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 117 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index 662b42a..72dda48 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -405,6 +405,9 @@ u32 aarch64_extract_system_register(u32 insn);
>  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);
> +
> +typedef bool (pstate_check_t)(unsigned long);
> +extern pstate_check_t * const opcode_condition_checks[16];
>  #endif /* __ASSEMBLY__ */
>  
>  #endif	/* __ASM_INSN_H */
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 83cd7e6..fd5f163 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -26,8 +26,7 @@ $(obj)/%.stub.o: $(obj)/%.o FORCE
>  	$(call if_changed,objcopy)
>  
>  arm64-obj-$(CONFIG_COMPAT)		+= sys32.o kuser32.o signal32.o 	\
> -					   sys_compat.o entry32.o		\
> -					   ../../arm/kernel/opcodes.o
> +					   sys_compat.o entry32.o
>  arm64-obj-$(CONFIG_FUNCTION_TRACER)	+= ftrace.o entry-ftrace.o
>  arm64-obj-$(CONFIG_MODULES)		+= arm64ksyms.o module.o
>  arm64-obj-$(CONFIG_PERF_EVENTS)		+= perf_regs.o perf_callchain.o
> diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
> index 3e01207..6d4d6fe 100644
> --- a/arch/arm64/kernel/armv8_deprecated.c
> +++ b/arch/arm64/kernel/armv8_deprecated.c
> @@ -369,6 +369,21 @@ static int emulate_swpX(unsigned int address, unsigned int *data,
>  	return res;
>  }
>  
> +#define	ARM_OPCODE_CONDITION_UNCOND	0xf
> +
> +static unsigned int __kprobes arm64_check_condition(u32 opcode, u32 psr)

Nit: since this is exclusively targeted at emulating 32bit code, having
arm64_ as a prefix is mildly confusing. Either keep the arm_ prefix, or
turn it into arm32_.

> +{
> +	u32 cc_bits  = opcode >> 28;
> +
> +	if (cc_bits != ARM_OPCODE_CONDITION_UNCOND) {
> +		if ((*opcode_condition_checks[cc_bits])(psr))
> +			return ARM_OPCODE_CONDTEST_PASS;
> +		else
> +			return ARM_OPCODE_CONDTEST_FAIL;
> +	}
> +	return ARM_OPCODE_CONDTEST_UNCOND;
> +}
> +
>  /*
>   * swp_handler logs the id of calling process, dissects the instruction, sanity
>   * checks the memory location, calls emulate_swpX for the actual operation and
> @@ -383,7 +398,7 @@ static int swp_handler(struct pt_regs *regs, u32 instr)
>  
>  	type = instr & TYPE_SWPB;
>  
> -	switch (arm_check_condition(instr, regs->pstate)) {
> +	switch (arm64_check_condition(instr, regs->pstate)) {
>  	case ARM_OPCODE_CONDTEST_PASS:
>  		break;
>  	case ARM_OPCODE_CONDTEST_FAIL:
> @@ -464,7 +479,7 @@ static int cp15barrier_handler(struct pt_regs *regs, u32 instr)
>  {
>  	perf_sw_event(PERF_COUNT_SW_EMULATION_FAULTS, 1, regs, regs->pc);
>  
> -	switch (arm_check_condition(instr, regs->pstate)) {
> +	switch (arm64_check_condition(instr, regs->pstate)) {
>  	case ARM_OPCODE_CONDTEST_PASS:
>  		break;
>  	case ARM_OPCODE_CONDTEST_FAIL:
> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
> index 60c1c71..f7f2f95 100644
> --- a/arch/arm64/kernel/insn.c
> +++ b/arch/arm64/kernel/insn.c
> @@ -1234,3 +1234,99 @@ u32 aarch32_insn_mcr_extract_crm(u32 insn)
>  {
>  	return insn & CRM_MASK;
>  }
> +
> +#define ARM_OPCODE_CONDITION_UNCOND 0xf

This is the second time you define this, which makes me think that this
should live in some include file. On the other hand, it doesn't seem to
be used in the following code, so maybe get rid of it altogether?

> +
> +static bool __kprobes __check_eq(unsigned long pstate)
> +{
> +	return (pstate & PSR_Z_BIT) != 0;
> +}
> +
> +static bool __kprobes __check_ne(unsigned long pstate)
> +{
> +	return (pstate & PSR_Z_BIT) == 0;
> +}
> +
> +static bool __kprobes __check_cs(unsigned long pstate)
> +{
> +	return (pstate & PSR_C_BIT) != 0;
> +}
> +
> +static bool __kprobes __check_cc(unsigned long pstate)
> +{
> +	return (pstate & PSR_C_BIT) == 0;
> +}
> +
> +static bool __kprobes __check_mi(unsigned long pstate)
> +{
> +	return (pstate & PSR_N_BIT) != 0;
> +}
> +
> +static bool __kprobes __check_pl(unsigned long pstate)
> +{
> +	return (pstate & PSR_N_BIT) == 0;
> +}
> +
> +static bool __kprobes __check_vs(unsigned long pstate)
> +{
> +	return (pstate & PSR_V_BIT) != 0;
> +}
> +
> +static bool __kprobes __check_vc(unsigned long pstate)
> +{
> +	return (pstate & PSR_V_BIT) == 0;
> +}
> +
> +static bool __kprobes __check_hi(unsigned long pstate)
> +{
> +	pstate &= ~(pstate >> 1);	/* PSR_C_BIT &= ~PSR_Z_BIT */
> +	return (pstate & PSR_C_BIT) != 0;
> +}
> +
> +static bool __kprobes __check_ls(unsigned long pstate)
> +{
> +	pstate &= ~(pstate >> 1);	/* PSR_C_BIT &= ~PSR_Z_BIT */
> +	return (pstate & PSR_C_BIT) == 0;
> +}
> +
> +static bool __kprobes __check_ge(unsigned long pstate)
> +{
> +	pstate ^= (pstate << 3);	/* PSR_N_BIT ^= PSR_V_BIT */
> +	return (pstate & PSR_N_BIT) == 0;
> +}
> +
> +static bool __kprobes __check_lt(unsigned long pstate)
> +{
> +	pstate ^= (pstate << 3);	/* PSR_N_BIT ^= PSR_V_BIT */
> +	return (pstate & PSR_N_BIT) != 0;
> +}
> +
> +static bool __kprobes __check_gt(unsigned long pstate)
> +{
> +	/*PSR_N_BIT ^= PSR_V_BIT */
> +	unsigned long temp = pstate ^ (pstate << 3);
> +
> +	temp |= (pstate << 1);	/*PSR_N_BIT |= PSR_Z_BIT */
> +	return (temp & PSR_N_BIT) == 0;
> +}
> +
> +static bool __kprobes __check_le(unsigned long pstate)
> +{
> +	/*PSR_N_BIT ^= PSR_V_BIT */
> +	unsigned long temp = pstate ^ (pstate << 3);
> +
> +	temp |= (pstate << 1);	/*PSR_N_BIT |= PSR_Z_BIT */
> +	return (temp & PSR_N_BIT) != 0;
> +}
> +
> +static bool __kprobes __check_al(unsigned long pstate)
> +{
> +	return true;
> +}
> +
> +pstate_check_t * const opcode_condition_checks[16] = {
> +	__check_eq, __check_ne, __check_cs, __check_cc,
> +	__check_mi, __check_pl, __check_vs, __check_vc,
> +	__check_hi, __check_ls, __check_ge, __check_lt,
> +	__check_gt, __check_le, __check_al, __check_al
> +};
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...