Re: [PATCH v3 33/52] xen/mpu: initialize frametable in MPU system

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Penny,

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.
>
>
> Xen is using page as the smallest granularity for memory managment.
> And we want to follow the same concept in MPU system.
> That is, structure page_info and the frametable which is used for storing
> and managing the smallest memory managment unit is also required in MPU system.
>
> In MPU system, since we can not use a fixed VA address(FRAMETABLE_VIRT_START)
> to map frametable like MMU system does and everything is 1:1 mapping, we
> instead define a variable "struct page_info *frame_table" as frametable
> pointer, and ask boot allocator to allocate appropriate memory for frametable.
>
> As frametable is successfully initialized, the convertion between machine frame
> number/machine address/"virtual address" and page-info structure is
> ready too, like mfn_to_page/maddr_to_page/virt_to_page, etc
>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> ---
> v3:
> - add ASSERT() to confirm the MFN you pass is covered by the frametable.
> ---
>   xen/arch/arm/include/asm/mm.h     | 14 ++++++++++++++
>   xen/arch/arm/include/asm/mpu/mm.h |  3 +++
>   xen/arch/arm/mpu/mm.c             | 27 +++++++++++++++++++++++++++
>   3 files changed, 44 insertions(+)
>
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index daa6329505..66d98b9a29 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -341,6 +341,19 @@ static inline uint64_t gvirt_to_maddr(vaddr_t va, paddr_t 
> *pa,
>   #define virt_to_mfn(va)     __virt_to_mfn(va)
>   #define mfn_to_virt(mfn)    __mfn_to_virt(mfn)
>
> +#ifdef CONFIG_HAS_MPU
> +/* Convert between virtual address to page-info structure. */
> +static inline struct page_info *virt_to_page(const void *v)
> +{
> +    unsigned long pdx;
> +
> +    pdx = paddr_to_pdx(virt_to_maddr(v));
> +    ASSERT(pdx >= frametable_base_pdx);
> +    ASSERT(pdx < frametable_pdx_end);
> +
> +    return frame_table + pdx - frametable_base_pdx;
> +}
This should go in xen/arch/arm/include/asm/mpu/mm.h. Then you don't need 
ifdef
> +#else
>   /* Convert between Xen-heap virtual addresses and page-info structures. */
>   static inline struct page_info *virt_to_page(const void *v)
>   {
> @@ -354,6 +367,7 @@ static inline struct page_info *virt_to_page(const void *v)
>       pdx += mfn_to_pdx(directmap_mfn_start);
>       return frame_table + pdx - frametable_base_pdx;
>   }
Consequently, this should be in xen/arch/arm/include/asm/mmu/mm.h
> +#endif
>
>   static inline void *page_to_virt(const struct page_info *pg)
>   {
> diff --git a/xen/arch/arm/include/asm/mpu/mm.h 
> b/xen/arch/arm/include/asm/mpu/mm.h
> index e26bd4f975..98f6df65b8 100644
> --- a/xen/arch/arm/include/asm/mpu/mm.h
> +++ b/xen/arch/arm/include/asm/mpu/mm.h
> @@ -2,6 +2,9 @@
>   #ifndef __ARCH_ARM_MM_MPU__
>   #define __ARCH_ARM_MM_MPU__
>
> +extern struct page_info *frame_table;
If you define frame_table in xen/arch/arm/include/asm/mm.h , then you 
don't need the above declaration.
> +extern unsigned long frametable_pdx_end;
Also you don't need extern as this is only used by xen/arch/arm/mpu/mm.c.
> +
>   extern int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags);
You don't need extern here as it should be used only in 
xen/arch/arm/mpu/mm.c
Currently, I see the following in xen/arch/arm/mm.c,

int map_pages_to_xen(unsigned long virt,
                     mfn_t mfn,
                     unsigned long nr_mfns,
                     unsigned int flags)
{
#ifndef CONFIG_HAS_MPU
    return xen_pt_update(virt, mfn, nr_mfns, flags);
#else
    return xen_mpumap_update(mfn_to_maddr(mfn),
                             mfn_to_maddr(mfn_add(mfn, nr_mfns)), flags);
#endif
}

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
}

int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int 
flags)
{
    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, flags);
#else
    return xen_mpumap_update(virt_to_maddr((void *)s),
                             virt_to_maddr((void *)e), flags);
#endif
}

It would be better to have 2 implementations for map_pages_to_xen(), 
destroy_xen_mappings() and modify_xen_mappings() in 
xen/arch/arm/mpu/mm.c and xen/arch/arm/mmu/mm.c.
>   extern void setup_staticheap_mappings(void);
>
> diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> index 7bd5609102..0a65b58dc4 100644
> --- a/xen/arch/arm/mpu/mm.c
> +++ b/xen/arch/arm/mpu/mm.c
> @@ -27,6 +27,10 @@
>   #include <asm/page.h>
>   #include <asm/setup.h>
>
> +/* Override macros from asm/mm.h to make them work with mfn_t */
> +#undef mfn_to_virt
Why do we have to do this ?
> +#define mfn_to_virt(mfn) __mfn_to_virt(mfn_x(mfn))
> +
>   #ifdef NDEBUG
>   static inline void __attribute__ ((__format__ (__printf__, 1, 2)))
>   region_printk(const char *fmt, ...) {}
> @@ -58,6 +62,9 @@ static DEFINE_SPINLOCK(xen_mpumap_lock);
>
>   static DEFINE_SPINLOCK(xen_mpumap_alloc_lock);
>
> +struct page_info *frame_table;
> +unsigned long frametable_pdx_end __read_mostly;
> +
>   /* Write a MPU protection region */
>   #define WRITE_PROTECTION_REGION(pr, prbar_el2, prlar_el2) ({    \
>       const pr_t *_pr = pr;                                       \
> @@ -513,6 +520,26 @@ void __init setup_staticheap_mappings(void)
>       }
>   }
>
> +/* Map a frame table to cover physical addresses ps through pe */
> +void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
> +{
> +    mfn_t base_mfn;
> +    unsigned long nr_pdxs = mfn_to_pdx(mfn_add(maddr_to_mfn(pe), -1)) -
> +                            mfn_to_pdx(maddr_to_mfn(ps)) + 1;
> +    unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
> +
> +    frametable_base_pdx = paddr_to_pdx(ps);
> +    frametable_size = ROUNDUP(frametable_size, PAGE_SIZE);
> +    frametable_pdx_end = frametable_base_pdx + nr_pdxs;
> +
> +    base_mfn = alloc_boot_pages(frametable_size >> PAGE_SHIFT, 1);
> +    frame_table = (struct page_info *)mfn_to_virt(base_mfn);
> +
> +    memset(&frame_table[0], 0, nr_pdxs * sizeof(struct page_info));
> +    memset(&frame_table[nr_pdxs], -1,
> +           frametable_size - (nr_pdxs * sizeof(struct page_info)));
> +}
> +
>   /*
>    * Local variables:
>    * mode: C
> --
> 2.25.1
- Ayan