[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Ayan Kumar Halder <ayankuma@xxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Penny Zheng <penny.zheng@xxxxxxx>
  • Date: Mon, 3 Jul 2023 14:10:27 +0800
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 40.67.248.234) smtp.rcpttodomain=amd.com smtp.mailfrom=arm.com; dmarc=pass (p=none sp=none pct=100) action=none header.from=arm.com; dkim=none (message not signed); arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=5ddtpptgBVErJpYyc7OJBm5OKLGxklqjneMQV/9ZNmc=; b=M/ukbSpRiajz2vEIrsqCgLXP1Wzj8LRkT23vnN9e5VlihRRRWThN3q6Peq0A8adsNGdBDbMjQuKC09mmllUlZ8OBQLRk8b6vZgtPKJ61n22yFcAiSDGhbi8KrNwQ2HT2fZIbwtEGsiNYVdAwK1EgoiScqdAgHdhbaR5fCYy8SpQsAYjJPkCjFTfKTC3urNeNWcvogDdnP0op17NODQUAzsrWU1clSP6TtGqZUaIMK/8qrGOlad5Kr+r4OIHyf7tccKflNUd1W4SlybDL8B02NDiqjXONVThxpCgBBmLC7FGaqcH8FvV5VBb2auFay48XrgXTjPOEMx8UcQ2EsKzFBw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=KTyA86J/FX6FJqcXzTN8eWEC4r0nt17sys+ZT5fM/1VuIVQr1mL045mQ3zqMkuSr5oXzer3FEFyLgYFO4mztKXF/eJwKOT42fHANy2uwWokI0lw8EaKfYBy6pWRpenE6OmwqXFRm2pQMusSCXjh9MC71tqEv2vNd4WJzmfmOpWL7xL8Y5aF5rkhaPbv6lZ2V79tvb//4BdFBE6/2KZpJvE6fGLVK/pnO1JrO8zWzvkSM94Be4RVhDdk14GfBiV8ZqM/p7PZhEoAOX7dXBFTFwxqQb1vlIN8/Ago0nx6C3joAfksIzv0ZWeF3XsBNaqAiwmjbuebKXs3IeQQ89O/Ikg==
  • Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Julien Grall <julien@xxxxxxx>, Bertrand Marquis <bertrand.marquis@xxxxxxx>, "Volodymyr Babchuk" <Volodymyr_Babchuk@xxxxxxxx>, Wei Chen <wei.chen@xxxxxxx>
  • Delivery-date: Mon, 03 Jul 2023 06:11:10 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.