qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Gustavo Romero <gustavo.romero@linaro.org>
To: "Philippe Mathieu-Daudé" <philmd@linaro.org>,
	qemu-devel@nongnu.org, alex.bennee@linaro.org,
	richard.henderson@linaro.org
Cc: peter.maydell@linaro.org
Subject: Re: [PATCH v3 4/4] tests/tcg/aarch64: Extend MTE gdbstub tests to system mode
Date: Tue, 27 Aug 2024 09:42:02 -0300	[thread overview]
Message-ID: <69229d7b-15d5-feda-ee4b-1c48992297f3@linaro.org> (raw)
In-Reply-To: <64200fe4-b824-4903-b5f5-fc48c9e00945@linaro.org>

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)
>>       }
> 


  reply	other threads:[~2024-08-27 12:43 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-08-25 14:52 [PATCH v3 0/4] gdbstub: Add support for MTE in system mode Gustavo Romero
2024-08-25 14:52 ` [PATCH v3 1/4] gdbstub: Use specific MMU index when probing MTE addresses Gustavo Romero
2024-08-25 14:52 ` [PATCH v3 2/4] gdbstub: Add support for MTE in system mode Gustavo Romero
2024-08-25 14:52 ` [PATCH v3 3/4] tests/guest-debug: Support passing arguments to the GDB test script Gustavo Romero
2024-09-06 13:49   ` Alex Bennée
2024-08-25 14:52 ` [PATCH v3 4/4] tests/tcg/aarch64: Extend MTE gdbstub tests to system mode Gustavo Romero
2024-08-26  6:10   ` Philippe Mathieu-Daudé
2024-08-27 12:42     ` Gustavo Romero [this message]
2024-08-27 18:01       ` Gustavo Romero
2024-08-29  0:43         ` Richard Henderson
2024-08-29 20:13           ` Gustavo Romero
2024-08-29 22:16             ` Richard Henderson
2024-08-30 22:35               ` Gustavo Romero

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=69229d7b-15d5-feda-ee4b-1c48992297f3@linaro.org \
    --to=gustavo.romero@linaro.org \
    --cc=alex.bennee@linaro.org \
    --cc=peter.maydell@linaro.org \
    --cc=philmd@linaro.org \
    --cc=qemu-devel@nongnu.org \
    --cc=richard.henderson@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).