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

[Penny Zheng <penny.zheng@xxxxxxx>]

Hi Ayan

On 2023/6/30 23:19, Ayan Kumar Halder wrote:
> 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
The reason why I don't put virt_to_page()/page_to_virt() in specific 
header is that we are using some helpers, which are defined just
a few lines before, like mfn_to_virt(), etc.
If you are moving mfn_to_virt() to specific header too, that will
result in a lot dulplication.

> > +#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.
It is a variable only in MPU. In MMU, it is a fixed value.
"#define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)"

> > +extern unsigned long frametable_pdx_end;
> Also you don't need extern as this is only used by xen/arch/arm/mpu/mm.c.
sure.

> > +
> >   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.
I prefer them staying in the common file, using #ifdef to go to the
different path.

Since if not and when anyone wants to update map_pages_to_xen(), 
destroy_xen_mappings() and modify_xen_mappings() in the future, it is 
possible for them to leave changes in only one file.
> >   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