[v1,03/19] x86/insn: Add an insn_decode() API
diff mbox series

Message ID 20201223174233.28638-4-bp@alien8.de
State New, archived
Headers show
Series
  • x86/insn: Add an insn_decode() API
Related show

Commit Message

Borislav Petkov Dec. 23, 2020, 5:42 p.m. UTC
From: Borislav Petkov <bp@suse.de>

Users of the instruction decoder should use this to decode instruction
bytes. For that, have insn*() helpers return an int value to denote
success/failure. When there's an error fetching the next insn byte and
the insn falls short, return -ENODATA to denote that.

While at it, make insn_get_opcode() more stricter as to whether what has
seen so far is a valid insn and if not.

Copy linux/kconfig.h for the tools-version of the decoder so that it can
use IS_ENABLED().

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/insn.h       |  24 ++-
 arch/x86/lib/insn.c               | 239 +++++++++++++++++++++++-------
 tools/arch/x86/include/asm/insn.h |  24 ++-
 tools/arch/x86/lib/insn.c         | 239 +++++++++++++++++++++++-------
 tools/include/linux/kconfig.h     |  73 +++++++++
 5 files changed, 475 insertions(+), 124 deletions(-)
 create mode 100644 tools/include/linux/kconfig.h

Comments

Masami Hiramatsu Dec. 28, 2020, 1:15 a.m. UTC | #1
Hi,

On Wed, 23 Dec 2020 18:42:17 +0100
Borislav Petkov <bp@alien8.de> wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> Users of the instruction decoder should use this to decode instruction
> bytes. For that, have insn*() helpers return an int value to denote
> success/failure. When there's an error fetching the next insn byte and
> the insn falls short, return -ENODATA to denote that.

-ENODATA sounds good to me :)

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

BTW, insn_decode() can return -EINVAL if !insn_complete(), is that OK?

> 
> While at it, make insn_get_opcode() more stricter as to whether what has
> seen so far is a valid insn and if not.
> 
> Copy linux/kconfig.h for the tools-version of the decoder so that it can
> use IS_ENABLED().

I think tools clone code must not use INSN_MODE_KERN because the tools may
not use kernel Kconfig.

Hmm, this may be better to make a different patch to introduce a NOSYNC tag
for sync checker in the tools. Something like;

> +int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mode m)
> +{
> +	int ret;
> +
> +	if (m == INSN_MODE_KERN)
		return -EINVAL;	/* NOSYNC */
> +	else
> +		insn_init(insn, kaddr, buf_len, m == INSN_MODE_64);
> +

Then we can simple file list for sync-check.sh.

Thank you,

> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/include/asm/insn.h       |  24 ++-
>  arch/x86/lib/insn.c               | 239 +++++++++++++++++++++++-------
>  tools/arch/x86/include/asm/insn.h |  24 ++-
>  tools/arch/x86/lib/insn.c         | 239 +++++++++++++++++++++++-------
>  tools/include/linux/kconfig.h     |  73 +++++++++
>  5 files changed, 475 insertions(+), 124 deletions(-)
>  create mode 100644 tools/include/linux/kconfig.h
> 
> diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
> index a8c3d284fa46..088aa90e9158 100644
> --- a/arch/x86/include/asm/insn.h
> +++ b/arch/x86/include/asm/insn.h
> @@ -87,13 +87,23 @@ struct insn {
>  #define X86_VEX_M_MAX	0x1f			/* VEX3.M Maximum value */
>  
>  extern void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64);
> -extern void insn_get_prefixes(struct insn *insn);
> -extern void insn_get_opcode(struct insn *insn);
> -extern void insn_get_modrm(struct insn *insn);
> -extern void insn_get_sib(struct insn *insn);
> -extern void insn_get_displacement(struct insn *insn);
> -extern void insn_get_immediate(struct insn *insn);
> -extern void insn_get_length(struct insn *insn);
> +extern int insn_get_prefixes(struct insn *insn);
> +extern int insn_get_opcode(struct insn *insn);
> +extern int insn_get_modrm(struct insn *insn);
> +extern int insn_get_sib(struct insn *insn);
> +extern int insn_get_displacement(struct insn *insn);
> +extern int insn_get_immediate(struct insn *insn);
> +extern int insn_get_length(struct insn *insn);
> +
> +enum insn_mode {
> +	INSN_MODE_32,
> +	INSN_MODE_64,
> +	/* Mode is determined by the current kernel build. */
> +	INSN_MODE_KERN,
> +	INSN_NUM_MODES,
> +};
> +
> +extern int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mode m);
>  
>  /* Attribute will be determined after getting ModRM (for opcode groups) */
>  static inline void insn_get_attribute(struct insn *insn)
> diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
> index 1ba994862b56..b373aadcdd02 100644
> --- a/arch/x86/lib/insn.c
> +++ b/arch/x86/lib/insn.c
> @@ -13,6 +13,9 @@
>  #include <asm/inat.h>
>  #include <asm/insn.h>
>  
> +#include <linux/errno.h>
> +#include <linux/kconfig.h>
> +
>  #include <asm/emulate_prefix.h>
>  
>  /* Verify next sizeof(t) bytes can be on the same instruction */
> @@ -80,12 +83,15 @@ static int __insn_get_emulate_prefix(struct insn *insn,
>  	return 1;
>  
>  err_out:
> -	return 0;
> +	return -ENODATA;
>  }
>  
>  static void insn_get_emulate_prefix(struct insn *insn)
>  {
> -	if (__insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix)))
> +	int ret;
> +
> +	ret = __insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix));
> +	if (ret > 0)
>  		return;
>  
>  	__insn_get_emulate_prefix(insn, kvm_prefix, sizeof(kvm_prefix));
> @@ -98,8 +104,12 @@ static void insn_get_emulate_prefix(struct insn *insn)
>   * Populates the @insn->prefixes bitmap, and updates @insn->next_byte
>   * to point to the (first) opcode.  No effect if @insn->prefixes.got
>   * is already set.
> + *
> + * * Returns:
> + * 0:  on success
> + * < 0: on error
>   */
> -void insn_get_prefixes(struct insn *insn)
> +int insn_get_prefixes(struct insn *insn)
>  {
>  	struct insn_field *prefixes = &insn->prefixes;
>  	insn_attr_t attr;
> @@ -107,7 +117,7 @@ void insn_get_prefixes(struct insn *insn)
>  	int i, nb;
>  
>  	if (prefixes->got)
> -		return;
> +		return 0;
>  
>  	insn_get_emulate_prefix(insn);
>  
> @@ -218,8 +228,10 @@ void insn_get_prefixes(struct insn *insn)
>  
>  	prefixes->got = 1;
>  
> +	return 0;
> +
>  err_out:
> -	return;
> +	return -ENODATA;
>  }
>  
>  /**
> @@ -231,16 +243,25 @@ void insn_get_prefixes(struct insn *insn)
>   * If necessary, first collects any preceding (prefix) bytes.
>   * Sets @insn->opcode.value = opcode1.  No effect if @insn->opcode.got
>   * is already 1.
> + *
> + * Returns:
> + * 0:  on success
> + * < 0: on error
>   */
> -void insn_get_opcode(struct insn *insn)
> +int insn_get_opcode(struct insn *insn)
>  {
>  	struct insn_field *opcode = &insn->opcode;
> +	int pfx_id, ret;
>  	insn_byte_t op;
> -	int pfx_id;
> +
>  	if (opcode->got)
> -		return;
> -	if (!insn->prefixes.got)
> -		insn_get_prefixes(insn);
> +		return 0;
> +
> +	if (!insn->prefixes.got) {
> +		ret = insn_get_prefixes(insn);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	/* Get first opcode */
>  	op = get_next(insn_byte_t, insn);
> @@ -255,9 +276,13 @@ void insn_get_opcode(struct insn *insn)
>  		insn->attr = inat_get_avx_attribute(op, m, p);
>  		if ((inat_must_evex(insn->attr) && !insn_is_evex(insn)) ||
>  		    (!inat_accept_vex(insn->attr) &&
> -		     !inat_is_group(insn->attr)))
> -			insn->attr = 0;	/* This instruction is bad */
> -		goto end;	/* VEX has only 1 byte for opcode */
> +		     !inat_is_group(insn->attr))) {
> +			/* This instruction is bad */
> +			insn->attr = 0;
> +			return 1;
> +		}
> +		/* VEX has only 1 byte for opcode */
> +		goto end;
>  	}
>  
>  	insn->attr = inat_get_opcode_attribute(op);
> @@ -268,13 +293,18 @@ void insn_get_opcode(struct insn *insn)
>  		pfx_id = insn_last_prefix_id(insn);
>  		insn->attr = inat_get_escape_attribute(op, pfx_id, insn->attr);
>  	}
> -	if (inat_must_vex(insn->attr))
> -		insn->attr = 0;	/* This instruction is bad */
> +
> +	if (inat_must_vex(insn->attr)) {
> +		/* This instruction is bad */
> +		insn->attr = 0;
> +		return 1;
> +	}
>  end:
>  	opcode->got = 1;
> +	return 0;
>  
>  err_out:
> -	return;
> +	return -ENODATA;
>  }
>  
>  /**
> @@ -284,15 +314,25 @@ void insn_get_opcode(struct insn *insn)
>   * Populates @insn->modrm and updates @insn->next_byte to point past the
>   * ModRM byte, if any.  If necessary, first collects the preceding bytes
>   * (prefixes and opcode(s)).  No effect if @insn->modrm.got is already 1.
> + *
> + * Returns:
> + * 0:  on success
> + * < 0: on error
>   */
> -void insn_get_modrm(struct insn *insn)
> +int insn_get_modrm(struct insn *insn)
>  {
>  	struct insn_field *modrm = &insn->modrm;
>  	insn_byte_t pfx_id, mod;
> +	int ret;
> +
>  	if (modrm->got)
> -		return;
> -	if (!insn->opcode.got)
> -		insn_get_opcode(insn);
> +		return 0;
> +
> +	if (!insn->opcode.got) {
> +		ret = insn_get_opcode(insn);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	if (inat_has_modrm(insn->attr)) {
>  		mod = get_next(insn_byte_t, insn);
> @@ -302,17 +342,22 @@ void insn_get_modrm(struct insn *insn)
>  			pfx_id = insn_last_prefix_id(insn);
>  			insn->attr = inat_get_group_attribute(mod, pfx_id,
>  							      insn->attr);
> -			if (insn_is_avx(insn) && !inat_accept_vex(insn->attr))
> -				insn->attr = 0;	/* This is bad */
> +			if (insn_is_avx(insn) && !inat_accept_vex(insn->attr)) {
> +				/* Bad insn */
> +				insn->attr = 0;
> +				return 1;
> +			}
>  		}
>  	}
>  
>  	if (insn->x86_64 && inat_is_force64(insn->attr))
>  		insn->opnd_bytes = 8;
> +
>  	modrm->got = 1;
> +	return 0;
>  
>  err_out:
> -	return;
> +	return -ENODATA;
>  }
>  
>  
> @@ -326,11 +371,16 @@ void insn_get_modrm(struct insn *insn)
>  int insn_rip_relative(struct insn *insn)
>  {
>  	struct insn_field *modrm = &insn->modrm;
> +	int ret;
>  
>  	if (!insn->x86_64)
>  		return 0;
> -	if (!modrm->got)
> -		insn_get_modrm(insn);
> +
> +	if (!modrm->got) {
> +		ret = insn_get_modrm(insn);
> +		if (ret)
> +			return ret;
> +	}
>  	/*
>  	 * For rip-relative instructions, the mod field (top 2 bits)
>  	 * is zero and the r/m field (bottom 3 bits) is 0x5.
> @@ -344,15 +394,25 @@ int insn_rip_relative(struct insn *insn)
>   *
>   * If necessary, first collects the instruction up to and including the
>   * ModRM byte.
> + *
> + * Returns:
> + * 0: if decoding succeeded
> + * < 0: otherwise.
>   */
> -void insn_get_sib(struct insn *insn)
> +int insn_get_sib(struct insn *insn)
>  {
>  	insn_byte_t modrm;
> +	int ret;
>  
>  	if (insn->sib.got)
> -		return;
> -	if (!insn->modrm.got)
> -		insn_get_modrm(insn);
> +		return 0;
> +
> +	if (!insn->modrm.got) {
> +		ret = insn_get_modrm(insn);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (insn->modrm.nbytes) {
>  		modrm = (insn_byte_t)insn->modrm.value;
>  		if (insn->addr_bytes != 2 &&
> @@ -363,8 +423,10 @@ void insn_get_sib(struct insn *insn)
>  	}
>  	insn->sib.got = 1;
>  
> +	return 0;
> +
>  err_out:
> -	return;
> +	return -ENODATA;
>  }
>  
>  
> @@ -375,15 +437,25 @@ void insn_get_sib(struct insn *insn)
>   * If necessary, first collects the instruction up to and including the
>   * SIB byte.
>   * Displacement value is sign-expanded.
> + *
> + * * Returns:
> + * 0: if decoding succeeded
> + * < 0: otherwise.
>   */
> -void insn_get_displacement(struct insn *insn)
> +int insn_get_displacement(struct insn *insn)
>  {
>  	insn_byte_t mod, rm, base;
> +	int ret;
>  
>  	if (insn->displacement.got)
> -		return;
> -	if (!insn->sib.got)
> -		insn_get_sib(insn);
> +		return 0;
> +
> +	if (!insn->sib.got) {
> +		ret = insn_get_sib(insn);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (insn->modrm.nbytes) {
>  		/*
>  		 * Interpreting the modrm byte:
> @@ -426,12 +498,13 @@ void insn_get_displacement(struct insn *insn)
>  	}
>  out:
>  	insn->displacement.got = 1;
> +	return 0;
>  
>  err_out:
> -	return;
> +	return -ENODATA;
>  }
>  
> -/* Decode moffset16/32/64. Return 0 if failed */
> +/* Decode moffset16/32/64. Return a negative value if failed. */
>  static int __get_moffset(struct insn *insn)
>  {
>  	switch (insn->addr_bytes) {
> @@ -457,10 +530,10 @@ static int __get_moffset(struct insn *insn)
>  	return 1;
>  
>  err_out:
> -	return 0;
> +	return -ENODATA;
>  }
>  
> -/* Decode imm v32(Iz). Return 0 if failed */
> +/* Decode imm v32(Iz). Return a negative value if failed. */
>  static int __get_immv32(struct insn *insn)
>  {
>  	switch (insn->opnd_bytes) {
> @@ -480,10 +553,10 @@ static int __get_immv32(struct insn *insn)
>  	return 1;
>  
>  err_out:
> -	return 0;
> +	return -ENODATA;
>  }
>  
> -/* Decode imm v64(Iv/Ov), Return 0 if failed */
> +/* Decode imm v64(Iv/Ov). Return a negative value if failed. */
>  static int __get_immv(struct insn *insn)
>  {
>  	switch (insn->opnd_bytes) {
> @@ -507,11 +580,12 @@ static int __get_immv(struct insn *insn)
>  	insn->immediate1.got = insn->immediate2.got = 1;
>  
>  	return 1;
> +
>  err_out:
> -	return 0;
> +	return -ENODATA;
>  }
>  
> -/* Decode ptr16:16/32(Ap) */
> +/* Decode ptr16:16/32(Ap). Return a negative value if failed. */
>  static int __get_immptr(struct insn *insn)
>  {
>  	switch (insn->opnd_bytes) {
> @@ -534,25 +608,36 @@ static int __get_immptr(struct insn *insn)
>  	insn->immediate1.got = insn->immediate2.got = 1;
>  
>  	return 1;
> +
>  err_out:
> -	return 0;
> +	return -ENODATA;
>  }
>  
>  /**
> - * insn_get_immediate() - Get the immediates of instruction
> + * insn_get_immediate() - Get the immediate in an instruction
>   * @insn:	&struct insn containing instruction
>   *
>   * If necessary, first collects the instruction up to and including the
>   * displacement bytes.
>   * Basically, most of immediates are sign-expanded. Unsigned-value can be
> - * get by bit masking with ((1 << (nbytes * 8)) - 1)
> + * computed by bit masking with ((1 << (nbytes * 8)) - 1)
> + *
> + * Returns:
> + * 0:  on success
> + * < 0: on error
>   */
> -void insn_get_immediate(struct insn *insn)
> +int insn_get_immediate(struct insn *insn)
>  {
> +	int ret;
> +
>  	if (insn->immediate.got)
> -		return;
> -	if (!insn->displacement.got)
> -		insn_get_displacement(insn);
> +		return 0;
> +
> +	if (!insn->displacement.got) {
> +		ret = insn_get_displacement(insn);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	if (inat_has_moffset(insn->attr)) {
>  		if (!__get_moffset(insn))
> @@ -605,9 +690,10 @@ void insn_get_immediate(struct insn *insn)
>  	}
>  done:
>  	insn->immediate.got = 1;
> +	return 0;
>  
>  err_out:
> -	return;
> +	return -ENODATA;
>  }
>  
>  /**
> @@ -616,13 +702,56 @@ void insn_get_immediate(struct insn *insn)
>   *
>   * If necessary, first collects the instruction up to and including the
>   * immediates bytes.
> - */
> -void insn_get_length(struct insn *insn)
> + *
> + * Returns:
> + *  - 0 on success
> + *  - < 0 on error
> +*/
> +int insn_get_length(struct insn *insn)
>  {
> +	int ret;
> +
>  	if (insn->length)
> -		return;
> -	if (!insn->immediate.got)
> -		insn_get_immediate(insn);
> +		return 0;
> +
> +	if (!insn->immediate.got) {
> +		ret = insn_get_immediate(insn);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	insn->length = (unsigned char)((unsigned long)insn->next_byte
>  				     - (unsigned long)insn->kaddr);
> +
> +	return 0;
> +}
> +
> +/**
> + * insn_decode() - Decode an x86 instruction
> + * @insn:	&struct insn to be initialized
> + * @kaddr:	address (in kernel memory) of instruction (or copy thereof)
> + * @buf_len:	length of the insn buffer at @kaddr
> + * @m:		insn mode, see enum insn_mode
> + *
> + * Returns:
> + * 0: if decoding succeeded
> + * < 0: otherwise.
> + */
> +int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mode m)
> +{
> +	int ret;
> +
> +	if (m == INSN_MODE_KERN)
> +		insn_init(insn, kaddr, buf_len, IS_ENABLED(CONFIG_X86_64));
> +	else
> +		insn_init(insn, kaddr, buf_len, m == INSN_MODE_64);
> +
> +	ret = insn_get_length(insn);
> +	if (ret)
> +		return ret;
> +
> +	if (insn_complete(insn))
> +		return 0;
> +
> +	return -EINVAL;
>  }
> diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
> index 52c6262e6bfd..ed43bf01ebcc 100644
> --- a/tools/arch/x86/include/asm/insn.h
> +++ b/tools/arch/x86/include/asm/insn.h
> @@ -87,13 +87,23 @@ struct insn {
>  #define X86_VEX_M_MAX	0x1f			/* VEX3.M Maximum value */
>  
>  extern void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64);
> -extern void insn_get_prefixes(struct insn *insn);
> -extern void insn_get_opcode(struct insn *insn);
> -extern void insn_get_modrm(struct insn *insn);
> -extern void insn_get_sib(struct insn *insn);
> -extern void insn_get_displacement(struct insn *insn);
> -extern void insn_get_immediate(struct insn *insn);
> -extern void insn_get_length(struct insn *insn);
> +extern int insn_get_prefixes(struct insn *insn);
> +extern int insn_get_opcode(struct insn *insn);
> +extern int insn_get_modrm(struct insn *insn);
> +extern int insn_get_sib(struct insn *insn);
> +extern int insn_get_displacement(struct insn *insn);
> +extern int insn_get_immediate(struct insn *insn);
> +extern int insn_get_length(struct insn *insn);
> +
> +enum insn_mode {
> +	INSN_MODE_32,
> +	INSN_MODE_64,
> +	/* Mode is determined by the current kernel build. */
> +	INSN_MODE_KERN,
> +	INSN_NUM_MODES,
> +};
> +
> +extern int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mode m);
>  
>  /* Attribute will be determined after getting ModRM (for opcode groups) */
>  static inline void insn_get_attribute(struct insn *insn)
> diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
> index f3277d6e4ef2..a2f9d9e177f2 100644
> --- a/tools/arch/x86/lib/insn.c
> +++ b/tools/arch/x86/lib/insn.c
> @@ -13,6 +13,9 @@
>  #include "../include/asm/inat.h"
>  #include "../include/asm/insn.h"
>  
> +#include <linux/errno.h>
> +#include <linux/kconfig.h>
> +
>  #include "../include/asm/emulate_prefix.h"
>  
>  /* Verify next sizeof(t) bytes can be on the same instruction */
> @@ -80,12 +83,15 @@ static int __insn_get_emulate_prefix(struct insn *insn,
>  	return 1;
>  
>  err_out:
> -	return 0;
> +	return -ENODATA;
>  }
>  
>  static void insn_get_emulate_prefix(struct insn *insn)
>  {
> -	if (__insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix)))
> +	int ret;
> +
> +	ret = __insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix));
> +	if (ret > 0)
>  		return;
>  
>  	__insn_get_emulate_prefix(insn, kvm_prefix, sizeof(kvm_prefix));
> @@ -98,8 +104,12 @@ static void insn_get_emulate_prefix(struct insn *insn)
>   * Populates the @insn->prefixes bitmap, and updates @insn->next_byte
>   * to point to the (first) opcode.  No effect if @insn->prefixes.got
>   * is already set.
> + *
> + * * Returns:
> + * 0:  on success
> + * < 0: on error
>   */
> -void insn_get_prefixes(struct insn *insn)
> +int insn_get_prefixes(struct insn *insn)
>  {
>  	struct insn_field *prefixes = &insn->prefixes;
>  	insn_attr_t attr;
> @@ -107,7 +117,7 @@ void insn_get_prefixes(struct insn *insn)
>  	int i, nb;
>  
>  	if (prefixes->got)
> -		return;
> +		return 0;
>  
>  	insn_get_emulate_prefix(insn);
>  
> @@ -218,8 +228,10 @@ void insn_get_prefixes(struct insn *insn)
>  
>  	prefixes->got = 1;
>  
> +	return 0;
> +
>  err_out:
> -	return;
> +	return -ENODATA;
>  }
>  
>  /**
> @@ -231,16 +243,25 @@ void insn_get_prefixes(struct insn *insn)
>   * If necessary, first collects any preceding (prefix) bytes.
>   * Sets @insn->opcode.value = opcode1.  No effect if @insn->opcode.got
>   * is already 1.
> + *
> + * Returns:
> + * 0:  on success
> + * < 0: on error
>   */
> -void insn_get_opcode(struct insn *insn)
> +int insn_get_opcode(struct insn *insn)
>  {
>  	struct insn_field *opcode = &insn->opcode;
> +	int pfx_id, ret;
>  	insn_byte_t op;
> -	int pfx_id;
> +
>  	if (opcode->got)
> -		return;
> -	if (!insn->prefixes.got)
> -		insn_get_prefixes(insn);
> +		return 0;
> +
> +	if (!insn->prefixes.got) {
> +		ret = insn_get_prefixes(insn);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	/* Get first opcode */
>  	op = get_next(insn_byte_t, insn);
> @@ -255,9 +276,13 @@ void insn_get_opcode(struct insn *insn)
>  		insn->attr = inat_get_avx_attribute(op, m, p);
>  		if ((inat_must_evex(insn->attr) && !insn_is_evex(insn)) ||
>  		    (!inat_accept_vex(insn->attr) &&
> -		     !inat_is_group(insn->attr)))
> -			insn->attr = 0;	/* This instruction is bad */
> -		goto end;	/* VEX has only 1 byte for opcode */
> +		     !inat_is_group(insn->attr))) {
> +			/* This instruction is bad */
> +			insn->attr = 0;
> +			return 1;
> +		}
> +		/* VEX has only 1 byte for opcode */
> +		goto end;
>  	}
>  
>  	insn->attr = inat_get_opcode_attribute(op);
> @@ -268,13 +293,18 @@ void insn_get_opcode(struct insn *insn)
>  		pfx_id = insn_last_prefix_id(insn);
>  		insn->attr = inat_get_escape_attribute(op, pfx_id, insn->attr);
>  	}
> -	if (inat_must_vex(insn->attr))
> -		insn->attr = 0;	/* This instruction is bad */
> +
> +	if (inat_must_vex(insn->attr)) {
> +		/* This instruction is bad */
> +		insn->attr = 0;
> +		return 1;
> +	}
>  end:
>  	opcode->got = 1;
> +	return 0;
>  
>  err_out:
> -	return;
> +	return -ENODATA;
>  }
>  
>  /**
> @@ -284,15 +314,25 @@ void insn_get_opcode(struct insn *insn)
>   * Populates @insn->modrm and updates @insn->next_byte to point past the
>   * ModRM byte, if any.  If necessary, first collects the preceding bytes
>   * (prefixes and opcode(s)).  No effect if @insn->modrm.got is already 1.
> + *
> + * Returns:
> + * 0:  on success
> + * < 0: on error
>   */
> -void insn_get_modrm(struct insn *insn)
> +int insn_get_modrm(struct insn *insn)
>  {
>  	struct insn_field *modrm = &insn->modrm;
>  	insn_byte_t pfx_id, mod;
> +	int ret;
> +
>  	if (modrm->got)
> -		return;
> -	if (!insn->opcode.got)
> -		insn_get_opcode(insn);
> +		return 0;
> +
> +	if (!insn->opcode.got) {
> +		ret = insn_get_opcode(insn);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	if (inat_has_modrm(insn->attr)) {
>  		mod = get_next(insn_byte_t, insn);
> @@ -302,17 +342,22 @@ void insn_get_modrm(struct insn *insn)
>  			pfx_id = insn_last_prefix_id(insn);
>  			insn->attr = inat_get_group_attribute(mod, pfx_id,
>  							      insn->attr);
> -			if (insn_is_avx(insn) && !inat_accept_vex(insn->attr))
> -				insn->attr = 0;	/* This is bad */
> +			if (insn_is_avx(insn) && !inat_accept_vex(insn->attr)) {
> +				/* Bad insn */
> +				insn->attr = 0;
> +				return 1;
> +			}
>  		}
>  	}
>  
>  	if (insn->x86_64 && inat_is_force64(insn->attr))
>  		insn->opnd_bytes = 8;
> +
>  	modrm->got = 1;
> +	return 0;
>  
>  err_out:
> -	return;
> +	return -ENODATA;
>  }
>  
>  
> @@ -326,11 +371,16 @@ void insn_get_modrm(struct insn *insn)
>  int insn_rip_relative(struct insn *insn)
>  {
>  	struct insn_field *modrm = &insn->modrm;
> +	int ret;
>  
>  	if (!insn->x86_64)
>  		return 0;
> -	if (!modrm->got)
> -		insn_get_modrm(insn);
> +
> +	if (!modrm->got) {
> +		ret = insn_get_modrm(insn);
> +		if (ret)
> +			return ret;
> +	}
>  	/*
>  	 * For rip-relative instructions, the mod field (top 2 bits)
>  	 * is zero and the r/m field (bottom 3 bits) is 0x5.
> @@ -344,15 +394,25 @@ int insn_rip_relative(struct insn *insn)
>   *
>   * If necessary, first collects the instruction up to and including the
>   * ModRM byte.
> + *
> + * Returns:
> + * 0: if decoding succeeded
> + * < 0: otherwise.
>   */
> -void insn_get_sib(struct insn *insn)
> +int insn_get_sib(struct insn *insn)
>  {
>  	insn_byte_t modrm;
> +	int ret;
>  
>  	if (insn->sib.got)
> -		return;
> -	if (!insn->modrm.got)
> -		insn_get_modrm(insn);
> +		return 0;
> +
> +	if (!insn->modrm.got) {
> +		ret = insn_get_modrm(insn);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (insn->modrm.nbytes) {
>  		modrm = (insn_byte_t)insn->modrm.value;
>  		if (insn->addr_bytes != 2 &&
> @@ -363,8 +423,10 @@ void insn_get_sib(struct insn *insn)
>  	}
>  	insn->sib.got = 1;
>  
> +	return 0;
> +
>  err_out:
> -	return;
> +	return -ENODATA;
>  }
>  
>  
> @@ -375,15 +437,25 @@ void insn_get_sib(struct insn *insn)
>   * If necessary, first collects the instruction up to and including the
>   * SIB byte.
>   * Displacement value is sign-expanded.
> + *
> + * * Returns:
> + * 0: if decoding succeeded
> + * < 0: otherwise.
>   */
> -void insn_get_displacement(struct insn *insn)
> +int insn_get_displacement(struct insn *insn)
>  {
>  	insn_byte_t mod, rm, base;
> +	int ret;
>  
>  	if (insn->displacement.got)
> -		return;
> -	if (!insn->sib.got)
> -		insn_get_sib(insn);
> +		return 0;
> +
> +	if (!insn->sib.got) {
> +		ret = insn_get_sib(insn);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (insn->modrm.nbytes) {
>  		/*
>  		 * Interpreting the modrm byte:
> @@ -426,12 +498,13 @@ void insn_get_displacement(struct insn *insn)
>  	}
>  out:
>  	insn->displacement.got = 1;
> +	return 0;
>  
>  err_out:
> -	return;
> +	return -ENODATA;
>  }
>  
> -/* Decode moffset16/32/64. Return 0 if failed */
> +/* Decode moffset16/32/64. Return a negative value if failed. */
>  static int __get_moffset(struct insn *insn)
>  {
>  	switch (insn->addr_bytes) {
> @@ -457,10 +530,10 @@ static int __get_moffset(struct insn *insn)
>  	return 1;
>  
>  err_out:
> -	return 0;
> +	return -ENODATA;
>  }
>  
> -/* Decode imm v32(Iz). Return 0 if failed */
> +/* Decode imm v32(Iz). Return a negative value if failed. */
>  static int __get_immv32(struct insn *insn)
>  {
>  	switch (insn->opnd_bytes) {
> @@ -480,10 +553,10 @@ static int __get_immv32(struct insn *insn)
>  	return 1;
>  
>  err_out:
> -	return 0;
> +	return -ENODATA;
>  }
>  
> -/* Decode imm v64(Iv/Ov), Return 0 if failed */
> +/* Decode imm v64(Iv/Ov). Return a negative value if failed. */
>  static int __get_immv(struct insn *insn)
>  {
>  	switch (insn->opnd_bytes) {
> @@ -507,11 +580,12 @@ static int __get_immv(struct insn *insn)
>  	insn->immediate1.got = insn->immediate2.got = 1;
>  
>  	return 1;
> +
>  err_out:
> -	return 0;
> +	return -ENODATA;
>  }
>  
> -/* Decode ptr16:16/32(Ap) */
> +/* Decode ptr16:16/32(Ap). Return a negative value if failed. */
>  static int __get_immptr(struct insn *insn)
>  {
>  	switch (insn->opnd_bytes) {
> @@ -534,25 +608,36 @@ static int __get_immptr(struct insn *insn)
>  	insn->immediate1.got = insn->immediate2.got = 1;
>  
>  	return 1;
> +
>  err_out:
> -	return 0;
> +	return -ENODATA;
>  }
>  
>  /**
> - * insn_get_immediate() - Get the immediates of instruction
> + * insn_get_immediate() - Get the immediate in an instruction
>   * @insn:	&struct insn containing instruction
>   *
>   * If necessary, first collects the instruction up to and including the
>   * displacement bytes.
>   * Basically, most of immediates are sign-expanded. Unsigned-value can be
> - * get by bit masking with ((1 << (nbytes * 8)) - 1)
> + * computed by bit masking with ((1 << (nbytes * 8)) - 1)
> + *
> + * Returns:
> + * 0:  on success
> + * < 0: on error
>   */
> -void insn_get_immediate(struct insn *insn)
> +int insn_get_immediate(struct insn *insn)
>  {
> +	int ret;
> +
>  	if (insn->immediate.got)
> -		return;
> -	if (!insn->displacement.got)
> -		insn_get_displacement(insn);
> +		return 0;
> +
> +	if (!insn->displacement.got) {
> +		ret = insn_get_displacement(insn);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	if (inat_has_moffset(insn->attr)) {
>  		if (!__get_moffset(insn))
> @@ -605,9 +690,10 @@ void insn_get_immediate(struct insn *insn)
>  	}
>  done:
>  	insn->immediate.got = 1;
> +	return 0;
>  
>  err_out:
> -	return;
> +	return -ENODATA;
>  }
>  
>  /**
> @@ -616,13 +702,56 @@ void insn_get_immediate(struct insn *insn)
>   *
>   * If necessary, first collects the instruction up to and including the
>   * immediates bytes.
> - */
> -void insn_get_length(struct insn *insn)
> + *
> + * Returns:
> + *  - 0 on success
> + *  - < 0 on error
> +*/
> +int insn_get_length(struct insn *insn)
>  {
> +	int ret;
> +
>  	if (insn->length)
> -		return;
> -	if (!insn->immediate.got)
> -		insn_get_immediate(insn);
> +		return 0;
> +
> +	if (!insn->immediate.got) {
> +		ret = insn_get_immediate(insn);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	insn->length = (unsigned char)((unsigned long)insn->next_byte
>  				     - (unsigned long)insn->kaddr);
> +
> +	return 0;
> +}
> +
> +/**
> + * insn_decode() - Decode an x86 instruction
> + * @insn:	&struct insn to be initialized
> + * @kaddr:	address (in kernel memory) of instruction (or copy thereof)
> + * @buf_len:	length of the insn buffer at @kaddr
> + * @m:		insn mode, see enum insn_mode
> + *
> + * Returns:
> + * 0: if decoding succeeded
> + * < 0: otherwise.
> + */
> +int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mode m)
> +{
> +	int ret;
> +
> +	if (m == INSN_MODE_KERN)
> +		insn_init(insn, kaddr, buf_len, IS_ENABLED(CONFIG_X86_64));
> +	else
> +		insn_init(insn, kaddr, buf_len, m == INSN_MODE_64);
> +
> +	ret = insn_get_length(insn);
> +	if (ret)
> +		return ret;
> +
> +	if (insn_complete(insn))
> +		return 0;
> +
> +	return -EINVAL;
>  }
> diff --git a/tools/include/linux/kconfig.h b/tools/include/linux/kconfig.h
> new file mode 100644
> index 000000000000..1555a0c4f345
> --- /dev/null
> +++ b/tools/include/linux/kconfig.h
> @@ -0,0 +1,73 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _TOOLS_LINUX_KCONFIG_H
> +#define _TOOLS_LINUX_KCONFIG_H
> +
> +/* CONFIG_CC_VERSION_TEXT (Do not delete this comment. See help in Kconfig) */
> +
> +#ifdef CONFIG_CPU_BIG_ENDIAN
> +#define __BIG_ENDIAN 4321
> +#else
> +#define __LITTLE_ENDIAN 1234
> +#endif
> +
> +#define __ARG_PLACEHOLDER_1 0,
> +#define __take_second_arg(__ignored, val, ...) val
> +
> +/*
> + * The use of "&&" / "||" is limited in certain expressions.
> + * The following enable to calculate "and" / "or" with macro expansion only.
> + */
> +#define __and(x, y)			___and(x, y)
> +#define ___and(x, y)			____and(__ARG_PLACEHOLDER_##x, y)
> +#define ____and(arg1_or_junk, y)	__take_second_arg(arg1_or_junk y, 0)
> +
> +#define __or(x, y)			___or(x, y)
> +#define ___or(x, y)			____or(__ARG_PLACEHOLDER_##x, y)
> +#define ____or(arg1_or_junk, y)		__take_second_arg(arg1_or_junk 1, y)
> +
> +/*
> + * Helper macros to use CONFIG_ options in C/CPP expressions. Note that
> + * these only work with boolean and tristate options.
> + */
> +
> +/*
> + * Getting something that works in C and CPP for an arg that may or may
> + * not be defined is tricky.  Here, if we have "#define CONFIG_BOOGER 1"
> + * we match on the placeholder define, insert the "0," for arg1 and generate
> + * the triplet (0, 1, 0).  Then the last step cherry picks the 2nd arg (a one).
> + * When CONFIG_BOOGER is not defined, we generate a (... 1, 0) pair, and when
> + * the last step cherry picks the 2nd arg, we get a zero.
> + */
> +#define __is_defined(x)			___is_defined(x)
> +#define ___is_defined(val)		____is_defined(__ARG_PLACEHOLDER_##val)
> +#define ____is_defined(arg1_or_junk)	__take_second_arg(arg1_or_junk 1, 0)
> +
> +/*
> + * IS_BUILTIN(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y', 0
> + * otherwise. For boolean options, this is equivalent to
> + * IS_ENABLED(CONFIG_FOO).
> + */
> +#define IS_BUILTIN(option) __is_defined(option)
> +
> +/*
> + * IS_MODULE(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'm', 0
> + * otherwise.
> + */
> +#define IS_MODULE(option) __is_defined(option##_MODULE)
> +
> +/*
> + * IS_REACHABLE(CONFIG_FOO) evaluates to 1 if the currently compiled
> + * code can call a function defined in code compiled based on CONFIG_FOO.
> + * This is similar to IS_ENABLED(), but returns false when invoked from
> + * built-in code when CONFIG_FOO is set to 'm'.
> + */
> +#define IS_REACHABLE(option) __or(IS_BUILTIN(option), \
> +				__and(IS_MODULE(option), __is_defined(MODULE)))
> +
> +/*
> + * IS_ENABLED(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y' or 'm',
> + * 0 otherwise.
> + */
> +#define IS_ENABLED(option) __or(IS_BUILTIN(option), IS_MODULE(option))
> +
> +#endif /* _TOOLS_LINUX_KCONFIG_H */
> -- 
> 2.29.2
>
Borislav Petkov Dec. 29, 2020, 8:06 p.m. UTC | #2
On Mon, Dec 28, 2020 at 10:15:10AM +0900, Masami Hiramatsu wrote:
> BTW, insn_decode() can return -EINVAL if !insn_complete(), is that OK?

It does with this change. Or are you asking whether it returning -EINVAL
in that case is ok?

I don't see why not - this way callers can differentiate where it failed
- at fetching bytes with -ENODATA or it wasn't decoded completely -
-EINVAL.

> I think tools clone code must not use INSN_MODE_KERN because the tools may
> not use kernel Kconfig.
> 
> Hmm, this may be better to make a different patch to introduce a NOSYNC tag
> for sync checker in the tools. Something like;

I'd actually prefer this:

diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index f8772b371452..545320c67855 100644
--- a/tools/arch/x86/include/asm/insn.h
+++ b/tools/arch/x86/include/asm/insn.h
@@ -98,8 +98,6 @@ extern int insn_get_length(struct insn *insn);
 enum insn_mode {
        INSN_MODE_32,
        INSN_MODE_64,
-       /* Mode is determined by the current kernel build. */
-       INSN_MODE_KERN,
        INSN_NUM_MODES,
 };
 

so that when a tool does use INSN_MODE_KERN, it would fail building:

In file included from util/intel-pt-decoder/intel-pt-insn-decoder.c:15:
util/intel-pt-decoder/../../../arch/x86/lib/insn.c: In function ‘insn_decode’:
util/intel-pt-decoder/../../../arch/x86/lib/insn.c:751:11: error: ‘INSN_MODE_KERN’ undeclared (first use in this function); did you mean ‘INSN_MODE_64’?
  751 |  if (m == INSN_MODE_KERN)
      |           ^~~~~~~~~~~~~~
      |           INSN_MODE_64
util/intel-pt-decoder/../../../arch/x86/lib/insn.c:751:11: note: each undeclared identifier is reported only once for each function it appears in
  LD       arch/perf-in.o
util/intel-pt-decoder/intel-pt-insn-decoder.c: In function ‘intel_pt_get_insn’:
util/intel-pt-decoder/intel-pt-insn-decoder.c:163:37: error: ‘INSN_MODE_KERN’ undeclared (first use in this function); did you mean ‘INSN_MODE_64’?
  163 |  ret = insn_decode(&insn, buf, len, INSN_MODE_KERN);
      |                                     ^~~~~~~~~~~~~~
      |                                     INSN_MODE_64

Thx.
Masami Hiramatsu Dec. 30, 2020, 9 a.m. UTC | #3
On Tue, 29 Dec 2020 21:06:54 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Mon, Dec 28, 2020 at 10:15:10AM +0900, Masami Hiramatsu wrote:
> > BTW, insn_decode() can return -EINVAL if !insn_complete(), is that OK?
> 
> It does with this change. Or are you asking whether it returning -EINVAL
> in that case is ok?
> 
> I don't see why not - this way callers can differentiate where it failed
> - at fetching bytes with -ENODATA or it wasn't decoded completely -
> -EINVAL.

Ah, I got it.

> 
> > I think tools clone code must not use INSN_MODE_KERN because the tools may
> > not use kernel Kconfig.
> > 
> > Hmm, this may be better to make a different patch to introduce a NOSYNC tag
> > for sync checker in the tools. Something like;
> 
> I'd actually prefer this:
> 
> diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
> index f8772b371452..545320c67855 100644
> --- a/tools/arch/x86/include/asm/insn.h
> +++ b/tools/arch/x86/include/asm/insn.h
> @@ -98,8 +98,6 @@ extern int insn_get_length(struct insn *insn);
>  enum insn_mode {
>         INSN_MODE_32,
>         INSN_MODE_64,
> -       /* Mode is determined by the current kernel build. */
> -       INSN_MODE_KERN,
>         INSN_NUM_MODES,
>  };

Agreed. This is much simpler.
Maybe I need to replace it with dummy lines but it is possible.

>  
> 
> so that when a tool does use INSN_MODE_KERN, it would fail building:
> 
> In file included from util/intel-pt-decoder/intel-pt-insn-decoder.c:15:
> util/intel-pt-decoder/../../../arch/x86/lib/insn.c: In function ‘insn_decode’:
> util/intel-pt-decoder/../../../arch/x86/lib/insn.c:751:11: error: ‘INSN_MODE_KERN’ undeclared (first use in this function); did you mean ‘INSN_MODE_64’?
>   751 |  if (m == INSN_MODE_KERN)
>       |           ^~~~~~~~~~~~~~
>       |           INSN_MODE_64

This part is OK. I can replace it with dummy lines.

> util/intel-pt-decoder/../../../arch/x86/lib/insn.c:751:11: note: each undeclared identifier is reported only once for each function it appears in
>   LD       arch/perf-in.o
> util/intel-pt-decoder/intel-pt-insn-decoder.c: In function ‘intel_pt_get_insn’:
> util/intel-pt-decoder/intel-pt-insn-decoder.c:163:37: error: ‘INSN_MODE_KERN’ undeclared (first use in this function); did you mean ‘INSN_MODE_64’?
>   163 |  ret = insn_decode(&insn, buf, len, INSN_MODE_KERN);
>       |                                     ^~~~~~~~~~~~~~
>       |                                     INSN_MODE_64

But in [17/19], your patch seems not using INSN_MODE_KERN there.

--- a/tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.c
+++ b/tools/perf/util/intel-pt-decoder/intel-pt-insn-decoder.c
@@ -158,11 +158,13 @@ int intel_pt_get_insn(const unsigned char *buf, size_t len, int x86_64,
 		      struct intel_pt_insn *intel_pt_insn)
 {
 	struct insn insn;
+	int ret;
 
-	insn_init(&insn, buf, len, x86_64);
-	insn_get_length(&insn);
-	if (!insn_complete(&insn) || insn.length > len)
+	ret = insn_decode(&insn, buf, len,
+			  x86_64 ? INSN_MODE_64 : INSN_MODE_32);
+	if (ret < 0 || insn.length > len)
 		return -1;
+
 	intel_pt_insn_decoder(&insn, intel_pt_insn);
 	if (insn.length < INTEL_PT_INSN_BUF_SZ)
 		memcpy(intel_pt_insn->buf, buf, insn.length);

Thank you,
Borislav Petkov Dec. 30, 2020, 9:28 a.m. UTC | #4
On Wed, Dec 30, 2020 at 06:00:52PM +0900, Masami Hiramatsu wrote:
> Maybe I need to replace it with dummy lines but it is possible.

Dummy lines like "IGNORE DURING CHECK" or something like that?

> But in [17/19], your patch seems not using INSN_MODE_KERN there.

I replaced it only locally just so that I can cause the build error and
show you what mean.

I still haven't thought about what do to exactly there wrt making
sync-check.sh happy but the aspect of failing the build is a nice one.
Masami Hiramatsu Jan. 6, 2021, 5:21 a.m. UTC | #5
On Wed, 30 Dec 2020 10:28:33 +0100
Borislav Petkov <bp@alien8.de> wrote:

> I still haven't thought about what do to exactly there wrt making
> sync-check.sh happy but the aspect of failing the build is a nice one.

So I think it is possible to introduce a keyword in a comment
for ignoring sync check something like below. This will allow us
a generic pattern matching.

The keyword is just an example, "no-sync-check" etc. is OK.

What would you think about it?

Thank you,

diff --git a/arch/x86/include/asm/inat.h b/arch/x86/include/asm/inat.h
index 4cf2ad521f65..ff3d119610f5 100644
--- a/arch/x86/include/asm/inat.h
+++ b/arch/x86/include/asm/inat.h
@@ -6,7 +6,7 @@
  *
  * Written by Masami Hiramatsu <mhiramat@redhat.com>
  */
-#include <asm/inat_types.h>
+#include <asm/inat_types.h>	/* Different from tools */
 
 /*
  * Internal bits. Don't use bitmasks directly, because these bits are
diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index a8c3d284fa46..dda4fe208659 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -8,7 +8,7 @@
  */
 
 /* insn_attr_t is defined in inat.h */
-#include <asm/inat.h>
+#include <asm/inat.h>	/* Different from tools */
 
 struct insn_field {
 	union {
diff --git a/tools/arch/x86/include/asm/inat.h b/tools/arch/x86/include/asm/inat.h
index 877827b7c2c3..1fcfb4efb5f4 100644
--- a/tools/arch/x86/include/asm/inat.h
+++ b/tools/arch/x86/include/asm/inat.h
@@ -6,7 +6,7 @@
  *
  * Written by Masami Hiramatsu <mhiramat@redhat.com>
  */
-#include "inat_types.h"
+#include "inat_types.h"	/* Different from kernel */
 
 /*
  * Internal bits. Don't use bitmasks directly, because these bits are
diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index 52c6262e6bfd..702135ddb4fd 100644
--- a/tools/arch/x86/include/asm/insn.h
+++ b/tools/arch/x86/include/asm/insn.h
@@ -8,7 +8,7 @@
  */
 
 /* insn_attr_t is defined in inat.h */
-#include "inat.h"
+#include "inat.h"	/* Different from kernel */
 
 struct insn_field {
 	union {
diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index dded93a2bc89..b112d68c5513 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -137,8 +137,8 @@ check include/uapi/linux/mman.h       '-I "^#include <\(uapi/\)*asm/mman.h>"'
 check include/linux/build_bug.h       '-I "^#\(ifndef\|endif\)\( \/\/\)* static_assert$"'
 check include/linux/ctype.h	      '-I "isdigit("'
 check lib/ctype.c		      '-I "^EXPORT_SYMBOL" -I "^#include <linux/export.h>" -B'
-check arch/x86/include/asm/inat.h     '-I "^#include [\"<]\(asm/\)*inat_types.h[\">]"'
-check arch/x86/include/asm/insn.h     '-I "^#include [\"<]\(asm/\)*inat.h[\">]"'
+check arch/x86/include/asm/inat.h     '-I "^.*/\* Different from \(tools\|kernel\) \*/"'
+check arch/x86/include/asm/insn.h     '-I "^.*/\* Different from \(tools\|kernel\) \*/"'
 check arch/x86/lib/inat.c	      '-I "^#include [\"<]\(../include/\)*asm/insn.h[\">]"'
 check arch/x86/lib/insn.c             '-I "^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]" -I "^#include [\"<]\(../include/\)*asm/emulate_prefix.h[\">]"'
Borislav Petkov Jan. 8, 2021, 6:59 p.m. UTC | #6
On Wed, Jan 06, 2021 at 02:21:14PM +0900, Masami Hiramatsu wrote:
> So I think it is possible to introduce a keyword in a comment
> for ignoring sync check something like below. This will allow us
> a generic pattern matching.
> 
> The keyword is just an example, "no-sync-check" etc. is OK.
> 
> What would you think about it?

Yeah, I'd prefer a single keyword which to slap everywhere, see below.
The patch is only for demonstration, though, it is not complete.

And while playing with that after having commented out INSN_MODE_KERN in
the tools/ version, I realized that the build would always fail because
insn.c references it:

In file included from arch/x86/decode.c:12:
arch/x86/../../../arch/x86/lib/insn.c: In function ‘insn_decode’:
arch/x86/../../../arch/x86/lib/insn.c:751:11: error: ‘INSN_MODE_KERN’ undeclared (first use in this function); did you mean ‘INSN_MODE_64’?
  751 |  if (m == INSN_MODE_KERN)
      |           ^~~~~~~~~~~~~~
      |           INSN_MODE_64

and making that work would turn pretty ugly because I wanna avoid
slapping that __ignore_sync_check__ or whatever on more than one line.

So I need to think about a better solution here... 

---
diff --git a/arch/x86/include/asm/inat.h b/arch/x86/include/asm/inat.h
index 4cf2ad521f65..b56c5741581a 100644
--- a/arch/x86/include/asm/inat.h
+++ b/arch/x86/include/asm/inat.h
@@ -6,7 +6,7 @@
  *
  * Written by Masami Hiramatsu <mhiramat@redhat.com>
  */
-#include <asm/inat_types.h>
+#include <asm/inat_types.h> /* __ignore_sync_check__ */
 
 /*
  * Internal bits. Don't use bitmasks directly, because these bits are
diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index 9f1910284861..601eac7a4973 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -8,7 +8,7 @@
  */
 
 /* insn_attr_t is defined in inat.h */
-#include <asm/inat.h>
+#include <asm/inat.h> /* __ignore_sync_check__ */
 
 struct insn_field {
 	union {
@@ -99,7 +99,7 @@ enum insn_mode {
 	INSN_MODE_32,
 	INSN_MODE_64,
 	/* Mode is determined by the current kernel build. */
-	INSN_MODE_KERN,
+	INSN_MODE_KERN, /* __ignore_sync_check__ */
 	INSN_NUM_MODES,
 };
 
diff --git a/arch/x86/lib/inat.c b/arch/x86/lib/inat.c
index 12539fca75c4..b0f3b2a62ae2 100644
--- a/arch/x86/lib/inat.c
+++ b/arch/x86/lib/inat.c
@@ -4,7 +4,7 @@
  *
  * Written by Masami Hiramatsu <mhiramat@redhat.com>
  */
-#include <asm/insn.h>
+#include <asm/insn.h> /* __ignore_sync_check__ */
 
 /* Attribute tables are generated from opcode map */
 #include "inat-tables.c"
diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
index 2ab1d0256313..1295003fb4f7 100644
--- a/arch/x86/lib/insn.c
+++ b/arch/x86/lib/insn.c
@@ -10,13 +10,13 @@
 #else
 #include <string.h>
 #endif
-#include <asm/inat.h>
-#include <asm/insn.h>
+#include <asm/inat.h> /*__ignore_sync_check__ */
+#include <asm/insn.h> /* __ignore_sync_check__ */
 
 #include <linux/errno.h>
 #include <linux/kconfig.h>
 
-#include <asm/emulate_prefix.h>
+#include <asm/emulate_prefix.h> /* __ignore_sync_check__ */
 
 /* Verify next sizeof(t) bytes can be on the same instruction */
 #define validate_next(t, insn, n)	\
diff --git a/tools/arch/x86/include/asm/inat.h b/tools/arch/x86/include/asm/inat.h
index 877827b7c2c3..a61051400311 100644
--- a/tools/arch/x86/include/asm/inat.h
+++ b/tools/arch/x86/include/asm/inat.h
@@ -6,7 +6,7 @@
  *
  * Written by Masami Hiramatsu <mhiramat@redhat.com>
  */
-#include "inat_types.h"
+#include "inat_types.h" /* __ignore_sync_check__ */
 
 /*
  * Internal bits. Don't use bitmasks directly, because these bits are
diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index f8772b371452..b12329de4e6e 100644
--- a/tools/arch/x86/include/asm/insn.h
+++ b/tools/arch/x86/include/asm/insn.h
@@ -8,7 +8,7 @@
  */
 
 /* insn_attr_t is defined in inat.h */
-#include "inat.h"
+#include "inat.h" /* __ignore_sync_check__ */
 
 struct insn_field {
 	union {
@@ -99,7 +99,7 @@ enum insn_mode {
 	INSN_MODE_32,
 	INSN_MODE_64,
 	/* Mode is determined by the current kernel build. */
-	INSN_MODE_KERN,
+	/* INSN_MODE_KERN, __ignore_sync_check__ */
 	INSN_NUM_MODES,
 };
 
diff --git a/tools/arch/x86/lib/inat.c b/tools/arch/x86/lib/inat.c
index 4f5ed49e1b4e..dfbcc6405941 100644
--- a/tools/arch/x86/lib/inat.c
+++ b/tools/arch/x86/lib/inat.c
@@ -4,7 +4,7 @@
  *
  * Written by Masami Hiramatsu <mhiramat@redhat.com>
  */
-#include "../include/asm/insn.h"
+#include "../include/asm/insn.h" /* __ignore_sync_check__ */
 
 /* Attribute tables are generated from opcode map */
 #include "inat-tables.c"
diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
index c224e1569034..0824ae531019 100644
--- a/tools/arch/x86/lib/insn.c
+++ b/tools/arch/x86/lib/insn.c
@@ -10,13 +10,13 @@
 #else
 #include <string.h>
 #endif
-#include "../include/asm/inat.h"
-#include "../include/asm/insn.h"
+#include "../include/asm/inat.h" /* __ignore_sync_check__ */
+#include "../include/asm/insn.h" /* __ignore_sync_check__ */
 
 #include <linux/errno.h>
 #include <linux/kconfig.h>
 
-#include "../include/asm/emulate_prefix.h"
+#include "../include/asm/emulate_prefix.h" /* __ignore_sync_check__ */
 
 /* Verify next sizeof(t) bytes can be on the same instruction */
 #define validate_next(t, insn, n)	\
diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index dded93a2bc89..46ee37c87a80 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -75,6 +75,13 @@ include/uapi/asm-generic/mman-common.h
 include/uapi/asm-generic/unistd.h
 '
 
+SYNC_CHECK_FILES='
+arch/x86/include/asm/inat.h
+arch/x86/include/asm/insn.h
+arch/x86/lib/inat.c
+arch/x86/lib/insn.c
+'
+
 # These copies are under tools/perf/trace/beauty/ as they are not used to in
 # building object files only by scripts in tools/perf/trace/beauty/ to generate
 # tables that then gets included in .c files for things like id->string syscall
@@ -129,6 +136,10 @@ for i in $FILES; do
   check $i -B
 done
 
+for i in $SYNC_CHECK_FILES; do
+  check $i '-I "^.*\/*.*__ignore_sync_check__ \*/.*$"'
+done
+
 # diff with extra ignore lines
 check arch/x86/lib/memcpy_64.S        '-I "^EXPORT_SYMBOL" -I "^#include <asm/export.h>" -I"^SYM_FUNC_START\(_LOCAL\)*(memcpy_\(erms\|orig\))"'
 check arch/x86/lib/memset_64.S        '-I "^EXPORT_SYMBOL" -I "^#include <asm/export.h>" -I"^SYM_FUNC_START\(_LOCAL\)*(memset_\(erms\|orig\))"'
@@ -137,10 +148,6 @@ check include/uapi/linux/mman.h       '-I "^#include <\(uapi/\)*asm/mman.h>"'
 check include/linux/build_bug.h       '-I "^#\(ifndef\|endif\)\( \/\/\)* static_assert$"'
 check include/linux/ctype.h	      '-I "isdigit("'
 check lib/ctype.c		      '-I "^EXPORT_SYMBOL" -I "^#include <linux/export.h>" -B'
-check arch/x86/include/asm/inat.h     '-I "^#include [\"<]\(asm/\)*inat_types.h[\">]"'
-check arch/x86/include/asm/insn.h     '-I "^#include [\"<]\(asm/\)*inat.h[\">]"'
-check arch/x86/lib/inat.c	      '-I "^#include [\"<]\(../include/\)*asm/insn.h[\">]"'
-check arch/x86/lib/insn.c             '-I "^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]" -I "^#include [\"<]\(../include/\)*asm/emulate_prefix.h[\">]"'
 
 # diff non-symmetric files
 check_2 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl arch/x86/entry/syscalls/syscall_64.tbl
Masami Hiramatsu Jan. 12, 2021, 11:34 a.m. UTC | #7
On Fri, 8 Jan 2021 19:59:50 +0100
Borislav Petkov <bp@alien8.de> wrote:

> On Wed, Jan 06, 2021 at 02:21:14PM +0900, Masami Hiramatsu wrote:
> > So I think it is possible to introduce a keyword in a comment
> > for ignoring sync check something like below. This will allow us
> > a generic pattern matching.
> > 
> > The keyword is just an example, "no-sync-check" etc. is OK.
> > 
> > What would you think about it?
> 
> Yeah, I'd prefer a single keyword which to slap everywhere, see below.
> The patch is only for demonstration, though, it is not complete.

Yes, that looks good to me too.

> 
> And while playing with that after having commented out INSN_MODE_KERN in
> the tools/ version, I realized that the build would always fail because
> insn.c references it:
> 
> In file included from arch/x86/decode.c:12:
> arch/x86/../../../arch/x86/lib/insn.c: In function ‘insn_decode’:
> arch/x86/../../../arch/x86/lib/insn.c:751:11: error: ‘INSN_MODE_KERN’ undeclared (first use in this function); did you mean ‘INSN_MODE_64’?
>   751 |  if (m == INSN_MODE_KERN)
>       |           ^~~~~~~~~~~~~~
>       |           INSN_MODE_64
> 
> and making that work would turn pretty ugly because I wanna avoid
> slapping that __ignore_sync_check__ or whatever on more than one line.
> 
> So I need to think about a better solution here... 

Hmm, instead of removing INSN_MODE_KERN, if you just return an error,
it will change one line.

	if (m == INSN_MODE_KERN)
		return -EINVAL;	/* __ignore_sync_check__ */
	else
		insn_init(insn, kaddr, buf_len, m == INSN_MODE_64);

Or, add one definition before that line.

#define INSN_MODE_KERN -1	/* __ignore_sync_check__ */

Thank you,


> 
> ---
> diff --git a/arch/x86/include/asm/inat.h b/arch/x86/include/asm/inat.h
> index 4cf2ad521f65..b56c5741581a 100644
> --- a/arch/x86/include/asm/inat.h
> +++ b/arch/x86/include/asm/inat.h
> @@ -6,7 +6,7 @@
>   *
>   * Written by Masami Hiramatsu <mhiramat@redhat.com>
>   */
> -#include <asm/inat_types.h>
> +#include <asm/inat_types.h> /* __ignore_sync_check__ */
>  
>  /*
>   * Internal bits. Don't use bitmasks directly, because these bits are
> diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
> index 9f1910284861..601eac7a4973 100644
> --- a/arch/x86/include/asm/insn.h
> +++ b/arch/x86/include/asm/insn.h
> @@ -8,7 +8,7 @@
>   */
>  
>  /* insn_attr_t is defined in inat.h */
> -#include <asm/inat.h>
> +#include <asm/inat.h> /* __ignore_sync_check__ */
>  
>  struct insn_field {
>  	union {
> @@ -99,7 +99,7 @@ enum insn_mode {
>  	INSN_MODE_32,
>  	INSN_MODE_64,
>  	/* Mode is determined by the current kernel build. */
> -	INSN_MODE_KERN,
> +	INSN_MODE_KERN, /* __ignore_sync_check__ */
>  	INSN_NUM_MODES,
>  };
>  
> diff --git a/arch/x86/lib/inat.c b/arch/x86/lib/inat.c
> index 12539fca75c4..b0f3b2a62ae2 100644
> --- a/arch/x86/lib/inat.c
> +++ b/arch/x86/lib/inat.c
> @@ -4,7 +4,7 @@
>   *
>   * Written by Masami Hiramatsu <mhiramat@redhat.com>
>   */
> -#include <asm/insn.h>
> +#include <asm/insn.h> /* __ignore_sync_check__ */
>  
>  /* Attribute tables are generated from opcode map */
>  #include "inat-tables.c"
> diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
> index 2ab1d0256313..1295003fb4f7 100644
> --- a/arch/x86/lib/insn.c
> +++ b/arch/x86/lib/insn.c
> @@ -10,13 +10,13 @@
>  #else
>  #include <string.h>
>  #endif
> -#include <asm/inat.h>
> -#include <asm/insn.h>
> +#include <asm/inat.h> /*__ignore_sync_check__ */
> +#include <asm/insn.h> /* __ignore_sync_check__ */
>  
>  #include <linux/errno.h>
>  #include <linux/kconfig.h>
>  
> -#include <asm/emulate_prefix.h>
> +#include <asm/emulate_prefix.h> /* __ignore_sync_check__ */
>  
>  /* Verify next sizeof(t) bytes can be on the same instruction */
>  #define validate_next(t, insn, n)	\
> diff --git a/tools/arch/x86/include/asm/inat.h b/tools/arch/x86/include/asm/inat.h
> index 877827b7c2c3..a61051400311 100644
> --- a/tools/arch/x86/include/asm/inat.h
> +++ b/tools/arch/x86/include/asm/inat.h
> @@ -6,7 +6,7 @@
>   *
>   * Written by Masami Hiramatsu <mhiramat@redhat.com>
>   */
> -#include "inat_types.h"
> +#include "inat_types.h" /* __ignore_sync_check__ */
>  
>  /*
>   * Internal bits. Don't use bitmasks directly, because these bits are
> diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
> index f8772b371452..b12329de4e6e 100644
> --- a/tools/arch/x86/include/asm/insn.h
> +++ b/tools/arch/x86/include/asm/insn.h
> @@ -8,7 +8,7 @@
>   */
>  
>  /* insn_attr_t is defined in inat.h */
> -#include "inat.h"
> +#include "inat.h" /* __ignore_sync_check__ */
>  
>  struct insn_field {
>  	union {
> @@ -99,7 +99,7 @@ enum insn_mode {
>  	INSN_MODE_32,
>  	INSN_MODE_64,
>  	/* Mode is determined by the current kernel build. */
> -	INSN_MODE_KERN,
> +	/* INSN_MODE_KERN, __ignore_sync_check__ */
>  	INSN_NUM_MODES,
>  };
>  
> diff --git a/tools/arch/x86/lib/inat.c b/tools/arch/x86/lib/inat.c
> index 4f5ed49e1b4e..dfbcc6405941 100644
> --- a/tools/arch/x86/lib/inat.c
> +++ b/tools/arch/x86/lib/inat.c
> @@ -4,7 +4,7 @@
>   *
>   * Written by Masami Hiramatsu <mhiramat@redhat.com>
>   */
> -#include "../include/asm/insn.h"
> +#include "../include/asm/insn.h" /* __ignore_sync_check__ */
>  
>  /* Attribute tables are generated from opcode map */
>  #include "inat-tables.c"
> diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
> index c224e1569034..0824ae531019 100644
> --- a/tools/arch/x86/lib/insn.c
> +++ b/tools/arch/x86/lib/insn.c
> @@ -10,13 +10,13 @@
>  #else
>  #include <string.h>
>  #endif
> -#include "../include/asm/inat.h"
> -#include "../include/asm/insn.h"
> +#include "../include/asm/inat.h" /* __ignore_sync_check__ */
> +#include "../include/asm/insn.h" /* __ignore_sync_check__ */
>  
>  #include <linux/errno.h>
>  #include <linux/kconfig.h>
>  
> -#include "../include/asm/emulate_prefix.h"
> +#include "../include/asm/emulate_prefix.h" /* __ignore_sync_check__ */
>  
>  /* Verify next sizeof(t) bytes can be on the same instruction */
>  #define validate_next(t, insn, n)	\
> diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
> index dded93a2bc89..46ee37c87a80 100755
> --- a/tools/perf/check-headers.sh
> +++ b/tools/perf/check-headers.sh
> @@ -75,6 +75,13 @@ include/uapi/asm-generic/mman-common.h
>  include/uapi/asm-generic/unistd.h
>  '
>  
> +SYNC_CHECK_FILES='
> +arch/x86/include/asm/inat.h
> +arch/x86/include/asm/insn.h
> +arch/x86/lib/inat.c
> +arch/x86/lib/insn.c
> +'
> +
>  # These copies are under tools/perf/trace/beauty/ as they are not used to in
>  # building object files only by scripts in tools/perf/trace/beauty/ to generate
>  # tables that then gets included in .c files for things like id->string syscall
> @@ -129,6 +136,10 @@ for i in $FILES; do
>    check $i -B
>  done
>  
> +for i in $SYNC_CHECK_FILES; do
> +  check $i '-I "^.*\/*.*__ignore_sync_check__ \*/.*$"'
> +done
> +
>  # diff with extra ignore lines
>  check arch/x86/lib/memcpy_64.S        '-I "^EXPORT_SYMBOL" -I "^#include <asm/export.h>" -I"^SYM_FUNC_START\(_LOCAL\)*(memcpy_\(erms\|orig\))"'
>  check arch/x86/lib/memset_64.S        '-I "^EXPORT_SYMBOL" -I "^#include <asm/export.h>" -I"^SYM_FUNC_START\(_LOCAL\)*(memset_\(erms\|orig\))"'
> @@ -137,10 +148,6 @@ check include/uapi/linux/mman.h       '-I "^#include <\(uapi/\)*asm/mman.h>"'
>  check include/linux/build_bug.h       '-I "^#\(ifndef\|endif\)\( \/\/\)* static_assert$"'
>  check include/linux/ctype.h	      '-I "isdigit("'
>  check lib/ctype.c		      '-I "^EXPORT_SYMBOL" -I "^#include <linux/export.h>" -B'
> -check arch/x86/include/asm/inat.h     '-I "^#include [\"<]\(asm/\)*inat_types.h[\">]"'
> -check arch/x86/include/asm/insn.h     '-I "^#include [\"<]\(asm/\)*inat.h[\">]"'
> -check arch/x86/lib/inat.c	      '-I "^#include [\"<]\(../include/\)*asm/insn.h[\">]"'
> -check arch/x86/lib/insn.c             '-I "^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]" -I "^#include [\"<]\(../include/\)*asm/emulate_prefix.h[\">]"'
>  
>  # diff non-symmetric files
>  check_2 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl arch/x86/entry/syscalls/syscall_64.tbl
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
Borislav Petkov Jan. 13, 2021, 6:06 p.m. UTC | #8
On Tue, Jan 12, 2021 at 08:34:46PM +0900, Masami Hiramatsu wrote:
> Or, add one definition before that line.
> 
> #define INSN_MODE_KERN -1	/* __ignore_sync_check__ */

I like that idea, thanks! And it seems to work:

---

diff --git a/arch/x86/include/asm/inat.h b/arch/x86/include/asm/inat.h
index 4cf2ad521f65..b56c5741581a 100644
--- a/arch/x86/include/asm/inat.h
+++ b/arch/x86/include/asm/inat.h
@@ -6,7 +6,7 @@
  *
  * Written by Masami Hiramatsu <mhiramat@redhat.com>
  */
-#include <asm/inat_types.h>
+#include <asm/inat_types.h> /* __ignore_sync_check__ */
 
 /*
  * Internal bits. Don't use bitmasks directly, because these bits are
diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index 9f1910284861..6df0d3da0d86 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -8,7 +8,7 @@
  */
 
 /* insn_attr_t is defined in inat.h */
-#include <asm/inat.h>
+#include <asm/inat.h> /* __ignore_sync_check__ */
 
 struct insn_field {
 	union {
diff --git a/arch/x86/lib/inat.c b/arch/x86/lib/inat.c
index 12539fca75c4..b0f3b2a62ae2 100644
--- a/arch/x86/lib/inat.c
+++ b/arch/x86/lib/inat.c
@@ -4,7 +4,7 @@
  *
  * Written by Masami Hiramatsu <mhiramat@redhat.com>
  */
-#include <asm/insn.h>
+#include <asm/insn.h> /* __ignore_sync_check__ */
 
 /* Attribute tables are generated from opcode map */
 #include "inat-tables.c"
diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
index 2ab1d0256313..aa6ee796a987 100644
--- a/arch/x86/lib/insn.c
+++ b/arch/x86/lib/insn.c
@@ -10,13 +10,13 @@
 #else
 #include <string.h>
 #endif
-#include <asm/inat.h>
-#include <asm/insn.h>
+#include <asm/inat.h> /*__ignore_sync_check__ */
+#include <asm/insn.h> /* __ignore_sync_check__ */
 
 #include <linux/errno.h>
 #include <linux/kconfig.h>
 
-#include <asm/emulate_prefix.h>
+#include <asm/emulate_prefix.h> /* __ignore_sync_check__ */
 
 /* Verify next sizeof(t) bytes can be on the same instruction */
 #define validate_next(t, insn, n)	\
@@ -748,6 +748,8 @@ int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mod
 {
 	int ret;
 
+/* #define INSN_MODE_KERN	-1 __ignore_sync_check__ mode is only valid in the kernel */
+
 	if (m == INSN_MODE_KERN)
 		insn_init(insn, kaddr, buf_len, IS_ENABLED(CONFIG_X86_64));
 	else
diff --git a/tools/arch/x86/include/asm/inat.h b/tools/arch/x86/include/asm/inat.h
index 877827b7c2c3..a61051400311 100644
--- a/tools/arch/x86/include/asm/inat.h
+++ b/tools/arch/x86/include/asm/inat.h
@@ -6,7 +6,7 @@
  *
  * Written by Masami Hiramatsu <mhiramat@redhat.com>
  */
-#include "inat_types.h"
+#include "inat_types.h" /* __ignore_sync_check__ */
 
 /*
  * Internal bits. Don't use bitmasks directly, because these bits are
diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index f8772b371452..4f219e3ae817 100644
--- a/tools/arch/x86/include/asm/insn.h
+++ b/tools/arch/x86/include/asm/insn.h
@@ -8,7 +8,7 @@
  */
 
 /* insn_attr_t is defined in inat.h */
-#include "inat.h"
+#include "inat.h" /* __ignore_sync_check__ */
 
 struct insn_field {
 	union {
diff --git a/tools/arch/x86/lib/inat.c b/tools/arch/x86/lib/inat.c
index 4f5ed49e1b4e..dfbcc6405941 100644
--- a/tools/arch/x86/lib/inat.c
+++ b/tools/arch/x86/lib/inat.c
@@ -4,7 +4,7 @@
  *
  * Written by Masami Hiramatsu <mhiramat@redhat.com>
  */
-#include "../include/asm/insn.h"
+#include "../include/asm/insn.h" /* __ignore_sync_check__ */
 
 /* Attribute tables are generated from opcode map */
 #include "inat-tables.c"
diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
index c224e1569034..13e8615edc15 100644
--- a/tools/arch/x86/lib/insn.c
+++ b/tools/arch/x86/lib/insn.c
@@ -10,13 +10,13 @@
 #else
 #include <string.h>
 #endif
-#include "../include/asm/inat.h"
-#include "../include/asm/insn.h"
+#include "../include/asm/inat.h" /* __ignore_sync_check__ */
+#include "../include/asm/insn.h" /* __ignore_sync_check__ */
 
 #include <linux/errno.h>
 #include <linux/kconfig.h>
 
-#include "../include/asm/emulate_prefix.h"
+#include "../include/asm/emulate_prefix.h" /* __ignore_sync_check__ */
 
 /* Verify next sizeof(t) bytes can be on the same instruction */
 #define validate_next(t, insn, n)	\
@@ -748,6 +748,8 @@ int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mod
 {
 	int ret;
 
+#define INSN_MODE_KERN	-1 /* __ignore_sync_check__ mode is only valid in the kernel */
+
 	if (m == INSN_MODE_KERN)
 		insn_init(insn, kaddr, buf_len, IS_ENABLED(CONFIG_X86_64));
 	else
diff --git a/tools/objtool/sync-check.sh b/tools/objtool/sync-check.sh
index 606a4b5e929f..4bbabaecab14 100755
--- a/tools/objtool/sync-check.sh
+++ b/tools/objtool/sync-check.sh
@@ -16,11 +16,14 @@ arch/x86/include/asm/emulate_prefix.h
 arch/x86/lib/x86-opcode-map.txt
 arch/x86/tools/gen-insn-attr-x86.awk
 include/linux/static_call_types.h
-arch/x86/include/asm/inat.h     -I '^#include [\"<]\(asm/\)*inat_types.h[\">]'
-arch/x86/include/asm/insn.h     -I '^#include [\"<]\(asm/\)*inat.h[\">]'
-arch/x86/lib/inat.c             -I '^#include [\"<]\(../include/\)*asm/insn.h[\">]'
-arch/x86/lib/insn.c             -I '^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]' -I '^#include [\"<]\(../include/\)*asm/emulate_prefix.h[\">]'
 "
+
+SYNC_CHECK_FILES='
+arch/x86/include/asm/inat.h
+arch/x86/include/asm/insn.h
+arch/x86/lib/inat.c
+arch/x86/lib/insn.c
+'
 fi
 
 check_2 () {
@@ -63,3 +66,9 @@ while read -r file_entry; do
 done <<EOF
 $FILES
 EOF
+
+if [ "$SRCARCH" = "x86" ]; then
+	for i in $SYNC_CHECK_FILES; do
+		check $i '-I "^.*\/\*.*__ignore_sync_check__.*\*\/.*$"'
+	done
+fi
diff --git a/tools/perf/check-headers.sh b/tools/perf/check-headers.sh
index dded93a2bc89..07857dfb4d91 100755
--- a/tools/perf/check-headers.sh
+++ b/tools/perf/check-headers.sh
@@ -75,6 +75,13 @@ include/uapi/asm-generic/mman-common.h
 include/uapi/asm-generic/unistd.h
 '
 
+SYNC_CHECK_FILES='
+arch/x86/include/asm/inat.h
+arch/x86/include/asm/insn.h
+arch/x86/lib/inat.c
+arch/x86/lib/insn.c
+'
+
 # These copies are under tools/perf/trace/beauty/ as they are not used to in
 # building object files only by scripts in tools/perf/trace/beauty/ to generate
 # tables that then gets included in .c files for things like id->string syscall
@@ -129,6 +136,10 @@ for i in $FILES; do
   check $i -B
 done
 
+for i in $SYNC_CHECK_FILES; do
+  check $i '-I "^.*\/\*.*__ignore_sync_check__.*\*\/.*$"'
+done
+
 # diff with extra ignore lines
 check arch/x86/lib/memcpy_64.S        '-I "^EXPORT_SYMBOL" -I "^#include <asm/export.h>" -I"^SYM_FUNC_START\(_LOCAL\)*(memcpy_\(erms\|orig\))"'
 check arch/x86/lib/memset_64.S        '-I "^EXPORT_SYMBOL" -I "^#include <asm/export.h>" -I"^SYM_FUNC_START\(_LOCAL\)*(memset_\(erms\|orig\))"'
@@ -137,10 +148,6 @@ check include/uapi/linux/mman.h       '-I "^#include <\(uapi/\)*asm/mman.h>"'
 check include/linux/build_bug.h       '-I "^#\(ifndef\|endif\)\( \/\/\)* static_assert$"'
 check include/linux/ctype.h	      '-I "isdigit("'
 check lib/ctype.c		      '-I "^EXPORT_SYMBOL" -I "^#include <linux/export.h>" -B'
-check arch/x86/include/asm/inat.h     '-I "^#include [\"<]\(asm/\)*inat_types.h[\">]"'
-check arch/x86/include/asm/insn.h     '-I "^#include [\"<]\(asm/\)*inat.h[\">]"'
-check arch/x86/lib/inat.c	      '-I "^#include [\"<]\(../include/\)*asm/insn.h[\">]"'
-check arch/x86/lib/insn.c             '-I "^#include [\"<]\(../include/\)*asm/in\(at\|sn\).h[\">]" -I "^#include [\"<]\(../include/\)*asm/emulate_prefix.h[\">]"'
 
 # diff non-symmetric files
 check_2 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl arch/x86/entry/syscalls/syscall_64.tbl

Patch
diff mbox series

diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
index a8c3d284fa46..088aa90e9158 100644
--- a/arch/x86/include/asm/insn.h
+++ b/arch/x86/include/asm/insn.h
@@ -87,13 +87,23 @@  struct insn {
 #define X86_VEX_M_MAX	0x1f			/* VEX3.M Maximum value */
 
 extern void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64);
-extern void insn_get_prefixes(struct insn *insn);
-extern void insn_get_opcode(struct insn *insn);
-extern void insn_get_modrm(struct insn *insn);
-extern void insn_get_sib(struct insn *insn);
-extern void insn_get_displacement(struct insn *insn);
-extern void insn_get_immediate(struct insn *insn);
-extern void insn_get_length(struct insn *insn);
+extern int insn_get_prefixes(struct insn *insn);
+extern int insn_get_opcode(struct insn *insn);
+extern int insn_get_modrm(struct insn *insn);
+extern int insn_get_sib(struct insn *insn);
+extern int insn_get_displacement(struct insn *insn);
+extern int insn_get_immediate(struct insn *insn);
+extern int insn_get_length(struct insn *insn);
+
+enum insn_mode {
+	INSN_MODE_32,
+	INSN_MODE_64,
+	/* Mode is determined by the current kernel build. */
+	INSN_MODE_KERN,
+	INSN_NUM_MODES,
+};
+
+extern int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mode m);
 
 /* Attribute will be determined after getting ModRM (for opcode groups) */
 static inline void insn_get_attribute(struct insn *insn)
diff --git a/arch/x86/lib/insn.c b/arch/x86/lib/insn.c
index 1ba994862b56..b373aadcdd02 100644
--- a/arch/x86/lib/insn.c
+++ b/arch/x86/lib/insn.c
@@ -13,6 +13,9 @@ 
 #include <asm/inat.h>
 #include <asm/insn.h>
 
+#include <linux/errno.h>
+#include <linux/kconfig.h>
+
 #include <asm/emulate_prefix.h>
 
 /* Verify next sizeof(t) bytes can be on the same instruction */
@@ -80,12 +83,15 @@  static int __insn_get_emulate_prefix(struct insn *insn,
 	return 1;
 
 err_out:
-	return 0;
+	return -ENODATA;
 }
 
 static void insn_get_emulate_prefix(struct insn *insn)
 {
-	if (__insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix)))
+	int ret;
+
+	ret = __insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix));
+	if (ret > 0)
 		return;
 
 	__insn_get_emulate_prefix(insn, kvm_prefix, sizeof(kvm_prefix));
@@ -98,8 +104,12 @@  static void insn_get_emulate_prefix(struct insn *insn)
  * Populates the @insn->prefixes bitmap, and updates @insn->next_byte
  * to point to the (first) opcode.  No effect if @insn->prefixes.got
  * is already set.
+ *
+ * * Returns:
+ * 0:  on success
+ * < 0: on error
  */
-void insn_get_prefixes(struct insn *insn)
+int insn_get_prefixes(struct insn *insn)
 {
 	struct insn_field *prefixes = &insn->prefixes;
 	insn_attr_t attr;
@@ -107,7 +117,7 @@  void insn_get_prefixes(struct insn *insn)
 	int i, nb;
 
 	if (prefixes->got)
-		return;
+		return 0;
 
 	insn_get_emulate_prefix(insn);
 
@@ -218,8 +228,10 @@  void insn_get_prefixes(struct insn *insn)
 
 	prefixes->got = 1;
 
+	return 0;
+
 err_out:
-	return;
+	return -ENODATA;
 }
 
 /**
@@ -231,16 +243,25 @@  void insn_get_prefixes(struct insn *insn)
  * If necessary, first collects any preceding (prefix) bytes.
  * Sets @insn->opcode.value = opcode1.  No effect if @insn->opcode.got
  * is already 1.
+ *
+ * Returns:
+ * 0:  on success
+ * < 0: on error
  */
-void insn_get_opcode(struct insn *insn)
+int insn_get_opcode(struct insn *insn)
 {
 	struct insn_field *opcode = &insn->opcode;
+	int pfx_id, ret;
 	insn_byte_t op;
-	int pfx_id;
+
 	if (opcode->got)
-		return;
-	if (!insn->prefixes.got)
-		insn_get_prefixes(insn);
+		return 0;
+
+	if (!insn->prefixes.got) {
+		ret = insn_get_prefixes(insn);
+		if (ret)
+			return ret;
+	}
 
 	/* Get first opcode */
 	op = get_next(insn_byte_t, insn);
@@ -255,9 +276,13 @@  void insn_get_opcode(struct insn *insn)
 		insn->attr = inat_get_avx_attribute(op, m, p);
 		if ((inat_must_evex(insn->attr) && !insn_is_evex(insn)) ||
 		    (!inat_accept_vex(insn->attr) &&
-		     !inat_is_group(insn->attr)))
-			insn->attr = 0;	/* This instruction is bad */
-		goto end;	/* VEX has only 1 byte for opcode */
+		     !inat_is_group(insn->attr))) {
+			/* This instruction is bad */
+			insn->attr = 0;
+			return 1;
+		}
+		/* VEX has only 1 byte for opcode */
+		goto end;
 	}
 
 	insn->attr = inat_get_opcode_attribute(op);
@@ -268,13 +293,18 @@  void insn_get_opcode(struct insn *insn)
 		pfx_id = insn_last_prefix_id(insn);
 		insn->attr = inat_get_escape_attribute(op, pfx_id, insn->attr);
 	}
-	if (inat_must_vex(insn->attr))
-		insn->attr = 0;	/* This instruction is bad */
+
+	if (inat_must_vex(insn->attr)) {
+		/* This instruction is bad */
+		insn->attr = 0;
+		return 1;
+	}
 end:
 	opcode->got = 1;
+	return 0;
 
 err_out:
-	return;
+	return -ENODATA;
 }
 
 /**
@@ -284,15 +314,25 @@  void insn_get_opcode(struct insn *insn)
  * Populates @insn->modrm and updates @insn->next_byte to point past the
  * ModRM byte, if any.  If necessary, first collects the preceding bytes
  * (prefixes and opcode(s)).  No effect if @insn->modrm.got is already 1.
+ *
+ * Returns:
+ * 0:  on success
+ * < 0: on error
  */
-void insn_get_modrm(struct insn *insn)
+int insn_get_modrm(struct insn *insn)
 {
 	struct insn_field *modrm = &insn->modrm;
 	insn_byte_t pfx_id, mod;
+	int ret;
+
 	if (modrm->got)
-		return;
-	if (!insn->opcode.got)
-		insn_get_opcode(insn);
+		return 0;
+
+	if (!insn->opcode.got) {
+		ret = insn_get_opcode(insn);
+		if (ret)
+			return ret;
+	}
 
 	if (inat_has_modrm(insn->attr)) {
 		mod = get_next(insn_byte_t, insn);
@@ -302,17 +342,22 @@  void insn_get_modrm(struct insn *insn)
 			pfx_id = insn_last_prefix_id(insn);
 			insn->attr = inat_get_group_attribute(mod, pfx_id,
 							      insn->attr);
-			if (insn_is_avx(insn) && !inat_accept_vex(insn->attr))
-				insn->attr = 0;	/* This is bad */
+			if (insn_is_avx(insn) && !inat_accept_vex(insn->attr)) {
+				/* Bad insn */
+				insn->attr = 0;
+				return 1;
+			}
 		}
 	}
 
 	if (insn->x86_64 && inat_is_force64(insn->attr))
 		insn->opnd_bytes = 8;
+
 	modrm->got = 1;
+	return 0;
 
 err_out:
-	return;
+	return -ENODATA;
 }
 
 
@@ -326,11 +371,16 @@  void insn_get_modrm(struct insn *insn)
 int insn_rip_relative(struct insn *insn)
 {
 	struct insn_field *modrm = &insn->modrm;
+	int ret;
 
 	if (!insn->x86_64)
 		return 0;
-	if (!modrm->got)
-		insn_get_modrm(insn);
+
+	if (!modrm->got) {
+		ret = insn_get_modrm(insn);
+		if (ret)
+			return ret;
+	}
 	/*
 	 * For rip-relative instructions, the mod field (top 2 bits)
 	 * is zero and the r/m field (bottom 3 bits) is 0x5.
@@ -344,15 +394,25 @@  int insn_rip_relative(struct insn *insn)
  *
  * If necessary, first collects the instruction up to and including the
  * ModRM byte.
+ *
+ * Returns:
+ * 0: if decoding succeeded
+ * < 0: otherwise.
  */
-void insn_get_sib(struct insn *insn)
+int insn_get_sib(struct insn *insn)
 {
 	insn_byte_t modrm;
+	int ret;
 
 	if (insn->sib.got)
-		return;
-	if (!insn->modrm.got)
-		insn_get_modrm(insn);
+		return 0;
+
+	if (!insn->modrm.got) {
+		ret = insn_get_modrm(insn);
+		if (ret)
+			return ret;
+	}
+
 	if (insn->modrm.nbytes) {
 		modrm = (insn_byte_t)insn->modrm.value;
 		if (insn->addr_bytes != 2 &&
@@ -363,8 +423,10 @@  void insn_get_sib(struct insn *insn)
 	}
 	insn->sib.got = 1;
 
+	return 0;
+
 err_out:
-	return;
+	return -ENODATA;
 }
 
 
@@ -375,15 +437,25 @@  void insn_get_sib(struct insn *insn)
  * If necessary, first collects the instruction up to and including the
  * SIB byte.
  * Displacement value is sign-expanded.
+ *
+ * * Returns:
+ * 0: if decoding succeeded
+ * < 0: otherwise.
  */
-void insn_get_displacement(struct insn *insn)
+int insn_get_displacement(struct insn *insn)
 {
 	insn_byte_t mod, rm, base;
+	int ret;
 
 	if (insn->displacement.got)
-		return;
-	if (!insn->sib.got)
-		insn_get_sib(insn);
+		return 0;
+
+	if (!insn->sib.got) {
+		ret = insn_get_sib(insn);
+		if (ret)
+			return ret;
+	}
+
 	if (insn->modrm.nbytes) {
 		/*
 		 * Interpreting the modrm byte:
@@ -426,12 +498,13 @@  void insn_get_displacement(struct insn *insn)
 	}
 out:
 	insn->displacement.got = 1;
+	return 0;
 
 err_out:
-	return;
+	return -ENODATA;
 }
 
-/* Decode moffset16/32/64. Return 0 if failed */
+/* Decode moffset16/32/64. Return a negative value if failed. */
 static int __get_moffset(struct insn *insn)
 {
 	switch (insn->addr_bytes) {
@@ -457,10 +530,10 @@  static int __get_moffset(struct insn *insn)
 	return 1;
 
 err_out:
-	return 0;
+	return -ENODATA;
 }
 
-/* Decode imm v32(Iz). Return 0 if failed */
+/* Decode imm v32(Iz). Return a negative value if failed. */
 static int __get_immv32(struct insn *insn)
 {
 	switch (insn->opnd_bytes) {
@@ -480,10 +553,10 @@  static int __get_immv32(struct insn *insn)
 	return 1;
 
 err_out:
-	return 0;
+	return -ENODATA;
 }
 
-/* Decode imm v64(Iv/Ov), Return 0 if failed */
+/* Decode imm v64(Iv/Ov). Return a negative value if failed. */
 static int __get_immv(struct insn *insn)
 {
 	switch (insn->opnd_bytes) {
@@ -507,11 +580,12 @@  static int __get_immv(struct insn *insn)
 	insn->immediate1.got = insn->immediate2.got = 1;
 
 	return 1;
+
 err_out:
-	return 0;
+	return -ENODATA;
 }
 
-/* Decode ptr16:16/32(Ap) */
+/* Decode ptr16:16/32(Ap). Return a negative value if failed. */
 static int __get_immptr(struct insn *insn)
 {
 	switch (insn->opnd_bytes) {
@@ -534,25 +608,36 @@  static int __get_immptr(struct insn *insn)
 	insn->immediate1.got = insn->immediate2.got = 1;
 
 	return 1;
+
 err_out:
-	return 0;
+	return -ENODATA;
 }
 
 /**
- * insn_get_immediate() - Get the immediates of instruction
+ * insn_get_immediate() - Get the immediate in an instruction
  * @insn:	&struct insn containing instruction
  *
  * If necessary, first collects the instruction up to and including the
  * displacement bytes.
  * Basically, most of immediates are sign-expanded. Unsigned-value can be
- * get by bit masking with ((1 << (nbytes * 8)) - 1)
+ * computed by bit masking with ((1 << (nbytes * 8)) - 1)
+ *
+ * Returns:
+ * 0:  on success
+ * < 0: on error
  */
-void insn_get_immediate(struct insn *insn)
+int insn_get_immediate(struct insn *insn)
 {
+	int ret;
+
 	if (insn->immediate.got)
-		return;
-	if (!insn->displacement.got)
-		insn_get_displacement(insn);
+		return 0;
+
+	if (!insn->displacement.got) {
+		ret = insn_get_displacement(insn);
+		if (ret)
+			return ret;
+	}
 
 	if (inat_has_moffset(insn->attr)) {
 		if (!__get_moffset(insn))
@@ -605,9 +690,10 @@  void insn_get_immediate(struct insn *insn)
 	}
 done:
 	insn->immediate.got = 1;
+	return 0;
 
 err_out:
-	return;
+	return -ENODATA;
 }
 
 /**
@@ -616,13 +702,56 @@  void insn_get_immediate(struct insn *insn)
  *
  * If necessary, first collects the instruction up to and including the
  * immediates bytes.
- */
-void insn_get_length(struct insn *insn)
+ *
+ * Returns:
+ *  - 0 on success
+ *  - < 0 on error
+*/
+int insn_get_length(struct insn *insn)
 {
+	int ret;
+
 	if (insn->length)
-		return;
-	if (!insn->immediate.got)
-		insn_get_immediate(insn);
+		return 0;
+
+	if (!insn->immediate.got) {
+		ret = insn_get_immediate(insn);
+		if (ret)
+			return ret;
+	}
+
 	insn->length = (unsigned char)((unsigned long)insn->next_byte
 				     - (unsigned long)insn->kaddr);
+
+	return 0;
+}
+
+/**
+ * insn_decode() - Decode an x86 instruction
+ * @insn:	&struct insn to be initialized
+ * @kaddr:	address (in kernel memory) of instruction (or copy thereof)
+ * @buf_len:	length of the insn buffer at @kaddr
+ * @m:		insn mode, see enum insn_mode
+ *
+ * Returns:
+ * 0: if decoding succeeded
+ * < 0: otherwise.
+ */
+int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mode m)
+{
+	int ret;
+
+	if (m == INSN_MODE_KERN)
+		insn_init(insn, kaddr, buf_len, IS_ENABLED(CONFIG_X86_64));
+	else
+		insn_init(insn, kaddr, buf_len, m == INSN_MODE_64);
+
+	ret = insn_get_length(insn);
+	if (ret)
+		return ret;
+
+	if (insn_complete(insn))
+		return 0;
+
+	return -EINVAL;
 }
diff --git a/tools/arch/x86/include/asm/insn.h b/tools/arch/x86/include/asm/insn.h
index 52c6262e6bfd..ed43bf01ebcc 100644
--- a/tools/arch/x86/include/asm/insn.h
+++ b/tools/arch/x86/include/asm/insn.h
@@ -87,13 +87,23 @@  struct insn {
 #define X86_VEX_M_MAX	0x1f			/* VEX3.M Maximum value */
 
 extern void insn_init(struct insn *insn, const void *kaddr, int buf_len, int x86_64);
-extern void insn_get_prefixes(struct insn *insn);
-extern void insn_get_opcode(struct insn *insn);
-extern void insn_get_modrm(struct insn *insn);
-extern void insn_get_sib(struct insn *insn);
-extern void insn_get_displacement(struct insn *insn);
-extern void insn_get_immediate(struct insn *insn);
-extern void insn_get_length(struct insn *insn);
+extern int insn_get_prefixes(struct insn *insn);
+extern int insn_get_opcode(struct insn *insn);
+extern int insn_get_modrm(struct insn *insn);
+extern int insn_get_sib(struct insn *insn);
+extern int insn_get_displacement(struct insn *insn);
+extern int insn_get_immediate(struct insn *insn);
+extern int insn_get_length(struct insn *insn);
+
+enum insn_mode {
+	INSN_MODE_32,
+	INSN_MODE_64,
+	/* Mode is determined by the current kernel build. */
+	INSN_MODE_KERN,
+	INSN_NUM_MODES,
+};
+
+extern int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mode m);
 
 /* Attribute will be determined after getting ModRM (for opcode groups) */
 static inline void insn_get_attribute(struct insn *insn)
diff --git a/tools/arch/x86/lib/insn.c b/tools/arch/x86/lib/insn.c
index f3277d6e4ef2..a2f9d9e177f2 100644
--- a/tools/arch/x86/lib/insn.c
+++ b/tools/arch/x86/lib/insn.c
@@ -13,6 +13,9 @@ 
 #include "../include/asm/inat.h"
 #include "../include/asm/insn.h"
 
+#include <linux/errno.h>
+#include <linux/kconfig.h>
+
 #include "../include/asm/emulate_prefix.h"
 
 /* Verify next sizeof(t) bytes can be on the same instruction */
@@ -80,12 +83,15 @@  static int __insn_get_emulate_prefix(struct insn *insn,
 	return 1;
 
 err_out:
-	return 0;
+	return -ENODATA;
 }
 
 static void insn_get_emulate_prefix(struct insn *insn)
 {
-	if (__insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix)))
+	int ret;
+
+	ret = __insn_get_emulate_prefix(insn, xen_prefix, sizeof(xen_prefix));
+	if (ret > 0)
 		return;
 
 	__insn_get_emulate_prefix(insn, kvm_prefix, sizeof(kvm_prefix));
@@ -98,8 +104,12 @@  static void insn_get_emulate_prefix(struct insn *insn)
  * Populates the @insn->prefixes bitmap, and updates @insn->next_byte
  * to point to the (first) opcode.  No effect if @insn->prefixes.got
  * is already set.
+ *
+ * * Returns:
+ * 0:  on success
+ * < 0: on error
  */
-void insn_get_prefixes(struct insn *insn)
+int insn_get_prefixes(struct insn *insn)
 {
 	struct insn_field *prefixes = &insn->prefixes;
 	insn_attr_t attr;
@@ -107,7 +117,7 @@  void insn_get_prefixes(struct insn *insn)
 	int i, nb;
 
 	if (prefixes->got)
-		return;
+		return 0;
 
 	insn_get_emulate_prefix(insn);
 
@@ -218,8 +228,10 @@  void insn_get_prefixes(struct insn *insn)
 
 	prefixes->got = 1;
 
+	return 0;
+
 err_out:
-	return;
+	return -ENODATA;
 }
 
 /**
@@ -231,16 +243,25 @@  void insn_get_prefixes(struct insn *insn)
  * If necessary, first collects any preceding (prefix) bytes.
  * Sets @insn->opcode.value = opcode1.  No effect if @insn->opcode.got
  * is already 1.
+ *
+ * Returns:
+ * 0:  on success
+ * < 0: on error
  */
-void insn_get_opcode(struct insn *insn)
+int insn_get_opcode(struct insn *insn)
 {
 	struct insn_field *opcode = &insn->opcode;
+	int pfx_id, ret;
 	insn_byte_t op;
-	int pfx_id;
+
 	if (opcode->got)
-		return;
-	if (!insn->prefixes.got)
-		insn_get_prefixes(insn);
+		return 0;
+
+	if (!insn->prefixes.got) {
+		ret = insn_get_prefixes(insn);
+		if (ret)
+			return ret;
+	}
 
 	/* Get first opcode */
 	op = get_next(insn_byte_t, insn);
@@ -255,9 +276,13 @@  void insn_get_opcode(struct insn *insn)
 		insn->attr = inat_get_avx_attribute(op, m, p);
 		if ((inat_must_evex(insn->attr) && !insn_is_evex(insn)) ||
 		    (!inat_accept_vex(insn->attr) &&
-		     !inat_is_group(insn->attr)))
-			insn->attr = 0;	/* This instruction is bad */
-		goto end;	/* VEX has only 1 byte for opcode */
+		     !inat_is_group(insn->attr))) {
+			/* This instruction is bad */
+			insn->attr = 0;
+			return 1;
+		}
+		/* VEX has only 1 byte for opcode */
+		goto end;
 	}
 
 	insn->attr = inat_get_opcode_attribute(op);
@@ -268,13 +293,18 @@  void insn_get_opcode(struct insn *insn)
 		pfx_id = insn_last_prefix_id(insn);
 		insn->attr = inat_get_escape_attribute(op, pfx_id, insn->attr);
 	}
-	if (inat_must_vex(insn->attr))
-		insn->attr = 0;	/* This instruction is bad */
+
+	if (inat_must_vex(insn->attr)) {
+		/* This instruction is bad */
+		insn->attr = 0;
+		return 1;
+	}
 end:
 	opcode->got = 1;
+	return 0;
 
 err_out:
-	return;
+	return -ENODATA;
 }
 
 /**
@@ -284,15 +314,25 @@  void insn_get_opcode(struct insn *insn)
  * Populates @insn->modrm and updates @insn->next_byte to point past the
  * ModRM byte, if any.  If necessary, first collects the preceding bytes
  * (prefixes and opcode(s)).  No effect if @insn->modrm.got is already 1.
+ *
+ * Returns:
+ * 0:  on success
+ * < 0: on error
  */
-void insn_get_modrm(struct insn *insn)
+int insn_get_modrm(struct insn *insn)
 {
 	struct insn_field *modrm = &insn->modrm;
 	insn_byte_t pfx_id, mod;
+	int ret;
+
 	if (modrm->got)
-		return;
-	if (!insn->opcode.got)
-		insn_get_opcode(insn);
+		return 0;
+
+	if (!insn->opcode.got) {
+		ret = insn_get_opcode(insn);
+		if (ret)
+			return ret;
+	}
 
 	if (inat_has_modrm(insn->attr)) {
 		mod = get_next(insn_byte_t, insn);
@@ -302,17 +342,22 @@  void insn_get_modrm(struct insn *insn)
 			pfx_id = insn_last_prefix_id(insn);
 			insn->attr = inat_get_group_attribute(mod, pfx_id,
 							      insn->attr);
-			if (insn_is_avx(insn) && !inat_accept_vex(insn->attr))
-				insn->attr = 0;	/* This is bad */
+			if (insn_is_avx(insn) && !inat_accept_vex(insn->attr)) {
+				/* Bad insn */
+				insn->attr = 0;
+				return 1;
+			}
 		}
 	}
 
 	if (insn->x86_64 && inat_is_force64(insn->attr))
 		insn->opnd_bytes = 8;
+
 	modrm->got = 1;
+	return 0;
 
 err_out:
-	return;
+	return -ENODATA;
 }
 
 
@@ -326,11 +371,16 @@  void insn_get_modrm(struct insn *insn)
 int insn_rip_relative(struct insn *insn)
 {
 	struct insn_field *modrm = &insn->modrm;
+	int ret;
 
 	if (!insn->x86_64)
 		return 0;
-	if (!modrm->got)
-		insn_get_modrm(insn);
+
+	if (!modrm->got) {
+		ret = insn_get_modrm(insn);
+		if (ret)
+			return ret;
+	}
 	/*
 	 * For rip-relative instructions, the mod field (top 2 bits)
 	 * is zero and the r/m field (bottom 3 bits) is 0x5.
@@ -344,15 +394,25 @@  int insn_rip_relative(struct insn *insn)
  *
  * If necessary, first collects the instruction up to and including the
  * ModRM byte.
+ *
+ * Returns:
+ * 0: if decoding succeeded
+ * < 0: otherwise.
  */
-void insn_get_sib(struct insn *insn)
+int insn_get_sib(struct insn *insn)
 {
 	insn_byte_t modrm;
+	int ret;
 
 	if (insn->sib.got)
-		return;
-	if (!insn->modrm.got)
-		insn_get_modrm(insn);
+		return 0;
+
+	if (!insn->modrm.got) {
+		ret = insn_get_modrm(insn);
+		if (ret)
+			return ret;
+	}
+
 	if (insn->modrm.nbytes) {
 		modrm = (insn_byte_t)insn->modrm.value;
 		if (insn->addr_bytes != 2 &&
@@ -363,8 +423,10 @@  void insn_get_sib(struct insn *insn)
 	}
 	insn->sib.got = 1;
 
+	return 0;
+
 err_out:
-	return;
+	return -ENODATA;
 }
 
 
@@ -375,15 +437,25 @@  void insn_get_sib(struct insn *insn)
  * If necessary, first collects the instruction up to and including the
  * SIB byte.
  * Displacement value is sign-expanded.
+ *
+ * * Returns:
+ * 0: if decoding succeeded
+ * < 0: otherwise.
  */
-void insn_get_displacement(struct insn *insn)
+int insn_get_displacement(struct insn *insn)
 {
 	insn_byte_t mod, rm, base;
+	int ret;
 
 	if (insn->displacement.got)
-		return;
-	if (!insn->sib.got)
-		insn_get_sib(insn);
+		return 0;
+
+	if (!insn->sib.got) {
+		ret = insn_get_sib(insn);
+		if (ret)
+			return ret;
+	}
+
 	if (insn->modrm.nbytes) {
 		/*
 		 * Interpreting the modrm byte:
@@ -426,12 +498,13 @@  void insn_get_displacement(struct insn *insn)
 	}
 out:
 	insn->displacement.got = 1;
+	return 0;
 
 err_out:
-	return;
+	return -ENODATA;
 }
 
-/* Decode moffset16/32/64. Return 0 if failed */
+/* Decode moffset16/32/64. Return a negative value if failed. */
 static int __get_moffset(struct insn *insn)
 {
 	switch (insn->addr_bytes) {
@@ -457,10 +530,10 @@  static int __get_moffset(struct insn *insn)
 	return 1;
 
 err_out:
-	return 0;
+	return -ENODATA;
 }
 
-/* Decode imm v32(Iz). Return 0 if failed */
+/* Decode imm v32(Iz). Return a negative value if failed. */
 static int __get_immv32(struct insn *insn)
 {
 	switch (insn->opnd_bytes) {
@@ -480,10 +553,10 @@  static int __get_immv32(struct insn *insn)
 	return 1;
 
 err_out:
-	return 0;
+	return -ENODATA;
 }
 
-/* Decode imm v64(Iv/Ov), Return 0 if failed */
+/* Decode imm v64(Iv/Ov). Return a negative value if failed. */
 static int __get_immv(struct insn *insn)
 {
 	switch (insn->opnd_bytes) {
@@ -507,11 +580,12 @@  static int __get_immv(struct insn *insn)
 	insn->immediate1.got = insn->immediate2.got = 1;
 
 	return 1;
+
 err_out:
-	return 0;
+	return -ENODATA;
 }
 
-/* Decode ptr16:16/32(Ap) */
+/* Decode ptr16:16/32(Ap). Return a negative value if failed. */
 static int __get_immptr(struct insn *insn)
 {
 	switch (insn->opnd_bytes) {
@@ -534,25 +608,36 @@  static int __get_immptr(struct insn *insn)
 	insn->immediate1.got = insn->immediate2.got = 1;
 
 	return 1;
+
 err_out:
-	return 0;
+	return -ENODATA;
 }
 
 /**
- * insn_get_immediate() - Get the immediates of instruction
+ * insn_get_immediate() - Get the immediate in an instruction
  * @insn:	&struct insn containing instruction
  *
  * If necessary, first collects the instruction up to and including the
  * displacement bytes.
  * Basically, most of immediates are sign-expanded. Unsigned-value can be
- * get by bit masking with ((1 << (nbytes * 8)) - 1)
+ * computed by bit masking with ((1 << (nbytes * 8)) - 1)
+ *
+ * Returns:
+ * 0:  on success
+ * < 0: on error
  */
-void insn_get_immediate(struct insn *insn)
+int insn_get_immediate(struct insn *insn)
 {
+	int ret;
+
 	if (insn->immediate.got)
-		return;
-	if (!insn->displacement.got)
-		insn_get_displacement(insn);
+		return 0;
+
+	if (!insn->displacement.got) {
+		ret = insn_get_displacement(insn);
+		if (ret)
+			return ret;
+	}
 
 	if (inat_has_moffset(insn->attr)) {
 		if (!__get_moffset(insn))
@@ -605,9 +690,10 @@  void insn_get_immediate(struct insn *insn)
 	}
 done:
 	insn->immediate.got = 1;
+	return 0;
 
 err_out:
-	return;
+	return -ENODATA;
 }
 
 /**
@@ -616,13 +702,56 @@  void insn_get_immediate(struct insn *insn)
  *
  * If necessary, first collects the instruction up to and including the
  * immediates bytes.
- */
-void insn_get_length(struct insn *insn)
+ *
+ * Returns:
+ *  - 0 on success
+ *  - < 0 on error
+*/
+int insn_get_length(struct insn *insn)
 {
+	int ret;
+
 	if (insn->length)
-		return;
-	if (!insn->immediate.got)
-		insn_get_immediate(insn);
+		return 0;
+
+	if (!insn->immediate.got) {
+		ret = insn_get_immediate(insn);
+		if (ret)
+			return ret;
+	}
+
 	insn->length = (unsigned char)((unsigned long)insn->next_byte
 				     - (unsigned long)insn->kaddr);
+
+	return 0;
+}
+
+/**
+ * insn_decode() - Decode an x86 instruction
+ * @insn:	&struct insn to be initialized
+ * @kaddr:	address (in kernel memory) of instruction (or copy thereof)
+ * @buf_len:	length of the insn buffer at @kaddr
+ * @m:		insn mode, see enum insn_mode
+ *
+ * Returns:
+ * 0: if decoding succeeded
+ * < 0: otherwise.
+ */
+int insn_decode(struct insn *insn, const void *kaddr, int buf_len, enum insn_mode m)
+{
+	int ret;
+
+	if (m == INSN_MODE_KERN)
+		insn_init(insn, kaddr, buf_len, IS_ENABLED(CONFIG_X86_64));
+	else
+		insn_init(insn, kaddr, buf_len, m == INSN_MODE_64);
+
+	ret = insn_get_length(insn);
+	if (ret)
+		return ret;
+
+	if (insn_complete(insn))
+		return 0;
+
+	return -EINVAL;
 }
diff --git a/tools/include/linux/kconfig.h b/tools/include/linux/kconfig.h
new file mode 100644
index 000000000000..1555a0c4f345
--- /dev/null
+++ b/tools/include/linux/kconfig.h
@@ -0,0 +1,73 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _TOOLS_LINUX_KCONFIG_H
+#define _TOOLS_LINUX_KCONFIG_H
+
+/* CONFIG_CC_VERSION_TEXT (Do not delete this comment. See help in Kconfig) */
+
+#ifdef CONFIG_CPU_BIG_ENDIAN
+#define __BIG_ENDIAN 4321
+#else
+#define __LITTLE_ENDIAN 1234
+#endif
+
+#define __ARG_PLACEHOLDER_1 0,
+#define __take_second_arg(__ignored, val, ...) val
+
+/*
+ * The use of "&&" / "||" is limited in certain expressions.
+ * The following enable to calculate "and" / "or" with macro expansion only.
+ */
+#define __and(x, y)			___and(x, y)
+#define ___and(x, y)			____and(__ARG_PLACEHOLDER_##x, y)
+#define ____and(arg1_or_junk, y)	__take_second_arg(arg1_or_junk y, 0)
+
+#define __or(x, y)			___or(x, y)
+#define ___or(x, y)			____or(__ARG_PLACEHOLDER_##x, y)
+#define ____or(arg1_or_junk, y)		__take_second_arg(arg1_or_junk 1, y)
+
+/*
+ * Helper macros to use CONFIG_ options in C/CPP expressions. Note that
+ * these only work with boolean and tristate options.
+ */
+
+/*
+ * Getting something that works in C and CPP for an arg that may or may
+ * not be defined is tricky.  Here, if we have "#define CONFIG_BOOGER 1"
+ * we match on the placeholder define, insert the "0," for arg1 and generate
+ * the triplet (0, 1, 0).  Then the last step cherry picks the 2nd arg (a one).
+ * When CONFIG_BOOGER is not defined, we generate a (... 1, 0) pair, and when
+ * the last step cherry picks the 2nd arg, we get a zero.
+ */
+#define __is_defined(x)			___is_defined(x)
+#define ___is_defined(val)		____is_defined(__ARG_PLACEHOLDER_##val)
+#define ____is_defined(arg1_or_junk)	__take_second_arg(arg1_or_junk 1, 0)
+
+/*
+ * IS_BUILTIN(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y', 0
+ * otherwise. For boolean options, this is equivalent to
+ * IS_ENABLED(CONFIG_FOO).
+ */
+#define IS_BUILTIN(option) __is_defined(option)
+
+/*
+ * IS_MODULE(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'm', 0
+ * otherwise.
+ */
+#define IS_MODULE(option) __is_defined(option##_MODULE)
+
+/*
+ * IS_REACHABLE(CONFIG_FOO) evaluates to 1 if the currently compiled
+ * code can call a function defined in code compiled based on CONFIG_FOO.
+ * This is similar to IS_ENABLED(), but returns false when invoked from
+ * built-in code when CONFIG_FOO is set to 'm'.
+ */
+#define IS_REACHABLE(option) __or(IS_BUILTIN(option), \
+				__and(IS_MODULE(option), __is_defined(MODULE)))
+
+/*
+ * IS_ENABLED(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y' or 'm',
+ * 0 otherwise.
+ */
+#define IS_ENABLED(option) __or(IS_BUILTIN(option), IS_MODULE(option))
+
+#endif /* _TOOLS_LINUX_KCONFIG_H */