Re: [PATCH v5 5/6] arm/mpu: Introduce utility functions for the pr_t type

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

On 13/05/2025 10:45, Luca Fancellu wrote:
> Introduce a few utility functions to manipulate and handle the
> pr_t type.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> v5 changes:
>  - Don't rely on bitfield and use the mask MPU_REGION_RES0 for
>    pr_set_base and pr_set_limit to make it explicit. Fixed typos
>    in commit message.
> v4 changes:
>  - Modify comment on top of the helpers. Clarify pr_set_limit
>    takes exclusive address.
>    Protected common code with #ifdef Arm64 until Arm32 is ready
>    with pr_t
> ---
>  xen/arch/arm/include/asm/mpu.h | 65 ++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
> index fb93b8b19d24..b90ae8eab662 100644
> --- a/xen/arch/arm/include/asm/mpu.h
> +++ b/xen/arch/arm/include/asm/mpu.h
> @@ -23,6 +23,71 @@
>  #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
>  #define MAX_MPU_REGION_NR       NUM_MPU_REGIONS_MASK
>  
> +#ifndef __ASSEMBLY__
> +
> +#ifdef CONFIG_ARM_64
> +/*
> + * Set base address of MPU protection region.
> + *
> + * @pr: pointer to the protection region structure.
> + * @base: base address as base of the protection region.
> + */
> +static inline void pr_set_base(pr_t *pr, paddr_t base)
> +{
> +    pr->prbar.reg.base = ((base & ~MPU_REGION_RES0) >> MPU_REGION_SHIFT);
> +}
> +
> +/*
> + * Set limit address of MPU protection region.
> + *
> + * @pr: pointer to the protection region structure.
> + * @limit: exclusive address as limit of the protection region.
> + */
> +static inline void pr_set_limit(pr_t *pr, paddr_t limit)
> +{
> +    pr->prlar.reg.limit = (((limit - 1) & ~MPU_REGION_RES0)
Might be worth adding a comment that PRLAR expects inclusive limit hence (limit 
-1).

> +                           >> MPU_REGION_SHIFT);
> +}
> +
> +/*
> + * Access to get base address of MPU protection region.
> + * The base address shall be zero extended.
> + *
> + * @pr: pointer to the protection region structure.
> + * @return: Base address configured for the passed protection region.
> + */
> +static inline paddr_t pr_get_base(pr_t *pr)
Why not const?

> +{
> +    return (paddr_t)(pr->prbar.reg.base << MPU_REGION_SHIFT);
> +}
> +
> +/*
> + * Access to get limit address of MPU protection region.
> + * The limit address shall be concatenated with 0x3f.
> + *
> + * @pr: pointer to the protection region structure.
> + * @return: Inclusive limit address configured for the passed protection 
> region.
> + */
> +static inline paddr_t pr_get_limit(pr_t *pr)
Why not const?

> +{
> +    return (paddr_t)((pr->prlar.reg.limit << MPU_REGION_SHIFT)
> +                     | ~MPU_REGION_MASK);
> +}
> +
> +/*
> + * Checks if the protection region is valid (enabled).
NIT: in other helpers you use imperative mood, so this should be "Check if"
> + *
> + * @pr: pointer to the protection region structure.
> + * @return: True if the region is valid (enabled), false otherwise.
> + */
> +static inline bool region_is_valid(pr_t *pr)
Why not const?

> +{
> +    return pr->prlar.reg.en;
> +}
> +#endif
Please add /* CONFIG_ARM_64 */

> +
> +#endif /* __ASSEMBLY__ */
> +
>  #endif /* __ARM_MPU_H__ */
>  
>  /*


~Michal