[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 33/52] xen/mpu: initialize frametable in MPU system
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@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)),
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
|
