Re: [PATCH v3 3/7] arm/mpu: Introduce utility functions for the pr_t type

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

On 11/04/2025 16:56, Luca Fancellu wrote:
> Introduce few utility function to manipulate and handle the
> pr_t type.
> 
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
>  xen/arch/arm/include/asm/mpu.h | 40 ++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/mpu.h b/xen/arch/arm/include/asm/mpu.h
> index 59ff22c804c1..6971507457fb 100644
> --- a/xen/arch/arm/include/asm/mpu.h
> +++ b/xen/arch/arm/include/asm/mpu.h
> @@ -20,6 +20,46 @@
>  #define NUM_MPU_REGIONS_MASK    (NUM_MPU_REGIONS - 1)
>  #define MAX_MPU_REGIONS         NUM_MPU_REGIONS_MASK
>  
> +#ifndef __ASSEMBLY__
> +
> +/* Set base address of MPU protection region(pr_t). */
What's the use of (pr_t) in this comment? pr_t is a data type. If at all, it
would want to be ...region @pr but I think you can skip it.

> +static inline void pr_set_base(pr_t *pr, paddr_t base)
> +{
> +    pr->prbar.reg.base = (base >> MPU_REGION_SHIFT);
Looking at pr_t definition, base/limit is 46 bits wide. However the spec says
that last 4bits are reserved (i.e. you should not write to them) unless FEAT_LPA
is implemented. What's our plan here?

> +}
> +
> +/* Set limit address of MPU protection region(pr_t). */
> +static inline void pr_set_limit(pr_t *pr, paddr_t limit)
> +{
> +    pr->prlar.reg.limit = ((limit - 1) >> MPU_REGION_SHIFT);
Why -1? AFAIR these registers take inclusive addresses, so is it because you
want caller to pass limit as exclusive and you convert it to inclusive? I think
it's quite error prone.


~Michal