core: add missing barrier to page table write

[stuyoder]

When updating a PTE, we need to make sure that the write to memory
is visible before any possible reads of that virtual address occur.

Signed-off-by: Stuart Yoder stuart.yoder@arm.com
Reviewed-by: James King james.king@arm.com

[stuyoder]

I found this issue when testing the dynamic shared memory patches. A mapping was created for incoming message arguments (i.e. PTE was written), followed fairly quickly by a read of the memory. It appears the read got executed before the PTE write, resulting in a data abort.

[jenswi-linaro]

I'd like to have this at a higher level in order to be able to update several entries before doing dsb().

[stuyoder]

core_mmu_set_entry_primitive() is called from 2 places:
core/arch/arm/mm/tee_pager.c: area_set_entry()
core/arch/arm/mm/core_mmu.c: core_mmu_set_entry()

There are quite a few calls to area_set_entry() and core_mmu_set_entry() sprinkled around. Are we really concerned with performance here? ...it seems much more error prone to put the burden on higher level callers to do the dsb primitive. It's going to be real easy for someone to forget to add an explicit dsb() after calling certain functions. If you look at all the other uses of dsb in OP-TEE it seems that the practice has been to put synchronization primitives directly where the barrier is needed.

[jenswi-linaro]

Usually when the tables are updated there's a need to invalidate tlb also, so this change doesn't solve the problem it just hides it for a little longer.

[stuyoder]

The TLB should be invalidated when something gets unmapped, as the virtual address is no longer valid. But, generally when something is mapped we should be guaranteed that the virtual address is not already in use, and so not sure why a TLB invalidate should be needed there.

In any case, if there are missing TLB invalidates, that is a separate issue. My patch is dealing with one discrete issue-- a barrier needed after writing the PTE.

[jenswi-linaro]

There's many exceptions to that rule. Paging is one case, another is mapping of TAs. The dsb should be placed at a higher level.

[etienne-lms]

@stuyoder, sequences that need to update mmu tables need to add the required dsb/isb synchro after a full mapping is updated and before system is allowed to use it. Syncing on each mmu descriptor update is inefficient and does not prevent from the final sync.

(...) it seems much more error prone to put the burden on higher level callers to do the dsb primitive. It's going to be real easy for someone to forget to add an explicit dsb() after calling certain functions. (...)

Indeed when one manipulates mmu table, he/she must be careful. It's not a common scenario to write to mmu tables. optee core developers usually use high level apis to map a virtual address range to a physical location.

[stuyoder]

Here are the callers of area_set_entry() that need to be fixed up:
core/arch/arm/mm/tee_pager.c:816: area_set_entry(area, pmem->pgidx, 0, 0);
core/arch/arm/mm/tee_pager.c:902: area_set_entry(pmem->area, pmem->pgidx, 0, 0);
core/arch/arm/mm/tee_pager.c:908: area_set_entry(pmem->area, pmem->pgidx, pa, f);
core/arch/arm/mm/tee_pager.c:969: area_set_entry(pmem->area, pmem->pgidx, pa, a);
core/arch/arm/mm/tee_pager.c:1014: area_set_entry(pmem->area, pmem->pgidx, pa, a);
core/arch/arm/mm/tee_pager.c:1043: area_set_entry(area, pgidx, 0, 0);
core/arch/arm/mm/tee_pager.c:1073: area_set_entry(pmem->area, pmem->pgidx, 0, 0);
core/arch/arm/mm/tee_pager.c:1149: area_set_entry(area, pgidx, pa,
core/arch/arm/mm/tee_pager.c:1163: area_set_entry(area, pgidx, pa,
core/arch/arm/mm/tee_pager.c:1301: area_set_entry(area, pmem->pgidx, get_pmem_pa(pmem), attr);
core/arch/arm/mm/tee_pager.c:1362: area_set_entry(pmem->area, pgidx, pa,
core/arch/arm/mm/tee_pager.c:1408: area_set_entry(pmem->area, pmem->pgidx, 0, 0);

Here are the callers of core_mmu_set_entry() that need to be fixed up:
core/arch/arm/mm/core_mmu.c:982: core_mmu_set_entry_primitive(tbl_info->table, tbl_info->level,
core/arch/arm/mm/core_mmu.c:1011: core_mmu_set_entry(tbl_info, idx, pa, region->attr);
core/arch/arm/mm/core_mmu.c:1050: core_mmu_set_entry(dir_info, idx,
core/arch/arm/mm/core_mmu.c:1119: core_mmu_set_entry(&tbl_info, idx, pages[i],
core/arch/arm/mm/core_mmu.c:1154: core_mmu_set_entry(&tbl_info, idx, 0, 0);
core/arch/arm/mm/core_mmu_lpae.c:711:void core_mmu_set_entry_primitive(void *table, size_t level, size_t idx,
core/arch/arm/mm/core_mmu_v7.c:560:void core_mmu_set_entry_primitive(void *table, size_t level, size_t idx,
core/arch/arm/mm/tee_pager.c:239: core_mmu_set_entry(ti, idx, 0, 0);
core/arch/arm/mm/tee_pager.c:272: core_mmu_set_entry(ti, idx, pa, attr);
core/arch/arm/mm/tee_pager.c:499: core_mmu_set_entry(ti, idx_alias, pa_alias, attr_alias);
core/arch/arm/mm/tee_pager.c:521: core_mmu_set_entry(ti, idx_alias, pa_alias, attr_alias);
core/arch/arm/mm/tee_pager.c:579: core_mmu_set_entry_primitive(area->pgt->tbl, tee_pager_tbl_info.level,
core/arch/arm/mm/tee_pager.c:704: core_mmu_set_entry(&dir_info, idx, pa, attr);
core/arch/arm/mm/tee_pager.c:747: core_mmu_set_entry(&old_ti, pmem->pgidx, 0, 0);
core/arch/arm/mm/tee_pager.c:757: core_mmu_set_entry(&new_ti, pmem->pgidx, pa, attr);
core/arch/arm/mm/tee_pager.c:1351: core_mmu_set_entry(ti, pgidx, 0, 0);

Fixing all those callers is beyond the scope of what I'm going to be able to do, so I guess I'll just open up an issue to log this.

But, it is a real issue. As you say, when updating a page table entry synchronization is required before letting the system use the mapping. OP-TEE does not do that today.

[stuyoder]

As an aside, here is how it's handled in the Linux kernel...if the mapping is a valid kernel mapping they do the dsb immediately at the lowest level:

    static inline void set_pte(pte_t *ptep, pte_t pte)
    {
            *ptep = pte;
    
            /*
             * Only if the new pte is valid and kernel, otherwise TLB maintenance
             * or update_mmu_cache() have the necessary barriers.
             */
            if (pte_valid_global(pte)) {
                    dsb(ishst);
                    isb();
            }
    }

[jenswi-linaro]

@stuyoder what are you trying to prove?
Almost all the references in the pager are very obvious as there's a tlb invalidate after most, others need some more analysis but as far as I can tell it's taken care of in calling functions one or two steps up.

Without digging very deep into this it looks like it's core_mmu_map_pages() that need to be fixed. Doing it in that function is the right level as it's after that function has returned that the caller expects the mapping to be in place, not earlier.

[stuyoder]

Not trying to prove anything :) ...just pointing out that there are various callers that write PTEs.

In addition to core_mmu_map_pages(), there are set_region(), set_pg_region(), core_mmu_unmap_pages(). For set_region/set_pg_region it's not clear to me where the dsb() is.

Given that I know relatively little about the optee kernel, my preference would be that someone who is more intimately familiar with this apply a fix where needed. I'm happy to test it.

Also, if it is expected that a caller must do the dsb() synchronization it would be a good idea to add a comment in the function header explicitly stating that, to avoid someone missing that in the future.

[stuyoder]

This is fixed with #1827