Re: [PATCH v4 3/3] xen/arm32: mpu: Stubs to build MPU for arm32

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 04/04/2025 10:11, Luca Fancellu wrote:
> Hi Ayan,
Hi Luca,
> > On 3 Apr 2025, at 18:12, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx> wrote:
> >
> > Add stubs to enable compilation
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> >
> > Changes from :-
> >
> > v1, v2 -
> > 1. New patch introduced in v3.
> > 2. Should be applied on top of
> > https://patchwork.kernel.org/project/xen-devel/cover/20250316192445.2376484-1-luca.fancellu@xxxxxxx/
> >
> > v3 -
> > 1. Add stubs for map_domain_page() and similar functions.
> >
> > 2. 'BUG_ON("unimplemented")' is kept in all the stubs.
> >
> > xen/arch/arm/arm32/mpu/Makefile  |  2 ++
> > xen/arch/arm/arm32/mpu/p2m.c     | 18 ++++++++++++++
> > xen/arch/arm/arm32/mpu/smpboot.c | 23 ++++++++++++++++++
> > xen/arch/arm/include/asm/mm.h    |  5 ++++
> > xen/arch/arm/mpu/Makefile        |  1 +
> > xen/arch/arm/mpu/domain_page.c   | 40 ++++++++++++++++++++++++++++++++
> > 6 files changed, 89 insertions(+)
> > create mode 100644 xen/arch/arm/arm32/mpu/p2m.c
> > create mode 100644 xen/arch/arm/arm32/mpu/smpboot.c
> > create mode 100644 xen/arch/arm/mpu/domain_page.c
> >
> >
> > diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> > index fbffaccef4..2a52cf530f 100644
> > --- a/xen/arch/arm/include/asm/mm.h
> > +++ b/xen/arch/arm/include/asm/mm.h
> > @@ -171,12 +171,17 @@ struct page_info
> > #define PGC_need_scrub    PGC_allocated
> >
> > #ifdef CONFIG_ARM_32
> > +#ifdef CONFIG_MPU
> > +#define is_xen_heap_page(page) ({ BUG_ON("unimplemented"); false; })
> > +#define is_xen_heap_mfn(mfn) ({ BUG_ON("unimplemented"); false; })
> > +#else /* !CONFIG_MPU */
> > #define is_xen_heap_page(page) is_xen_heap_mfn(page_to_mfn(page))
> > #define is_xen_heap_mfn(mfn) ({                                 \
> >      unsigned long mfn_ = mfn_x(mfn);                            \
> >      (mfn_ >= mfn_x(directmap_mfn_start) &&                      \
> >       mfn_ < mfn_x(directmap_mfn_end));                          \
> > })
> > +#endif /* !CONFIG_MPU */
> > #else
> > #define is_xen_heap_page(page) ((page)->count_info & PGC_xen_heap)
> > #define is_xen_heap_mfn(mfn) \
> I’ve had a look on this, if we want to get rid of some #ifdef we could do this:
>
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index 2a52cf530f25..5b67c0f8bb1c 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -170,19 +170,7 @@ struct page_info
>   #define _PGC_need_scrub   _PGC_allocated
>   #define PGC_need_scrub    PGC_allocated
>   
> -#ifdef CONFIG_ARM_32
> -#ifdef CONFIG_MPU
> -#define is_xen_heap_page(page) ({ BUG_ON("unimplemented"); false; })
> -#define is_xen_heap_mfn(mfn) ({ BUG_ON("unimplemented"); false; })
> -#else /* !CONFIG_MPU */
> -#define is_xen_heap_page(page) is_xen_heap_mfn(page_to_mfn(page))
> -#define is_xen_heap_mfn(mfn) ({                                 \
> -    unsigned long mfn_ = mfn_x(mfn);                            \
> -    (mfn_ >= mfn_x(directmap_mfn_start) &&                      \
> -     mfn_ < mfn_x(directmap_mfn_end));                          \
> -})
> -#endif /* !CONFIG_MPU */
> -#else
> +#ifdef CONFIG_ARM_64
>   #define is_xen_heap_page(page) ((page)->count_info & PGC_xen_heap)
>   #define is_xen_heap_mfn(mfn) \
>       (mfn_valid(mfn) && is_xen_heap_page(mfn_to_page(mfn)))
> diff --git a/xen/arch/arm/include/asm/mmu/mm.h 
> b/xen/arch/arm/include/asm/mmu/mm.h
> index caba987edc85..9b98d12b076b 100644
> --- a/xen/arch/arm/include/asm/mmu/mm.h
> +++ b/xen/arch/arm/include/asm/mmu/mm.h
> @@ -21,6 +21,15 @@ extern unsigned long directmap_base_pdx;
>   
>   #define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)
>   
> +#ifdef CONFIG_ARM_32
> +#define is_xen_heap_page(page) is_xen_heap_mfn(page_to_mfn(page))
> +#define is_xen_heap_mfn(mfn) ({                                 \
> +    unsigned long mfn_ = mfn_x(mfn);                            \
> +    (mfn_ >= mfn_x(directmap_mfn_start) &&                      \
> +     mfn_ < mfn_x(directmap_mfn_end));                          \
> +})
> +#endif
> +
>   #define virt_to_maddr(va) ({                                                  
>  \
>       vaddr_t va_ = (vaddr_t)(va);                                              
>  \
>       (paddr_t)((va_to_par(va_) & PADDR_MASK & PAGE_MASK) | (va_ & 
> ~PAGE_MASK)); \
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h 
> b/xen/arch/arm/include/asm/mpu/mm.h
> index 86f33d9836b7..bfd840fa5d31 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -13,6 +13,11 @@ extern struct page_info *frame_table;
>   
>   #define virt_to_maddr(va) ((paddr_t)((vaddr_t)(va) & PADDR_MASK))
>   
> +#ifdef CONFIG_ARM_32
> +#define is_xen_heap_page(page) ({ BUG_ON("unimplemented"); false; })
> +#define is_xen_heap_mfn(mfn) ({ BUG_ON("unimplemented"); false; })
> +#endif
> +
>   /* On MPU systems there is no translation, ma == va. */
>   static inline void *maddr_to_virt(paddr_t ma)
>   {
>
> But feel free to ignore if it’s not desirable.
Actually, makes sense. I have included this in my v5.
> > diff --git a/xen/arch/arm/mpu/Makefile b/xen/arch/arm/mpu/Makefile
> > index 21bbc517b5..ff221011d5 100644
> > --- a/xen/arch/arm/mpu/Makefile
> > +++ b/xen/arch/arm/mpu/Makefile
> > @@ -2,3 +2,4 @@ obj-y += mm.o
> > obj-y += p2m.o
> > obj-y += setup.init.o
> > obj-y += vmap.o
> > +obj-$(CONFIG_ARM_32) += domain_page.o
> > diff --git a/xen/arch/arm/mpu/domain_page.c b/xen/arch/arm/mpu/domain_page.c
> > new file mode 100644
> > index 0000000000..b9ebb03d67
> > --- /dev/null
> > +++ b/xen/arch/arm/mpu/domain_page.c
> > @@ -0,0 +1,40 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +#include <xen/domain_page.h>
> > +
> > +void *map_domain_page_global(mfn_t mfn)
> > +{
> > +    BUG_ON("unimplemented");
> > +    return (void*)0;
> return NULL? here and below
>
> > +}
> > +
> > +/* Map a page of domheap memory */
> > +void *map_domain_page(mfn_t mfn)
> > +{
> > +    BUG_ON("unimplemented");
> > +    return (void*)0;
> > +}
> > +
> > +/* Release a mapping taken with map_domain_page() */
> > +void unmap_domain_page(const void *ptr)
> > +{
> > +    BUG_ON("unimplemented");
> > +}
> > +
> > +mfn_t domain_page_map_to_mfn(const void *ptr)
> > +{
> > +    BUG_ON("unimplemented");
> Not really necessary, but if you want: return INVALID_MFN;
The function is still technically unimplemented, so I will keep that and 
add return INVALID_MFN;
The rest of your suggestions are perfect. :)

I have incorporated them in v5.

- Ayan

> > +}
> > +
> > +void unmap_domain_page_global(const void *va)
> > +{
> > +    BUG_ON("unimplemented");
> > +}
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > --
> > 2.25.1