Hi Penny,
On 13/01/2023 05:28, Penny Zheng wrote:
> This commit expands xen_mpumap_update/xen_mpumap_update_entry to include
> destroying an existing entry.
>
> We define a new helper "control_xen_mpumap_region_from_index" to enable/disable
> the MPU region based on index. If region is within [0, 31], we could quickly
> disable the MPU region through PRENR_EL2 which provides direct access to the
> PRLAR_EL2.EN bits of EL2 MPU regions.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> ---
> xen/arch/arm/include/asm/arm64/mpu.h | 20 ++++++
> xen/arch/arm/include/asm/arm64/sysregs.h | 3 +
> xen/arch/arm/mm_mpu.c | 77 ++++++++++++++++++++++--
> 3 files changed, 95 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h b/xen/arch/arm/include/asm/arm64/mpu.h
> index 0044bbf05d..c1dea1c8e9 100644
> --- a/xen/arch/arm/include/asm/arm64/mpu.h
> +++ b/xen/arch/arm/include/asm/arm64/mpu.h
> @@ -16,6 +16,8 @@
> */
> #define ARM_MAX_MPU_MEMORY_REGIONS 255
>
> +#define MPU_PRENR_BITS 32
> +
> /* Access permission attributes. */
> /* Read/Write at EL2, No Access at EL1/EL0. */
> #define AP_RW_EL2 0x0
> @@ -132,6 +134,24 @@ typedef struct {
> _pr->prlar.reg.en; \
> })
>
> +/*
> + * Access to get base address of MPU protection region(pr_t).
> + * The base address shall be zero extended.
> + */
> +#define pr_get_base(pr) ({ \
> + pr_t *_pr = pr; \
> + (uint64_t)_pr->prbar.reg.base << MPU_REGION_SHIFT; \
> +})
Can this be a static inline?
> +
> +/*
> + * Access to get limit address of MPU protection region(pr_t).
> + * The limit address shall be concatenated with 0x3f.
> + */
> +#define pr_get_limit(pr) ({ \
> + pr_t *_pr = pr; \
> + (uint64_t)((_pr->prlar.reg.limit << MPU_REGION_SHIFT) | 0x3f); \
> +})
Same.
> +
> #endif /* __ASSEMBLY__ */
>
> #endif /* __ARM64_MPU_H__ */
> diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h b/xen/arch/arm/include/asm/arm64/sysregs.h
> index aca9bca5b1..c46daf6f69 100644
> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
> @@ -505,6 +505,9 @@
> /* MPU Type registers encode */
> #define MPUIR_EL2 S3_4_C0_C0_4
>
> +/* MPU Protection Region Enable Register encode */
> +#define PRENR_EL2 S3_4_C6_C1_1
> +
> #endif
>
> /* Access to system registers */
> diff --git a/xen/arch/arm/mm_mpu.c b/xen/arch/arm/mm_mpu.c
> index d2e19e836c..3a0d110b13 100644
> --- a/xen/arch/arm/mm_mpu.c
> +++ b/xen/arch/arm/mm_mpu.c
> @@ -385,6 +385,45 @@ static int mpumap_contain_region(pr_t *mpu, uint64_t nr_regions,
> return MPUMAP_REGION_FAILED;
> }
>
> +/* Disable or enable EL2 MPU memory region at index #index */
> +static void control_mpu_region_from_index(uint64_t index, bool enable)
> +{
> + pr_t region;
> +
> + access_protection_region(true, ®ion, NULL, index);
> + if ( (region_is_valid(®ion) && enable) ||
> + (!region_is_valid(®ion) && !enable) )
You could write:
!(region_is_valid(®ion) ^ enable)
> + {
> + printk(XENLOG_WARNING
> + "mpu: MPU memory region[%lu] is already %s\n", index,
> + enable ? "enabled" : "disabled");
> + return;
> + }
> +
> + /*
> + * ARM64v8R provides PRENR_EL2 to have direct access to the
> + * PRLAR_EL2.EN bits of EL2 MPU regions from 0 to 31.
> + */
> + if ( index < MPU_PRENR_BITS )
> + {
> + uint64_t orig, after;
> +
> + orig = READ_SYSREG(PRENR_EL2);
> + if ( enable )
> + /* Set respective bit */
> + after = orig | (1UL << index);
> + else
> + /* Clear respective bit */
> + after = orig & (~(1UL << index));
> + WRITE_SYSREG(after, PRENR_EL2);
Don't you need an isb (or similar) to ensure this is visible before...
> + }
> + else
> + {
> + region.prlar.reg.en = enable ? 1 : 0;
> + access_protection_region(false, NULL, (const pr_t*)®ion, index);
> + }
> +}
> +
> /*
> * Update an entry at the index @idx.
> * @base: base address
> @@ -449,6 +488,30 @@ static int xen_mpumap_update_entry(paddr_t base, paddr_t limit,
> if ( system_state <= SYS_STATE_active )
> update_boot_xen_mpumap_idx(idx);
> }
> + else
> + {
> + /*
> + * Currently, we only support destroying a *WHOLE* MPU memory region,
> + * part-region removing is not supported, as in worst case, it will
> + * lead to two fragments in result after destroying.
> + * part-region removing will be introduced only when actual usage
> + * comes.
> + */
> + if ( rc == MPUMAP_REGION_INCLUSIVE )
> + {
> + region_printk("mpu: part-region removing is not supported\n");
> + return -EINVAL;
> + }
> +
> + /* We are removing the region */
> + if ( rc != MPUMAP_REGION_FOUND )
> + return -EINVAL;
> +
> + control_mpu_region_from_index(idx, false);
> +
> + /* Clear the according MPU memory region entry.*/
> + memset(&xen_mpumap[idx], 0, sizeof(pr_t));
... zeroing the entry? Also, you could use memzero() here.
> + }
>
> return 0;
> }
> @@ -589,6 +652,15 @@ static void __init map_mpu_memory_section_on_boot(enum mpu_section_info type,
> }
> }
>
> +int destroy_xen_mappings(unsigned long s, unsigned long e)
> +{
> + ASSERT(IS_ALIGNED(s, PAGE_SIZE));
> + ASSERT(IS_ALIGNED(e, PAGE_SIZE));
> + ASSERT(s <= e);
> +
> + return xen_mpumap_update(s, e, 0);
> +}
> +
> /*
> * System RAM is statically partitioned into different functionality
> * section in Device Tree, including static xenheap, guest memory
> @@ -656,11 +728,6 @@ void *ioremap(paddr_t pa, size_t len)
> return NULL;
> }
>
> -int destroy_xen_mappings(unsigned long s, unsigned long e)
> -{
> - return -ENOSYS;
> -}
> -
> int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
> {
> return -ENOSYS;
Cheers,
--
Julien Grall