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

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

On 13/05/2025 10:45, 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>
> ---
> v5 changes:
>  - removed AP_RW_EL2 used only by pr_of_xenaddr(), fixed
>    comments and typos
>  - Given some comments to the page.h flags and modifications
>    to the prbar_t fields ap, xn, the constructor now takes
>    flags instead of attr_idx, which I believe it's better,
>    deleted PRBAR_EL2_XN_ENABLED since now the PAGE_XN_MASK()
>    is used instead.
>  - renamed to pr_of_addr since it will be used also in p2m
> 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/mpu/mm.h | 10 +++++
>  xen/arch/arm/mpu/mm.c             | 66 +++++++++++++++++++++++++++++++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h 
> b/xen/arch/arm/include/asm/mpu/mm.h
> index 2ee908801665..f0facee51692 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -75,6 +75,16 @@ void read_protection_region(pr_t *pr_read, uint8_t sel);
>   */
>  void write_protection_region(const pr_t *pr_write, uint8_t sel);
>  
> +/*
> + * 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.
> + * @flags: memory flags for the mapping.
> + * @return: pr_t structure describing a protection region.
> + */
> +pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags);
> +
>  #endif /* __ARM_MPU_MM_H__ */
>  
>  /*
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 46883cbd4af9..ac83227e607e 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;
> @@ -153,6 +154,71 @@ void write_protection_region(const pr_t *pr_write, 
> uint8_t sel)
>          BUG(); /* Can't happen */
>      }
>  }
> +
> +pr_t pr_of_addr(paddr_t base, paddr_t limit, unsigned int flags)
> +{
> +    unsigned int attr_idx = PAGE_AI_MASK(flags);
> +    prbar_t prbar;
> +    prlar_t prlar;
> +    pr_t region;
> +
> +    /* Build up value for PRBAR_EL2. */
> +    prbar = (prbar_t) {
> +        .reg = {
> +            .ro = PAGE_RO_MASK(flags),
> +            .xn = PAGE_XN_MASK(flags),
Shouldn't you also initialize .xn_0 and .ap_0 or you rely on compound literal
implicit zero initialization of unspecified fields? But then you do initialize
.ns to 0 fror prlar...

> +        }};
> +
> +    switch ( attr_idx )
> +    {
> +    /*
> +     * ARM ARM: Shareable, Inner Shareable, and Outer Shareable Normal memory
> +     * (DDI 0487L.a B2.10.1.1.1 Note section):
> +     *
> +     * Because all data accesses to Non-cacheable locations are data coherent
> +     * to all observers, Non-cacheable locations are always treated as Outer
> +     * Shareable
> +     *
> +     * ARM ARM: Device memory (DDI 0487L.a B2.10.2)
> +     *
> +     * All of these memory types have the following properties:
> +     * [...]
> +     *  - Data accesses to memory locations are coherent for all observers in
> +     *    the system, and correspondingly are treated as being Outer 
> Shareable
> +     */
> +    case MT_NORMAL_NC:
> +        /* Fall through */
> +    case MT_DEVICE_nGnRnE:
> +        /* Fall through */
> +    case MT_DEVICE_nGnRE:
> +        prbar.reg.sh = LPAE_SH_OUTER;
> +        break;
> +    default:
> +        /* Xen mappings are SMP coherent */
> +        prbar.reg.sh = LPAE_SH_INNER;
> +        break;
> +    }
> +
> +    /* 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)

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal