Thanks Andrew.
The places that mm_ptable_defrag_entry() is assigned should also be using mm_replace_entry() if there was a change and there are likely some missing barriers there too.
[RK] Agree. Can you elaborate on what you mean by "there are likely some missing barriers there too"? Are you talking about the DSB+ISB missing after line 280 in src/arch/aarch64/mm.c? It seems like if you change the assignments you referred to with mm_replace_entry() and change mm_ptable_defrag to invalidate TLB(possibly the entire TLB for simplicity) or add a DSB+ISB at the end of the function should give a working implementation.
there's a final invalidation after the entire mapping has been completed [2].
[RK] Agree. This works. However, is an invalidation really required if we did use break-before-make or add new entries? Break-before-make invalidates replaced entries and the architecture does not allow invalid translations to be cached in the TLB. With respect to line 478 in src/mm.c, It seemed odd to me to rely on that line for the final barriers, since it is under an if statement. It appears that after one_time_init, we always invalidate page tables whether it is new mappings or replacing mappings. What is the reason we wanted to delay invalidation until end of one_time_init? Did it give a significant performance boost? It seems cleaner to me to explicitly have a DSB+ISB before line 478 or remove the if condition around line 478 and have some code comments about previous PTE writes becoming entirely visible after the invalidation.
Let me know what you think.
Thanks Raghu
-----Original Message----- From: Andrew Scull ascull@google.com Sent: Tuesday, January 5, 2021 8:00 AM To: Andrew Walbran qwandor@google.com Cc: Raghu K raghu.ncstate@icloud.com; Wedson Almeida Filho wedsonaf@google.com; hafnium@lists.trustedfirmware.org Subject: Re: [Hafnium] Possible bugs with page table management
I was going through src/mm.c in the hafnium repo and noticed a couple of things that I think are problem:
mm_vm_defrag and mm_defrag, which eventually call mm_ptable_defragappears to be changing page entries into block entries. Per D5.10.1 of ARM ARM, this should use break-before-make, but I don't see it being done in code. Am I missing where this is done in code, or is there some reason break-before-make is not required during defrag(that is not obvious to me from code or comments)?
This does seem like it's just wrong. Despite there not being a change in the attributes, the ARM does say that a change of mapping size should also break-before-make, but that's not what it does. The places that mm_ptable_defrag_entry() is being assigned should also be using mm_replace_entry() if there was a change and there are likely some missing barriers there too.
In mm_replace_entry, line 278(https://git.trustedfirmware.org/hafnium/hafnium.git/tree/src/mm.c#n2 78) is not followed by a DSB + ISB as required by the break-before-make. Without the DSB, a load/store that uses a VA translated by the PTE being replaced could be reordered before line 278 and fault. Once again, this looks like a bug to me but is there some reason that is not obvious due to which it does not require barriers?
From a quick scan, it looks like the barriers in this case are in the arch code [1] for the TLB invalidation. The intermediate ones happen as the break is being made and there's a final invalidation after the entire mapping has been completed [2].
[1] -- https://git.trustedfirmware.org/hafnium/hafnium.git/tree/src/arch/aarch64/mm... [2] -- https://git.trustedfirmware.org/hafnium/hafnium.git/tree/src/mm.c#n478