Hi Ayan
On 2023/7/1 00:17, Ayan Kumar Halder wrote:
>
> On 26/06/2023 04:34, Penny Zheng wrote:
>> CAUTION: This message has originated from an External Source. Please
>> use proper judgment and caution when opening attachments, clicking
>> links, or responding to this email.
>>
>>
>> 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.
>>
>> Rignt now, we only support destroying a *WHOLE* MPU memory region,
>> part-region removing is not supported, as in worst case, it will
>> leave two fragments behind.
>>
>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>> ---
>> v3:
>> - make pr_get_base()/pr_get_limit() static inline
>> - need an isb to ensure register write visible before zeroing the entry
>> ---
>> xen/arch/arm/include/asm/arm64/mpu.h | 2 +
>> xen/arch/arm/include/asm/arm64/sysregs.h | 3 +
>> xen/arch/arm/mm.c | 5 ++
>> xen/arch/arm/mpu/mm.c | 74 ++++++++++++++++++++++++
>> 4 files changed, 84 insertions(+)
>>
>> diff --git a/xen/arch/arm/include/asm/arm64/mpu.h
>> b/xen/arch/arm/include/asm/arm64/mpu.h
>> index 715ea69884..aee7947223 100644
>> --- a/xen/arch/arm/include/asm/arm64/mpu.h
>> +++ b/xen/arch/arm/include/asm/arm64/mpu.h
>> @@ -25,6 +25,8 @@
>> #define REGION_UART_SEL 0x07
>> #define MPUIR_REGION_MASK ((_AC(1, UL) << 8) - 1)
>>
>> +#define MPU_PRENR_BITS 32
>
> This is common to R52 and R82.
>
> Thus, you can put it in the common file (may be
> xen/arch/arm/include/asm/mpu/mm.h)
>
Will do.
>> +
>> /* Access permission attributes. */
>> /* Read/Write at EL2, No Access at EL1/EL0. */
>> #define AP_RW_EL2 0x0
>> diff --git a/xen/arch/arm/include/asm/arm64/sysregs.h
>> b/xen/arch/arm/include/asm/arm64/sysregs.h
>> index c8a679afdd..96c025053b 100644
>> --- a/xen/arch/arm/include/asm/arm64/sysregs.h
>> +++ b/xen/arch/arm/include/asm/arm64/sysregs.h
>> @@ -509,6 +509,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.c b/xen/arch/arm/mm.c
>> index 8625066256..247d17cfa1 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -164,7 +164,12 @@ 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);
>> +#ifndef CONFIG_HAS_MPU
>> return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, 0);
>> +#else
>> + return xen_mpumap_update(virt_to_maddr((void *)s),
>> + virt_to_maddr((void *)e), 0);
>> +#endif
>> }
>
> Refer my comment in previous patch.
>
> You can have two implementations of this function 1)
> xen/arch/arm/mmu/mm.c 2) xen/arch/arm/mpu/mm.h
>
Refer my comment in previous patch.
I prefer #ifdef in destroy_xen_mappings()
>>
>> int modify_xen_mappings(unsigned long s, unsigned long e, unsigned
>> int flags)
>> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
>> index 0a65b58dc4..a40055ae5e 100644
>> --- a/xen/arch/arm/mpu/mm.c
>> +++ b/xen/arch/arm/mpu/mm.c
>> @@ -425,6 +425,59 @@ static int mpumap_contain_region(pr_t *table,
>> uint8_t nr_regions,
>> return MPUMAP_REGION_FAILED;
>> }
>>
>> +/* Disable or enable EL2 MPU memory region at index #index */
>> +static void control_mpu_region_from_index(uint8_t index, bool enable)
>> +{
>> + pr_t region;
>> +
>> + read_protection_region(®ion, index);
>> + if ( !region_is_valid(®ion) ^ enable )
>> + {
>> + printk(XENLOG_WARNING
>> + "mpu: MPU memory region[%u] 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);
>> + }
>> + else
>> + {
>> + region.prlar.reg.en = enable ? 1 : 0;
>> + write_protection_region((const pr_t*)®ion, index);
>> + }
>> + /* Ensure the write before zeroing the entry */
> dsb(); /* to ensure write completes */
>> + isb();
>> +
>> + /* Update according bitfield in xen_mpumap_mask */
>> + spin_lock(&xen_mpumap_alloc_lock);
>> +
>> + if ( enable )
>> + set_bit(index, xen_mpumap_mask);
>> + else
>> + {
>> + clear_bit(index, xen_mpumap_mask);
>> + memset(&xen_mpumap[index], 0, sizeof(pr_t));
>> + }
>> +
>> + spin_unlock(&xen_mpumap_alloc_lock);
>> +}
>> +
>> /*
>> * Update an entry in Xen MPU memory region mapping
>> table(xen_mpumap) at
>> * the index @idx.
>> @@ -461,6 +514,27 @@ static int xen_mpumap_update_entry(paddr_t base,
>> paddr_t limit,
>>
>> write_protection_region((const pr_t*)(&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
>> + * leave two fragments behind.
>> + * 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);
>> + }
>>
>> return 0;
>> }
>> --
>> 2.25.1
>>
>>
> - Ayan