[PATCH V4 0/6] populate/unpopulate memory when domain on static

[PATCH V4 0/6] populate/unpopulate memory when domain on static

[Penny Zheng]

Today when a domain unpopulates the memory on runtime, they will always
hand the memory over to the heap allocator. And it will be a problem if it
is a static domain.
Pages used as guest RAM for static domain shall always be reserved to this
domain only, and not be used for any other purposes, so they shall never go
back to heap allocator.

This patch serie intends to fix this issue, by adding pages on the new list
resv_page_list after having taken them off the "normal" list, when unpopulating
memory, and retrieving pages from resv page list(resv_page_list) when
populating memory.

---
v4 changes:
- commit message refinement
- miss dropping __init in acquire_domstatic_pages
- add the page back to the reserved list in case of error
- remove redundant printk
- refine log message and make it warn level
---
v3 changes:
- fix possible racy issue in free_staticmem_pages()
- introduce a stub free_staticmem_pages() for the !CONFIG_STATIC_MEMORY case
- move the change to free_heap_pages() to cover other potential call sites
- change fixed width type uint32_t to unsigned int
- change "flags" to a more descriptive name "cdf"
- change name from "is_domain_static()" to "is_domain_using_staticmem"
- have page_list_del() just once out of the if()
- remove resv_pages counter
- make arch_free_heap_page be an expression, not a compound statement.
- move #ifndef is_domain_using_staticmem to the common header file
- remove #ifdef CONFIG_STATIC_MEMORY-ary
- remove meaningless page_to_mfn(page) in error log
---
v2 changes:
- let "flags" live in the struct domain. So other arch can take
advantage of it in the future
- change name from "is_domain_on_static_allocation" to "is_domain_static()"
- put reserved pages on resv_page_list after having taken them off
the "normal" list
- introduce acquire_reserved_page to retrieve reserved pages from
resv_page_list
- forbid non-zero-order requests in populate_physmap
- let is_domain_static return ((void)(d), false) on x86
- fix coding style

Penny Zheng (6):
  xen: do not free reserved memory into heap
  xen: do not merge reserved pages in free_heap_pages()
  xen: add field "flags" to cover all internal CDF_XXX
  xen/arm: introduce CDF_staticmem
  xen/arm: unpopulate memory when domain is static
  xen: retrieve reserved pages on populate_physmap

 xen/arch/arm/domain.c             |  2 --
 xen/arch/arm/domain_build.c       |  5 ++-
 xen/arch/arm/include/asm/domain.h |  5 +--
 xen/arch/arm/include/asm/mm.h     | 12 +++++++
 xen/common/domain.c               |  7 ++++
 xen/common/memory.c               | 23 +++++++++++++
 xen/common/page_alloc.c           | 54 ++++++++++++++++++++++++++++---
 xen/include/xen/domain.h          |  6 ++++
 xen/include/xen/mm.h              |  3 +-
 xen/include/xen/sched.h           |  6 ++++
 10 files changed, 112 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCH v4 1/6] xen: do not free reserved memory into heap

[Penny Zheng]

Pages used as guest RAM for static domain, shall be reserved to this
domain only.
So in case reserved pages being used for other purpose, users
shall not free them back to heap, even when last ref gets dropped.

free_staticmem_pages will be called by free_heap_pages in runtime
for static domain freeing memory resource, so let's drop the __init
flag.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
v4 changes:
- no changes
---
v3 changes:
- fix possible racy issue in free_staticmem_pages()
- introduce a stub free_staticmem_pages() for the !CONFIG_STATIC_MEMORY case
- move the change to free_heap_pages() to cover other potential call sites
- fix the indentation
---
v2 changes:
- new commit
---
 xen/common/page_alloc.c | 17 ++++++++++++++---
 xen/include/xen/mm.h    |  2 +-
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 319029140f..5e569a48a2 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1443,6 +1443,10 @@ static void free_heap_pages(
 
     ASSERT(order <= MAX_ORDER);
 
+    if ( pg->count_info & PGC_reserved )
+        /* Reserved page shall not go back to the heap. */
+        return free_staticmem_pages(pg, 1UL << order, need_scrub);
+
     spin_lock(&heap_lock);
 
     for ( i = 0; i < (1 << order); i++ )
@@ -2636,8 +2640,8 @@ struct domain *get_pg_owner(domid_t domid)
 
 #ifdef CONFIG_STATIC_MEMORY
 /* Equivalent of free_heap_pages to free nr_mfns pages of static memory. */
-void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
-                                 bool need_scrub)
+void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
+                          bool need_scrub)
 {
     mfn_t mfn = page_to_mfn(pg);
     unsigned long i;
@@ -2653,7 +2657,8 @@ void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
         }
 
         /* In case initializing page of static memory, mark it PGC_reserved. */
-        pg[i].count_info |= PGC_reserved;
+        if ( !(pg[i].count_info & PGC_reserved) )
+            pg[i].count_info |= PGC_reserved;
     }
 }
 
@@ -2762,6 +2767,12 @@ int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
 
     return 0;
 }
+#else
+void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
+                          bool need_scrub)
+{
+    ASSERT_UNREACHABLE();
+}
 #endif
 
 /*
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 3be754da92..9fd95deaec 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -85,10 +85,10 @@ bool scrub_free_pages(void);
 } while ( false )
 #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
 
-#ifdef CONFIG_STATIC_MEMORY
 /* These functions are for static memory */
 void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
                           bool need_scrub);
+#ifdef CONFIG_STATIC_MEMORY
 int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int nr_mfns,
                             unsigned int memflags);
 #endif
-- 
2.25.1

[PATCH v4 2/6] xen: do not merge reserved pages in free_heap_pages()

[Penny Zheng]

The code in free_heap_pages() will try to merge pages with the
successor/predecessor if pages are suitably aligned. So if the pages
reserved are right next to the pages given to the heap allocator,
free_heap_pages() will merge them, and give the reserved pages to heap
allocator accidently as a result.

So in order to avoid the above scenario, this commit updates free_heap_pages()
to check whether the predecessor and/or successor has PGC_reserved set,
when trying to merge the about-to-be-freed chunk with the predecessor
and/or successor.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Suggested-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v4 changes:
- commit message refinement
---
v3 changes:
- no changes
---
v2 changes:
- new commit
---
 xen/common/page_alloc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 5e569a48a2..290526adaf 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1483,6 +1483,7 @@ static void free_heap_pages(
             /* Merge with predecessor block? */
             if ( !mfn_valid(page_to_mfn(predecessor)) ||
                  !page_state_is(predecessor, free) ||
+                 (predecessor->count_info & PGC_reserved) ||
                  (PFN_ORDER(predecessor) != order) ||
                  (phys_to_nid(page_to_maddr(predecessor)) != node) )
                 break;
@@ -1506,6 +1507,7 @@ static void free_heap_pages(
             /* Merge with successor block? */
             if ( !mfn_valid(page_to_mfn(successor)) ||
                  !page_state_is(successor, free) ||
+                 (successor->count_info & PGC_reserved) ||
                  (PFN_ORDER(successor) != order) ||
                  (phys_to_nid(page_to_maddr(successor)) != node) )
                 break;
-- 
2.25.1

[PATCH v4 3/6] xen: add field "flags" to cover all internal CDF_XXX

[Penny Zheng]

With more and more CDF_xxx internal flags in and to save the space, this
commit introduces a new field "flags" in struct domain to store CDF_*
internal flags directly.

Another new CDF_xxx will be introduced in the next patch.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
v4 changes:
- no change
---
v3 changes:
- change fixed width type uint32_t to unsigned int
- change "flags" to a more descriptive name "cdf"
---
v2 changes:
- let "flags" live in the struct domain. So other arch can take
advantage of it in the future
- fix coding style
---
 xen/arch/arm/domain.c             | 2 --
 xen/arch/arm/include/asm/domain.h | 3 +--
 xen/common/domain.c               | 3 +++
 xen/include/xen/sched.h           | 3 +++
 4 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 8110c1df86..74189d9878 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -709,8 +709,6 @@ int arch_domain_create(struct domain *d,
     ioreq_domain_init(d);
 #endif
 
-    d->arch.directmap = flags & CDF_directmap;
-
     /* p2m_init relies on some value initialized by the IOMMU subsystem */
     if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
         goto fail;
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index ed63c2b6f9..fe7a029ebf 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -29,7 +29,7 @@ enum domain_type {
 #define is_64bit_domain(d) (0)
 #endif
 
-#define is_domain_direct_mapped(d) (d)->arch.directmap
+#define is_domain_direct_mapped(d) ((d)->cdf & CDF_directmap)
 
 /*
  * Is the domain using the host memory layout?
@@ -103,7 +103,6 @@ struct arch_domain
     void *tee;
 #endif
 
-    bool directmap;
 }  __cacheline_aligned;
 
 struct arch_vcpu
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 8d2c2a9897..6373407047 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -567,6 +567,9 @@ struct domain *domain_create(domid_t domid,
     /* Sort out our idea of is_system_domain(). */
     d->domain_id = domid;
 
+    /* Holding CDF_* internal flags. */
+    d->cdf = flags;
+
     /* Debug sanity. */
     ASSERT(is_system_domain(d) ? config == NULL : config != NULL);
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index ed8539f6d2..49415a113a 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -591,6 +591,9 @@ struct domain
         struct ioreq_server     *server[MAX_NR_IOREQ_SERVERS];
     } ioreq_server;
 #endif
+
+    /* Holding CDF_* constant. Internal flags for domain creation. */
+    unsigned int cdf;
 };
 
 static inline struct page_list_head *page_to_list(
-- 
2.25.1

[PATCH v4 4/6] xen/arm: introduce CDF_staticmem

[Penny Zheng]

In order to have an easy and quick way to find out whether this domain memory
is statically configured, this commit introduces a new flag CDF_staticmem and a
new helper is_domain_using_staticmem() to tell.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v4 changes:
- no changes
---
v3 changes:
- change name from "is_domain_static()" to "is_domain_using_staticmem"
---
v2 changes:
- change name from "is_domain_on_static_allocation" to "is_domain_static()"
---
 xen/arch/arm/domain_build.c       | 5 ++++-
 xen/arch/arm/include/asm/domain.h | 2 ++
 xen/include/xen/domain.h          | 2 ++
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 1472ca4972..6830a282a0 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3190,9 +3190,12 @@ void __init create_domUs(void)
         if ( !dt_device_is_compatible(node, "xen,domain") )
             continue;
 
+        if ( dt_find_property(node, "xen,static-mem", NULL) )
+            flags |= CDF_staticmem;
+
         if ( dt_property_read_bool(node, "direct-map") )
         {
-            if ( !IS_ENABLED(CONFIG_STATIC_MEMORY) || !dt_find_property(node, "xen,static-mem", NULL) )
+            if ( !IS_ENABLED(CONFIG_STATIC_MEMORY) || !(flags & CDF_staticmem) )
                 panic("direct-map is not valid for domain %s without static allocation.\n",
                       dt_node_name(node));
 
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index fe7a029ebf..110c672589 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -31,6 +31,8 @@ enum domain_type {
 
 #define is_domain_direct_mapped(d) ((d)->cdf & CDF_directmap)
 
+#define is_domain_using_staticmem(d) ((d)->cdf & CDF_staticmem)
+
 /*
  * Is the domain using the host memory layout?
  *
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 1c3c88a14d..35dc7143a4 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -34,6 +34,8 @@ void arch_get_domain_info(const struct domain *d,
 #ifdef CONFIG_ARM
 /* Should domain memory be directly mapped? */
 #define CDF_directmap            (1U << 1)
+/* Is domain memory on static allocation? */
+#define CDF_staticmem            (1U << 2)
 #endif
 
 /*
-- 
2.25.1

[PATCH v4 5/6] xen/arm: unpopulate memory when domain is static

[Penny Zheng]

Today when a domain unpopulates the memory on runtime, they will always
hand the memory back to the heap allocator. And it will be a problem if domain
is static.

Pages as guest RAM for static domain shall be reserved to only this domain
and not be used for any other purposes, so they shall never go back to heap
allocator.

This commit puts reserved pages on the new list resv_page_list only after
having taken them off the "normal" list, when the last ref dropped.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
v4 changes:
- no changes
---
v3 changes:
- have page_list_del() just once out of the if()
- remove resv_pages counter
- make arch_free_heap_page be an expression, not a compound statement.
---
v2 changes:
- put reserved pages on resv_page_list after having taken them off
the "normal" list
---
 xen/arch/arm/include/asm/mm.h | 12 ++++++++++++
 xen/common/domain.c           |  4 ++++
 xen/include/xen/sched.h       |  3 +++
 3 files changed, 19 insertions(+)

diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index 424aaf2823..c6426c1705 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -358,6 +358,18 @@ void clear_and_clean_page(struct page_info *page);
 
 unsigned int arch_get_dma_bitsize(void);
 
+/*
+ * Put free pages on the resv page list after having taken them
+ * off the "normal" page list, when pages from static memory
+ */
+#ifdef CONFIG_STATIC_MEMORY
+#define arch_free_heap_page(d, pg) ({                   \
+    page_list_del(pg, page_to_list(d, pg));             \
+    if ( (pg)->count_info & PGC_reserved )              \
+        page_list_add_tail(pg, &(d)->resv_page_list);   \
+})
+#endif
+
 #endif /*  __ARCH_ARM_MM__ */
 /*
  * Local variables:
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 6373407047..13fe7cecff 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -604,6 +604,10 @@ struct domain *domain_create(domid_t domid,
     INIT_PAGE_LIST_HEAD(&d->page_list);
     INIT_PAGE_LIST_HEAD(&d->extra_page_list);
     INIT_PAGE_LIST_HEAD(&d->xenpage_list);
+#ifdef CONFIG_STATIC_MEMORY
+    INIT_PAGE_LIST_HEAD(&d->resv_page_list);
+#endif
+
 
     spin_lock_init(&d->node_affinity_lock);
     d->node_affinity = NODE_MASK_ALL;
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 49415a113a..368e5c1c53 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -376,6 +376,9 @@ struct domain
     struct page_list_head page_list;  /* linked list */
     struct page_list_head extra_page_list; /* linked list (size extra_pages) */
     struct page_list_head xenpage_list; /* linked list (size xenheap_pages) */
+#ifdef CONFIG_STATIC_MEMORY
+    struct page_list_head resv_page_list; /* linked list (size resv_pages) */
+#endif
 
     /*
      * This field should only be directly accessed by domain_adjust_tot_pages()
-- 
2.25.1

[PATCH v4 6/6] xen: retrieve reserved pages on populate_physmap

[Penny Zheng]

When static domain populates memory through populate_physmap on runtime,
other than allocating from heap, it shall retrieve reserved pages from
resv_page_list to make sure that guest RAM is still restricted in statically
configured memory regions. And this commit introduces a new helper
acquire_reserved_page to make it work.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
v4 changes：
- miss dropping __init in acquire_domstatic_pages
- add the page back to the reserved list in case of error
- remove redundant printk
- refine log message and make it warn level
---
v3 changes:
- move is_domain_using_staticmem to the common header file
- remove #ifdef CONFIG_STATIC_MEMORY-ary
- remove meaningless page_to_mfn(page) in error log
---
v2 changes:
- introduce acquire_reserved_page to retrieve reserved pages from
resv_page_list
- forbid non-zero-order requests in populate_physmap
- let is_domain_static return ((void)(d), false) on x86
---
 xen/common/memory.c      | 23 +++++++++++++++++++++++
 xen/common/page_alloc.c  | 35 +++++++++++++++++++++++++++++++++--
 xen/include/xen/domain.h |  4 ++++
 xen/include/xen/mm.h     |  1 +
 4 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index f2d009843a..cb330ce877 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -245,6 +245,29 @@ static void populate_physmap(struct memop_args *a)
 
                 mfn = _mfn(gpfn);
             }
+            else if ( is_domain_using_staticmem(d) )
+            {
+                /*
+                 * No easy way to guarantee the retrieved pages are contiguous,
+                 * so forbid non-zero-order requests here.
+                 */
+                if ( a->extent_order != 0 )
+                {
+                    gdprintk(XENLOG_WARNING,
+                             "Cannot allocate static order-%u pages for static %pd\n",
+                             a->extent_order, d);
+                    goto out;
+                }
+
+                mfn = acquire_reserved_page(d, a->memflags);
+                if ( mfn_eq(mfn, INVALID_MFN) )
+                {
+                    gdprintk(XENLOG_WARNING,
+                             "%pd: failed to retrieve a reserved page\n",
+                             d);
+                    goto out;
+                }
+            }
             else
             {
                 page = alloc_domheap_pages(d, a->extent_order, a->memflags);
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 290526adaf..06e7037a28 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2740,8 +2740,8 @@ static struct page_info * __init acquire_staticmem_pages(mfn_t smfn,
  * Acquire nr_mfns contiguous pages, starting at #smfn, of static memory,
  * then assign them to one specific domain #d.
  */
-int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
-                                   unsigned int nr_mfns, unsigned int memflags)
+int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int nr_mfns,
+                            unsigned int memflags)
 {
     struct page_info *pg;
 
@@ -2769,12 +2769,43 @@ int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
 
     return 0;
 }
+
+/*
+ * Acquire a page from reserved page list(resv_page_list), when populating
+ * memory for static domain on runtime.
+ */
+mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
+{
+    struct page_info *page;
+    mfn_t smfn;
+
+    /* Acquire a page from reserved page list(resv_page_list). */
+    page = page_list_remove_head(&d->resv_page_list);
+    if ( unlikely(!page) )
+        return INVALID_MFN;
+
+    smfn = page_to_mfn(page);
+
+    if ( acquire_domstatic_pages(d, smfn, 1, memflags) )
+    {
+        page_list_add_tail(page, &d->resv_page_list);
+        return INVALID_MFN;
+    }
+
+    return smfn;
+}
 #else
 void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
                           bool need_scrub)
 {
     ASSERT_UNREACHABLE();
 }
+
+mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
+{
+    ASSERT_UNREACHABLE();
+    return INVALID_MFN;
+}
 #endif
 
 /*
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 35dc7143a4..c613afa57e 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -38,6 +38,10 @@ void arch_get_domain_info(const struct domain *d,
 #define CDF_staticmem            (1U << 2)
 #endif
 
+#ifndef is_domain_using_staticmem
+#define is_domain_using_staticmem(d) ((void)(d), false)
+#endif
+
 /*
  * Arch-specifics.
  */
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 9fd95deaec..74810e1f54 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -92,6 +92,7 @@ void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
 int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int nr_mfns,
                             unsigned int memflags);
 #endif
+mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags);
 
 /* Map machine page range in Xen virtual address space. */
 int map_pages_to_xen(
-- 
2.25.1

Re: [PATCH v4 2/6] xen: do not merge reserved pages in free_heap_pages()

[Julien Grall]

Hi Penny,

On 10/05/2022 03:27, Penny Zheng wrote:
> The code in free_heap_pages() will try to merge pages with the
> successor/predecessor if pages are suitably aligned. So if the pages
> reserved are right next to the pages given to the heap allocator,
> free_heap_pages() will merge them, and give the reserved pages to heap
> allocator accidently as a result.
> 
> So in order to avoid the above scenario, this commit updates free_heap_pages()
> to check whether the predecessor and/or successor has PGC_reserved set,
> when trying to merge the about-to-be-freed chunk with the predecessor
> and/or successor.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Suggested-by: Julien Grall <jgrall@amazon.com>

NIT: In general, the tags are historically ordered. I.e I first sugested 
and then you wrote the patch. So the two tags should be inverted.

This can be done on commit:

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v4 1/6] xen: do not free reserved memory into heap

[Julien Grall]

Hi Penny,

On 10/05/2022 03:27, Penny Zheng wrote:
> Pages used as guest RAM for static domain, shall be reserved to this
> domain only.
> So in case reserved pages being used for other purpose, users
> shall not free them back to heap, even when last ref gets dropped.
> 
> free_staticmem_pages will be called by free_heap_pages in runtime
> for static domain freeing memory resource, so let's drop the __init
> flag.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> v4 changes:
> - no changes
> ---
> v3 changes:
> - fix possible racy issue in free_staticmem_pages()
> - introduce a stub free_staticmem_pages() for the !CONFIG_STATIC_MEMORY case
> - move the change to free_heap_pages() to cover other potential call sites
> - fix the indentation
> ---
> v2 changes:
> - new commit
> ---
>   xen/common/page_alloc.c | 17 ++++++++++++++---
>   xen/include/xen/mm.h    |  2 +-
>   2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 319029140f..5e569a48a2 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1443,6 +1443,10 @@ static void free_heap_pages(
>   
>       ASSERT(order <= MAX_ORDER);
>   
> +    if ( pg->count_info & PGC_reserved )

NIT: I would suggest to use "unlikely()" here.

> +        /* Reserved page shall not go back to the heap. */
> +        return free_staticmem_pages(pg, 1UL << order, need_scrub);
> +
>       spin_lock(&heap_lock);
>   
>       for ( i = 0; i < (1 << order); i++ )
> @@ -2636,8 +2640,8 @@ struct domain *get_pg_owner(domid_t domid)
>   
>   #ifdef CONFIG_STATIC_MEMORY
>   /* Equivalent of free_heap_pages to free nr_mfns pages of static memory. */
> -void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> -                                 bool need_scrub)
> +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> +                          bool need_scrub)

Looking at the implementation of free_staticmem_pages(), the page will 
be scrubbed synchronously.

If I am not mistaken, static memory is not yet supported so I would be 
OK to continue with synchronous scrubbing. However, this will need to be 
asynchronous before we even consider to security support it.

BTW, SUPPORT.md doesn't seem to explicitely say whether static memory is 
supported. Would you be able to send a patch to update it? I think this 
should be tech preview for now.

>   {
>       mfn_t mfn = page_to_mfn(pg);
>       unsigned long i;
> @@ -2653,7 +2657,8 @@ void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
>           }
>   
>           /* In case initializing page of static memory, mark it PGC_reserved. */
> -        pg[i].count_info |= PGC_reserved;
> +        if ( !(pg[i].count_info & PGC_reserved) )

NIT: I understand the flag may have already been set, but I am not 
convinced if it is worth checking it and then set.

> +            pg[i].count_info |= PGC_reserved;


>       }
>   }
>   
> @@ -2762,6 +2767,12 @@ int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
>   
>       return 0;
>   }
> +#else
> +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> +                          bool need_scrub)
> +{
> +    ASSERT_UNREACHABLE();
> +}
>   #endif
>   
>   /*
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 3be754da92..9fd95deaec 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -85,10 +85,10 @@ bool scrub_free_pages(void);
>   } while ( false )
>   #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
>   
> -#ifdef CONFIG_STATIC_MEMORY
>   /* These functions are for static memory */
>   void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
>                             bool need_scrub);
> +#ifdef CONFIG_STATIC_MEMORY
>   int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int nr_mfns,
>                               unsigned int memflags);
>   #endif

Cheers,

-- 
Julien Grall

Re: [PATCH v4 6/6] xen: retrieve reserved pages on populate_physmap

[Julien Grall]

Hi Penny,

On 10/05/2022 03:27, Penny Zheng wrote:
> When static domain populates memory through populate_physmap on runtime,

Typo: s/when static/when a static/ or "when static domains populate"

s/on runtime/at runtime/

> other than allocating from heap, it shall retrieve reserved pages from

I am not sure to understand the part before the comma. But it doens't 
sound necessary so maybe drop it?

> resv_page_list to make sure that guest RAM is still restricted in statically
> configured memory regions. And this commit introduces a new helper
> acquire_reserved_page to make it work.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>

[...]

> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 290526adaf..06e7037a28 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2740,8 +2740,8 @@ static struct page_info * __init acquire_staticmem_pages(mfn_t smfn,
>    * Acquire nr_mfns contiguous pages, starting at #smfn, of static memory,
>    * then assign them to one specific domain #d.
>    */
> -int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
> -                                   unsigned int nr_mfns, unsigned int memflags)
> +int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int nr_mfns,
> +                            unsigned int memflags)
>   {
>       struct page_info *pg;
>   
> @@ -2769,12 +2769,43 @@ int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
>   
>       return 0;
>   }
> +
> +/*
> + * Acquire a page from reserved page list(resv_page_list), when populating
> + * memory for static domain on runtime.
> + */
> +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
> +{
> +    struct page_info *page;
> +    mfn_t smfn;
> +
> +    /* Acquire a page from reserved page list(resv_page_list). */
> +    page = page_list_remove_head(&d->resv_page_list);
Alloc/free of memory can happen concurrently. So access to rsv_page_list 
needs to be protected with a spinlock (mostly like d->page_alloc_lock).

> +    if ( unlikely(!page) )
> +        return INVALID_MFN;
> +
> +    smfn = page_to_mfn(page);
> +
> +    if ( acquire_domstatic_pages(d, smfn, 1, memflags) )

I am OK if we call acquire_domstatic_pages() for now. But long term, I 
think we should consider to optimize it because we know the page is 
valid and belong to the guest. So there are a lot of pointless work 
(checking mfn_valid(), scrubbing in the free part, cleaning the cache...).

> +    {
> +        page_list_add_tail(page, &d->resv_page_list);
> +        return INVALID_MFN;
> +    }
> +
> +    return smfn;
> +}
>   #else
>   void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
>                             bool need_scrub)
>   {
>       ASSERT_UNREACHABLE();
>   }
> +
> +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
> +{
> +    ASSERT_UNREACHABLE();
> +    return INVALID_MFN;
> +}
>   #endif
>   
>   /*
> diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
> index 35dc7143a4..c613afa57e 100644
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -38,6 +38,10 @@ void arch_get_domain_info(const struct domain *d,
>   #define CDF_staticmem            (1U << 2)
>   #endif
>   
> +#ifndef is_domain_using_staticmem
> +#define is_domain_using_staticmem(d) ((void)(d), false)
> +#endif
> +
>   /*
>    * Arch-specifics.
>    */
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index 9fd95deaec..74810e1f54 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -92,6 +92,7 @@ void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
>   int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int nr_mfns,
>                               unsigned int memflags);
>   #endif
> +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags);
>   
>   /* Map machine page range in Xen virtual address space. */
>   int map_pages_to_xen(

Cheers,

-- 
Julien Grall

RE: [PATCH v4 6/6] xen: retrieve reserved pages on populate_physmap

[Penny Zheng]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Tuesday, May 17, 2022 2:29 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Jan Beulich <jbeulich@suse.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v4 6/6] xen: retrieve reserved pages on populate_physmap
> 
> Hi Penny,
> 
> On 10/05/2022 03:27, Penny Zheng wrote:
> > When static domain populates memory through populate_physmap on
> > runtime,
> 
> Typo: s/when static/when a static/ or "when static domains populate"
> 
> s/on runtime/at runtime/
> 

Sure, 

> > other than allocating from heap, it shall retrieve reserved pages from
> 
> I am not sure to understand the part before the comma. But it doens't sound
> necessary so maybe drop it?
>  

Sure,

> > resv_page_list to make sure that guest RAM is still restricted in
> > statically configured memory regions. And this commit introduces a new
> > helper acquire_reserved_page to make it work.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> 
> [...]
> 
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index
> > 290526adaf..06e7037a28 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2740,8 +2740,8 @@ static struct page_info * __init
> acquire_staticmem_pages(mfn_t smfn,
> >    * Acquire nr_mfns contiguous pages, starting at #smfn, of static memory,
> >    * then assign them to one specific domain #d.
> >    */
> > -int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
> > -                                   unsigned int nr_mfns, unsigned int memflags)
> > +int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int
> nr_mfns,
> > +                            unsigned int memflags)
> >   {
> >       struct page_info *pg;
> >
> > @@ -2769,12 +2769,43 @@ int __init acquire_domstatic_pages(struct
> > domain *d, mfn_t smfn,
> >
> >       return 0;
> >   }
> > +
> > +/*
> > + * Acquire a page from reserved page list(resv_page_list), when
> > +populating
> > + * memory for static domain on runtime.
> > + */
> > +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
> > +{
> > +    struct page_info *page;
> > +    mfn_t smfn;
> > +
> > +    /* Acquire a page from reserved page list(resv_page_list). */
> > +    page = page_list_remove_head(&d->resv_page_list);
> Alloc/free of memory can happen concurrently. So access to rsv_page_list
> needs to be protected with a spinlock (mostly like d->page_alloc_lock).
>

Oh, understood, will fix.
 
> > +    if ( unlikely(!page) )
> > +        return INVALID_MFN;
> > +
> > +    smfn = page_to_mfn(page);
> > +
> > +    if ( acquire_domstatic_pages(d, smfn, 1, memflags) )
> 
> I am OK if we call acquire_domstatic_pages() for now. But long term, I think we
> should consider to optimize it because we know the page is valid and belong
> to the guest. So there are a lot of pointless work (checking mfn_valid(),
> scrubbing in the free part, cleaning the cache...).
> 

I'm willing to fix it here since this fix is not blocking any other patch serie~~
I'm considering that maybe we could add a new memflag MEMF_xxx, (oh,
Naming something is really "killing" me), then all these pointless work, checking
mfn_valid, flushing TLB and cache, we could exclude them by checking
memflags & MEMF_xxxx.
Wdyt?

> > +    {
> > +        page_list_add_tail(page, &d->resv_page_list);
> > +        return INVALID_MFN;
> > +    }
> > +
> > +    return smfn;
> > +}
> >   #else
> >   void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> >                             bool need_scrub)
> >   {
> >       ASSERT_UNREACHABLE();
> >   }
> > +
> > +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
> > +{
> > +    ASSERT_UNREACHABLE();
> > +    return INVALID_MFN;
> > +}
> >   #endif
> >
> >   /*
> > diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h index
> > 35dc7143a4..c613afa57e 100644
> > --- a/xen/include/xen/domain.h
> > +++ b/xen/include/xen/domain.h
> > @@ -38,6 +38,10 @@ void arch_get_domain_info(const struct domain *d,
> >   #define CDF_staticmem            (1U << 2)
> >   #endif
> >
> > +#ifndef is_domain_using_staticmem
> > +#define is_domain_using_staticmem(d) ((void)(d), false) #endif
> > +
> >   /*
> >    * Arch-specifics.
> >    */
> > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index
> > 9fd95deaec..74810e1f54 100644
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -92,6 +92,7 @@ void free_staticmem_pages(struct page_info *pg,
> unsigned long nr_mfns,
> >   int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int
> nr_mfns,
> >                               unsigned int memflags);
> >   #endif
> > +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags);
> >
> >   /* Map machine page range in Xen virtual address space. */
> >   int map_pages_to_xen(
> 
> Cheers,
> 
> --
> Julien Grall

RE: [PATCH v4 1/6] xen: do not free reserved memory into heap

[Penny Zheng]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Tuesday, May 17, 2022 2:01 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Jan Beulich <jbeulich@suse.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v4 1/6] xen: do not free reserved memory into heap
> 
> Hi Penny,
> 
> On 10/05/2022 03:27, Penny Zheng wrote:
> > Pages used as guest RAM for static domain, shall be reserved to this
> > domain only.
> > So in case reserved pages being used for other purpose, users shall
> > not free them back to heap, even when last ref gets dropped.
> >
> > free_staticmem_pages will be called by free_heap_pages in runtime for
> > static domain freeing memory resource, so let's drop the __init flag.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > v4 changes:
> > - no changes
> > ---
> > v3 changes:
> > - fix possible racy issue in free_staticmem_pages()
> > - introduce a stub free_staticmem_pages() for the
> > !CONFIG_STATIC_MEMORY case
> > - move the change to free_heap_pages() to cover other potential call
> > sites
> > - fix the indentation
> > ---
> > v2 changes:
> > - new commit
> > ---
> >   xen/common/page_alloc.c | 17 ++++++++++++++---
> >   xen/include/xen/mm.h    |  2 +-
> >   2 files changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index
> > 319029140f..5e569a48a2 100644
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -1443,6 +1443,10 @@ static void free_heap_pages(
> >
> >       ASSERT(order <= MAX_ORDER);
> >
> > +    if ( pg->count_info & PGC_reserved )
> 
> NIT: I would suggest to use "unlikely()" here.
> 
> > +        /* Reserved page shall not go back to the heap. */
> > +        return free_staticmem_pages(pg, 1UL << order, need_scrub);
> > +
> >       spin_lock(&heap_lock);
> >
> >       for ( i = 0; i < (1 << order); i++ ) @@ -2636,8 +2640,8 @@
> > struct domain *get_pg_owner(domid_t domid)
> >
> >   #ifdef CONFIG_STATIC_MEMORY
> >   /* Equivalent of free_heap_pages to free nr_mfns pages of static
> > memory. */ -void __init free_staticmem_pages(struct page_info *pg,
> unsigned long nr_mfns,
> > -                                 bool need_scrub)
> > +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> > +                          bool need_scrub)
> 
> Looking at the implementation of free_staticmem_pages(), the page will be
> scrubbed synchronously.
> 
> If I am not mistaken, static memory is not yet supported so I would be OK to
> continue with synchronous scrubbing. However, this will need to be
> asynchronous before we even consider to security support it.
> 

Yes,  I remembered that asynchronous is still on the to-do list for static memory.

If it doesn't bother too much to you, I would like to ask some help on this issue, ;).
I only knew basic knowledge on the scrubbing, I knew that dirty pages is placed at the
end of list heap(node, zone, order) for scrubbing and "first_dirty" is used to track down
the dirty pages. IMO, Both two parts are restricted to the heap thingy,  not reusable for
static memory, so maybe I need to re-write scrub_free_page for static memory, and also
link the need-to-scrub reserved pages to a new global list e.g.  dirty_resv_list for aync
scrubbing?
 Any suggestions?

> BTW, SUPPORT.md doesn't seem to explicitely say whether static memory is
> supported. Would you be able to send a patch to update it? I think this should
> be tech preview for now.
> 

Sure, will do.

> >   {
> >       mfn_t mfn = page_to_mfn(pg);
> >       unsigned long i;
> > @@ -2653,7 +2657,8 @@ void __init free_staticmem_pages(struct page_info
> *pg, unsigned long nr_mfns,
> >           }
> >
> >           /* In case initializing page of static memory, mark it PGC_reserved. */
> > -        pg[i].count_info |= PGC_reserved;
> > +        if ( !(pg[i].count_info & PGC_reserved) )
> 
> NIT: I understand the flag may have already been set, but I am not convinced if
> it is worth checking it and then set.
> 

Jan suggested that since we remove the __init from free_staticmem_pages, it's now in preemptable
state at runtime, so better be adding this check here. 

> > +            pg[i].count_info |= PGC_reserved;
> 
> 
> >       }
> >   }
> >
> > @@ -2762,6 +2767,12 @@ int __init acquire_domstatic_pages(struct
> > domain *d, mfn_t smfn,
> >
> >       return 0;
> >   }
> > +#else
> > +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> > +                          bool need_scrub) {
> > +    ASSERT_UNREACHABLE();
> > +}
> >   #endif
> >
> >   /*
> > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index
> > 3be754da92..9fd95deaec 100644
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -85,10 +85,10 @@ bool scrub_free_pages(void);
> >   } while ( false )
> >   #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
> >
> > -#ifdef CONFIG_STATIC_MEMORY
> >   /* These functions are for static memory */
> >   void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> >                             bool need_scrub);
> > +#ifdef CONFIG_STATIC_MEMORY
> >   int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int
> nr_mfns,
> >                               unsigned int memflags);
> >   #endif
> 
> Cheers,
> 
> --
> Julien Grall

Re: [PATCH v4 6/6] xen: retrieve reserved pages on populate_physmap

[Julien Grall]

On 17/05/2022 07:24, Penny Zheng wrote:
> Hi Julien

Hi Penny,

>>> +    if ( unlikely(!page) )
>>> +        return INVALID_MFN;
>>> +
>>> +    smfn = page_to_mfn(page);
>>> +
>>> +    if ( acquire_domstatic_pages(d, smfn, 1, memflags) )
>>
>> I am OK if we call acquire_domstatic_pages() for now. But long term, I think we
>> should consider to optimize it because we know the page is valid and belong
>> to the guest. So there are a lot of pointless work (checking mfn_valid(),
>> scrubbing in the free part, cleaning the cache...).
>>
> 
> I'm willing to fix it here since this fix is not blocking any other patch serie~~
> I'm considering that maybe we could add a new memflag MEMF_xxx, (oh,
> Naming something is really "killing" me), then all these pointless work, checking
> mfn_valid, flushing TLB and cache, we could exclude them by checking
> memflags & MEMF_xxxx.
> Wdyt?

I don't think we need a new flag because the decision is internal to the 
page allocator. Instead, acquire_staticmem_pages() could be split in two 
parts. Something like (function names are random):


static bool foo(struct page_info *pg,
	        unsigned long nr,
	        unsigned long memflags)
{
	spin_lock(&heap_lock);

	for ( i = 0; i < nr; i++ )
		...

	spin_unlock(&heap_lock);

	if ( need_tlbflush )
	  filtered...

  	return true;

out_err:
	for ( ... )
	  ...
	return false;
}

static struct page_info *bar(mfn_t smfn,
			     unsigned long mfn,
			     unsigned int memflags)
{
	ASSERT(nr_mfns);
	for ( i = 0; i < nr_mfns; i++ )
	    if ( !mfn_valid(mfn_add(smfn, i)) )
		return NULL;

	pg = mfn_to_page(mfn);
	if ( !foo(...) )
	  return NULL;

	for ( i = 0; i < nr_mfns; i++ )
		flush_page_to_ram(...);
}


acquire_reserved_page() would then only call foo() and assign_pages().

Cheers,

-- 
Julien Grall

Re: [PATCH v4 1/6] xen: do not free reserved memory into heap

[Julien Grall]

Hi,

On 17/05/2022 09:21, Penny Zheng wrote:
> Yes,  I remembered that asynchronous is still on the to-do list for static memory.
> 
> If it doesn't bother too much to you, I would like to ask some help on this issue, ;).
> I only knew basic knowledge on the scrubbing, 
My kwnoledge on the scrubbing code is not much better than yours :).

> I knew that dirty pages is placed at the
> end of list heap(node, zone, order) for scrubbing and "first_dirty" is used to track down
> the dirty pages. IMO, Both two parts are restricted to the heap thingy,  not reusable for
> static memory, 

That's correct.

> so maybe I need to re-write scrub_free_page for static memory, and also
> link the need-to-scrub reserved pages to a new global list e.g.  dirty_resv_list for aync
> scrubbing?

So I can foresee two problems with scrubbing static memory:
   1) Once the page is scrubbed, we need to know which domain it belongs 
so we can link the page again
   2) A page may still wait for scrubbing when the domain allocate 
memory (IOW the reserved list may be empty). So we need to find a page 
belonging to the domain and then scrubbed.

The two problems above would indicate that a per-domain scrub list would 
be the best here. We would need to deal with initial scrubbing 
differently (maybe a global list as you suggested).

I expect it will take some times to implement it properly. While writing 
this, I was wondering if there is actually any point to scrub pages when 
the domain is releasing them. Even if they are free they are still 
belonging to the domain, so scrubbing them is technically not necessary.

Any thoughts?

>>>    {
>>>        mfn_t mfn = page_to_mfn(pg);
>>>        unsigned long i;
>>> @@ -2653,7 +2657,8 @@ void __init free_staticmem_pages(struct page_info
>> *pg, unsigned long nr_mfns,
>>>            }
>>>
>>>            /* In case initializing page of static memory, mark it PGC_reserved. */
>>> -        pg[i].count_info |= PGC_reserved;
>>> +        if ( !(pg[i].count_info & PGC_reserved) )
>>
>> NIT: I understand the flag may have already been set, but I am not convinced if
>> it is worth checking it and then set.
>>
> 
> Jan suggested that since we remove the __init from free_staticmem_pages, it's now in preemptable
> state at runtime, so better be adding this check here.

Well, count_info is already modified within that loop (see 
mark_page_free()). So I think the impact of setting PGC_reserved is 
going to be meaningless.

However... mark_page_free() is going to set count_info to PGC_state_free 
and by consequence clear PGC_reserved. Theferore, in the current 
implementation we always need to re-set PGC_reserved.

So effectively, the "if" is pointless here.

Cheers,

-- 
Julien Grall

Re: [PATCH v4 1/6] xen: do not free reserved memory into heap

[Jan Beulich]

On 17.05.2022 11:28, Julien Grall wrote:
> On 17/05/2022 09:21, Penny Zheng wrote:
>>>> @@ -2653,7 +2657,8 @@ void __init free_staticmem_pages(struct page_info
>>> *pg, unsigned long nr_mfns,
>>>>            }
>>>>
>>>>            /* In case initializing page of static memory, mark it PGC_reserved. */
>>>> -        pg[i].count_info |= PGC_reserved;
>>>> +        if ( !(pg[i].count_info & PGC_reserved) )
>>>
>>> NIT: I understand the flag may have already been set, but I am not convinced if
>>> it is worth checking it and then set.
>>>
>>
>> Jan suggested that since we remove the __init from free_staticmem_pages, it's now in preemptable
>> state at runtime, so better be adding this check here.
> 
> Well, count_info is already modified within that loop (see 
> mark_page_free()). So I think the impact of setting PGC_reserved is 
> going to be meaningless.
> 
> However... mark_page_free() is going to set count_info to PGC_state_free 
> and by consequence clear PGC_reserved. Theferore, in the current 
> implementation we always need to re-set PGC_reserved.

Oh, indeed - I didn't pay attention to that aspect. Which then, however,
means that the comment also wants adjusting.

Jan

> So effectively, the "if" is pointless here.
> 
> Cheers,
> 

Re: [PATCH v4 1/6] xen: do not free reserved memory into heap

[Jan Beulich]

On 10.05.2022 04:27, Penny Zheng wrote:
> @@ -2762,6 +2767,12 @@ int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
>  
>      return 0;
>  }
> +#else
> +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> +                          bool need_scrub)
> +{
> +    ASSERT_UNREACHABLE();
> +}
>  #endif

As before I do not agree that we need this (or similar) stub functions. As
already suggested I think that instead Arm wants to #define PGC_reserved
(to non-zero) only when !CONFIG_STATIC_MEMORY, just like is already the
case on x86.

Jan

Re: [PATCH v4 6/6] xen: retrieve reserved pages on populate_physmap

[Jan Beulich]

On 10.05.2022 04:27, Penny Zheng wrote:
> @@ -2769,12 +2769,43 @@ int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
>  
>      return 0;
>  }
> +
> +/*
> + * Acquire a page from reserved page list(resv_page_list), when populating
> + * memory for static domain on runtime.
> + */
> +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
> +{
> +    struct page_info *page;
> +    mfn_t smfn;
> +
> +    /* Acquire a page from reserved page list(resv_page_list). */
> +    page = page_list_remove_head(&d->resv_page_list);
> +    if ( unlikely(!page) )
> +        return INVALID_MFN;
> +
> +    smfn = page_to_mfn(page);
> +
> +    if ( acquire_domstatic_pages(d, smfn, 1, memflags) )
> +    {
> +        page_list_add_tail(page, &d->resv_page_list);
> +        return INVALID_MFN;
> +    }
> +
> +    return smfn;
> +}
>  #else
>  void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
>                            bool need_scrub)
>  {
>      ASSERT_UNREACHABLE();
>  }
> +
> +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
> +{
> +    ASSERT_UNREACHABLE();
> +    return INVALID_MFN;
> +}
>  #endif

Much like for the other stub function added in the earlier patch: If
is_domain_using_staticmem() was compile time constant "false" when
!CONFIG_STATIC_MEM, there would be no need for this one since the
compiler would DCE the only call site.

RE: [PATCH v4 1/6] xen: do not free reserved memory into heap

[Penny Zheng]

Hi Jan and Julien

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, May 18, 2022 12:11 AM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Wei Chen <Wei.Chen@arm.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Julien Grall <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>; Wei
> Liu <wl@xen.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v4 1/6] xen: do not free reserved memory into heap
> 
> On 10.05.2022 04:27, Penny Zheng wrote:
> > @@ -2762,6 +2767,12 @@ int __init acquire_domstatic_pages(struct
> > domain *d, mfn_t smfn,
> >
> >      return 0;
> >  }
> > +#else
> > +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> > +                          bool need_scrub) {
> > +    ASSERT_UNREACHABLE();
> > +}
> >  #endif
> 
> As before I do not agree that we need this (or similar) stub functions. As
> already suggested I think that instead Arm wants to #define PGC_reserved (to
> non-zero) only when !CONFIG_STATIC_MEMORY, just like is already the case
> on x86.
> 

Ok, if you do not like the stub function, then what about I putting the #ifdef-array back
to the common where free_staticmem_pages is used:
#ifdef CONFIG_STATIC_MEMORY
	if ( pg->count_info & PGC_reserved )
	    /* Reserved page shall not go back to the heap. */
	    return free_staticmem_pages(pg, 1UL << order, need_scrub);
#endif
If this is not the option here too, before I make the change about guarding the
PGC_reserved with CONFIG_STATIC_MEMORY on ARM, I'd like to cc Julien here,
Since in the very beginning when we introduced PGC_reserved flag, he might have
concerns about limiting the usage of PGC_reserved to static memory, if he is
okay now, I'm fine too. ;)

> Jan

RE: [PATCH v4 1/6] xen: do not free reserved memory into heap

[Penny Zheng]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Tuesday, May 17, 2022 5:29 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Jan Beulich <jbeulich@suse.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v4 1/6] xen: do not free reserved memory into heap
> 
> Hi,
> 
> On 17/05/2022 09:21, Penny Zheng wrote:
> > Yes,  I remembered that asynchronous is still on the to-do list for static
> memory.
> >
> > If it doesn't bother too much to you, I would like to ask some help on this
> issue, ;).
> > I only knew basic knowledge on the scrubbing,
> My kwnoledge on the scrubbing code is not much better than yours :).
> 
> > I knew that dirty pages is placed at the
> > end of list heap(node, zone, order) for scrubbing and "first_dirty" is used to
> track down
> > the dirty pages. IMO, Both two parts are restricted to the heap thingy,  not
> reusable for
> > static memory,
> 
> That's correct.
> 
> > so maybe I need to re-write scrub_free_page for static memory, and also
> > link the need-to-scrub reserved pages to a new global list e.g.  dirty_resv_list
> for aync
> > scrubbing?
> 
> So I can foresee two problems with scrubbing static memory:
>    1) Once the page is scrubbed, we need to know which domain it belongs
> so we can link the page again
>    2) A page may still wait for scrubbing when the domain allocate
> memory (IOW the reserved list may be empty). So we need to find a page
> belonging to the domain and then scrubbed.
>

Understood, thanks for the to-the-point instruction! ;)
For scrubbing on runtime, un-populating memory will free reserved pages
to reserved list, then async scrubbing will move them to a per-domain list. Later
when scrubbing is finished, we need to again move it back to the reserved
list.
And if we failed on acquiring a page from reserved list, then trying to get a page
from the per-domain list and scrub it. 

And with initial scrubbing, since the concept of domain is not constructed, a
global list is better.
Right now, we always allocate static memory from specified starting address,
so just make sure that page is scrubbed before allocation.

> The two problems above would indicate that a per-domain scrub list would
> be the best here. We would need to deal with initial scrubbing
> differently (maybe a global list as you suggested).
> 
> I expect it will take some times to implement it properly. While writing
> this, I was wondering if there is actually any point to scrub pages when
> the domain is releasing them. Even if they are free they are still
> belonging to the domain, so scrubbing them is technically not necessary.
> 

True, true, if static memory used as guest memory, even if they are free, they are still
belonging to the domain. Even as static shared memory, it is pre-configured in boot-time
and could not be used for any other purpose.
Hmmm, may I ask that if we reboot the domain and didn't scrub the pages before, the
old stale contents will not affect the rebooting machine?
Or it should be the guest's responsibility to do the cleaning up before using it?

If it is the guest's responsibility, yeah, maybe scrubbing them is technically not
necessary, flushing TLB and cleaning cache is enough~ So should I remove the scrubbing
totally for static memory?

> Any thoughts?
> 
> >>>    {
> >>>        mfn_t mfn = page_to_mfn(pg);
> >>>        unsigned long i;
> >>> @@ -2653,7 +2657,8 @@ void __init free_staticmem_pages(struct
> page_info
> >> *pg, unsigned long nr_mfns,
> >>>            }
> >>>
> >>>            /* In case initializing page of static memory, mark it PGC_reserved. */
> >>> -        pg[i].count_info |= PGC_reserved;
> >>> +        if ( !(pg[i].count_info & PGC_reserved) )
> >>
> >> NIT: I understand the flag may have already been set, but I am not
> convinced if
> >> it is worth checking it and then set.
> >>
> >
> > Jan suggested that since we remove the __init from free_staticmem_pages,
> it's now in preemptable
> > state at runtime, so better be adding this check here.
> 
> Well, count_info is already modified within that loop (see
> mark_page_free()). So I think the impact of setting PGC_reserved is
> going to be meaningless.
> 
> However... mark_page_free() is going to set count_info to PGC_state_free
> and by consequence clear PGC_reserved. Theferore, in the current
> implementation we always need to re-set PGC_reserved.
> 
> So effectively, the "if" is pointless here.
> 
> Cheers,
> 
> --
> Julien Grall

Re: [PATCH v4 1/6] xen: do not free reserved memory into heap

[Jan Beulich]

On 18.05.2022 04:29, Penny Zheng wrote:
> Hi Jan and Julien
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Wednesday, May 18, 2022 12:11 AM
>> To: Penny Zheng <Penny.Zheng@arm.com>
>> Cc: Wei Chen <Wei.Chen@arm.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
>> Julien Grall <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>; Wei
>> Liu <wl@xen.org>; xen-devel@lists.xenproject.org
>> Subject: Re: [PATCH v4 1/6] xen: do not free reserved memory into heap
>>
>> On 10.05.2022 04:27, Penny Zheng wrote:
>>> @@ -2762,6 +2767,12 @@ int __init acquire_domstatic_pages(struct
>>> domain *d, mfn_t smfn,
>>>
>>>      return 0;
>>>  }
>>> +#else
>>> +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
>>> +                          bool need_scrub) {
>>> +    ASSERT_UNREACHABLE();
>>> +}
>>>  #endif
>>
>> As before I do not agree that we need this (or similar) stub functions. As
>> already suggested I think that instead Arm wants to #define PGC_reserved (to
>> non-zero) only when !CONFIG_STATIC_MEMORY, just like is already the case
>> on x86.
>>
> 
> Ok, if you do not like the stub function, then what about I putting the #ifdef-array back
> to the common where free_staticmem_pages is used:
> #ifdef CONFIG_STATIC_MEMORY
> 	if ( pg->count_info & PGC_reserved )
> 	    /* Reserved page shall not go back to the heap. */
> 	    return free_staticmem_pages(pg, 1UL << order, need_scrub);
> #endif

No. Stub functions, when they are really needed, are generally preferable
over #ifdef-ary inside functions (for readability reasons at least). Yet
even better is if stub functions can be avoided altogether.

Jan