Re: [PATCH v4 3/7] arm/mpu: Provide and populate MPU C data structures

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

On 29/04/2025 17:20, Luca Fancellu wrote:
> Provide some data structure in the C world to track the MPU
> status, these structures will be filled at boot by the assembly
> early code with the boot MPU regions and afterwards they will be
> used at runtime.
> 
> Provide methods to update a bitmap created with DECLARE_BITMAP
> from the assembly code for both Arm32 and Arm64.
> 
> Modify Arm64 assembly boot code to reset any unused MPU region,
> initialise 'max_xen_mpumap' with the number of supported MPU
IMO this is not a good name because there's nothing there suggesting that this
variable stores the number. Maybe max_mpu_regions or max_xen_mpumap_regions.

> regions and modify the common asm macro 'prepare_xen_region' to
> load into xen_mpumap the MPU status and set/clear the bitmap
> 'xen_mpumap_mask' used to track the enabled regions.
> 
> Changed parameters name from 'base', 'limit' to 'tmp1' and 'tmp2'
> in order to use them also for intermediate operations on the
> MPU and the bitmap C data structures, the help on top of the macro
> is enough to understand how the macro will work and this will
> save some registers.
> 
> Provide a stub implementation for the pr_t type and few asm
> macro for the Arm32 to prevent compilation break, they will
> be implemented later.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> v4 changes:
>  - new patch
> ---
>  xen/arch/arm/arm64/mpu/head.S            | 13 +++++
>  xen/arch/arm/include/asm/arm32/mpu.h     | 25 +++++++++
>  xen/arch/arm/include/asm/bitmap-op.inc   | 67 ++++++++++++++++++++++
>  xen/arch/arm/include/asm/mpu.h           |  5 ++
>  xen/arch/arm/include/asm/mpu/mm.h        |  7 +++
>  xen/arch/arm/include/asm/mpu/regions.inc | 71 ++++++++++++++++++++----
>  xen/arch/arm/mpu/mm.c                    | 16 ++++++
>  7 files changed, 194 insertions(+), 10 deletions(-)
>  create mode 100644 xen/arch/arm/include/asm/arm32/mpu.h
>  create mode 100644 xen/arch/arm/include/asm/bitmap-op.inc
> 
> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
> index 6d336cafbbaf..c0cac06b015f 100644
> --- a/xen/arch/arm/arm64/mpu/head.S
> +++ b/xen/arch/arm/arm64/mpu/head.S
> @@ -40,6 +40,9 @@ FUNC(enable_boot_cpu_mm)
>      mrs   x5, MPUIR_EL2
>      and   x5, x5, #NUM_MPU_REGIONS_MASK
>  
> +    ldr   x0, =max_xen_mpumap
> +    strb  w5, [x0]
> +
>      /* x0: region sel */
>      mov   x0, xzr
>      /* Xen text section. */
> @@ -74,6 +77,16 @@ FUNC(enable_boot_cpu_mm)
>      prepare_xen_region x0, x1, x2, x3, x4, x5, 
> attr_prbar=REGION_DEVICE_PRBAR, attr_prlar=REGION_DEVICE_PRLAR
>  #endif
>  
> +zero_mpu:
> +    /* Reset remaining MPU regions */
> +    cmp   x0, x5
> +    beq   out_zero_mpu
> +    mov   x1, #0
> +    mov   x2, #1
Shouldn't we mark the region as emtpy (base == limit) when doing region clear?

> +    prepare_xen_region x0, x1, x2, x3, x4, x5, 
> attr_prlar=REGION_DISABLED_PRLAR
> +    b     zero_mpu
> +
> +out_zero_mpu:
>      b    enable_mpu
>      ret
>  END(enable_boot_cpu_mm)
> diff --git a/xen/arch/arm/include/asm/arm32/mpu.h 
> b/xen/arch/arm/include/asm/arm32/mpu.h
> new file mode 100644
> index 000000000000..1bdae4c309dc
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/arm32/mpu.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ARM_ARM32_MPU_H__
> +#define __ARM_ARM32_MPU_H__
> +
> +#ifndef __ASSEMBLY__
> +
> +/* MPU Protection Region */
> +typedef struct {
> +    uint32_t prbar;
> +    uint32_t prlar;
> +} pr_t;
> +
> +#endif /* __ASSEMBLY__ */
> +
> +#endif /* __ARM_ARM32_MPU_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/bitmap-op.inc 
> b/xen/arch/arm/include/asm/bitmap-op.inc
> new file mode 100644
> index 000000000000..e316d9417bb9
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/bitmap-op.inc
I'll just skim through this file for now. I won't check for algorithm being 
correct.

> @@ -0,0 +1,67 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +/*
> + * Sets a bit in a bitmap declared by DECLARE_BITMAP, symbol name passed 
> through
> + * bitmap_symbol.
> + *
> + * bitmap_set_bit:      symbol of the bitmap declared by DECLARE_BITMAP
> + * bit:                 bit number to be set in the bitmap
> + * tmp1-tmp4:           temporary registers used for the computation
> + *
> + * Preserves bit.
Here you say it is preserved, yet...

> + * Output:
> + *  tmp1: Address of the word containing the changed bit.
> + * Clobbers: bit, tmp1, tmp2, tmp3, tmp4.
... here you list is as clobbered.

> + */
> +.macro bitmap_set_bit bitmap_symbol, bit, tmp1, tmp2, tmp3, tmp4
> +    adr_l   \tmp1, \bitmap_symbol
> +    mov     \tmp2, #(BYTES_PER_LONG - 1)
> +    mvn     \tmp2, \tmp2
> +    lsr     \tmp3, \bit, #3
> +    and     \tmp2, \tmp3, \tmp2
> +    add     \tmp1, \tmp1, \tmp2                 // bitmap_symbol + 
> (bit/BITS_PER_LONG)*BYTES_PER_LONG
We don't use // style comments. Please use /* */

> +    and     \tmp2, \bit, #(BITS_PER_LONG - 1)   // bit offset inside word
> +
> +    ldr     \tmp3, [\tmp1]
> +    mov     \tmp4, #1
> +    lsl     \tmp4, \tmp4, \tmp2                 // (1 << offset)
> +    orr     \tmp3, \tmp3, \tmp4                 // set the bit
> +    str     \tmp3, [\tmp1]
> +.endm
> +
> +/*
> + * Clears a bit in a bitmap declared by DECLARE_BITMAP, symbol name passed
> + * through bitmap_symbol.
> + *
> + * bitmap_set_bit:      symbol of the bitmap declared by DECLARE_BITMAP
> + * bit:                 bit number to be set in the bitmap
> + * tmp1-tmp4:           temporary registers used for the computation
> + *
> + * Preserves bit.
> + * Output:
> + *  tmp1: Address of the word containing the changed bit.
> + * Clobbers: bit, tmp1, tmp2, tmp3, tmp4.
> + */
> +.macro bitmap_clear_bit bitmap_symbol, bit, tmp1, tmp2, tmp3, tmp4
> +    adr_l   \tmp1, \bitmap_symbol
> +    mov     \tmp2, #(BYTES_PER_LONG - 1)
> +    mvn     \tmp2, \tmp2
> +    lsr     \tmp3, \bit, #3
> +    and     \tmp2, \tmp3, \tmp2
> +    add     \tmp1, \tmp1, \tmp2                 // bitmap_symbol + 
> (bit/BITS_PER_LONG)*BYTES_PER_LONG
> +    and     \tmp2, \bit, #(BITS_PER_LONG - 1)   // bit offset inside word
> +
> +    ldr     \tmp3, [\tmp1]
> +    mov     \tmp4, #1
> +    lsl     \tmp4, \tmp4, \tmp2                 // (1 << offset)
> +    mvn     \tmp4, \tmp4                        // ~(1 << offset)
> +    and     \tmp3, \tmp3, \tmp4                 // clear the bit
> +    str     \tmp3, [\tmp1]
> +.endm
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
> index bb83f5a5f580..1368b2eb990f 100644
> --- a/xen/arch/arm/include/asm/mpu.h
> +++ b/xen/arch/arm/include/asm/mpu.h
> @@ -8,6 +8,10 @@
>  
>  #if defined(CONFIG_ARM_64)
>  # include <asm/arm64/mpu.h>
> +#elif defined(CONFIG_ARM_32)
> +# include <asm/arm32/mpu.h>
> +#else
> +# error "unknown ARM variant"
>  #endif
>  
>  #define MPU_REGION_SHIFT  6
> @@ -17,6 +21,7 @@
>  #define NUM_MPU_REGIONS_SHIFT   8
>  #define NUM_MPU_REGIONS         (_AC(1, UL) << NUM_MPU_REGIONS_SHIFT)
>  #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
> +#define MAX_MPU_REGION_NR       255
Shouldn't you define it using NUM_MPU_REGIONS? It should have the same
definition as mask.

>  
>  #endif /* __ARM_MPU_H__ */
>  
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h 
> b/xen/arch/arm/include/asm/mpu/mm.h
> index bfd840fa5d31..28339259c458 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -8,9 +8,16 @@
>  #include <xen/page-size.h>
>  #include <xen/types.h>
>  #include <asm/mm.h>
> +#include <asm/mpu.h>
>  
>  extern struct page_info *frame_table;
>  
> +extern uint8_t max_xen_mpumap;
> +
> +extern DECLARE_BITMAP(xen_mpumap_mask, MAX_MPU_REGION_NR);
> +
> +extern pr_t xen_mpumap[MAX_MPU_REGION_NR];
> +
>  #define virt_to_maddr(va) ((paddr_t)((vaddr_t)(va) & PADDR_MASK))
>  
>  #ifdef CONFIG_ARM_32
> diff --git a/xen/arch/arm/include/asm/mpu/regions.inc 
> b/xen/arch/arm/include/asm/mpu/regions.inc
> index 47868a152662..dc0306f8c5fc 100644
> --- a/xen/arch/arm/include/asm/mpu/regions.inc
> +++ b/xen/arch/arm/include/asm/mpu/regions.inc
> @@ -1,22 +1,50 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  
> +#include <asm/bitmap-op.inc>
>  #include <asm/mpu.h>
>  #include <asm/sysregs.h>
>  
>  /* Backgroud region enable/disable */
>  #define SCTLR_ELx_BR    BIT(17, UL)
>  
> +#define REGION_DISABLED_PRLAR   0x00    /* NS=0 ATTR=000 EN=0 */
>  #define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 EN=1 */
>  #define REGION_DEVICE_PRLAR     0x09    /* NS=0 ATTR=100 EN=1 */
>  
> +#define PRLAR_ELx_EN            0x1
> +
> +#ifdef CONFIG_ARM_64
> +#define XEN_MPUMAP_ENTRY_SHIFT  0x4     /* 16 byte structure */
> +
> +.macro store_pair reg1, reg2, dst
> +        stp \reg1, \reg2, [\dst]
Why 8 instead of 4 spaces?

> +.endm
> +
> +.macro invalidate_dcache_one reg
> +        dc ivac, \reg
> +.endm
> +
> +#else
> +#define XEN_MPUMAP_ENTRY_SHIFT  0x2     /* 8 byte structure */
> +
> +.macro store_pair reg1, reg2, dst
> +        nop
> +.endm
> +
> +.macro invalidate_dcache_one reg
> +        nop
> +.endm
> +
> +#endif
> +
>  /*
>   * Macro to prepare and set a EL2 MPU memory region.
>   * We will also create an according MPU memory region entry, which
>   * is a structure of pr_t,  in table \prmap.
>   *
>   * sel:         region selector
> - * base:        reg storing base address
> - * limit:       reg storing limit address
> + * tmp1:        reg storing base address
> + * tmp2:        reg storing limit address
I think this change is not needed. The parameters should be named base and limit
because this is what you expect caller to pass. Inside the function, you can do
whatever you want with these registers and caller does not care as long as you
mention if they are clobbered or not. Same in C world. You can reuse the
parameter for a different internal purpose inside a function.

>   * prbar:       store computed PRBAR_EL2 value
>   * prlar:       store computed PRLAR_EL2 value
>   * maxcount:    maximum number of EL2 regions supported
> @@ -28,13 +56,13 @@
>   * Preserves maxcount
>   * Output:
>   *  sel: Next available region selector index.
> - * Clobbers base, limit, prbar, prlar
> + * Clobbers tmp1, tmp2, prbar, prlar
>   *
>   * Note that all parameters using registers should be distinct.
>   */
> -.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, 
> attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR
> +.macro prepare_xen_region, sel, tmp1, tmp2, prbar, prlar, maxcount, 
> attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR
>      /* Check if the region is empty */
> -    cmp   \base, \limit
> +    cmp   \tmp1, \tmp2
>      beq   1f
>  
>      /* Check if the number of regions exceeded the count specified in 
> MPUIR_EL2 */
> @@ -42,20 +70,43 @@
>      bge   fail_insufficient_regions
>  
>      /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/
> -    and   \base, \base, #MPU_REGION_MASK
> +    and   \tmp1, \tmp1, #MPU_REGION_MASK
>      mov   \prbar, #\attr_prbar
> -    orr   \prbar, \prbar, \base
> +    orr   \prbar, \prbar, \tmp1
>  
>      /* Limit address should be inclusive */
> -    sub   \limit, \limit, #1
> -    and   \limit, \limit, #MPU_REGION_MASK
> +    sub   \tmp2, \tmp2, #1
> +    and   \tmp2, \tmp2, #MPU_REGION_MASK
>      mov   \prlar, #\attr_prlar
> -    orr   \prlar, \prlar, \limit
> +    orr   \prlar, \prlar, \tmp2
>  
>      WRITE_SYSREG_ASM(\sel, PRSELR_EL2)
>      isb
>      WRITE_SYSREG_ASM(\prbar, PRBAR_EL2)
>      WRITE_SYSREG_ASM(\prlar, PRLAR_EL2)
> +
> +    /* Load pair into xen_mpumap and invalidate cache */
> +    mov   \tmp1, \sel
> +    lsl   \tmp1, \tmp1, #XEN_MPUMAP_ENTRY_SHIFT
You could get rid of these 2 extra instructions and instead do:

> +    adr_l \tmp2, xen_mpumap
> +    add   \tmp2, \tmp2, \tmp1
add \tmp2, \tmp2, \sel, lsl #XEN_MPUMAP_ENTRY_SHIFT
which combines everything in one go.

> +    store_pair \prbar, \prlar, \tmp2
> +    invalidate_dcache_one \tmp2
> +
> +    /* Set/clear xen_mpumap_mask bitmap */
> +    tst   \prlar, #PRLAR_ELx_EN
> +    bne   2f
> +    // Region is disabled, clear the bit in the bitmap
Comment style, here and elsewhere

> +    bitmap_clear_bit xen_mpumap_mask, \sel, \tmp1, \tmp2, \prbar, \prlar
> +    b     3f
> +
> +2:
> +    // Region is enabled, set the bit in the bitmap
> +    bitmap_set_bit xen_mpumap_mask, \sel, \tmp1, \tmp2, \prbar, \prlar
Wouldn't it be better to first clear the entire bitmap before setting up the
regions (i.e. all regions disabled) and then only have the set part here?

> +
> +3:
> +    invalidate_dcache_one \tmp1
> +
>      dsb   sy
>      isb
>  
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 07c8959f4ee9..9eab09ff2044 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -7,9 +7,25 @@
>  #include <xen/mm.h>
>  #include <xen/sizes.h>
>  #include <xen/types.h>
> +#include <asm/mpu.h>
>  
>  struct page_info *frame_table;
>  
> +/* Maximum number of supported MPU memory regions by the EL2 MPU. */
> +uint8_t __ro_after_init max_xen_mpumap;
> +
> +/*
> + * Bitmap xen_mpumap_mask is to record the usage of EL2 MPU memory regions.
> + * Bit 0 represents MPU memory region 0, bit 1 represents MPU memory
> + * region 1, ..., and so on.
> + * If a MPU memory region gets enabled, set the according bit to 1.
> + */
> +DECLARE_BITMAP(xen_mpumap_mask, MAX_MPU_REGION_NR) \
> +    __section(".data.page_aligned");
> +
> +/* EL2 Xen MPU memory region mapping table. */
> +pr_t __section(".data.page_aligned") xen_mpumap[MAX_MPU_REGION_NR];
> +
>  static void __init __maybe_unused build_assertions(void)
>  {
>      /*

~Michal