[PATCH 0/4] gdbstub: Add support for MTE in system mode

[PATCH 0/4] gdbstub: Add support for MTE in system mode

[Gustavo Romero]

This patchset makes handle_q_memtag, handle_q_isaddresstagged, and
handle_Q_memtag stubs build for system mode, allowing all GDB
'memory-tag' subcommands to work with QEMU gdbstub on aarch64 system
mode.

It also extends the MTE gdbstub tests to run in system mode, sharing the
tests between user mode and system mode.

For running the tests, a GDB that supports MTE for baremetal targets is
necessary. The patch for this support is currently under review [0].
For convenience, this patch is available in this GDB branch [1], and GDB
can be built and installed into a /tmp directory in a straightforward
way. After cloning [1], in a 'build' directory, configure GDB to build:

 $ git clone --depth 1 https://github.com/gromero/binutils-gdb.git --branch mte_baremetal mte_baremetal && cd mte_baremetal
 $ mkdir build && cd build
 $ ../configure --disable-binutils --disable-ld --disable-gold --disable-gas --disable-sim --disable-gprof --disable-gprofng --with-python=python3 --enable-libctf --enable-unit-tests --prefix=/tmp/gdb --with-additional-debug-dirs=/usr/lib/debug --enable-targets=all
 $ make -j 32
 $ make install

Configure QEMU, specifying where GDB is installed. For example:

 $ cd build
 $ ../configure --target-list=aarch64-linux-user,aarch64-softmmu --disable-docs --gdb=/tmp/gdb/bin/gdb
 $ make -j 32
 $ cd..

And, finally, run the MTE gdbstub tests for system mode:

 $ make -C build -j 32 run-tcg-tests-aarch64-softmmu


Cheers,
Gustavo

[0] https://sourceware.org/pipermail/gdb-patches/2024-July/210584.html
[1] https://github.com/gromero/binutils-gdb/tree/mte_baremetal

Gustavo Romero (4):
  gdbstub: Use specific MMU index when probing MTE addresses
  gdbstub: Add support for MTE in system mode
  tests/guest-debug: Support passing arguments to the GDB test script
  tests/tcg/aarch64: Extend MTE gdbstub tests to system mode

 target/arm/gdbstub64.c                    | 21 +++++++----
 tests/guest-debug/run-test.py             |  4 +++
 tests/tcg/aarch64/Makefile.softmmu-target | 36 +++++++++++++++++--
 tests/tcg/aarch64/Makefile.target         |  3 +-
 tests/tcg/aarch64/gdbstub/test-mte.py     | 44 +++++++++++++++--------
 tests/tcg/aarch64/system/boot.S           | 26 ++++++++++++--
 tests/tcg/aarch64/system/kernel.ld        |  5 +++
 tests/tcg/aarch64/system/mte.c            | 40 +++++++++++++++++++++
 8 files changed, 151 insertions(+), 28 deletions(-)
 create mode 100644 tests/tcg/aarch64/system/mte.c

-- 
2.34.1

[PATCH 1/4] gdbstub: Use specific MMU index when probing MTE addresses

[Gustavo Romero]

Use arm_mmu_idx() to determine the specific translation regime (MMU
index) before probing addresses using allocation_tag_mem_probe().

Currently, the MMU index is hardcoded to 0 and only works for user mode.
By obtaining the specific MMU index according to the translation regime,
future use of the stubs relying on allocation_tag_mem_probe in other
regimes will be possible, like in EL1.

This commit also changes the ptr_size value passed to
allocation_tag_mem_probe() from 8 to 1. The ptr_size parameter actually
represents the number of bytes in the memory access (which can be as
small as 1 byte), rather than the number of bits used in the address
space pointed to by ptr.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 target/arm/gdbstub64.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index 2e2bc2700b..d59272fdbb 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -431,6 +431,7 @@ static void handle_q_memtag(GArray *params, void *user_ctx)
 {
     ARMCPU *cpu = ARM_CPU(user_ctx);
     CPUARMState *env = &cpu->env;
+    uint32_t mmu_index;
 
     uint64_t addr = gdb_get_cmd_param(params, 0)->val_ull;
     uint64_t len = gdb_get_cmd_param(params, 1)->val_ul;
@@ -454,8 +455,10 @@ static void handle_q_memtag(GArray *params, void *user_ctx)
         gdb_put_packet("E03");
     }
 
+    /* Find out the current translation regime for probe. */
+    mmu_index = arm_mmu_idx(env) & ARM_MMU_IDX_COREIDX_MASK;
     /* Note that tags are packed here (2 tags packed in one byte). */
-    tags = allocation_tag_mem_probe(env, 0, addr, MMU_DATA_LOAD, 8 /* 64-bit */,
+    tags = allocation_tag_mem_probe(env, mmu_index, addr, MMU_DATA_LOAD, 1,
                                     MMU_DATA_LOAD, true, 0);
     if (!tags) {
         /* Address is not in a tagged region. */
@@ -474,13 +477,16 @@ static void handle_q_isaddresstagged(GArray *params, void *user_ctx)
 {
     ARMCPU *cpu = ARM_CPU(user_ctx);
     CPUARMState *env = &cpu->env;
+    uint32_t mmu_index;
 
     uint64_t addr = gdb_get_cmd_param(params, 0)->val_ull;
 
     uint8_t *tags;
     const char *reply;
 
-    tags = allocation_tag_mem_probe(env, 0, addr, MMU_DATA_LOAD, 8 /* 64-bit */,
+    /* Find out the current translation regime for probe. */
+    mmu_index = arm_mmu_idx(env) & ARM_MMU_IDX_COREIDX_MASK;
+    tags = allocation_tag_mem_probe(env, mmu_index, addr, MMU_DATA_LOAD, 1,
                                     MMU_DATA_LOAD, true, 0);
     reply = tags ? "01" : "00";
 
@@ -491,6 +497,7 @@ static void handle_Q_memtag(GArray *params, void *user_ctx)
 {
     ARMCPU *cpu = ARM_CPU(user_ctx);
     CPUARMState *env = &cpu->env;
+    uint32_t mmu_index;
 
     uint64_t start_addr = gdb_get_cmd_param(params, 0)->val_ull;
     uint64_t len = gdb_get_cmd_param(params, 1)->val_ul;
@@ -523,8 +530,10 @@ static void handle_Q_memtag(GArray *params, void *user_ctx)
      * Get all tags in the page starting from the tag of the start address.
      * Note that there are two tags packed into a single byte here.
      */
-    tags = allocation_tag_mem_probe(env, 0, start_addr, MMU_DATA_STORE,
-                                    8 /* 64-bit */, MMU_DATA_STORE, true, 0);
+    /* Find out the current translation regime for probe. */
+    mmu_index = arm_mmu_idx(env) & ARM_MMU_IDX_COREIDX_MASK;
+    tags = allocation_tag_mem_probe(env, mmu_index, start_addr, MMU_DATA_STORE,
+                                    1, MMU_DATA_STORE, true, 0);
     if (!tags) {
         /* Address is not in a tagged region. */
         gdb_put_packet("E04");
-- 
2.34.1

Re: [PATCH 1/4] gdbstub: Use specific MMU index when probing MTE addresses

[Richard Henderson]

On 7/23/24 02:07, Gustavo Romero wrote:
> +    /* Find out the current translation regime for probe. */
> +    mmu_index = arm_mmu_idx(env) & ARM_MMU_IDX_COREIDX_MASK;

The official interface is cpu_mmu_index().

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH 1/4] gdbstub: Use specific MMU index when probing MTE addresses

[Gustavo Romero]

Hi Richard,

On 7/24/24 7:14 AM, Richard Henderson wrote:
> On 7/23/24 02:07, Gustavo Romero wrote:
>> +    /* Find out the current translation regime for probe. */
>> +    mmu_index = arm_mmu_idx(env) & ARM_MMU_IDX_COREIDX_MASK;
> 
> The official interface is cpu_mmu_index().
> 
> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks for the review! I'll change it in v2.

I'm just wondering if you plan to review patches 3/4 and 4/4 in
this series too so I could wait for the reviews before sending v2.


Cheers,
Gustavo

Re: [PATCH 1/4] gdbstub: Use specific MMU index when probing MTE addresses

[Gustavo Romero]

Hi Richard,

On 7/24/24 7:14 AM, Richard Henderson wrote:
> On 7/23/24 02:07, Gustavo Romero wrote:
>> +    /* Find out the current translation regime for probe. */
>> +    mmu_index = arm_mmu_idx(env) & ARM_MMU_IDX_COREIDX_MASK;
> 
> The official interface is cpu_mmu_index().
> 
> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

s/arm_mmu_idx/cpu_mmu_index/ done in v2. Thanks.


Cheers,
Gustavo

[PATCH 2/4] gdbstub: Add support for MTE in system mode

[Gustavo Romero]

This commit makes handle_q_memtag, handle_q_isaddresstagged, and
handle_Q_memtag stubs build for system mode, allowing all GDB
'memory-tag' subcommands to work with QEMU gdbstub on aarch64 system
mode.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 target/arm/gdbstub64.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index d59272fdbb..fd5a9d0609 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -426,6 +426,7 @@ int aarch64_gdb_set_tag_ctl_reg(CPUState *cs, uint8_t *buf, int reg)
 
     return 1;
 }
+#endif /* CONFIG_USER_ONLY */
 
 static void handle_q_memtag(GArray *params, void *user_ctx)
 {
@@ -596,12 +597,10 @@ static GdbCmdParseEntry cmd_handler_table[NUM_CMDS] = {
         .need_cpu_context = true
     },
 };
-#endif /* CONFIG_USER_ONLY */
 
 void aarch64_cpu_register_gdb_commands(ARMCPU *cpu, GString *qsupported,
                                        GArray *qtable, GArray *stable)
 {
-#ifdef CONFIG_USER_ONLY
     /* MTE */
     if (cpu_isar_feature(aa64_mte, cpu)) {
         g_string_append(qsupported, ";memory-tagging+");
@@ -611,5 +610,4 @@ void aarch64_cpu_register_gdb_commands(ARMCPU *cpu, GString *qsupported,
 
         g_array_append_val(stable, cmd_handler_table[QMemTags]);
     }
-#endif
 }
-- 
2.34.1

Re: [PATCH 2/4] gdbstub: Add support for MTE in system mode

[Richard Henderson]

On 7/23/24 02:07, Gustavo Romero wrote:
> This commit makes handle_q_memtag, handle_q_isaddresstagged, and
> handle_Q_memtag stubs build for system mode, allowing all GDB
> 'memory-tag' subcommands to work with QEMU gdbstub on aarch64 system
> mode.
> 
> Signed-off-by: Gustavo Romero<gustavo.romero@linaro.org>
> ---
>   target/arm/gdbstub64.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 3/4] tests/guest-debug: Support passing arguments to the GDB test script

[Gustavo Romero]

This commit adds a new option to run-test.py, --test-args, which can
be used to pass arguments to the GDB test script specified by the --test
option. The arguments passed are in the key=value form, and multiple
pairs can be passed, separated by a space. For example:

run-test.py [...] --test <GDB_TEST_SCRIPT> --test-args v0="string" v1=10

The 'v0' and 'v1' variables will then be available in the GDB test
script, like this:

print(v0)
print(v1)

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 tests/guest-debug/run-test.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/guest-debug/run-test.py b/tests/guest-debug/run-test.py
index 368ff8a890..63b55fb8bd 100755
--- a/tests/guest-debug/run-test.py
+++ b/tests/guest-debug/run-test.py
@@ -27,6 +27,7 @@ def get_args():
     parser.add_argument("--binary", help="Binary to debug",
                         required=True)
     parser.add_argument("--test", help="GDB test script")
+    parser.add_argument("--test-args", help="Arguments to GDB test script")
     parser.add_argument("--gdb", help="The gdb binary to use",
                         default=None)
     parser.add_argument("--gdb-args", help="Additional gdb arguments")
@@ -91,6 +92,9 @@ def log(output, msg):
     gdb_cmd += " -ex 'target remote %s'" % (socket_name)
     # finally the test script itself
     if args.test:
+        if args.test_args:
+            test_args = args.test_args.replace(" ",";")
+            gdb_cmd += f" -ex 'py {test_args}'"
         gdb_cmd += " -x %s" % (args.test)
 
 
-- 
2.34.1

Re: [PATCH 3/4] tests/guest-debug: Support passing arguments to the GDB test script

[Alex Bennée]

Gustavo Romero <gustavo.romero@linaro.org> writes:

> This commit adds a new option to run-test.py, --test-args, which can
> be used to pass arguments to the GDB test script specified by the --test
> option. The arguments passed are in the key=value form, and multiple
> pairs can be passed, separated by a space. For example:
>
> run-test.py [...] --test <GDB_TEST_SCRIPT> --test-args v0="string" v1=10
>
> The 'v0' and 'v1' variables will then be available in the GDB test
> script, like this:
>
> print(v0)
> print(v1)
>
> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> ---
>  tests/guest-debug/run-test.py | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/tests/guest-debug/run-test.py b/tests/guest-debug/run-test.py
> index 368ff8a890..63b55fb8bd 100755
> --- a/tests/guest-debug/run-test.py
> +++ b/tests/guest-debug/run-test.py
> @@ -27,6 +27,7 @@ def get_args():
>      parser.add_argument("--binary", help="Binary to debug",
>                          required=True)
>      parser.add_argument("--test", help="GDB test script")
> +    parser.add_argument("--test-args", help="Arguments to GDB test script")
>      parser.add_argument("--gdb", help="The gdb binary to use",
>                          default=None)
>      parser.add_argument("--gdb-args", help="Additional gdb
> arguments")

I might be easier to do:

    parser.add_argument('test_args', nargs='*',
                        help="Additional args for test. "
                        "You should precede with -- "
                        "to avoid confusion with flags for runner script")


> @@ -91,6 +92,9 @@ def log(output, msg):
>      gdb_cmd += " -ex 'target remote %s'" % (socket_name)
>      # finally the test script itself
>      if args.test:
> +        if args.test_args:
> +            test_args = args.test_args.replace(" ",";")
> +            gdb_cmd += f" -ex 'py {test_args}'"
>          gdb_cmd += " -x %s" % (args.test)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 3/4] tests/guest-debug: Support passing arguments to the GDB test script

[Gustavo Romero]

Hi Alex,

On 7/25/24 5:38 PM, Alex Bennée wrote:
> Gustavo Romero <gustavo.romero@linaro.org> writes:
> 
>> This commit adds a new option to run-test.py, --test-args, which can
>> be used to pass arguments to the GDB test script specified by the --test
>> option. The arguments passed are in the key=value form, and multiple
>> pairs can be passed, separated by a space. For example:
>>
>> run-test.py [...] --test <GDB_TEST_SCRIPT> --test-args v0="string" v1=10
>>
>> The 'v0' and 'v1' variables will then be available in the GDB test
>> script, like this:
>>
>> print(v0)
>> print(v1)
>>
>> Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
>> ---
>>   tests/guest-debug/run-test.py | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/tests/guest-debug/run-test.py b/tests/guest-debug/run-test.py
>> index 368ff8a890..63b55fb8bd 100755
>> --- a/tests/guest-debug/run-test.py
>> +++ b/tests/guest-debug/run-test.py
>> @@ -27,6 +27,7 @@ def get_args():
>>       parser.add_argument("--binary", help="Binary to debug",
>>                           required=True)
>>       parser.add_argument("--test", help="GDB test script")
>> +    parser.add_argument("--test-args", help="Arguments to GDB test script")
>>       parser.add_argument("--gdb", help="The gdb binary to use",
>>                           default=None)
>>       parser.add_argument("--gdb-args", help="Additional gdb
>> arguments")
> 
> I might be easier to do:
> 
>      parser.add_argument('test_args', nargs='*',
>                          help="Additional args for test. "
>                          "You should precede with -- "
>                          "to avoid confusion with flags for runner script")
> 
> 
>> @@ -91,6 +92,9 @@ def log(output, msg):
>>       gdb_cmd += " -ex 'target remote %s'" % (socket_name)
>>       # finally the test script itself
>>       if args.test:
>> +        if args.test_args:
>> +            test_args = args.test_args.replace(" ",";")
>> +            gdb_cmd += f" -ex 'py {test_args}'"
>>           gdb_cmd += " -x %s" % (args.test)
As we discussed, -ex 'py [...]' just allow setting variables in Python,
so it won't understand plain argparse'd arguments. For instance, if we
do 'run-test.py [...] -- --mode=system' this will fail because
"--mode-system" not a valid Python syntax. Moreover, there isn't another
way to pass args to the GDB Python scripts afaict.

But since that's a nice idea (to be able to parse args in the GDB Python
scripts) I found a workaround. It's possible to set the sys.argv in the
script env. So -ex "py sys.argv=['--mode=user', '--arg1=1', '--arg2=2', ...]",
for instance, correctly sets the argv and make argparse work normally in
the script. Thus I used that approach to address your suggestion. Please
see v2.


Cheers,
Gustavo

[PATCH 4/4] tests/tcg/aarch64: Extend MTE gdbstub tests to system mode

[Gustavo Romero]

Extend MTE gdbstub tests to also run in system mode (share tests
between user mode and system mode). The tests will only run if a version
of GDB that supports MTE is available in the test environment and if
available compiler supports the 'memtag' flag (-march=armv8.5-a+memtag).

For the tests running in system mode, a page that supports MTE
operations is necessary. Therefore, an MTE-enabled page is made
available (mapped) in the third 2 MB chunk of the second 1 GB space in
the flat mapping set in boot.S. A new binary is also introduced (mte.c)
for the test. It's linked against boot.S and run by QEMU in system mode.

Also, in boot.S bits ATA[43] and TCF[40] are set in SCTLR_EL1 to enable
access to allocation tags at EL1 and enable MTE_SYNC exceptions in case
of Tag Check Faults, and bit TBI0[37] is set in TCR_EL1 so the top byte
of the addresses are ignored in the translation and used for tagged
addresses.

Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
---
 tests/tcg/aarch64/Makefile.softmmu-target | 36 +++++++++++++++++--
 tests/tcg/aarch64/Makefile.target         |  3 +-
 tests/tcg/aarch64/gdbstub/test-mte.py     | 44 +++++++++++++++--------
 tests/tcg/aarch64/system/boot.S           | 26 ++++++++++++--
 tests/tcg/aarch64/system/kernel.ld        |  5 +++
 tests/tcg/aarch64/system/mte.c            | 40 +++++++++++++++++++++
 6 files changed, 133 insertions(+), 21 deletions(-)
 create mode 100644 tests/tcg/aarch64/system/mte.c

diff --git a/tests/tcg/aarch64/Makefile.softmmu-target b/tests/tcg/aarch64/Makefile.softmmu-target
index dd6d595830..225a073e79 100644
--- a/tests/tcg/aarch64/Makefile.softmmu-target
+++ b/tests/tcg/aarch64/Makefile.softmmu-target
@@ -2,7 +2,9 @@
 # Aarch64 system tests
 #
 
-AARCH64_SYSTEM_SRC=$(SRC_PATH)/tests/tcg/aarch64/system
+AARCH64_SRC=$(SRC_PATH)/tests/tcg/aarch64
+AARCH64_SYSTEM_SRC=$(AARCH64_SRC)/system
+
 VPATH+=$(AARCH64_SYSTEM_SRC)
 
 # These objects provide the basic boot code and helper functions for all tests
@@ -21,7 +23,8 @@ LDFLAGS+=-static -nostdlib $(CRT_OBJS) $(MINILIB_OBJS) -lgcc
 
 config-cc.mak: Makefile
 	$(quiet-@)( \
-	    $(call cc-option,-march=armv8.3-a, CROSS_CC_HAS_ARMV8_3)) 3> config-cc.mak
+	    $(call cc-option,-march=armv8.3-a, CROSS_CC_HAS_ARMV8_3); \
+	    $(call cc-option,-march=armv8.5-a+memtag, CROSS_CC_HAS_ARMV8_MTE)) 3> config-cc.mak
 -include config-cc.mak
 
 # building head blobs
@@ -88,3 +91,32 @@ pauth-3:
 run-pauth-3:
 	$(call skip-test, "RUN of pauth-3", "not built")
 endif
+
+ifneq ($(CROSS_CC_HAS_ARMV8_MTE),)
+QEMU_MTE_ENABLED_MACHINE=-M virt,mte=on -cpu max -display none
+QEMU_OPTS_WITH_MTE_ON = $(QEMU_MTE_ENABLED_MACHINE) $(QEMU_BASE_ARGS) -kernel
+mte: CFLAGS+=-march=armv8.5-a+memtag
+
+run-mte: QEMU_OPTS=$(QEMU_OPTS_WITH_MTE_ON)
+run-mte: mte
+
+ifeq ($(GDB_HAS_MTE),y)
+run-gdbstub-mte: QEMU_OPTS=$(QEMU_OPTS_WITH_MTE_ON)
+run-gdbstub-mte: mte
+	$(call run-test, $@, $(GDB_SCRIPT) \
+		--gdb $(GDB) \
+		--qemu $(QEMU) --qargs "-chardev file$(COMMA)path=$<.out$(COMMA)id=output $(QEMU_OPTS)" \
+		--bin $< --test $(AARCH64_SRC)/gdbstub/test-mte.py --test-args mode=\"system\", \
+	gdbstub MTE support)
+
+EXTRA_RUNS += run-gdbstub-mte
+else # !GDB_HAS_MTE
+run-gdbstub-mte:
+	$(call skip-test "RUN of gdbstub-mte", "GDB without MTE support")
+endif
+else # !CROSS_CC_HAS_ARMV8_MTE
+mte:
+	$(call skip-test, "BUILD of $@", "missing compiler support")
+run-mte:
+	$(call skip-test, "RUN of mte", "not build")
+endif
diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index 8cc62eb456..2504517176 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -138,7 +138,8 @@ run-gdbstub-mte: mte-8
 	$(call run-test, $@, $(GDB_SCRIPT) \
 		--gdb $(GDB) \
 		--qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
-		--bin $< --test $(AARCH64_SRC)/gdbstub/test-mte.py, \
+		--bin $< --test $(AARCH64_SRC)/gdbstub/test-mte.py \
+		--test-args mode=\"user\", \
 	gdbstub MTE support)
 
 EXTRA_RUNS += run-gdbstub-mte
diff --git a/tests/tcg/aarch64/gdbstub/test-mte.py b/tests/tcg/aarch64/gdbstub/test-mte.py
index 2db0663c1a..727999f277 100644
--- a/tests/tcg/aarch64/gdbstub/test-mte.py
+++ b/tests/tcg/aarch64/gdbstub/test-mte.py
@@ -1,13 +1,13 @@
 from __future__ import print_function
 #
 # Test GDB memory-tag commands that exercise the stubs for the qIsAddressTagged,
-# qMemTag, and QMemTag packets. Logical tag-only commands rely on local
-# operations, hence don't exercise any stub.
+# qMemTag, and QMemTag packets, which are used for manipulating allocation tags.
+# Logical tags-related commands rely on local operations, hence don't exercise
+# any stub and so are not used in this test.
 #
-# The test consists in breaking just after a atag() call (which sets the
-# allocation tag -- see mte-8.c for details) and setting/getting tags in
-# different memory locations and ranges starting at the address of the array
-# 'a'.
+# The test consists in breaking just after a tag is set in a specific memory
+# chunk, and then using the GDB 'memory-tagging' subcommands to set/get tags in
+# different memory locations and ranges in the MTE-enabled memory chunk.
 #
 # This is launched via tests/guest-debug/run-test.py
 #
@@ -23,12 +23,26 @@
 
 
 def run_test():
-    gdb.execute("break 95", False, True)
+    if mode == "system":
+        # Break address: where to break before performing the tests
+        # Addresss is the last insn. before 'main' returns. See mte.c
+        ba = "*main+52"
+        # Tagged address: the start of the MTE-enabled memory chunk to be tested
+        # Address is in the x0 register
+        ta = "$x0"
+    else: # mode="user"
+        # Line 95 in mte-8.c
+        ba = "95"
+        # 'a' is an array defined in C code. See mte-8.c
+        ta = "a"
+
+    gdb.execute(f"break {ba}", False, True)
     gdb.execute("continue", False, True)
+
     try:
         # Test if we can check correctly that the allocation tag for
         # array 'a' matches the logical tag after atag() is called.
-        co = gdb.execute("memory-tag check a", False, True)
+        co = gdb.execute(f"memory-tag check {ta}", False, True)
         tags_match = re.findall(PATTERN_0, co, re.MULTILINE)
         if tags_match:
             report(True, f"{tags_match[0]}")
@@ -40,19 +54,19 @@ def run_test():
 
         # Set the allocation tag for the first granule (16 bytes) of
         # address starting at 'a' address to a known value, i.e. 0x04.
-        gdb.execute("memory-tag set-allocation-tag a 1 04", False, True)
+        gdb.execute(f"memory-tag set-allocation-tag {ta} 1 04", False, True)
 
         # Then set the allocation tag for the second granule to a known
         # value, i.e. 0x06. This tests that contiguous tag granules are
         # set correct and don't run over each other.
-        gdb.execute("memory-tag set-allocation-tag a+16 1 06", False, True)
+        gdb.execute(f"memory-tag set-allocation-tag {ta}+16 1 06", False, True)
 
         # Read the known values back and check if they remain the same.
 
-        co = gdb.execute("memory-tag print-allocation-tag a", False, True)
+        co = gdb.execute(f"memory-tag print-allocation-tag {ta}", False, True)
         first_tag = re.match(PATTERN_1, co)[1]
 
-        co = gdb.execute("memory-tag print-allocation-tag a+16", False, True)
+        co = gdb.execute(f"memory-tag print-allocation-tag {ta}+16", False, True)
         second_tag = re.match(PATTERN_1, co)[1]
 
         if first_tag == "0x4" and second_tag == "0x6":
@@ -61,15 +75,15 @@ def run_test():
             report(False, "Can't set/print allocation tags!")
 
         # Now test fill pattern by setting a whole page with a pattern.
-        gdb.execute("memory-tag set-allocation-tag a 4096 0a0b", False, True)
+        gdb.execute(f"memory-tag set-allocation-tag {ta} 4096 0a0b", False, True)
 
         # And read back the tags of the last two granules in page so
         # we also test if the pattern is set correctly up to the end of
         # the page.
-        co = gdb.execute("memory-tag print-allocation-tag a+4096-32", False, True)
+        co = gdb.execute(f"memory-tag print-allocation-tag {ta}+4096-32", False, True)
         tag = re.match(PATTERN_1, co)[1]
 
-        co = gdb.execute("memory-tag print-allocation-tag a+4096-16", False, True)
+        co = gdb.execute(f"memory-tag print-allocation-tag {ta}+4096-16", False, True)
         last_tag = re.match(PATTERN_1, co)[1]
 
         if tag == "0xa" and last_tag == "0xb":
diff --git a/tests/tcg/aarch64/system/boot.S b/tests/tcg/aarch64/system/boot.S
index 501685d0ec..a12393d00b 100644
--- a/tests/tcg/aarch64/system/boot.S
+++ b/tests/tcg/aarch64/system/boot.S
@@ -135,11 +135,22 @@ __start:
 	orr	x1, x1, x3
 	str	x1, [x2]			/* 2nd 2mb (.data & .bss)*/
 
+        /* Third block: .mte_page */
+	adrp	x1, .mte_page
+	add	x1, x1, :lo12:.mte_page
+	bic	x1, x1, #(1 << 21) - 1
+	and 	x4, x1, x5
+	add	x2, x0, x4, lsr #(21 - 3)
+	ldr	x3, =(3 << 53) | 0x401 | 1 << 2	/* attr(AF, NX, block, AttrIndx=Attr1) */
+	orr	x1, x1, x3
+	str	x1, [x2]
+
 	/* Setup/enable the MMU.  */
 
 	/*
 	 * TCR_EL1 - Translation Control Registers
 	 *
+	 * TBI0[37] = 0b1 => Top Byte ignored and used for tagged addresses
 	 * IPS[34:32] = 40-bit PA, 1TB
 	 * TG0[14:15] = b00 => 4kb granuale
 	 * ORGN0[11:10] = Outer: Normal, WB Read-Alloc No Write-Alloc Cacheable
@@ -152,16 +163,22 @@ __start:
 	 * with at least 1gb range to see RAM. So we start with a
 	 * level 1 lookup.
 	 */
-	ldr	x0, = (2 << 32) | 25 | (3 << 10) | (3 << 8)
+	ldr	x0, = (1 << 37) | (2 << 32) | 25 | (3 << 10) | (3 << 8)
 	msr	tcr_el1, x0
 
-	mov	x0, #0xee			/* Inner/outer cacheable WB */
+	/*
+	 * Attr0: Normal, Inner/outer cacheable WB
+	 * Attr1: Tagged Normal (MTE)
+	 */
+	mov	x0, #0xf0ee
 	msr	mair_el1, x0
 	isb
 
 	/*
 	 * SCTLR_EL1 - System Control Register
 	 *
+	 * ATA[43] = 1 = enable access to allocation tags at EL1
+	 * TCF[40] = 1 = Tag Check Faults cause a synchronous exception
 	 * WXN[19] = 0 = no effect, Write does not imply XN (execute never)
 	 * I[12] = Instruction cachability control
 	 * SA[3] = SP alignment check
@@ -169,7 +186,8 @@ __start:
 	 * M[0] = 1, enable stage 1 address translation for EL0/1
 	 */
 	mrs	x0, sctlr_el1
-	ldr	x1, =0x100d			/* bits I(12) SA(3) C(2) M(0) */
+	/* Bits set: ATA(43) TCF(40) I(12) SA(3) C(2) M(0) */
+	ldr	x1, =(0x100d | 1 << 43 | 1 << 40)
 	bic	x0, x0, #(1 << 1)		/* clear bit A(1) */
 	bic	x0, x0, #(1 << 19)		/* clear WXN */
 	orr	x0, x0, x1			/* set bits */
@@ -239,3 +257,5 @@ ttb_stage2:
 stack:
 	.space 65536, 0
 stack_end:
+
+	.section .mte_page
diff --git a/tests/tcg/aarch64/system/kernel.ld b/tests/tcg/aarch64/system/kernel.ld
index 7b3a76dcbf..7c00c1c378 100644
--- a/tests/tcg/aarch64/system/kernel.ld
+++ b/tests/tcg/aarch64/system/kernel.ld
@@ -18,6 +18,11 @@ SECTIONS
     .bss : {
         *(.bss)
     }
+    /* align MTE section to next (third) 2mb */
+    . = ALIGN(1 << 22);
+    .mte : {
+        *(.mte_page)
+    }
     /DISCARD/ : {
         *(.ARM.attributes)
     }
diff --git a/tests/tcg/aarch64/system/mte.c b/tests/tcg/aarch64/system/mte.c
new file mode 100644
index 0000000000..58a5ac31ff
--- /dev/null
+++ b/tests/tcg/aarch64/system/mte.c
@@ -0,0 +1,40 @@
+#include <inttypes.h>
+
+int main(void)
+{
+    uint8_t *addr;
+
+    /*
+     * Third 2MB chunk in the second 1GB block.
+     * See .mte_page section in kernel.ld.
+     */
+    addr = (void *)((1UL << 30) | (1UL << 22));
+
+    asm (
+        /*
+         * Set GCR for randon tag generation. 0xA5 is just a random value to set
+         * GCR != 0 so the tag generated by 'irg' is not zero.
+         */
+        "ldr x1, =0xA5;"
+        "msr gcr_el1, x1;"
+
+         /* Generate a logical tag and put it in 'addr' pointer. */
+         "irg %[addr], %[addr];"
+
+         /*
+          * Store the generated tag to memory region pointed to by 'addr', i.e.
+          * set the allocation tag for the memory region.
+          */
+         "stg %[addr], [%[addr]];"
+
+         /*
+          * Store a random value (0xdeadbeef) to *addr. This must not cause any
+          * Tag Check Fault since logical and allocation tags are set the same.
+          */
+	 "ldr x1, =0xdeadbeef;"
+	 "str x1, [x0];"
+         : [addr] "+r" (addr)
+         :
+         :
+    );
+}
-- 
2.34.1

Re: [PATCH 4/4] tests/tcg/aarch64: Extend MTE gdbstub tests to system mode

[Richard Henderson]

On 7/23/24 02:07, Gustavo Romero wrote:
>   def run_test():
> -    gdb.execute("break 95", False, True)
> +    if mode == "system":
> +        # Break address: where to break before performing the tests
> +        # Addresss is the last insn. before 'main' returns. See mte.c
> +        ba = "*main+52"

Ugly.  You can add labels in your inline asm block instead.

> diff --git a/tests/tcg/aarch64/system/boot.S b/tests/tcg/aarch64/system/boot.S
> index 501685d0ec..a12393d00b 100644
> --- a/tests/tcg/aarch64/system/boot.S
> +++ b/tests/tcg/aarch64/system/boot.S
> @@ -135,11 +135,22 @@ __start:
>   	orr	x1, x1, x3
>   	str	x1, [x2]			/* 2nd 2mb (.data & .bss)*/
>   
> +        /* Third block: .mte_page */
> +	adrp	x1, .mte_page
> +	add	x1, x1, :lo12:.mte_page
> +	bic	x1, x1, #(1 << 21) - 1
> +	and 	x4, x1, x5
> +	add	x2, x0, x4, lsr #(21 - 3)
> +	ldr	x3, =(3 << 53) | 0x401 | 1 << 2	/* attr(AF, NX, block, AttrIndx=Attr1) */
> +	orr	x1, x1, x3
> +	str	x1, [x2]
> +
>   	/* Setup/enable the MMU.  */
>   
>   	/*
>   	 * TCR_EL1 - Translation Control Registers
>   	 *
> +	 * TBI0[37] = 0b1 => Top Byte ignored and used for tagged addresses
>   	 * IPS[34:32] = 40-bit PA, 1TB
>   	 * TG0[14:15] = b00 => 4kb granuale
>   	 * ORGN0[11:10] = Outer: Normal, WB Read-Alloc No Write-Alloc Cacheable
> @@ -152,16 +163,22 @@ __start:
>   	 * with at least 1gb range to see RAM. So we start with a
>   	 * level 1 lookup.
>   	 */
> -	ldr	x0, = (2 << 32) | 25 | (3 << 10) | (3 << 8)
> +	ldr	x0, = (1 << 37) | (2 << 32) | 25 | (3 << 10) | (3 << 8)
>   	msr	tcr_el1, x0
>   
> -	mov	x0, #0xee			/* Inner/outer cacheable WB */
> +	/*
> +	 * Attr0: Normal, Inner/outer cacheable WB
> +	 * Attr1: Tagged Normal (MTE)
> +	 */
> +	mov	x0, #0xf0ee

Up to here, I think we're fine, no matter the emulated cpu model.

>   	msr	mair_el1, x0
>   	isb
>   
>   	/*
>   	 * SCTLR_EL1 - System Control Register
>   	 *
> +	 * ATA[43] = 1 = enable access to allocation tags at EL1
> +	 * TCF[40] = 1 = Tag Check Faults cause a synchronous exception
>   	 * WXN[19] = 0 = no effect, Write does not imply XN (execute never)
>   	 * I[12] = Instruction cachability control
>   	 * SA[3] = SP alignment check
> @@ -169,7 +186,8 @@ __start:
>   	 * M[0] = 1, enable stage 1 address translation for EL0/1
>   	 */
>   	mrs	x0, sctlr_el1
> -	ldr	x1, =0x100d			/* bits I(12) SA(3) C(2) M(0) */
> +	/* Bits set: ATA(43) TCF(40) I(12) SA(3) C(2) M(0) */
> +	ldr	x1, =(0x100d | 1 << 43 | 1 << 40)

But here, it's only legal to run this modified boot.S on -cpu max.
We should check for MTE enabled before setting those, or
set them elsewhere, e.g. in main of the specific MTE test.

> @@ -239,3 +257,5 @@ ttb_stage2:
>   stack:
>   	.space 65536, 0
>   stack_end:
> +
> +	.section .mte_page

Why?

> diff --git a/tests/tcg/aarch64/system/kernel.ld b/tests/tcg/aarch64/system/kernel.ld
> index 7b3a76dcbf..7c00c1c378 100644
> --- a/tests/tcg/aarch64/system/kernel.ld
> +++ b/tests/tcg/aarch64/system/kernel.ld
> @@ -18,6 +18,11 @@ SECTIONS
>       .bss : {
>           *(.bss)
>       }
> +    /* align MTE section to next (third) 2mb */
> +    . = ALIGN(1 << 22);
> +    .mte : {
> +        *(.mte_page)
> +    }

Why?

>       /DISCARD/ : {
>           *(.ARM.attributes)
>       }
> diff --git a/tests/tcg/aarch64/system/mte.c b/tests/tcg/aarch64/system/mte.c
> new file mode 100644
> index 0000000000..58a5ac31ff
> --- /dev/null
> +++ b/tests/tcg/aarch64/system/mte.c
> @@ -0,0 +1,40 @@
> +#include <inttypes.h>
> +
> +int main(void)
> +{
> +    uint8_t *addr;
> +
> +    /*
> +     * Third 2MB chunk in the second 1GB block.
> +     * See .mte_page section in kernel.ld.
> +     */
> +    addr = (void *)((1UL << 30) | (1UL << 22));

... because you're not using .mte_page here, just computing it.

> +
> +    asm (
> +        /*
> +         * Set GCR for randon tag generation. 0xA5 is just a random value to set

random

> +         * GCR != 0 so the tag generated by 'irg' is not zero.
> +         */
> +        "ldr x1, =0xA5;"
> +        "msr gcr_el1, x1;"

I think it might be easier to split the asm:

   asm volatile("msr gcr_el1, %0" : : "r"(0xA5));

> +
> +         /* Generate a logical tag and put it in 'addr' pointer. */
> +         "irg %[addr], %[addr];"

   asm("irg %0,%0" : "+r"(addr));

> +
> +         /*
> +          * Store the generated tag to memory region pointed to by 'addr', i.e.
> +          * set the allocation tag for the memory region.
> +          */
> +         "stg %[addr], [%[addr]];"

Storing addr into addr is a titch confusing, clearer with zero?

   asm("stg xzr,[%0]" : : "r"(addr));

> +
> +         /*
> +          * Store a random value (0xdeadbeef) to *addr. This must not cause any
> +          * Tag Check Fault since logical and allocation tags are set the same.
> +          */
> +	 "ldr x1, =0xdeadbeef;"
> +	 "str x1, [x0];"

Where does x0 come from?  Certainly not "addr"...
Can you use "addr" directly in the gdb script?


r~

Re: [PATCH 4/4] tests/tcg/aarch64: Extend MTE gdbstub tests to system mode

[Gustavo Romero]

Hi Richard,

On 7/25/24 8:17 PM, Richard Henderson wrote:
> On 7/23/24 02:07, Gustavo Romero wrote:
>>   def run_test():
>> -    gdb.execute("break 95", False, True)
>> +    if mode == "system":
>> +        # Break address: where to break before performing the tests
>> +        # Addresss is the last insn. before 'main' returns. See mte.c
>> +        ba = "*main+52"
> 
> Ugly.  You can add labels in your inline asm block instead.

I forgot about that possibility. Thanks. This motivated me to
convert mte.c to mte.S. I think there is no benefit in keeping
the test in C with a bunch of inline asm blocks. Please see v2.


>> diff --git a/tests/tcg/aarch64/system/boot.S b/tests/tcg/aarch64/system/boot.S
>> index 501685d0ec..a12393d00b 100644
>> --- a/tests/tcg/aarch64/system/boot.S
>> +++ b/tests/tcg/aarch64/system/boot.S
>> @@ -135,11 +135,22 @@ __start:
>>       orr    x1, x1, x3
>>       str    x1, [x2]            /* 2nd 2mb (.data & .bss)*/
>> +        /* Third block: .mte_page */
>> +    adrp    x1, .mte_page
>> +    add    x1, x1, :lo12:.mte_page
>> +    bic    x1, x1, #(1 << 21) - 1
>> +    and     x4, x1, x5
>> +    add    x2, x0, x4, lsr #(21 - 3)
>> +    ldr    x3, =(3 << 53) | 0x401 | 1 << 2    /* attr(AF, NX, block, AttrIndx=Attr1) */
>> +    orr    x1, x1, x3
>> +    str    x1, [x2]
>> +
>>       /* Setup/enable the MMU.  */
>>       /*
>>        * TCR_EL1 - Translation Control Registers
>>        *
>> +     * TBI0[37] = 0b1 => Top Byte ignored and used for tagged addresses
>>        * IPS[34:32] = 40-bit PA, 1TB
>>        * TG0[14:15] = b00 => 4kb granuale
>>        * ORGN0[11:10] = Outer: Normal, WB Read-Alloc No Write-Alloc Cacheable
>> @@ -152,16 +163,22 @@ __start:
>>        * with at least 1gb range to see RAM. So we start with a
>>        * level 1 lookup.
>>        */
>> -    ldr    x0, = (2 << 32) | 25 | (3 << 10) | (3 << 8)
>> +    ldr    x0, = (1 << 37) | (2 << 32) | 25 | (3 << 10) | (3 << 8)
>>       msr    tcr_el1, x0
>> -    mov    x0, #0xee            /* Inner/outer cacheable WB */
>> +    /*
>> +     * Attr0: Normal, Inner/outer cacheable WB
>> +     * Attr1: Tagged Normal (MTE)
>> +     */
>> +    mov    x0, #0xf0ee
> 
> Up to here, I think we're fine, no matter the emulated cpu model.
> 
>>       msr    mair_el1, x0
>>       isb
>>       /*
>>        * SCTLR_EL1 - System Control Register
>>        *
>> +     * ATA[43] = 1 = enable access to allocation tags at EL1
>> +     * TCF[40] = 1 = Tag Check Faults cause a synchronous exception
>>        * WXN[19] = 0 = no effect, Write does not imply XN (execute never)
>>        * I[12] = Instruction cachability control
>>        * SA[3] = SP alignment check
>> @@ -169,7 +186,8 @@ __start:
>>        * M[0] = 1, enable stage 1 address translation for EL0/1
>>        */
>>       mrs    x0, sctlr_el1
>> -    ldr    x1, =0x100d            /* bits I(12) SA(3) C(2) M(0) */
>> +    /* Bits set: ATA(43) TCF(40) I(12) SA(3) C(2) M(0) */
>> +    ldr    x1, =(0x100d | 1 << 43 | 1 << 40)
> 
> But here, it's only legal to run this modified boot.S on -cpu max.
> We should check for MTE enabled before setting those, or
> set them elsewhere, e.g. in main of the specific MTE test.

Right. I move all these bits for MTE to mte.S, under main function,
and just left the page table setting in boot.S, since we need to
set it before the MMU is turned on.


>> @@ -239,3 +257,5 @@ ttb_stage2:
>>   stack:
>>       .space 65536, 0
>>   stack_end:
>> +
>> +    .section .mte_page
> 
> Why?
> 
>> diff --git a/tests/tcg/aarch64/system/kernel.ld b/tests/tcg/aarch64/system/kernel.ld
>> index 7b3a76dcbf..7c00c1c378 100644
>> --- a/tests/tcg/aarch64/system/kernel.ld
>> +++ b/tests/tcg/aarch64/system/kernel.ld
>> @@ -18,6 +18,11 @@ SECTIONS
>>       .bss : {
>>           *(.bss)
>>       }
>> +    /* align MTE section to next (third) 2mb */
>> +    . = ALIGN(1 << 22);
>> +    .mte : {
>> +        *(.mte_page)
>> +    }
> 
> Why?
> 
>>       /DISCARD/ : {
>>           *(.ARM.attributes)
>>       }
>> diff --git a/tests/tcg/aarch64/system/mte.c b/tests/tcg/aarch64/system/mte.c
>> new file mode 100644
>> index 0000000000..58a5ac31ff
>> --- /dev/null
>> +++ b/tests/tcg/aarch64/system/mte.c
>> @@ -0,0 +1,40 @@
>> +#include <inttypes.h>
>> +
>> +int main(void)
>> +{
>> +    uint8_t *addr;
>> +
>> +    /*
>> +     * Third 2MB chunk in the second 1GB block.
>> +     * See .mte_page section in kernel.ld.
>> +     */
>> +    addr = (void *)((1UL << 30) | (1UL << 22));
> 
> ... because you're not using .mte_page here, just computing it.

.mte_page is used in boot.S for setting the page table entry, in:

         /* Third block: .mte_page */
         adrp    x1, .mte_page
         add     x1, x1, :lo12:.mte_page
	[...]

That's why it's being computed in kernel.ld. That said, you're
right that it's better to also tie 'addr' to it instead of having
a hard-coded value.

I'm using the .mte_page address in the new mte.S test in v2, so
now the value for 'addr' used in the test is tied to that computed
value.


>> +
>> +    asm (
>> +        /*
>> +         * Set GCR for randon tag generation. 0xA5 is just a random value to set
> 
> random
> 
>> +         * GCR != 0 so the tag generated by 'irg' is not zero.
>> +         */
>> +        "ldr x1, =0xA5;"
>> +        "msr gcr_el1, x1;"
> 
> I think it might be easier to split the asm:
> 
>    asm volatile("msr gcr_el1, %0" : : "r"(0xA5));

hmm, all the tests are built with optimizations disabled,
so no need to use volatile? Anyways, in v2 I'm using mov
instead of ldr, and since it's .S, no more inline asm.


>> +
>> +         /* Generate a logical tag and put it in 'addr' pointer. */
>> +         "irg %[addr], %[addr];"
> 
>    asm("irg %0,%0" : "+r"(addr));
> 
>> +
>> +         /*
>> +          * Store the generated tag to memory region pointed to by 'addr', i.e.
>> +          * set the allocation tag for the memory region.
>> +          */
>> +         "stg %[addr], [%[addr]];"
> 
> Storing addr into addr is a titch confusing, clearer with zero?
> 
>    asm("stg xzr,[%0]" : : "r"(addr))

This an invalid insn. xzr reg. can't be used as operand 1 in 'stg',
because register 31 is encoded as sp, not xzr.

However, I don't want to zero the allocation tag. Actually, it's
the opposite, I want it != 0, that's why I set gcr_el1 != 0: to
get a non-zero tag from 'irg'.

I agree it's a tad confusing, but the thing is, 'stg' expects a
tagged address in the source operand, not a "pure" tag, hence the
use of 'addr' in operand 1. But I think it could be:

stg tagged_addr, [addr]

In v2 I put a comment that stg actually extracts the tag to be
stored in [addr] from the address in 'tagged_addr'


>> +
>> +         /*
>> +          * Store a random value (0xdeadbeef) to *addr. This must not cause any
>> +          * Tag Check Fault since logical and allocation tags are set the same.
>> +          */
>> +     "ldr x1, =0xdeadbeef;"
>> +     "str x1, [x0];"
> 
> Where does x0 come from?  Certainly not "addr"...
> Can you use "addr" directly in the gdb script?

Yep, it should be 'addr' instead of x0 here. It's a leftover.
Fixed in v2. Thanks.


Cheers,
Gustavo