Re: [PATCH v3 4/6] xen/arm: mpu: Create boot-time MPU protection regions

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

Hi Ayan,


> diff --git a/xen/arch/arm/arm64/mpu/head.S b/xen/arch/arm/arm64/mpu/head.S
> new file mode 100644
> index 0000000000..4a21bc815c
> --- /dev/null
> +++ b/xen/arch/arm/arm64/mpu/head.S
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Start-of-day code for an Armv8-R MPU system.
> + */
> +
> +#include <asm/mm.h>
> +#include <asm/arm64/mpu/sysregs.h>
> +
> +#define REGION_TEXT_PRBAR       0x38    /* SH=11 AP=10 XN=00 */
> +#define REGION_RO_PRBAR         0x3A    /* SH=11 AP=10 XN=10 */

NIT alignment

> +#define REGION_DATA_PRBAR       0x32    /* SH=11 AP=00 XN=10 */
> +
> +#define REGION_NORMAL_PRLAR     0x0f    /* NS=0 ATTR=111 EN=1 */
> +
> +/*
> + * 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.
> + *
> + * Inputs:
> + * sel:         region selector
> + * base:        reg storing base address (should be page-aligned)
> + * 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
> + */
> +.macro prepare_xen_region, sel, base, limit, prbar, prlar, maxcount, 
> attr_prbar=REGION_DATA_PRBAR, attr_prlar=REGION_NORMAL_PRLAR
> +
> +    /* Check if the number of regions exceeded the count specified in 
> MPUIR_EL2 */
> +    add   \sel, \sel, #1

I think there is an issue adding 1 here, because the very first region we are 
going to fill will be the 1st even if we intended the 0th.
Probably moving this one at the end will fix the issue

> +    cmp   \sel, \maxcount
> +    bgt   fail
> +
> +    /* 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
> +
> +    msr   PRSELR_EL2, \sel
> +    isb
> +    msr   PRBAR_EL2, \prbar
> +    msr   PRLAR_EL2, \prlar
> +    dsb   sy
> +    isb
> +.endm
> +
> +/* Load the physical address of a symbol into xb */
> +.macro load_paddr xb, sym
> +    ldr \xb, =\sym
> +    add \xb, \xb, x20       /* x20 - Phys offset */
> +.endm
> +
> +/*
> + * Maps the various sections of Xen (described in xen.lds.S) as different MPU
> + * regions.
> + *
> + * Inputs:
> + *   lr : Address to return to.
> + *
> + * Clobbers x0 - x5
> + *
> + */
> +FUNC(enable_boot_cpu_mm)
> +
> +    /* Check if the number of regions exceeded the count specified in 
> MPUIR_EL2 */
> +    mrs   x5, MPUIR_EL2
> +
> +    /* x0: region sel */
> +    mov   x0, xzr
> +    /* Xen text section. */
> +    load_paddr x1, _stext
> +    load_paddr x2, _etext
> +    cmp x1, x2
> +    beq 1f
> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> +
> +1:  /* Xen read-only data section. */
> +    load_paddr x1, _srodata
> +    load_paddr x2, _erodata
> +    cmp x1, x2
> +    beq 2f
> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_RO_PRBAR
> +
> +2:  /* Xen read-only after init and data section. (RW data) */
> +    load_paddr x1, __ro_after_init_start
> +    load_paddr x2, __init_begin
> +    cmp x1, x2
> +    beq 3f
> +    prepare_xen_region x0, x1, x2, x3, x4, x5
> +
> +3:  /* Xen code section. */
> +    load_paddr x1, __init_begin
> +    load_paddr x2, __init_data_begin
> +    cmp x1, x2
> +    beq 4f
> +    prepare_xen_region x0, x1, x2, x3, x4, x5, attr_prbar=REGION_TEXT_PRBAR
> +
> +4:  /* Xen data and BSS section. */
> +    load_paddr x1, __init_data_begin
> +    load_paddr x2, __bss_end
> +    cmp x1, x2
> +    beq 5f
> +    prepare_xen_region x0, x1, x2, x3, x4, x5
> +
> +5:
> +    ret
> +
> +fail:
> +    PRINT("- Number of MPU regions set in MPUIR_EL2 is too less -\r\n")
> +    wfe
> +    b   1b
> +END(enable_boot_cpu_mm)
> +
> +/*
> + * Local variables:
> + * mode: ASM
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/include/asm/arm64/mpu/sysregs.h 
> b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
> new file mode 100644
> index 0000000000..b0c31a58ec
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/arm64/mpu/sysregs.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ASM_ARM_ARM64_MPU_SYSREGS_H
> +#define __ASM_ARM_ARM64_MPU_SYSREGS_H

Same comment about define name as in patch 3, here and in every
new file of this patch

Cheers,
Luca