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

[Penny Zheng <penny.zheng@xxxxxxx>]

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@xxxxxxx>
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> > 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(&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