Re: [PATCH 10/10] arm/mpu: Create the skeleton for MPU compilation

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 12/03/2025 14:52, Luca Fancellu wrote:
> 
> 
> This commit introduces the skeleton for the MPU memory management
> subsystem that allows the compilation.
You forgot to mention that you're talking about Arm64 only. For Arm32, Ayan has
a series containing a few patches to enable compilation.

P.S.
Once your series are merged, I'll send a patch enabling CI compilation for both
Arm64 and Arm32 MPU.

> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> The implementation of setup_virt_paging() is under arm64/mpu
> because it will have a different implementation from arm32.
> ---
> ---
>  xen/arch/arm/arm64/mpu/Makefile    |  2 ++
>  xen/arch/arm/arm64/mpu/p2m.c       | 18 ++++++++++
>  xen/arch/arm/arm64/mpu/smpboot.c   | 23 ++++++++++++
>  xen/arch/arm/include/asm/mpu/p2m.h | 20 +++++++++++
>  xen/arch/arm/include/asm/p2m.h     |  2 +-
>  xen/arch/arm/mpu/Makefile          |  1 +
>  xen/arch/arm/mpu/mm.c              | 22 ++++++++++++
>  xen/arch/arm/mpu/p2m.c             | 58 ++++++++++++++++++++++++++++++
>  xen/arch/arm/mpu/setup.c           | 24 +++++++++++++
>  xen/arch/arm/mpu/vmap-mpu.c        | 24 +++++++++++++
>  10 files changed, 193 insertions(+), 1 deletion(-)
>  create mode 100644 xen/arch/arm/arm64/mpu/p2m.c
>  create mode 100644 xen/arch/arm/arm64/mpu/smpboot.c
>  create mode 100644 xen/arch/arm/include/asm/mpu/p2m.h
>  create mode 100644 xen/arch/arm/mpu/vmap-mpu.c
> 
> diff --git a/xen/arch/arm/arm64/mpu/Makefile b/xen/arch/arm/arm64/mpu/Makefile
> index 3340058c08c6..cf0540aecc84 100644
> --- a/xen/arch/arm/arm64/mpu/Makefile
> +++ b/xen/arch/arm/arm64/mpu/Makefile
> @@ -1 +1,3 @@
>  obj-y += head.o
> +obj-y += p2m.o
> +obj-y += smpboot.o
> diff --git a/xen/arch/arm/arm64/mpu/p2m.c b/xen/arch/arm/arm64/mpu/p2m.c
> new file mode 100644
> index 000000000000..df8de5c7d823
> --- /dev/null
> +++ b/xen/arch/arm/arm64/mpu/p2m.c
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/init.h>
> +#include <asm/p2m.h>
Why do we need to include p2m.h at this stage?

> +
> +void __init setup_virt_paging(void)
> +{
> +    BUG_ON("unimplemented");
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/arm64/mpu/smpboot.c 
> b/xen/arch/arm/arm64/mpu/smpboot.c
> new file mode 100644
> index 000000000000..3f3e54294e54
> --- /dev/null
> +++ b/xen/arch/arm/arm64/mpu/smpboot.c
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/mm.h>
I can't see anything mm.h related here. I'd expect types.h/stdbool.h and bug.h

> +
> +int prepare_secondary_mm(int cpu)
> +{
> +    BUG_ON("unimplemented");
> +    return -EINVAL;
> +}
> +
> +void update_boot_mapping(bool enable)
> +{
> +    BUG_ON("unimplemented");
> +}
> +
> +/*
> + * 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/mpu/p2m.h 
> b/xen/arch/arm/include/asm/mpu/p2m.h
> new file mode 100644
> index 000000000000..e5c0e302167c
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/mpu/p2m.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
Empty line here please

> +#ifndef __ARM_MPU_P2M_H__
> +#define __ARM_MPU_P2M_H__
> +
> +/* Not used on MPU system */
I personally don't see a value in such comments. Initially when there were just
1, 2 instances I was ok, but then I realized you keep adding them. It's pretty
clear that if a stub does not have BUG_ON, it means it's not used. I worry the
files will look awful with so many "Not used on MPU system" comments. If at all,
the only place they would make sense is if they were in MMU/MPU common code. Can
we get rid of them completely?

> +static inline void p2m_clear_root_pages(struct p2m_domain *p2m) {}
You should at least forward declare struct p2m_domain.

> +
> +/* Not used on MPU system */
> +static inline void p2m_tlb_flush_sync(struct p2m_domain *p2m) {}
> +
> +#endif /* __ARM_MPU_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 594dc4004138..2d53bf9b6177 100644
> --- a/xen/arch/arm/include/asm/p2m.h
> +++ b/xen/arch/arm/include/asm/p2m.h
> @@ -168,7 +168,7 @@ typedef enum {
>  #if defined(CONFIG_MMU)
>  # include <asm/mmu/p2m.h>
>  #else
> -# error "Unknown memory management layout"
> +# include <asm/mpu/p2m.h>
>  #endif
> 
>  static inline bool arch_acquire_resource_check(struct domain *d)
> diff --git a/xen/arch/arm/mpu/Makefile b/xen/arch/arm/mpu/Makefile
> index f1417cd1b9db..a963b35db88d 100644
> --- a/xen/arch/arm/mpu/Makefile
> +++ b/xen/arch/arm/mpu/Makefile
> @@ -1,3 +1,4 @@
>  obj-y += mm.o
>  obj-y += p2m.o
>  obj-y += setup.init.o
> +obj-y += vmap-mpu.o
What's the point of adding -mpu suffix to the file located already under /mpu?

> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index b196661a2312..12703c4f3d4f 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -18,6 +18,28 @@ static void __init __maybe_unused build_assertions(void)
>      BUILD_BUG_ON(PAGE_SIZE != SZ_4K);
>  }
> 
> +void __init setup_mm(void)
> +{
> +    BUG_ON("unimplemented");
> +}
> +
> +int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int nf)
> +{
> +    BUG_ON("unimplemented");
> +    return -EINVAL;
> +}
> +
> +void dump_hyp_walk(vaddr_t addr)
> +{
> +    BUG_ON("unimplemented");
> +}
> +
> +/* Release all __init and __initdata ranges to be reused */
> +void free_init_memory(void)
> +{
> +    BUG_ON("unimplemented");
> +}
> +
>  void __iomem *ioremap_attr(paddr_t start, size_t len, unsigned int flags)
>  {
>      BUG_ON("unimplemented");
> diff --git a/xen/arch/arm/mpu/p2m.c b/xen/arch/arm/mpu/p2m.c
> index 7525d9b00bcb..b059c86bde43 100644
> --- a/xen/arch/arm/mpu/p2m.c
> +++ b/xen/arch/arm/mpu/p2m.c
> @@ -4,6 +4,64 @@
>  #include <xen/domain.h>
>  #include <asm/p2m.h>
> 
> +int p2m_set_entry(struct p2m_domain *p2m, gfn_t sgfn, unsigned long nr,
> +                  mfn_t smfn, p2m_type_t t, p2m_access_t a)
> +{
> +    BUG_ON("unimplemented");
> +    return -EINVAL;
> +}
> +
> +mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn, p2m_type_t *t,
> +                    p2m_access_t *a, unsigned int *page_order, bool *valid)
> +{
> +    BUG_ON("unimplemented");
> +    return INVALID_MFN;
> +}
> +
> +void p2m_dump_info(struct domain *d)
> +{
> +    BUG_ON("unimplemented");
> +}
> +
> +int p2m_init(struct domain *d)
> +{
> +    BUG_ON("unimplemented");
> +    return -EINVAL;
> +}
> +
> +void p2m_save_state(struct vcpu *p)
> +{
> +    BUG_ON("unimplemented");
> +}
> +
> +void p2m_restore_state(struct vcpu *n)
> +{
> +    BUG_ON("unimplemented");
> +}
> +
> +void p2m_final_teardown(struct domain *d)
> +{
> +    BUG_ON("unimplemented");
> +}
> +
> +/* Not used on MPU system */
> +void p2m_flush_vm(struct vcpu *v) {}
> +
> +bool p2m_resolve_translation_fault(struct domain *d, gfn_t gfn)
> +{
> +    BUG_ON("unimplemented");
> +    return false;
> +}
> +
> +/* Not used on MPU system */
> +int relinquish_p2m_mapping(struct domain *d)
> +{
> +    return 0;
> +}
> +
> +/* Not used on MPU system */
> +void p2m_domain_creation_finished(struct domain *d) {}
> +
>  /* Not used on MPU system */
>  int p2m_teardown(struct domain *d)
>  {
> diff --git a/xen/arch/arm/mpu/setup.c b/xen/arch/arm/mpu/setup.c
> index 45214bfeb661..28119e008341 100644
> --- a/xen/arch/arm/mpu/setup.c
> +++ b/xen/arch/arm/mpu/setup.c
> @@ -2,12 +2,36 @@
> 
>  #include <xen/bug.h>
>  #include <xen/init.h>
> +#include <xen/types.h>
> +#include <asm/setup.h>
Please sort alphabetically

> 
>  void __init setup_mappings(void)
>  {
>      BUG_ON("unimplemented");
>  }
> 
> +void * __init early_fdt_map(paddr_t fdt_paddr)
> +{
> +    BUG_ON("unimplemented");
> +    return NULL;
> +}
> +
> +/*
> + * copy_from_paddr - copy data from a physical address
> + * @dst: destination virtual address
> + * @paddr: source physical address
> + * @len: length to copy
> + */
> +void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
> +{
> +    BUG_ON("unimplemented");
> +}
> +
> +void __init remove_early_mappings(void)
> +{
> +    BUG_ON("unimplemented");
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/mpu/vmap-mpu.c b/xen/arch/arm/mpu/vmap-mpu.c
> new file mode 100644
> index 000000000000..cb92407476de
> --- /dev/null
> +++ b/xen/arch/arm/mpu/vmap-mpu.c
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#include <xen/mm.h>
> +#include <xen/vmap.h>
I don't see any use of vmap.h in this file

> +
> +void *vmap_contig(mfn_t mfn, unsigned int nr)
> +{
> +    BUG_ON("unimplemented");
> +    return NULL;
> +}
> +
> +void vunmap(const void *va)
> +{
> +    BUG_ON("unimplemented");
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> --
> 2.34.1
> 

~Michal