Hi Julien
> -----Original Message-----
> From: Julien Grall <[email protected]>
> Sent: Monday, February 6, 2023 5:46 AM
> To: Penny Zheng <[email protected]>; [email protected]
> Cc: Wei Chen <[email protected]>; Stefano Stabellini
> <[email protected]>; Bertrand Marquis <[email protected]>;
> Volodymyr Babchuk <[email protected]>
> 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 <[email protected]>
> > Signed-off-by: Wei Chen <[email protected]>
> > ---
> > 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
