Re: [PATCH v4 6/7] arm/mpu: Provide a constructor for pr_t type

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

On 29/04/2025 17:20, Luca Fancellu wrote:
> Provide a function that creates a pr_t object from a memory
> range and some attributes.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> v4 changes:
>  - update helper comments
>  - rename XN_EL2_ENABLED to PRBAR_EL2_XN_ENABLED
>  - protected pr_of_xenaddr() with #ifdef Arm64 until Arm32
>    can build with it
> ---
>  xen/arch/arm/include/asm/arm64/mpu.h | 11 +++++
>  xen/arch/arm/include/asm/mpu.h       |  4 ++
>  xen/arch/arm/include/asm/mpu/mm.h    | 10 ++++
>  xen/arch/arm/mpu/mm.c                | 68 ++++++++++++++++++++++++++++
>  4 files changed, 93 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h 
> b/xen/arch/arm/include/asm/arm64/mpu.h
> index b27fccd77550..39233b43a5aa 100644
> --- a/xen/arch/arm/include/asm/arm64/mpu.h
> +++ b/xen/arch/arm/include/asm/arm64/mpu.h
> @@ -3,6 +3,17 @@
>  #ifndef __ARM_ARM64_MPU_H__
>  #define __ARM_ARM64_MPU_H__
>  
> +/*
> + * Excute never.
> + * Stage 1 EL2 translation regime.
> + * XN[1] determines whether execution of the instruction fetched from the MPU
> + * memory region is permitted.
> + * Stage 2 EL1/EL0 translation regime.
> + * XN[0] determines whether execution of the instruction fetched from the MPU
> + * memory region is permitted.
> + */
Why do we need this comment? If any, why do we need EL1 description if the macro
is EL2?

> +#define PRBAR_EL2_XN_ENABLED  0x2
> +
>  #ifndef __ASSEMBLY__
>  
>  /* Protection Region Base Address Register */
> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
> index 0e0a7f05ade9..7b82f10d336b 100644
> --- a/xen/arch/arm/include/asm/mpu.h
> +++ b/xen/arch/arm/include/asm/mpu.h
> @@ -24,6 +24,10 @@
>  #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
>  #define MAX_MPU_REGION_NR       255
>  
> +/* Access permission attributes. */
> +/* Read/Write at EL2, No Access at EL1/EL0. */
> +#define AP_RW_EL2 0x0
This macro and the previous one are used only once (I also checked your full
tree) and cannot be set by the caller. What's the purpose of the macros then?
Why can't we set these values in pr_of_xenaddr() and add comment next to value
there?

> +
>  #ifndef __ASSEMBLY__
>  
>  #ifdef CONFIG_ARM_64
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h 
> b/xen/arch/arm/include/asm/mpu/mm.h
> index e2235e568e81..296fe74c5d61 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -75,6 +75,16 @@ extern void read_protection_region(pr_t *pr_read, uint8_t 
> sel);
>   */
>  extern void write_protection_region(const pr_t *pr_write, uint8_t sel);
Here and ...

>  
> +/*
> + * Creates a pr_t structure describing a protection region.
> + *
> + * @base: base address as base of the protection region.
> + * @limit: exclusive address as limit of the protection region.
> + * @attr: attribute index for the memory type.
> + * @return: pr_t structure describing a protection region.
> + */
> +extern pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned int 
> attr_idx);
here. Please don't use extern in prototypes. It's not needed.

> +
>  #endif /* __ARM_MPU_MM_H__ */
>  
>  /*
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 40ccf99adc94..2e0aeb486ff8 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -9,6 +9,7 @@
>  #include <xen/types.h>
>  #include <asm/mpu.h>
>  #include <asm/mpu/mm.h>
> +#include <asm/page.h>
>  #include <asm/sysregs.h>
>  
>  struct page_info *frame_table;
> @@ -151,6 +152,73 @@ void write_protection_region(const pr_t *pr_write, 
> uint8_t sel)
>          BUG(); /* Can't happen */
>      }
>  }
> +
> +pr_t pr_of_xenaddr(paddr_t base, paddr_t limit, unsigned int attr_idx)
> +{
> +    prbar_t prbar;
> +    prlar_t prlar;
> +    pr_t region;
> +
> +    /* Build up value for PRBAR_EL2. */
> +    prbar = (prbar_t) {
> +        .reg = {
> +            .ap = AP_RW_EL2,      /* Read/Write at EL2, no access at 
> EL1/EL0. */
> +            .xn = PRBAR_EL2_XN_ENABLED,   /* No need to execute outside 
> .text */
> +        }};
> +
> +    switch ( attr_idx )
> +    {
> +    case MT_NORMAL_NC:
> +        /*
> +         * ARM ARM: Overlaying the shareability attribute (DDI
> +         * 0406C.b B3-1376 to 1377)
It's a bit odd to provide here the manual for Armv7.
Also, our general advice is to use the latest revision.

> +         *
> +         * A memory region with a resultant memory type attribute of normal,
> +         * and a resultant cacheability attribute of Inner non-cacheable,
> +         * outer non-cacheable, must have a resultant shareability attribute
> +         * of outer shareable, otherwise shareability is UNPREDICTABLE.
> +         *
> +         * On ARMv8 sharability is ignored and explicitly treated as outer
> +         * shareable for normal inner non-cacheable, outer non-cacheable.
> +         */
> +        prbar.reg.sh = LPAE_SH_OUTER;
> +        break;
> +    case MT_DEVICE_nGnRnE:
> +    case MT_DEVICE_nGnRE:
> +        /*
> +         * Shareability is ignored for non-normal memory, Outer is as
> +         * good as anything.
> +         *
> +         * On ARMv8 sharability is ignored and explicitly treated as outer
Does this Armv8 comments make sense? We don't support Armv7 MPU.

> +         * shareable for any device memory type.
> +         */
> +        prbar.reg.sh = LPAE_SH_OUTER;
> +        break;
> +    default:
> +        /* Xen mappings are SMP coherent */
> +        prbar.reg.sh = LPAE_SH_INNER;
AFAIR MISRA C requires every clause to be terminated with break.
That said I don't remember if we accepted this rule...

> +    }
> +
> +    /* Build up value for PRLAR_EL2. */
> +    prlar = (prlar_t) {
> +        .reg = {
> +            .ns = 0,        /* Hyp mode is in secure world */
> +            .ai = attr_idx,
> +            .en = 1,        /* Region enabled */
> +        }};
> +
> +    /* Build up MPU memory region. */
> +    region = (pr_t) {
> +        .prbar = prbar,
> +        .prlar = prlar,
> +    };
> +
> +    /* Set base address and limit address. */
> +    pr_set_base(&region, base);
> +    pr_set_limit(&region, limit);
> +
> +    return region;
> +}
>  #endif
>  
>  void __init setup_mm(void)

~Michal