Re: [PATCH v3 34/52] xen/mpu: destroy an existing entry in Xen MPU memory mapping table

[Ayan Kumar Halder <ayankuma@amd.com>]

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)

> +
>   /* 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

>
>   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(&region, index);
> +    if ( !region_is_valid(&region) ^ 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*)&region, 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