Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API
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
*/
-#include
+#include /* __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
+#include /* __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
*/
-#include
+#include /* __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
#endif
-#include
-#include
+#include /*__ignore_sync_check__ */
+#include /* __ignore_sync_check__ */
#include
#include
-#include
+#include /* __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
*/
-#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
*/
-#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
#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
#include
-#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
Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API
On Fri, 8 Jan 2021 19:59:50 +0100
Borislav Petkov 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
> */
> -#include
> +#include /* __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
> +#include /* __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
> */
> -#include
> +#include /* __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
> #endif
> -#include
> -#include
> +#include /*__ignore_sync_check__ */
> +#include /* __ignore_sync_check__ */
>
> #include
> #include
>
> -#include
> +#include /* __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
> */
> -#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
> */
> -#include "../include/asm/insn.h"
> +#include
Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API
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
*/
-#include
+#include /* __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
+#include /* __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
*/
-#include
+#include /* __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
#endif
-#include
-#include
+#include /*__ignore_sync_check__ */
+#include /* __ignore_sync_check__ */
#include
#include
-#include
+#include /* __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
*/
-#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
*/
-#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
#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
#include
-#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 */
Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API
On Wed, 30 Dec 2020 10:28:33 +0100
Borislav Petkov 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
*/
-#include
+#include /* 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
+#include /* 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
*/
-#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
" -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[\">]"'
--
Masami Hiramatsu
Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API
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. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API
On Tue, 29 Dec 2020 21:06:54 +0100
Borislav Petkov 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(, 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(, buf, len, x86_64);
- insn_get_length();
- if (!insn_complete() || insn.length > len)
+ ret = insn_decode(, buf, len,
+ x86_64 ? INSN_MODE_64 : INSN_MODE_32);
+ if (ret < 0 || insn.length > len)
return -1;
+
intel_pt_insn_decoder(, intel_pt_insn);
if (insn.length < INTEL_PT_INSN_BUF_SZ)
memcpy(intel_pt_insn->buf, buf, insn.length);
Thank you,
--
Masami Hiramatsu
Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API
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(, buf, len, INSN_MODE_KERN);
| ^~
| INSN_MODE_64
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API
Hi,
On Wed, 23 Dec 2020 18:42:17 +0100
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.
-ENODATA sounds good to me :)
Acked-by: Masami Hiramatsu
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
> ---
> 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_MAX0x1f/* 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
> #include
>
> +#include
> +#include
> +
> #include
>
> /* 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 = >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
[PATCH v1 03/19] x86/insn: Add an insn_decode() API
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().
Signed-off-by: Borislav Petkov
---
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
#include
+#include
+#include
+
#include
/* 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 = >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 = >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 =
