| Message ID | 20230626033443.2943270-34-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Add Armv8-R64 MPU support to Xen - Part#1 | expand |
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,
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,
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, >
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