Re: [kvm-unit-tests PATCH v2] arm64: Add basic MTE test

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi Vladimir,

On Mon, Dec 23, 2024 at 02:37:40PM +0000, Vladimir Murzin wrote:
> Hi Alexandru,
> 
> On 12/23/24 12:03, Alexandru Elisei wrote:
> > Hi Vladimir,
> > 
> > The patch looks good, but it just occured to me, the tests do a great job
> > checking that tagged accesses fail when they should be failing, but they don't
> > check that taggedd accesses *succeed* when they should not be failing. I think
> > that's useful to have at the start of each test, if nothing just as a sanity
> > check.
> > 
> 
> Well, there are successful tagged access, yet with zero tag :) We can move to
> non-zero tag, something like
> 
> static inline void mte_memset(void *addr, int val, size_t size)
> {
> 	unsigned long old = mte_set_tcf(MTE_TCF_SYNC);
> 	memset(addr, val, size);
> 	mte_set_tcf(old);
> }
> 
> ...
> 
> static void mte_async_test(void)
> {
> 	unsigned int *mem = tagged(alloc_page(), 3);
> 	unsigned int val = 0;
> 
> 	mte_init();
> 	mmu_set_tagged(current_thread_info()->pgtable, (unsigned long)mem);
> 	mte_set_tag(mem, PAGE_SIZE, 3);
> 	mte_memset(mem, 0xff, PAGE_SIZE);
> 	mte_set_tcf(MTE_TCF_ASYNC);
> ...

I have a few ideas here:

* Tag checked accesses when the address bits [59:55] are 0 are also controlled
with TCR_EL1.TCMA0, and the bit is set to 0 in asm_mmu_enabled() - this is what
we want. But if you want to explicitely check tagged accesses with the zero tag
I would suggest that the bit is explicitely cleared first (probably in
mte_init()), to make sure the tests starts with a known good configuration (very
useful for debugging!) and to avoid surprises if something changes in the
startup sequence of a test.

* All tag checked accesses should probably happen after setting SCTLR_EL1.TCF,
since the field is set to 0b00 in cstart64.S (when SCTLR_EL1 is set to
INIT_SCTLR_EL1_MMU_OFF).

I don't have a preference, up to you to decide (you can even go so far as to
check that TCR_EL1.TCMA0 works as expected if you feel it's useful), I was just
pointing out some of the choices you have.

[..]
> >> +}
> >> diff --git a/arm/run b/arm/run
> >> index efdd44ce..b129e4e0 100755
> >> --- a/arm/run
> >> +++ b/arm/run
> >> @@ -29,7 +29,8 @@ if ! $qemu -machine '?' | grep -q 'ARM Virtual Machine'; then
> >>  	exit 2
> >>  fi
> >>  
> >> -M='-machine virt'
> >> +MACHINE="virt"
> >> +M="-machine $MACHINE$MACHINE_PROPS"
> > Sorry, but I still don't understand what $MACHINE_PROPS does, I can't seem to
> > find where it's initialized :(
> > 
> 
> I'm not sure what you are looking/asking for :( 
> 
> My last reply [1] demonstrates usage for $MACHINE_PROPS
> 
> QEMU=qemu-system-aarch64 ACCEL=tcg MACHINE_PROPS=",mte=on" arm/run arm/mte.flat -append "sync"

Sorry about that, I must have missed your reply :( Adding MACHINE_PROPS is a
nifty idea, but I don't think it's good practice to depend on that to be able to
run a test. ./run_tests.sh <testname> should work without user
intervention/configuration.

For your particular example, you don't need to add a new environment
variable if you want to run the test with arm/run, you can do this instead:

QEMU=/path/to/qemu ACCEL=tcg arm/run arm/mte.flat -machine mte=on -append "sync"

arm/run passes all the parameters as they are to qemu.

In general, you can have the machine type in the unit test definition in
arm/unitests.cfg, in the 'extra_params' field, just like for the gicv2 and gicv3
tests. In theory, there's a separate 'machine' field in the test definition (see
docs/unittests.txt), but arm/arm64 ignores it.

This is what I had in mind for the test definition:

diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index 9b428c02dabc..fe1011454f88 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -275,18 +275,18 @@ arch = arm64
 # MTE tests
 [mte-sync]
 file = mte.flat
-groups = nodefault
-extra_params = -append 'sync'
+groups = mte
+extra_params = -machine mte=on -append 'sync'
 arch = arm64

 [mte-async]
 file = mte.flat
-groups = nodefault
-extra_params = -append 'async'
+groups = mte
+extra_params = -machine mte=on -append 'async'
 arch = arm64

 [mte-asymm]
 file = mte.flat
-groups = nodefault
-extra_params = -append 'asymm'
+groups = mte
+extra_params = -machine mte=on -append 'asymm'
 arch = arm64

Interestingly though, when I try to run a test (either with ./run_tests.sh
mte-sync, or by trying out your example), I get this error:

qemu-system-aarch64: MTE requested, but not supported by the guest CPU

My guess is that's caused by kvm-unit-tests defaulting to cortex-a57 as the
model CPU. Changing the cpu to 'max' makes the test run. How are you getting
around this on your end?

Thanks,
Alex