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~