Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API

From: Borislav Petkov
Date: Fri Jan 08 2021 - 14:00:51 EST

Next message: Linus Torvalds: "Re: [PATCH 0/2] page_count can't be used to decide when wp_page_copy"
Previous message: Shuah Khan: "[GIT PULL] KUnit update for Linux 5.11-rc3"
Next in thread: Masami Hiramatsu: "Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>
*/
-#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@xxxxxxxxxx>
*/
-#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@xxxxxxxxxx>
*/
-#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@xxxxxxxxxx>
*/
-#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

Next message: Linus Torvalds: "Re: [PATCH 0/2] page_count can't be used to decide when wp_page_copy"
Previous message: Shuah Khan: "[GIT PULL] KUnit update for Linux 5.11-rc3"
Next in thread: Masami Hiramatsu: "Re: [PATCH v1 03/19] x86/insn: Add an insn_decode() API"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]