H Jens, Richard,
Thanks for your replies.
See comments inline [OD].
Regards, Olivier.
________________________________ From: Richard Henderson richard.henderson@linaro.org Sent: 15 February 2023 23:34 To: Jens Wiklander jens.wiklander@linaro.org; Olivier Deprez Olivier.Deprez@arm.com Cc: hafnium@lists.trustedfirmware.org hafnium@lists.trustedfirmware.org; Peter Maydell peter.maydell@linaro.org Subject: Re: splitting NS/S IPA spaces
On 2/15/23 11:19, Jens Wiklander wrote:
Hi Olivier,
[+ Peter and Richard]
Comments below.
On Wed, Feb 15, 2023 at 5:02 PM Olivier Deprez Olivier.Deprez@arm.com wrote:
Hi Jens,
I have a couple of Hafnium changes implementing the use of VSTTBR_EL2/VTTBR_EL2 to split an SP IPA into secure and non-secure IPA spaces. They're very much in experimental stage so difficult to share just now (I will do some time later in February).
Aha, so everything has effectively been mapped as secure until now.
[OD] Not exactly. So far we've had VTTBR=VSTTBR, that is both secure and non-secure IPA spaces directed to the same S2 PTs. I agree this isn't ideal however we concluded with arch team this isn't a security issue per se. If the TOS maps TZ non-secure memory as S1 secure then this hits the TZC400 controller anyways by a synchronous ext. abort or Serror. Similarly, for TZ secure memory mapped S1 non-secure. Splitting S2 into separate secure/non-secure PTs is a better hardening as it permits hitting a S2 fault early at the (secure) hypervisor rather than the TZC. In other words this better spots/reports a mismatch done by the TOS when configuring the S1 PTE NS bit.
However I'd like to report some possible issue observed with qemu.
Essentially, when normal world driver inits, it performs a first share operation for a single NS page:
INFO: 1> 1 0 FFA_MEM_SHARE_32(84000073) 50 50 0 0 0 0 0 [...] VERBOSE: Marked sending complete. Current share states: SHARE 0x0 (from VM 0x0, attributes 0x6f (NS), flags 0x8, tag 0, to 1 recipients [VM 0x8001: 0x6 (offset 48)]): fully sent with 1 fragments, 0 retrieved, sender's original mode: 0x87 INFO: 1< 1 0 FFA_SUCCESS_32(84000061) 0 0 0 0 0 0 0 INFO: 1> 1 0 FFA_MSG_SEND_DIRECT_REQ_32(8400006f) 8001 0 80000000 0 0 0 0 E/TC:1 0 mobj_ffa_get_by_cookie:387 Populating mobj from rx buffer, cookie 0
Retrieve operation happens:
E/TC:1 0 spmc_retrieve_req:1415 spmc_retrieve_req enter. INFO: 1> 1 8001 FFA_MEM_RETRIEVE_REQ_32(84000074) 30 30 0 0 0 0 0 Current share states: SHARE 0x0 (from VM 0x0, attributes 0x6f (NS), flags 0x8, tag 0, to 1 recipients [VM 0x8001: 0x6 (offset 48)]): fully sent with 1 fragments, 0 retrieved, sender's original mode: 0x87 Current share states: SHARE 0x0 (from VM 0x0, attributes 0x6f (NS), flags 0x8, tag 0, to 1 recipients [VM 0x8001: 0x6 (offset 48)]): fully sent with 1 fragments, 1 retrieved, sender's original mode: 0x87 INFO: 1< 1 8001 Unknown(84000075) 50 50 0 0 0 0 0
Hafnium maps the NS page into OP-TEE's S2 page tables rooted to by VTTBR_EL2
0: e178003 S 1: e179003 S f: e17a003 S 186: 240000041f867ff NS
(similar dump from VSTTBR_EL2 show OP-TEE secure pages properly mapped)
OP-TEE then maps the page in its S1 PTs as NS:
E/TC:1 0 spmc_retrieve_req:1428 spmc_retrieve_req exit. E/TC:1 0 thread_spmc_populate_mobj_from_rx:1506 thread_spmc_populate_mobj_from_rx exit. E/TC:1 0 set_pages:1461 set_pages 0 addr 41f86000 count 1 E/TC:1 0 mobj_ffa_add_pages_at:220 mobj_ffa_add_pages_at is_ns 0 INFO: 1> 1 8001 FFA_RX_RELEASE_32(84000065) 0 0 0 0 0 0 0 INFO: 1< 1 8001 FFA_SUCCESS_32(84000061) 0 0 0 0 0 0 0 E/TC:1 0 ffa_inc_map:566 ffa_inc_map addr fa00000 pages 0x90000000e3eadd0 sz 4096 D/TC:1 0 core_mmu_xlat_table_alloc:526 xlat tables used 4 / 5
A page fault is hit when OP-TEE accesses the page from its VA:
WARNING: Stage-2 page fault: pc=0xe30b764, vmid=0x8001, vcpu=1, vaddr=0xfa0001c, ipaddr=0x41f8601c, mode=0x1 0x40000000000007c
This issue is not observed with the TC2 FVP and similar Hafnium+OP-TEE SW stack, at the same point of initialization. So it seems qemu is not doing the translation properly from VTTBR_EL2 for a page mapped NS by OP-TEE (hence NS IPA space).
Who should I report this problem to?
When I have a narrowed down error like this I'm usually reaching out to Peter Maydell on the QEMU mailing list. Peter will probably ask if you have binaries to reproduce the problem with.
Indeed, a binary would be helpful.
[OD] I sent reproducer binaries as separate email.
We believe that we're properly merging the bits shared between V{S,}TTBR_EL2, and we believe that we're properly dropping from Secure to NonSecure output address space.
[OD] Thanks for confirming. I will continue polishing the changes, and compare with what happens with FVP which doesn't exhibit this issue. I noticed this assert hit around the location of failure (this isn't always reported though, this might have to do with gdb connection): qemu-system-aarch64: ../target/arm/ptw.c:301: S1_ptw_translate: Assertion `fi->type != ARMFault_None' failed.
Also note the VTCR/VSTCR configuration which might help narrowing down: https://git.trustedfirmware.org/hafnium/hafnium.git/tree/src/arch/aarch64/mm... https://git.trustedfirmware.org/hafnium/hafnium.git/tree/src/arch/aarch64/mm...
It might be helpful if you could double-check with
https://gitlab.com/rth7680/qemu/-/tree/tgt-arm-rme
where there have been some changes in that area to incorporate the RME Realm and Root spaces, but also cleanups that might have affected Secure (for better or worse).
[OD] I gave a quick try, unfortunately it doesn't boot when keeping options same as Jens' qemu setup. I suspect this requires rebuilding TF-A with RME support, which I don't have just immediately....
Btw, noticed this slight build failure I fixed this way:
../target/arm/ptw.c:1802:13: error: a label can only be part of a statement and a declaration is not a statement 1802 | int nse = extract32(attrs, 11, 1); | ^~~
diff --git a/target/arm/ptw.c b/target/arm/ptw.c index c8f13528ca..70112d1de2 100644 --- a/target/arm/ptw.c +++ b/target/arm/ptw.c @@ -1794,7 +1794,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, S1Translate *ptw, } else { int ns = extract32(attrs, 5, 1); switch (out_space) { - case ARMSS_Root: + case ARMSS_Root:; /* * R_GVZML: Bit 11 becomes the NSE field in the EL3 regime. * R_XTYPW: NSE and NS together select the output pa space.
r~