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@arm.com>
Signed-off-by: Wei Chen <wei.chen@arm.com>
---
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;
+}
+#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;
 }
+#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;
+extern unsigned long frametable_pdx_end;
+
 extern int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned int flags);
 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
+#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

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@arm.com>
> Signed-off-by: Wei Chen <wei.chen@arm.com>
> ---
> 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

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@arm.com>
>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>> ---
>> 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

Hi,

On 03/07/2023 07:10, Penny Zheng wrote:
> 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@arm.com>
>>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>>> ---
>>> 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)), 
You are ignoring 'virt' here. Shouldn't you at least check that 'virt == 
mfn_to_maddr(mfn)'?

>> 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.
I don't like the #ifdef solution in this situation. You are only doing 
it for the benefits of the ASSERT(). But they don't seem to have any 
value for xen_mpumap_update() (you are still passing an address rather 
than a frame).

> 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.

The helpers are just wrappers. I doubt they will change in the future. 
So I think it would be OK to duplicate.

The alternative would to have a common prototype for xen_pt_update() and 
xen_mpumap_update() and avoid any #ifdery. That said, this is not my 
preference at least if they are not static inline.

Cheers,

-- 
Julien Grall

Hi，

On 2023/7/3 17:31, Julien Grall wrote:
> Hi,
> 
> On 03/07/2023 07:10, Penny Zheng wrote:
>> 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@arm.com>
>>>> Signed-off-by: Wei Chen <wei.chen@arm.com>
>>>> ---
>>>> 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)), 
> You are ignoring 'virt' here. Shouldn't you at least check that 'virt == 
> mfn_to_maddr(mfn)'?
> 

Sure, I'll do the check.

>>> 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.
> I don't like the #ifdef solution in this situation. You are only doing 
> it for the benefits of the ASSERT(). But they don't seem to have any 
> value for xen_mpumap_update() (you are still passing an address rather 
> than a frame).
> 

Okay， I will pass a page or a valid mfn next version.

>> 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.
> 
> The helpers are just wrappers. I doubt they will change in the future. 
> So I think it would be OK to duplicate.
> 
> The alternative would to have a common prototype for xen_pt_update() and 
> xen_mpumap_update() and avoid any #ifdery. That said, this is not my 
> preference at least if they are not static inline.
> 

Correct me if I'm wrong, you are suggesting something like this:
A more-generic wrapper like xen_mm_update, and we introduce static 
inline implementation in mmu/mm.h with xen_pt_update(), and static
inline implementation in mpu/mm.h with xen_mpumap_update().

> Cheers,
> 

Hi,

On 05/07/2023 10:53, Penny Zheng wrote:
>>> 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.
>>
>> The helpers are just wrappers. I doubt they will change in the future. 
>> So I think it would be OK to duplicate.
>>
>> The alternative would to have a common prototype for xen_pt_update() 
>> and xen_mpumap_update() and avoid any #ifdery. That said, this is not 
>> my preference at least if they are not static inline.
>>
> 
> Correct me if I'm wrong, you are suggesting something like this:
> A more-generic wrapper like xen_mm_update, and we introduce static 
> inline implementation in mmu/mm.h with xen_pt_update(), and static
> inline implementation in mpu/mm.h with xen_mpumap_update().

Yes as an alternative proposal. But my preference here is to duplicate 
the helpers in mm-mmu.c and mm-mpu.c.

Cheers,

-- 
Julien Grall

Hi,

On 2023/7/5 21:52, Julien Grall wrote:
> Hi,
> 
> On 05/07/2023 10:53, Penny Zheng wrote:
>>>> 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.
>>>
>>> The helpers are just wrappers. I doubt they will change in the 
>>> future. So I think it would be OK to duplicate.
>>>
>>> The alternative would to have a common prototype for xen_pt_update() 
>>> and xen_mpumap_update() and avoid any #ifdery. That said, this is not 
>>> my preference at least if they are not static inline.
>>>
>>
>> Correct me if I'm wrong, you are suggesting something like this:
>> A more-generic wrapper like xen_mm_update, and we introduce static 
>> inline implementation in mmu/mm.h with xen_pt_update(), and static
>> inline implementation in mpu/mm.h with xen_mpumap_update().
> 
> Yes as an alternative proposal. But my preference here is to duplicate 
> the helpers in mm-mmu.c and mm-mpu.c.
> 

Understood, I'll do the duplication.

> Cheers,
>