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

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 09/10/2023 02:03, 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(), and setup_virt_paging_one() for further MPU usage.
>
> 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.
>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
Some remarks about some of the code not moved:
 * struct p2m_domain: The bulk of the fields seems to be MMU specific. 
So depending on the number of common fields we either want to split or 
move the structure to p2m_domain. I would be ok to wait until the MPU 
code is present.
 * p2m_type_t: It is not yet clear how this will apply to the MPU. I am 
ok to wait before moving it.
 * p2m_cache_flush_range(): I expect the code will need some change 
because you may get large chunk of memory for the MPU.
 * p2m_set_way_flush()/p2m_toggle_cache(): This was a giant hack to 
support cache flush operations via set/way. To make it efficient, we 
track the pages that have been touched and only flush them. For the MPU, 
this would not work. Can we attempt to not emulate the instructions?
> ---
> v7:
> - No change.
> v6:
> - Also move relinquish_p2m_mapping() to mmu/p2m.c, make
>    __p2m_set_entry() static.
> - Also move p2m_clear_root_pages() and p2m_flush_vm() to mmu/p2m.c.
> - Don't add #ifdef CONFIG_MMU to the p2m_tlb_flush_sync() in
>    p2m_write_unlock(), this need further discussion.
> - Correct typo in commit message.
> v5:
> - No change
> v4:
> - Rework the patch to drop the unnecessary changes.
> - Rework the commit msg a bit.
> v3:
> - remove MPU stubs
> - adapt to the introduction of new directories: mmu/
> v2:
> - new commit
> ---
>   xen/arch/arm/include/asm/mmu/p2m.h |   18 +
>   xen/arch/arm/include/asm/p2m.h     |   26 +-
>   xen/arch/arm/mmu/Makefile          |    1 +
>   xen/arch/arm/mmu/p2m.c             | 1736 ++++++++++++++++++++++++++
>   xen/arch/arm/p2m.c                 | 1837 +---------------------------
>   5 files changed, 1832 insertions(+), 1786 deletions(-)
>   create mode 100644 xen/arch/arm/include/asm/mmu/p2m.h
>   create mode 100644 xen/arch/arm/mmu/p2m.c
>
> diff --git a/xen/arch/arm/include/asm/mmu/p2m.h 
> b/xen/arch/arm/include/asm/mmu/p2m.h
> new file mode 100644
> index 0000000000..f829e325ce
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/mmu/p2m.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +#ifndef __ARM_MMU_P2M_H__
> +#define __ARM_MMU_P2M_H__
> +
> +struct p2m_domain;
> +void p2m_force_tlb_flush_sync(struct p2m_domain *p2m);
> +void p2m_tlb_flush_sync(struct p2m_domain *p2m);
> +
> +#endif /* __ARM_MMU_P2M_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
> index 940495d42b..a9622dac9a 100644
> --- a/xen/arch/arm/include/asm/p2m.h
> +++ b/xen/arch/arm/include/asm/p2m.h
> @@ -19,6 +19,22 @@ extern unsigned int p2m_root_level;
>   #define P2M_ROOT_ORDER    p2m_root_order
You seem to use P2M_ROOT_ORDER to allocate p2m->root in arm/p2m.c. 
However, as I mentioned before, I don't think the defintion of p2m->root 
is suitable for the MPU. I think the two functions using p2m->root 
should be moved in mmu/p2m.c and P2M_ROOT_ORDER should be moved in 
mmu/p2m.h.
>   #define P2M_ROOT_LEVEL p2m_root_level
P2M_ROOT_LEVEL doesn't seem to make sense for the MPU. The only use in 
arch/arm/p2m.c seems to be in dump_p2m_lookup() which is calling an MMU 
specific function. So I think this wants to be moved in the MMU code.
>   
> +#define MAX_VMID_8_BIT  (1UL << 8)
> +#define MAX_VMID_16_BIT (1UL << 16)
> +
> +#define INVALID_VMID 0 /* VMID 0 is reserved */
> +
> +#ifdef CONFIG_ARM_64
> +extern unsigned int max_vmid;
> +/* VMID is by default 8 bit width on AArch64 */
> +#define MAX_VMID       max_vmid
> +#else
> +/* VMID is always 8 bit width on AArch32 */
> +#define MAX_VMID        MAX_VMID_8_BIT
> +#endif
> +
> +#define P2M_ROOT_PAGES    (1<<P2M_ROOT_ORDER)
See above.

Also NIT, I would suggest to take the opportunity to use 1U and add 
space before/after <<.
> +
>   struct domain;
>   
>   extern void memory_type_changed(struct domain *);
> @@ -156,6 +172,10 @@ typedef enum {
>   #endif
>   #include <xen/p2m-common.h>
>   
> +#ifdef CONFIG_MMU
> +#include <asm/mmu/p2m.h>
> +#endif
> +
>   static inline bool arch_acquire_resource_check(struct domain *d)
>   {
>       /*
> @@ -180,7 +200,11 @@ void p2m_altp2m_check(struct vcpu *v, uint16_t idx)
>    */
>   void p2m_restrict_ipa_bits(unsigned int ipa_bits);
>   
> +void p2m_vmid_allocator_init(void);
> +int p2m_alloc_vmid(struct domain *d);
> +
>   /* Second stage paging setup, to be called on all CPUs */
> +void setup_virt_paging_one(void *data);
I don't much like the idea to export setup_virt_paging_one(). Could we 
instead move cpu_virt_paging_callback() & co to mmu/p2m.c?
Cheers,

--
Julien Grall