Re: [PATCH v2 2/4] xen/arm32: mpu: Create boot-time MPU protection regions

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

On 13/03/2025 19:28, Ayan Kumar Halder wrote:
> Define enable_boot_cpu_mm() for the Armv8-R AArch32.
> 
> Like boot-time page table in MMU system, we need a boot-time MPU protection
> region configuration in MPU system so Xen can fetch code and data from normal
> memory.
> 
> To do this, Xen maps the following sections of the binary as separate regions
> (with permissions) :-
> 1. Text (Read only at EL2, execution is permitted)
> 2. RO data (Read only at EL2)
> 3. RO after init data and RW data (Read/Write at EL2)
> 4. Init Text (Read only at EL2, execution is permitted)
> 5. Init data and BSS (Read/Write at EL2)
> 
> Before creating a region, we check if the count exceeds the number defined in
> MPUIR_EL2. If so, then the boot fails.
> 
> Also we check if the region is empty or not. IOW, if the start and end address
> are same, we skip mapping the region.
> 
> One needs to set up HMAIR0 and HMAIR1 registers in enable_mpu(). The register
> configurations are the same as in enable_mmu().
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
> Changes from
> 
> v1 -
> 
> 1. enable_mpu() now sets HMAIR{0,1} registers. This is similar to what is
> being done in enable_mmu(). All the mm related configurations happen in this
> function.
> 
> 2. Fixed some typos.
> 
>  xen/arch/arm/arm32/Makefile           |   1 +
>  xen/arch/arm/arm32/mpu/Makefile       |   1 +
>  xen/arch/arm/arm32/mpu/head.S         | 170 ++++++++++++++++++++++++++
>  xen/arch/arm/include/asm/cpregs.h     |   4 +
>  xen/arch/arm/include/asm/mpu/cpregs.h |  21 ++++
>  5 files changed, 197 insertions(+)
>  create mode 100644 xen/arch/arm/arm32/mpu/Makefile
>  create mode 100644 xen/arch/arm/arm32/mpu/head.S
>  create mode 100644 xen/arch/arm/include/asm/mpu/cpregs.h
> 
> diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
> index 40a2b4803f..537969d753 100644
> --- a/xen/arch/arm/arm32/Makefile
> +++ b/xen/arch/arm/arm32/Makefile
> @@ -1,5 +1,6 @@
>  obj-y += lib/
>  obj-$(CONFIG_MMU) += mmu/
> +obj-$(CONFIG_MPU) += mpu/
>  
>  obj-$(CONFIG_EARLY_PRINTK) += debug.o
>  obj-y += domctl.o
> diff --git a/xen/arch/arm/arm32/mpu/Makefile b/xen/arch/arm/arm32/mpu/Makefile
> new file mode 100644
> index 0000000000..3340058c08
> --- /dev/null
> +++ b/xen/arch/arm/arm32/mpu/Makefile
> @@ -0,0 +1 @@
> +obj-y += head.o
> diff --git a/xen/arch/arm/arm32/mpu/head.S b/xen/arch/arm/arm32/mpu/head.S
> new file mode 100644
> index 0000000000..40648ce1a8
> --- /dev/null
> +++ b/xen/arch/arm/arm32/mpu/head.S
> @@ -0,0 +1,170 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Start-of-day code for an Armv8-R MPU system.
> + */
> +
> +#include <asm/arm32/sysregs.h>
> +#include <asm/early_printk.h>
> +#include <asm/page.h>
> +
> +/* Backgroud region enable/disable */
> +#define SCTLR_ELx_BR    BIT(17, UL)
> +
> +#define REGION_TEXT_PRBAR       0x18    /* SH=11 AP=10 XN=0 */
> +#define REGION_RO_PRBAR         0x1D    /* SH=11 AP=10 XN=1 */
> +#define REGION_DATA_PRBAR       0x19    /* SH=11 AP=00 XN=1 */
> +#define REGION_DEVICE_PRBAR     0x11    /* SH=10 AP=00 XN=1 */
> +
> +#define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 EN=1 */
> +#define REGION_DEVICE_PRLAR     0x09    /* NS=0 ATTR=100 EN=1 */
> +
> +/*
Lines from here...

> + * 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
> + * prbar:       store computed PRBAR_EL2 value
> + * prlar:       store computed PRLAR_EL2 value
> + * maxcount:    maximum number of EL2 regions supported
> + * attr_prbar:  PRBAR_EL2-related memory attributes. If not specified it 
> will be
> + *              REGION_DATA_PRBAR
> + * attr_prlar:  PRLAR_EL2-related memory attributes. If not specified it 
> will be
> + *              REGION_NORMAL_PRLAR
> + *
> + * Preserves \maxcount
> + * Output:
> + *  \sel: Next available region selector index.
> + * Clobbers \base, \limit, \prbar, \prlar
Just a question: I know it was done like that for arm64 but where does this
format of specifying registers in the comment also with leading '\' come from?
At least for me, it reads odd.

> + *
> + * 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
> +    /* Check if the region is empty */
> +    cmp   \base, \limit
> +    beq   1f
> +
> +    /* Check if the number of regions exceeded the count specified in 
> MPUIR_EL2 */
> +    cmp   \sel, \maxcount
> +    bge   fail_insufficient_regions
> +
> +    /* Prepare value for PRBAR_EL2 reg and preserve it in \prbar.*/
> +    and   \base, \base, #MPU_REGION_MASK
> +    mov   \prbar, #\attr_prbar
> +    orr   \prbar, \prbar, \base
> +
> +    /* Limit address should be inclusive */
> +    sub   \limit, \limit, #1
> +    and   \limit, \limit, #MPU_REGION_MASK
> +    mov   \prlar, #\attr_prlar
> +    orr   \prlar, \prlar, \limit
> +
... to here (40+ in total) are exactly the same for arm64 and arm32 MPU. I don't
think it's great to have this code duplication. There are/will be also other
functions that will be the same. Can we have a common MPU head.S or a header
file to avoid duplication?

> +    mcr   CP32(\sel, PRSELR_EL2)
> +    isb
> +    mcr   CP32(\prbar, PRBAR_EL2)
> +    mcr   CP32(\prlar,  PRLAR_EL2)
> +    dsb   sy
> +    isb
> +
> +    add   \sel, \sel, #1
> +
> +1:
> +.endm
> +
> +/*
> + * Failure caused due to insufficient MPU regions.
> + */
No need for multi-line comment for a single sentence.

> +FUNC_LOCAL(fail_insufficient_regions)
> +    PRINT("- Selected MPU region is above the implemented number in 
> MPUIR_EL2 -\r\n")
> +1:  wfe
> +    b   1b
> +END(fail_insufficient_regions)
> +
> +/*
> + * Enable EL2 MPU and data cache
> + * If the Background region is enabled, then the MPU uses the default memory
> + * map as the Background region for generating the memory
> + * attributes when MPU is disabled.
> + * Since the default memory map of the Armv8-R AArch32 architecture is
> + * IMPLEMENTATION DEFINED, we intend to turn off the Background region here.
> + *
> + * Clobbers x0
> + *
No need for this extra line in comment

> + */
> +FUNC_LOCAL(enable_mpu)
> +    /* Set up memory attribute type tables */
> +    mov_w r0, MAIR0VAL
> +    mov_w r1, MAIR1VAL
> +    mcr   CP32(r0, HMAIR0)
> +    mcr   CP32(r1, HMAIR1)
> +
> +    mrc   CP32(r0, HSCTLR)
> +    bic   r0, r0, #SCTLR_ELx_BR       /* Disable Background region */
> +    orr   r0, r0, #SCTLR_Axx_ELx_M    /* Enable MPU */
> +    orr   r0, r0, #SCTLR_Axx_ELx_C    /* Enable D-cache */
> +    mcr   CP32(r0, HSCTLR)
> +    isb
> +
> +    ret
> +END(enable_mpu)
> +
> +/*
> + * Maps the various sections of Xen (decsribed in xen.lds.S) as different MPU
> + * regions.
> + *
> + * Clobbers r0
> + *
No need for this extra line in comment

> + */
> +
No empty line here

> +FUNC(enable_boot_cpu_mm)
This entire function, except for reading sysreg and register prefix (x,r) is the
same between arm64 and arm32. This could also be made common.

> +    /* Get the number of regions specified in MPUIR_EL2 */
> +    mrc   CP32(r5, MPUIR_EL2)
> +    and   r5, r5, #NUM_MPU_REGIONS_MASK
> +
> +    /* x0: region sel */
> +    mov   r0, #0
> +
> +    /* Xen text section. */
> +    ldr   r1, =_stext
> +    ldr   r2, =_etext
> +    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_TEXT_PRBAR
> +
> +    /* Xen read-only data section. */
> +    ldr   r1, =_srodata
> +    ldr   r2, =_erodata
> +    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_RO_PRBAR
> +
> +    /* Xen read-only after init and data section. (RW data) */
> +    ldr   r1, =__ro_after_init_start
> +    ldr   r2, =__init_begin
> +    prepare_xen_region r0, r1, r2, r3, r4, r5
> +
> +    /* Xen code section. */
> +    ldr   r1, =__init_begin
> +    ldr   r2, =__init_data_begin
> +    prepare_xen_region r0, r1, r2, r3, r4, r5, attr_prbar=REGION_TEXT_PRBAR
> +
> +    /* Xen data and BSS section. */
> +    ldr   r1, =__init_data_begin
> +    ldr   r2, =__bss_end
> +    prepare_xen_region r0, r1, r2, r3, r4, r5
> +
> +#ifdef CONFIG_EARLY_PRINTK
> +    /* Xen early UART section. */
> +    ldr   r1, =CONFIG_EARLY_UART_BASE_ADDRESS
> +    ldr   r2, =(CONFIG_EARLY_UART_BASE_ADDRESS + CONFIG_EARLY_UART_SIZE)
> +    prepare_xen_region r0, r1, r2, r3, r4, r5, 
> attr_prbar=REGION_DEVICE_PRBAR, attr_prlar=REGION_DEVICE_PRLAR
> +#endif
> +
> +    b    enable_mpu
> +    ret
> +END(enable_boot_cpu_mm)
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/include/asm/cpregs.h 
> b/xen/arch/arm/include/asm/cpregs.h
> index aec9e8f329..6019a2cbdd 100644
> --- a/xen/arch/arm/include/asm/cpregs.h
> +++ b/xen/arch/arm/include/asm/cpregs.h
> @@ -1,6 +1,10 @@
>  #ifndef __ASM_ARM_CPREGS_H
>  #define __ASM_ARM_CPREGS_H
>  
> +#ifdef CONFIG_MPU
> +#include <asm/mpu/cpregs.h>
> +#endif
> +
>  /*
>   * AArch32 Co-processor registers.
>   *
> diff --git a/xen/arch/arm/include/asm/mpu/cpregs.h 
> b/xen/arch/arm/include/asm/mpu/cpregs.h
> new file mode 100644
> index 0000000000..bd17a8c75a
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/mpu/cpregs.h
> @@ -0,0 +1,21 @@
> +#ifndef __ASM_ARM_MPU_CPREGS_H
Missing SPDX specifier?

> +#define __ASM_ARM_MPU_CPREGS_H
> +
> +#define HMPUIR          p15,4,c0,c0,4
> +
> +/* CP15 CR6: MPU Protection Region Base/Limit/Select Address Register */
> +#define HPRSELR         p15,4,c6,c2,1
> +#define PRBAR_EL2       p15,4,c6,c3,0
> +#define PRLAR_EL2       p15,4,c6,c8,1
> +
> +#define MPUIR_EL2               HMPUIR
> +#define PRSELR_EL2              HPRSELR
> +
> +#endif
Please provide comment /* __ASM_ARM_MPU_CPREGS_H */

> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */

~Michal