RE: [PATCH v2 19/40] xen/mpu: populate a new region in Xen MPU mapping table

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Monday, February 6, 2023 5:46 AM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v2 19/40] xen/mpu: populate a new region in Xen MPU
> mapping table
> 
> Hi,
> 
> On 13/01/2023 05:28, Penny Zheng wrote:
> > The new helper xen_mpumap_update() is responsible for updating an
> > entry in Xen MPU memory mapping table, including creating a new entry,
> > updating or destroying an existing one.
> >
> > This commit only talks about populating a new entry in Xen MPU mapping
> > table( xen_mpumap). Others will be introduced in the following commits.
> >
> > In xen_mpumap_update_entry(), firstly, we shall check if requested
> > address range [base, limit) is not mapped. Then we use pr_of_xenaddr()
> > to build up the structure of MPU memory region(pr_t).
> > In the last, we set memory attribute and permission based on variable
> @flags.
> >
> > To summarize all region attributes in one variable @flags, layout of
> > the flags is elaborated as follows:
> > [0:2] Memory attribute Index
> > [3:4] Execute Never
> > [5:6] Access Permission
> > [7]   Region Present
> > Also, we provide a set of definitions(REGION_HYPERVISOR_RW, etc) that
> > combine the memory attribute and permission for common combinations.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > ---
> >   xen/arch/arm/include/asm/arm64/mpu.h |  72 +++++++
> >   xen/arch/arm/mm_mpu.c                | 276 ++++++++++++++++++++++++++-
> >   2 files changed, 340 insertions(+), 8 deletions(-)
> >
[...]
> > +
> > +#define MPUMAP_REGION_FAILED    0
> > +#define MPUMAP_REGION_FOUND     1
> > +#define MPUMAP_REGION_INCLUSIVE 2
> > +#define MPUMAP_REGION_OVERLAP   3
> > +
> > +/*
> > + * Check whether memory range [base, limit] is mapped in MPU memory
> > + * region table \mpu. Only address range is considered, memory
> > +attributes
> > + * and permission are not considered here.
> > + * If we find the match, the associated index will be filled up.
> > + * If the entry is not present, INVALID_REGION will be set in \index
> > + *
> > + * Make sure that parameter \base and \limit are both referring
> > + * inclusive addresss
> > + *
> > + * Return values:
> > + *  MPUMAP_REGION_FAILED: no mapping and no overmapping
> > + *  MPUMAP_REGION_FOUND: find an exact match in address
> > + *  MPUMAP_REGION_INCLUSIVE: find an inclusive match in address
> > + *  MPUMAP_REGION_OVERLAP: overlap with the existing mapping  */
> > +static int mpumap_contain_region(pr_t *mpu, uint64_t nr_regions,
> > +                                 paddr_t base, paddr_t limit,
> > +uint64_t *index)
> 
> Is it really possible to support 2^64 - 1 region? If so, is that the case on 
> arm32
> as well?
> 

No, the allowable bitwidth is 8 bit. I'll change it uint8_t instead

> > +{
> > +    uint64_t i = 0;
> > +    uint64_t _index = INVALID_REGION;
> > +
> > +    /* Allow index to be NULL */
> > +    index = index ?: &_index;
> > +
> > +    for ( ; i < nr_regions; i++ )
> > +    {
> > +        paddr_t iter_base = pr_get_base(&mpu[i]);
> > +        paddr_t iter_limit = pr_get_limit(&mpu[i]);
> > +
> > +        /* Found an exact valid match */
> > +        if ( (iter_base == base) && (iter_limit == limit) &&
> > +             region_is_valid(&mpu[i]) )
> > +        {
> > +            *index = i;
> > +            return MPUMAP_REGION_FOUND;
> > +        }
> > +
> > +        /* No overlapping */
> > +        if ( (iter_limit < base) || (iter_base > limit) )
> > +            continue;
> > +        /* Inclusive and valid */
> > +        else if ( (base >= iter_base) && (limit <= iter_limit) &&
> > +                  region_is_valid(&mpu[i]) )
> > +        {
> > +            *index = i;
> > +            return MPUMAP_REGION_INCLUSIVE;
> > +        }
> > +        else
> > +        {
> > +            region_printk("Range 0x%"PRIpaddr" - 0x%"PRIpaddr" overlaps
> with the existing region 0x%"PRIpaddr" - 0x%"PRIpaddr"\n",
> > +                          base, limit, iter_base, iter_limit);
> > +            return MPUMAP_REGION_OVERLAP;
> > +        }
> > +    }
> > +
> > +    return MPUMAP_REGION_FAILED;
> > +}
> > +
> > +/*
> > + * Update an entry at the index @idx.
> > + * @base:  base address
> > + * @limit: limit address(exclusive)
> > + * @flags: region attributes, should be the combination of
> > +REGION_HYPERVISOR_xx  */ static int
> xen_mpumap_update_entry(paddr_t
> > +base, paddr_t limit,
> > +                                   unsigned int flags) {
> > +    uint64_t idx;
> > +    int rc;
> > +
> > +    rc = mpumap_contain_region(xen_mpumap, max_xen_mpumap, base,
> limit - 1,
> > +                               &idx);
> > +    if ( rc == MPUMAP_REGION_OVERLAP )
> > +        return -EINVAL;
> > +
> > +    /* We are inserting a mapping => Create new region. */
> > +    if ( flags & _REGION_PRESENT )
> > +    {
> > +        if ( rc != MPUMAP_REGION_FAILED )
> > +            return -EINVAL;
> > +
> > +        if ( xen_boot_mpu_regions_is_full() )
> > +        {
> > +            region_printk("There is no room left in EL2 MPU memory region
> mapping\n");
> > +            return -ENOMEM;
> > +        }
> > +
> > +        /* During boot time, the default index is next_fixed_region_idx. */
> > +        if ( system_state <= SYS_STATE_active )
> > +            idx = next_fixed_region_idx;
> > +
> > +        xen_mpumap[idx] = pr_of_xenaddr(base, limit - 1,
> REGION_AI_MASK(flags));
> > +        /* Set permission */
> > +        xen_mpumap[idx].prbar.reg.ap = REGION_AP_MASK(flags);
> > +        xen_mpumap[idx].prbar.reg.xn = REGION_XN_MASK(flags);
> > +
> > +        /* Update and enable the region */
> > +        access_protection_region(false, NULL, (const
> pr_t*)(&xen_mpumap[idx]),
> > +                                 idx);
> > +
> > +        if ( system_state <= SYS_STATE_active )
> > +            update_boot_xen_mpumap_idx(idx);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned
> > +int flags) {
> > +    int rc;
> > +
> > +    /*
> > +     * The hardware was configured to forbid mapping both writeable and
> > +     * executable.
> > +     * When modifying/creating mapping (i.e _REGION_PRESENT is set),
> > +     * prevent any update if this happen.
> > +     */
> > +    if ( (flags & _REGION_PRESENT) && !REGION_RO_MASK(flags) &&
> > +         !REGION_XN_MASK(flags) )
> > +    {
> > +        region_printk("Mappings should not be both Writeable and
> Executable.\n");
> > +        return -EINVAL;
> > +    }
> > +
> > +    if ( !IS_ALIGNED(base, PAGE_SIZE) || !IS_ALIGNED(limit, PAGE_SIZE) )
> > +    {
> > +        region_printk("base address 0x%"PRIpaddr", or limit address
> 0x%"PRIpaddr" is not page aligned.\n",
> > +                      base, limit);
> > +        return -EINVAL;
> > +    }
> > +
> > +    spin_lock(&xen_mpumap_lock);
> > +
> > +    rc = xen_mpumap_update_entry(base, limit, flags);
> > +
> > +    spin_unlock(&xen_mpumap_lock);
> > +
> > +    return rc;
> > +}
> > +
> > +int map_pages_to_xen(unsigned long virt,
> > +                     mfn_t mfn,
> > +                     unsigned long nr_mfns,
> > +                     unsigned int flags) {
> > +    ASSERT(virt == mfn_to_maddr(mfn));
> > +
> > +    return xen_mpumap_update(mfn_to_maddr(mfn),
> > +                             mfn_to_maddr(mfn_add(mfn, nr_mfns)),
> > +flags); }
> > +
> >   /* TODO: Implementation on the first usage */
> >   void dump_hyp_walk(vaddr_t addr)
> >   {
> > @@ -230,14 +498,6 @@ void *ioremap(paddr_t pa, size_t len)
> >       return NULL;
> >   }
> >
> > -int map_pages_to_xen(unsigned long virt,
> > -                     mfn_t mfn,
> > -                     unsigned long nr_mfns,
> > -                     unsigned int flags)
> > -{
> > -    return -ENOSYS;
> > -}
> > -
> 
> Why do you implement map_pages_to_xen() at a different place?
> 
> 
> >   int destroy_xen_mappings(unsigned long s, unsigned long e)
> >   {
> >       return -ENOSYS;
> 
> Cheers,
> 
> --
> Julien Grall