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

[Gustavo Romero]

Hi Phil!

On 8/26/24 3:10 AM, Philippe Mathieu-Daudé wrote:
> Hi Gustavo,
>
> On 25/8/24 16:52, Gustavo Romero wrote:
> > 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 on baremetal 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 ops. 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, mte.S, is also introduced for the tests. It links
> > against boot.S and is executed by QEMU in system mode.
> >
> > Signed-off-by: Gustavo Romero <gustavo.romero@linaro.org>
> > ---
> >   configure                                 |   5 +
> >   tests/tcg/aarch64/Makefile.softmmu-target |  49 +++++++++-
> >   tests/tcg/aarch64/Makefile.target         |   3 +-
> >   tests/tcg/aarch64/gdbstub/test-mte.py     |  71 +++++++++-----
> >   tests/tcg/aarch64/system/boot.S           |  11 +++
> >   tests/tcg/aarch64/system/kernel.ld        |   7 ++
> >   tests/tcg/aarch64/system/mte.S            | 109 ++++++++++++++++++++++
> >   7 files changed, 227 insertions(+), 28 deletions(-)
> >   create mode 100644 tests/tcg/aarch64/system/mte.S
>
>
> > diff --git a/tests/tcg/aarch64/system/kernel.ld 
> > b/tests/tcg/aarch64/system/kernel.ld
> > index 7b3a76dcbf..46f1092522 100644
> > --- a/tests/tcg/aarch64/system/kernel.ld
> > +++ b/tests/tcg/aarch64/system/kernel.ld
> > @@ -18,6 +18,13 @@ SECTIONS
> >       .bss : {
> >           *(.bss)
> >       }
> > +    /*
> > +     * Align the MTE page to the next 2mb boundary (i.e., the third 2mb chunk
> > +     * starting from 1gb) by setting the address for symbol 'mte_page', which 
> > is
> > +     * used in boot.S to setup the PTE and in the mte.S test as the address 
> > that
> > +     * the MTE instructions operate on.
> > +     */
> > +    mte_page = ALIGN(1 << 22);
>
> Comment says 2MiB but you use 4MiB.
> Matter of taste, 2MiB is easier to review as:
>
>        mte_page = ALIGN(2 << 20);

This is incorrect. Aligning here at 2MiB will clash with r/w data section. I 
tried to
clarify it when I wrote "[...] third 2mb chunk starting from 1gb". The memory 
layout
so is:

0         ---- 1GiB range: avoid/skipped
1GiB      ---- 1GiB+2MiB (1st 2MiB chunk): text
1GiB+2MiB ---- 1GiB+4MiB (2nd 2MiB chunk): data
1GiB+4MIB ---- 1GiB+8MiB (3rd 2MiB chunk): MTE-enabled page

ALIGN implicitly uses "." (current position), so we're defining consecutive 
sections
here starting from 1GiB.

All the other parts of the code uses the 1 << n form, which I prefer, specially 
when
reading asm code, so I prefer 1 << 22 instead of 4 << 20.

But apparently my comment failed miserable to explain the situation here, so I'm
happy to change the wording to make it clearer if you have any suggestion.


Cheers,
Gustavo

> >       /DISCARD/ : {
> >           *(.ARM.attributes)
> >       }