Re: [PATCH v5 11/13] xen/arm: mmu: move MMU specific P2M code to mmu/p2m.{c,h}

[Penny Zheng <penny.zheng@xxxxxxx>]

Hi Julien

On 2023/8/23 02:01, Julien Grall wrote:
> Hi Henry,
>
> On 14/08/2023 05:25, Henry Wang wrote:
> > From: Penny Zheng <penny.zheng@xxxxxxx>
> >
> > Current P2M implementation is designed for MMU system only.
> > We move the MMU-specific codes into mmu/p2m.c, and only keep generic
> > codes in p2m.c, like VMID allocator, etc. We also move MMU-specific
> > definitions and declarations to mmu/p2m.h, such as p2m_tlb_flush_sync().
> > Also expose previously static functions p2m_vmid_allocator_init(),
> > p2m_alloc_vmid(), __p2m_set_entry() and setup_virt_paging_one()
> Looking at the code, it seemsm that you need to keep expose 
> __p2m_set_entry() because of p2m_relinquish_mapping(). However, it is 
> not clear how this code is supposed to work for the MPU. So should we 
> instead from p2m_relinquish_mapping() to mmu/p2m.c?
p2m->root stores per-domain P2M table, which is actually an array of MPU
region(pr_t). So maybe we should relinquish mapping region by region,
instead of page by page. Nevertheless, p2m_relinquish_mapping() shall be
moved to mmu/p2m.c and we need MPU version of it.

> Other functions which doesn't seem to make sense in p2m.c are:
>    * p2m_clear_root_pages(): AFAIU there is no concept of root in the 
> MPU. This also means that we possibly want to move out anything specific 
> to the MMU from 'struct p2m'. This could be done separately.
Current MPU implementation is re-using p2m->root to store P2M table.
Do you agree on this, or should we create a new variable, like 
p2m->mpu_table, for MPU P2M table only?
How we treat p2m_clear_root_pages also decides how we destroy P2M at 
domain destruction stage, current MPU flow is as follows:
```
    PROGRESS(mapping):
        ret = relinquish_p2m_mapping(d);
        if ( ret )
            return ret;

    PROGRESS(p2m_root):
        /*
         * We are about to free the intermediate page-tables, so clear the
         * root to prevent any walk to use them.
         */
        p2m_clear_root_pages(&d->arch.p2m);

#ifdef CONFIG_HAS_PAGING_MEMPOOL
    PROGRESS(p2m):
        ret = p2m_teardown(d);
        if ( ret )
            return ret;

    PROGRESS(p2m_pool):
        ret = p2m_teardown_allocation(d);
        if( ret )
            return ret;
#endif
```
We guard MMU-specific intermediate page-tables destroy with the new 
Kconfig CONFIG_HAS_PAGING_MEMPOOL, check 
https://gitlab.com/xen-project/people/henryw/xen/-/commit/7ff6d351e65f43fc34ea694adea0e030a51b1576
for more details.

If we destroy MPU P2M table in relinquish_p2m_mapping, region by region,
we could provide empty stub for p2m_clear_root_pages, and move it to 
mmu/p2m.c
>    * p2m_flush_vm(): This is built with MMU in mind as we can use the 
> page-table to track access pages. You don't have that fine granularity 
> in the MPU.
Understood

> > for futher MPU usage.
> typo: futher/further/
>
> > With the code movement, global variable max_vmid is used in multiple
> > files instead of a single file (and will be used in MPU P2M
> > implementation), declare it in the header and remove the "static" of
> > this variable.
> >
> > Add #ifdef CONFIG_HAS_MMU to p2m_write_unlock() since future MPU
> > work does not need p2m_tlb_flush_sync().
> And there are no specific barrier required? Overall, I am not sure I 
> like the #ifdef rather than providing a stub helper.
> If the other like the idea of the #ifdef. I think a comment on top would 
> be necessary to explain why there is nothing to do in the context of the 
> MPU.
> Cheers,