Re: [PATCH v3 04/21] x86/insn: Add an insn_decode() API

2021-03-24 Thread Arnaldo Carvalho de Melo
Em Wed, Mar 24, 2021 at 11:21:19AM -0700, Ian Rogers escreveu:
> On Wed, Mar 24, 2021 at 6:54 AM Borislav Petkov  wrote:
> >
> > On Wed, Mar 24, 2021 at 10:43:20AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Borislav, was this addressed? Ian?
> >
> > Yap:
> >
> > https://git.kernel.org/tip/0705ef64d1ff52b817e278ca6e28095585ff31e1
> 
> Tested on PPC and ARM64 fwiw. Thanks,

Thank you guys for clearing this up,

- Arnaldo


Re: [PATCH v3 04/21] x86/insn: Add an insn_decode() API

2021-03-24 Thread Ian Rogers
On Wed, Mar 24, 2021 at 6:54 AM Borislav Petkov  wrote:
>
> On Wed, Mar 24, 2021 at 10:43:20AM -0300, Arnaldo Carvalho de Melo wrote:
> > Borislav, was this addressed? Ian?
>
> Yap:
>
> https://git.kernel.org/tip/0705ef64d1ff52b817e278ca6e28095585ff31e1

Tested on PPC and ARM64 fwiw. Thanks,
Ian

> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette


Re: [PATCH v3 04/21] x86/insn: Add an insn_decode() API

2021-03-24 Thread Borislav Petkov
On Wed, Mar 24, 2021 at 10:43:20AM -0300, Arnaldo Carvalho de Melo wrote:
> Borislav, was this addressed? Ian?

Yap:

https://git.kernel.org/tip/0705ef64d1ff52b817e278ca6e28095585ff31e1

-- 
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette


Re: [PATCH v3 04/21] x86/insn: Add an insn_decode() API

2021-03-24 Thread Arnaldo Carvalho de Melo
Em Tue, Mar 16, 2021 at 06:14:54PM -0700, Ian Rogers escreveu:
> On Thu, Mar 4, 2021 at 9:56 AM Borislav Petkov  wrote:
> > From: Borislav Petkov 
> >
> > 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().
> >
> > Also, cast the INSN_MODE_KERN dummy define value to (enum insn_mode)
> > for tools use of the decoder because perf tool builds with -Werror and
> > errors out with -Werror=sign-compare otherwise.
> >
> > Signed-off-by: Borislav Petkov 
> > Acked-by: Masami Hiramatsu 

> > +++ b/tools/arch/x86/lib/insn.c
> > @@ -11,10 +11,13 @@
> >  #else
> >  #include 
> >  #endif
> > -#include "../include/asm/inat.h" /* __ignore_sync_check__ */
> > -#include "../include/asm/insn.h" /* __ignore_sync_check__ */
> > +#include  /*__ignore_sync_check__ */
> > +#include  /* __ignore_sync_check__ */
> 
> Hi, this change is breaking non-x86 builds of perf for me in
> tip.git/master. The reason being that non-x86 builds compile the
> intel-pt-decoder, which includes this file, but don't have their
> include paths set to find tools/arch/x86. I think we want to keep the
> relative paths.

Borislav, was this addressed? Ian?

- Arnaldo


Re: [PATCH v3 04/21] x86/insn: Add an insn_decode() API

2021-03-16 Thread Ian Rogers
On Thu, Mar 4, 2021 at 9:56 AM Borislav Petkov  wrote:
>
> From: Borislav Petkov 
>
> 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().
>
> Also, cast the INSN_MODE_KERN dummy define value to (enum insn_mode)
> for tools use of the decoder because perf tool builds with -Werror and
> errors out with -Werror=sign-compare otherwise.
>
> Signed-off-by: Borislav Petkov 
> Acked-by: Masami Hiramatsu 
> ---
>  arch/x86/include/asm/insn.h   |  24 +++-
>  arch/x86/lib/insn.c   | 216 +++--
>  tools/arch/x86/include/asm/insn.h |  24 +++-
>  tools/arch/x86/lib/insn.c | 222 +++---
>  tools/include/linux/kconfig.h |  73 ++
>  5 files changed, 452 insertions(+), 107 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 93f84600ac2c..de9fe760c1e7 100644
> --- a/arch/x86/include/asm/insn.h
> +++ b/arch/x86/include/asm/insn.h
> @@ -132,13 +132,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 ed1e0335aa14..bb58004497f8 100644
> --- a/arch/x86/lib/insn.c
> +++ b/arch/x86/lib/insn.c
> @@ -14,6 +14,9 @@
>  #include  /*__ignore_sync_check__ */
>  #include  /* __ignore_sync_check__ */
>
> +#include 
> +#include 
> +
>  #include  /* __ignore_sync_check__ */
>
>  #define leXX_to_cpu(t, r)  \
> @@ -112,8 +115,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 = >prefixes;
> insn_attr_t attr;
> @@ -121,7 +128,7 @@ void insn_get_prefixes(struct insn *insn)
> int i, nb;
>
> if (prefixes->got)
> -   return;
> +   return 0;
>
> insn_get_emulate_prefix(insn);
>
> @@ -231,8 +238,10 @@ void insn_get_prefixes(struct insn *insn)
>
> prefixes->got = 1;
>
> +   return 0;
> +
>  err_out:
> -   return;
> +   return -ENODATA;
>  }
>
>  /**
> @@ -244,16 +253,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 = >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 =