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.fance...@arm.com>
> ---
> 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
