[PATCH v7 0/9] populate/unpopulate memory when domain on static allocation

[PATCH v7 0/9] populate/unpopulate memory when domain on static allocation

[Penny Zheng]

MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.

---
v7 changes:
- protect free_staticmem_pages with heap_lock to match its reverse function
acquire_staticmem_pages
- IS_ENABLED(CONFIG_STATIC_MEMORY) would not be needed anymore
- add page on the rsv_page_list *after* it has been freed
- remove the lock, since we add the page to rsv_page_list after it has
been totally freed.
---
v6 changes:
- rename PGC_staticmem to PGC_static
- remove #ifdef aroud function declaration
- use domain instead of sub-systems
- move non-zero is_domain_using_staticmem() from ARM header to common
header
- move PGC_static !CONFIG_STATIC_MEMORY definition to common header
- drop the lock before returning
---
v5 changes:
- introduce three new commits
- In order to avoid stub functions, we #define PGC_staticmem to non-zero only
when CONFIG_STATIC_MEMORY
- use "unlikely()" around pg->count_info & PGC_staticmem
- remove pointless "if", since mark_page_free() is going to set count_info
to PGC_state_free and by consequence clear PGC_staticmem
- move #define PGC_staticmem 0 to mm.h
- guard "is_domain_using_staticmem" under CONFIG_STATIC_MEMORY
- #define is_domain_using_staticmem zero if undefined
- extract common codes for assigning pages into a helper assign_domstatic_pages
- refine commit message
- remove stub function acquire_reserved_page
- Alloc/free of memory can happen concurrently. So access to rsv_page_list
needs to be protected with a spinlock
---
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
- guard "is_domain_using_staticmem" under CONFIG_STATIC_MEMORY
- #define is_domain_using_staticmem zero if undefined
---
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 (9):
  xen/arm: rename PGC_reserved to PGC_static
  xen: do not free reserved memory into heap
  xen: update SUPPORT.md for static allocation
  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: introduce prepare_staticmem_pages
  xen: retrieve reserved pages on populate_physmap

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

-- 
2.25.1

[PATCH v7 1/9] xen/arm: rename PGC_reserved to PGC_static

[Penny Zheng]

PGC_reserved could be ambiguous, and we have to tell what the pages are
reserved for, so this commit intends to rename PGC_reserved to
PGC_static, which clearly indicates the page is reserved for static
memory.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
v7 changes:
- no change
---
v6 changes:
- rename PGC_staticmem to PGC_static
---
v5 changes:
- new commit
---
 xen/arch/arm/include/asm/mm.h |  6 +++---
 xen/common/page_alloc.c       | 22 +++++++++++-----------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index 045a8ba4bb..daef12e740 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -108,9 +108,9 @@ struct page_info
   /* Page is Xen heap? */
 #define _PGC_xen_heap     PG_shift(2)
 #define PGC_xen_heap      PG_mask(1, 2)
-  /* Page is reserved */
-#define _PGC_reserved     PG_shift(3)
-#define PGC_reserved      PG_mask(1, 3)
+  /* Page is static memory */
+#define _PGC_static    PG_shift(3)
+#define PGC_static     PG_mask(1, 3)
 /* ... */
 /* Page is broken? */
 #define _PGC_broken       PG_shift(7)
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 000ae6b972..743e3543fd 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -151,8 +151,8 @@
 #define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL)
 #endif
 
-#ifndef PGC_reserved
-#define PGC_reserved 0
+#ifndef PGC_static
+#define PGC_static 0
 #endif
 
 /*
@@ -2286,7 +2286,7 @@ int assign_pages(
 
         for ( i = 0; i < nr; i++ )
         {
-            ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_reserved)));
+            ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_static)));
             if ( pg[i].count_info & PGC_extra )
                 extra_pages++;
         }
@@ -2346,7 +2346,7 @@ int assign_pages(
         page_set_owner(&pg[i], d);
         smp_wmb(); /* Domain pointer must be visible before updating refcnt. */
         pg[i].count_info =
-            (pg[i].count_info & (PGC_extra | PGC_reserved)) | PGC_allocated | 1;
+            (pg[i].count_info & (PGC_extra | PGC_static)) | PGC_allocated | 1;
 
         page_list_add_tail(&pg[i], page_to_list(d, &pg[i]));
     }
@@ -2652,8 +2652,8 @@ void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
             scrub_one_page(pg);
         }
 
-        /* In case initializing page of static memory, mark it PGC_reserved. */
-        pg[i].count_info |= PGC_reserved;
+        /* In case initializing page of static memory, mark it PGC_static. */
+        pg[i].count_info |= PGC_static;
     }
 }
 
@@ -2682,8 +2682,8 @@ static struct page_info * __init acquire_staticmem_pages(mfn_t smfn,
 
     for ( i = 0; i < nr_mfns; i++ )
     {
-        /* The page should be reserved and not yet allocated. */
-        if ( pg[i].count_info != (PGC_state_free | PGC_reserved) )
+        /* The page should be static and not yet allocated. */
+        if ( pg[i].count_info != (PGC_state_free | PGC_static) )
         {
             printk(XENLOG_ERR
                    "pg[%lu] Static MFN %"PRI_mfn" c=%#lx t=%#x\n",
@@ -2697,10 +2697,10 @@ static struct page_info * __init acquire_staticmem_pages(mfn_t smfn,
                                 &tlbflush_timestamp);
 
         /*
-         * Preserve flag PGC_reserved and change page state
+         * Preserve flag PGC_static and change page state
          * to PGC_state_inuse.
          */
-        pg[i].count_info = PGC_reserved | PGC_state_inuse;
+        pg[i].count_info = PGC_static | PGC_state_inuse;
         /* Initialise fields which have other uses for free pages. */
         pg[i].u.inuse.type_info = 0;
         page_set_owner(&pg[i], NULL);
@@ -2722,7 +2722,7 @@ static struct page_info * __init acquire_staticmem_pages(mfn_t smfn,
 
  out_err:
     while ( i-- )
-        pg[i].count_info = PGC_reserved | PGC_state_free;
+        pg[i].count_info = PGC_static | PGC_state_free;
 
     spin_unlock(&heap_lock);
 
-- 
2.25.1

[PATCH v7 2/9] 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>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
v7 changes:
- protect free_staticmem_pages with heap_lock to match its reverse function
acquire_staticmem_pages
---
v6 changes:
- adapt to PGC_static
- remove #ifdef aroud function declaration
---
v5 changes:
- In order to avoid stub functions, we #define PGC_staticmem to non-zero only
when CONFIG_STATIC_MEMORY
- use "unlikely()" around pg->count_info & PGC_staticmem
- remove pointless "if", since mark_page_free() is going to set count_info
to PGC_state_free and by consequence clear PGC_staticmem
- move #define PGC_staticmem 0 to mm.h
---
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/arch/arm/include/asm/mm.h |  4 +++-
 xen/common/page_alloc.c       | 16 +++++++++++++---
 xen/include/xen/mm.h          |  2 --
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index daef12e740..066a869783 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -108,9 +108,11 @@ struct page_info
   /* Page is Xen heap? */
 #define _PGC_xen_heap     PG_shift(2)
 #define PGC_xen_heap      PG_mask(1, 2)
-  /* Page is static memory */
+#ifdef CONFIG_STATIC_MEMORY
+/* Page is static memory */
 #define _PGC_static    PG_shift(3)
 #define PGC_static     PG_mask(1, 3)
+#endif
 /* ... */
 /* Page is broken? */
 #define _PGC_broken       PG_shift(7)
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 743e3543fd..f27fa90ec4 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1443,6 +1443,13 @@ static void free_heap_pages(
 
     ASSERT(order <= MAX_ORDER);
 
+    if ( unlikely(pg->count_info & PGC_static) )
+    {
+        /* Pages of static memory shall not go back to the heap. */
+        free_staticmem_pages(pg, 1UL << order, need_scrub);
+        return;
+    }
+
     spin_lock(&heap_lock);
 
     for ( i = 0; i < (1 << order); i++ )
@@ -2636,12 +2643,14 @@ 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;
 
+    spin_lock(&heap_lock);
+
     for ( i = 0; i < nr_mfns; i++ )
     {
         mark_page_free(&pg[i], mfn_add(mfn, i));
@@ -2652,9 +2661,10 @@ void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
             scrub_one_page(pg);
         }
 
-        /* In case initializing page of static memory, mark it PGC_static. */
         pg[i].count_info |= PGC_static;
     }
+
+    spin_unlock(&heap_lock);
 }
 
 /*
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 3be754da92..1c4ddb336b 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -85,13 +85,11 @@ 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);
 int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int nr_mfns,
                             unsigned int memflags);
-#endif
 
 /* Map machine page range in Xen virtual address space. */
 int map_pages_to_xen(
-- 
2.25.1

[PATCH v7 3/9] xen: update SUPPORT.md for static allocation

[Penny Zheng]

SUPPORT.md doesn't seem to explicitly say whether static memory is
supported, so this commit updates SUPPORT.md to add feature static
allocation tech preview for now.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v7 changes:
- no change
---
v6 changes:
- use domain instead of sub-systems
---
v5 changes:
- new commit
---
 SUPPORT.md | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/SUPPORT.md b/SUPPORT.md
index 70e98964cb..8e040d1c1e 100644
--- a/SUPPORT.md
+++ b/SUPPORT.md
@@ -286,6 +286,13 @@ to boot with memory < maxmem.
 
     Status, x86 HVM: Supported
 
+### Static Allocation
+
+Static allocation refers to domains for which memory areas are
+pre-defined by configuration using physical address ranges.
+
+    Status, ARM: Tech Preview
+
 ### Memory Sharing
 
 Allow sharing of identical pages between guests
-- 
2.25.1

[PATCH v7 4/9] 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.

Suggested-by: Julien Grall <jgrall@amazon.com>
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
v7 changes:
- no change
---
v6 changes:
- adapt to PGC_static
---
v5 changes:
- change PGC_reserved to adapt to PGC_staticmem
---
v4 changes:
- no changes
---
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 f27fa90ec4..d9253df270 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1486,6 +1486,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_static) ||
                  (PFN_ORDER(predecessor) != order) ||
                  (phys_to_nid(page_to_maddr(predecessor)) != node) )
                 break;
@@ -1509,6 +1510,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_static) ||
                  (PFN_ORDER(successor) != order) ||
                  (phys_to_nid(page_to_maddr(successor)) != node) )
                 break;
-- 
2.25.1

[PATCH v7 5/9] 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>
---
v7 changes:
- no change
---
v6 changes:
- no change
---
v5 changes:
- no change
---
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 7570eae91a..a3ef991bd1 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 463d41ffb6..5191853c18 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -596,6 +596,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 v7 6/9] 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>
---
v7 changes:
- IS_ENABLED(CONFIG_STATIC_MEMORY) would not be needed anymore
---
v6 changes:
- move non-zero is_domain_using_staticmem() from ARM header to common
header
---
v5 changes:
- guard "is_domain_using_staticmem" under CONFIG_STATIC_MEMORY
- #define is_domain_using_staticmem zero if undefined
---
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/include/xen/domain.h    | 12 ++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 7ddd16c26d..17cd886be8 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3287,9 +3287,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 ( !(flags & CDF_staticmem) )
                 panic("direct-map is not valid for domain %s without static allocation.\n",
                       dt_node_name(node));
 
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 1c3c88a14d..cf34eddf6d 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -35,6 +35,18 @@ void arch_get_domain_info(const struct domain *d,
 /* Should domain memory be directly mapped? */
 #define CDF_directmap            (1U << 1)
 #endif
+/* Is domain memory on static allocation? */
+#ifdef CONFIG_STATIC_MEMORY
+#define CDF_staticmem            (1U << 2)
+#else
+#define CDF_staticmem            0
+#endif
+
+#ifdef CONFIG_STATIC_MEMORY
+#define is_domain_using_staticmem(d) ((d)->cdf & CDF_staticmem)
+#else
+#define is_domain_using_staticmem(d) ((void)(d), false)
+#endif
 
 /*
  * Arch-specifics.
-- 
2.25.1

[PATCH v7 7/9] 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>
---
v7 changes:
- Add page on the rsv_page_list *after* it has been freed
---
v6 changes:
- refine in-code comment
- move PGC_static !CONFIG_STATIC_MEMORY definition to common header
---
v5 changes:
- adapt this patch for PGC_staticmem
---
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/common/domain.c     | 4 ++++
 xen/common/page_alloc.c | 4 ++++
 xen/include/xen/mm.h    | 9 +++++++++
 xen/include/xen/sched.h | 3 +++
 4 files changed, 20 insertions(+)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index a3ef991bd1..a49574fa24 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/common/page_alloc.c b/xen/common/page_alloc.c
index d9253df270..7d223087c0 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2498,6 +2498,10 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
         }
 
         free_heap_pages(pg, order, scrub);
+
+        /* Add page on the resv_page_list *after* it has been freed. */
+        if ( unlikely(pg->count_info & PGC_static) )
+            put_static_pages(d, pg, order);
     }
 
     if ( drop_dom_ref )
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 1c4ddb336b..68a647ceae 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -90,6 +90,15 @@ void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
                           bool need_scrub);
 int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int nr_mfns,
                             unsigned int memflags);
+#ifdef CONFIG_STATIC_MEMORY
+#define put_static_pages(d, page, order) ({                 \
+    unsigned int i;                                         \
+    for ( i = 0; i < (1 << (order)); i++ )                  \
+        page_list_add_tail((pg) + i, &(d)->resv_page_list); \
+})
+#else
+#define put_static_pages(d, page, order) ((void)(d), (void)(page), (void)(order))
+#endif
 
 /* Map machine page range in Xen virtual address space. */
 int map_pages_to_xen(
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 5191853c18..bd2782b3c5 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -381,6 +381,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 */
+#endif
 
     /*
      * This field should only be directly accessed by domain_adjust_tot_pages()
-- 
2.25.1

[PATCH v7 8/9] xen: introduce prepare_staticmem_pages

[Penny Zheng]

Later, we want to use acquire_domstatic_pages() for populating memory
for static domain on runtime, however, there are a lot of pointless work
(checking mfn_valid(), scrubbing the free part, cleaning the cache...)
considering we know the page is valid and belong to the guest.

This commit splits acquire_staticmem_pages() in two parts, and
introduces prepare_staticmem_pages to bypass all "pointless work".

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
v7 changes:
- no change
---
v6 changes:
- adapt to PGC_static
---
v5 changes:
- new commit
---
 xen/common/page_alloc.c | 61 ++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 25 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 7d223087c0..fee396a92d 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2673,26 +2673,13 @@ void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
     spin_unlock(&heap_lock);
 }
 
-/*
- * Acquire nr_mfns contiguous reserved pages, starting at #smfn, of
- * static memory.
- * This function needs to be reworked if used outside of boot.
- */
-static struct page_info * __init acquire_staticmem_pages(mfn_t smfn,
-                                                         unsigned long nr_mfns,
-                                                         unsigned int memflags)
+static bool __init prepare_staticmem_pages(struct page_info *pg,
+                                           unsigned long nr_mfns,
+                                           unsigned int memflags)
 {
     bool need_tlbflush = false;
     uint32_t tlbflush_timestamp = 0;
     unsigned long i;
-    struct page_info *pg;
-
-    ASSERT(nr_mfns);
-    for ( i = 0; i < nr_mfns; i++ )
-        if ( !mfn_valid(mfn_add(smfn, i)) )
-            return NULL;
-
-    pg = mfn_to_page(smfn);
 
     spin_lock(&heap_lock);
 
@@ -2703,7 +2690,7 @@ static struct page_info * __init acquire_staticmem_pages(mfn_t smfn,
         {
             printk(XENLOG_ERR
                    "pg[%lu] Static MFN %"PRI_mfn" c=%#lx t=%#x\n",
-                   i, mfn_x(smfn) + i,
+                   i, mfn_x(page_to_mfn(pg)) + i,
                    pg[i].count_info, pg[i].tlbflush_timestamp);
             goto out_err;
         }
@@ -2727,6 +2714,38 @@ static struct page_info * __init acquire_staticmem_pages(mfn_t smfn,
     if ( need_tlbflush )
         filtered_flush_tlb_mask(tlbflush_timestamp);
 
+    return true;
+
+ out_err:
+    while ( i-- )
+        pg[i].count_info = PGC_static | PGC_state_free;
+
+    spin_unlock(&heap_lock);
+
+    return false;
+}
+
+/*
+ * Acquire nr_mfns contiguous reserved pages, starting at #smfn, of
+ * static memory.
+ * This function needs to be reworked if used outside of boot.
+ */
+static struct page_info * __init acquire_staticmem_pages(mfn_t smfn,
+                                                         unsigned long nr_mfns,
+                                                         unsigned int memflags)
+{
+    unsigned long i;
+    struct page_info *pg;
+
+    ASSERT(nr_mfns);
+    for ( i = 0; i < nr_mfns; i++ )
+        if ( !mfn_valid(mfn_add(smfn, i)) )
+            return NULL;
+
+    pg = mfn_to_page(smfn);
+    if ( !prepare_staticmem_pages(pg, nr_mfns, memflags) )
+        return NULL;
+
     /*
      * Ensure cache and RAM are consistent for platforms where the guest
      * can control its own visibility of/through the cache.
@@ -2735,14 +2754,6 @@ static struct page_info * __init acquire_staticmem_pages(mfn_t smfn,
         flush_page_to_ram(mfn_x(smfn) + i, !(memflags & MEMF_no_icache_flush));
 
     return pg;
-
- out_err:
-    while ( i-- )
-        pg[i].count_info = PGC_static | PGC_state_free;
-
-    spin_unlock(&heap_lock);
-
-    return NULL;
 }
 
 /*
-- 
2.25.1

[PATCH v7 9/9] xen: retrieve reserved pages on populate_physmap

[Penny Zheng]

When a static domain populates memory through populate_physmap at runtime,
it shall retrieve reserved pages from resv_page_list to make sure that
guest RAM is still restricted in statically configured memory regions.
This commit also introduces a new helper acquire_reserved_page to make it work.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
v7 changes:
- remove the lock, since we add the page to rsv_page_list after it has
been totally freed.
---
v6 changes:
- drop the lock before returning
---
v5 changes:
- extract common codes for assigning pages into a helper assign_domstatic_pages
- refine commit message
- remove stub function acquire_reserved_page
- Alloc/free of memory can happen concurrently. So access to rsv_page_list
needs to be protected with a spinlock
---
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 | 68 ++++++++++++++++++++++++++++++-----------
 xen/include/xen/mm.h    |  1 +
 3 files changed, 75 insertions(+), 17 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 fee396a92d..74628889ea 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2673,9 +2673,8 @@ void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
     spin_unlock(&heap_lock);
 }
 
-static bool __init prepare_staticmem_pages(struct page_info *pg,
-                                           unsigned long nr_mfns,
-                                           unsigned int memflags)
+static bool prepare_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
+                                    unsigned int memflags)
 {
     bool need_tlbflush = false;
     uint32_t tlbflush_timestamp = 0;
@@ -2756,21 +2755,9 @@ static struct page_info * __init acquire_staticmem_pages(mfn_t smfn,
     return pg;
 }
 
-/*
- * 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)
+static int assign_domstatic_pages(struct domain *d, struct page_info *pg,
+                                  unsigned int nr_mfns, unsigned int memflags)
 {
-    struct page_info *pg;
-
-    ASSERT_ALLOC_CONTEXT();
-
-    pg = acquire_staticmem_pages(smfn, nr_mfns, memflags);
-    if ( !pg )
-        return -ENOENT;
-
     if ( !d || (memflags & (MEMF_no_owner | MEMF_no_refcount)) )
     {
         /*
@@ -2789,6 +2776,53 @@ int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
 
     return 0;
 }
+
+/*
+ * 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)
+{
+    struct page_info *pg;
+
+    ASSERT_ALLOC_CONTEXT();
+
+    pg = acquire_staticmem_pages(smfn, nr_mfns, memflags);
+    if ( !pg )
+        return -ENOENT;
+
+    if ( assign_domstatic_pages(d, pg, nr_mfns, memflags) )
+        return -EINVAL;
+
+    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;
+
+    /* 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;
+
+    if ( !prepare_staticmem_pages(page, 1, memflags) )
+        goto fail;
+
+    if ( assign_domstatic_pages(d, page, 1, memflags) )
+        goto fail;
+
+    return page_to_mfn(page);
+
+ fail:
+    page_list_add_tail(page, &d->resv_page_list);
+    return INVALID_MFN;
+}
 #endif
 
 /*
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 68a647ceae..e6803fd8a2 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -99,6 +99,7 @@ int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int nr_mfns,
 #else
 #define put_static_pages(d, page, order) ((void)(d), (void)(page), (void)(order))
 #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 v7 6/9] xen/arm: introduce CDF_staticmem

[Jan Beulich]

On 20.06.2022 04:44, Penny Zheng wrote:
> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -35,6 +35,18 @@ void arch_get_domain_info(const struct domain *d,
>  /* Should domain memory be directly mapped? */
>  #define CDF_directmap            (1U << 1)
>  #endif
> +/* Is domain memory on static allocation? */
> +#ifdef CONFIG_STATIC_MEMORY
> +#define CDF_staticmem            (1U << 2)
> +#else
> +#define CDF_staticmem            0
> +#endif

With this do you really need ...

> +#ifdef CONFIG_STATIC_MEMORY
> +#define is_domain_using_staticmem(d) ((d)->cdf & CDF_staticmem)
> +#else
> +#define is_domain_using_staticmem(d) ((void)(d), false)
> +#endif

... the #ifdef-ary here anymore?

Jan

Re: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is static

[Jan Beulich]

On 20.06.2022 04:44, Penny Zheng wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2498,6 +2498,10 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
>          }
>  
>          free_heap_pages(pg, order, scrub);
> +
> +        /* Add page on the resv_page_list *after* it has been freed. */
> +        if ( unlikely(pg->count_info & PGC_static) )
> +            put_static_pages(d, pg, order);

Unless I'm overlooking something the list addition done there / ...

> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -90,6 +90,15 @@ void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
>                            bool need_scrub);
>  int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int nr_mfns,
>                              unsigned int memflags);
> +#ifdef CONFIG_STATIC_MEMORY
> +#define put_static_pages(d, page, order) ({                 \
> +    unsigned int i;                                         \
> +    for ( i = 0; i < (1 << (order)); i++ )                  \
> +        page_list_add_tail((pg) + i, &(d)->resv_page_list); \
> +})

... here isn't guarded by any lock. Feels like we've been there before.
It's not really clear to me why the freeing of staticmem pages needs to
be split like this - if it wasn't split, the list addition would
"naturally" occur with the lock held, I think.

Furthermore careful with the local variable name used here. Consider
what would happen with an invocation of

    put_static_pages(d, page, i);

To common approach is to suffix an underscore to the variable name.
Such names are not supposed to be used outside of macros definitions,
and hence there's then no potential for such a conflict.

Finally I think you mean (1u << (order)) to be on the safe side against
UB if order could ever reach 31. Then again - is "order" as a parameter
needed here in the first place? Wasn't it that staticmem operations are
limited to order-0 regions?

Jan

Re: [PATCH v7 9/9] xen: retrieve reserved pages on populate_physmap

[Jan Beulich]

On 20.06.2022 04:44, Penny Zheng wrote:
> When a static domain populates memory through populate_physmap at runtime,
> it shall retrieve reserved pages from resv_page_list to make sure that
> guest RAM is still restricted in statically configured memory regions.
> This commit also introduces a new helper acquire_reserved_page to make it work.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> v7 changes:
> - remove the lock, since we add the page to rsv_page_list after it has
> been totally freed.

Was this meant to go into another patch? I can't seem to locate respective
code here, and the remark also doesn't give enough suitable context.

Jan

Re: [PATCH v7 9/9] xen: retrieve reserved pages on populate_physmap

[Julien Grall]

Hi Penny,

On 20/06/2022 03:44, Penny Zheng wrote:
> When a static domain populates memory through populate_physmap at runtime,
> it shall retrieve reserved pages from resv_page_list to make sure that
> guest RAM is still restricted in statically configured memory regions.
> This commit also introduces a new helper acquire_reserved_page to make it work.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> v7 changes:
> - remove the lock, since we add the page to rsv_page_list after it has
> been totally freed.

Hmmm... Adding the page after it has been totally freed doesn't mean you 
can get away with the lock. AFAICT you can still have concurrent 
free/allocate that could modify the list.

Therefore if you add/remove pages without the list, you would end up to 
corrupt the list.

If you disagree, then please point out which lock (or mechanism) will 
prevent concurrent access.

Cheers,

-- 
Julien Grall

RE: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is static

[Penny Zheng]

Hi jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, June 22, 2022 5:24 PM
> 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 v7 7/9] xen/arm: unpopulate memory when domain is
> static
> 
> On 20.06.2022 04:44, Penny Zheng wrote:
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2498,6 +2498,10 @@ void free_domheap_pages(struct page_info *pg,
> unsigned int order)
> >          }
> >
> >          free_heap_pages(pg, order, scrub);
> > +
> > +        /* Add page on the resv_page_list *after* it has been freed. */
> > +        if ( unlikely(pg->count_info & PGC_static) )
> > +            put_static_pages(d, pg, order);
> 
> Unless I'm overlooking something the list addition done there / ...
> 
> > --- a/xen/include/xen/mm.h
> > +++ b/xen/include/xen/mm.h
> > @@ -90,6 +90,15 @@ void free_staticmem_pages(struct page_info *pg,
> unsigned long nr_mfns,
> >                            bool need_scrub);  int
> > acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int
> nr_mfns,
> >                              unsigned int memflags);
> > +#ifdef CONFIG_STATIC_MEMORY
> > +#define put_static_pages(d, page, order) ({                 \
> > +    unsigned int i;                                         \
> > +    for ( i = 0; i < (1 << (order)); i++ )                  \
> > +        page_list_add_tail((pg) + i, &(d)->resv_page_list); \
> > +})
> 
> ... here isn't guarded by any lock. Feels like we've been there before.
> It's not really clear to me why the freeing of staticmem pages needs to be
> split like this - if it wasn't split, the list addition would "naturally" occur with
> the lock held, I think.

Reminded by you and Julien, I need to add a lock for operations(free/allocation) on
resv_page_list, I'll guard the put_static_pages with d->page_alloc_lock. And bring
back the lock in acquire_reserved_page.

put_static_pages, that is, adding pages to the reserved list, is only for freeing static
pages on runtime. In static page initialization stage, I also use free_statimem_pages,
and in which stage, I think the domain has not been constructed at all. So I prefer
the freeing of staticmem pages is split into two parts: free_staticmem_pages and
put_static_pages 

> 
> Furthermore careful with the local variable name used here. Consider what
> would happen with an invocation of
> 
>     put_static_pages(d, page, i);
> 
> To common approach is to suffix an underscore to the variable name.
> Such names are not supposed to be used outside of macros definitions, and
> hence there's then no potential for such a conflict.
> 

Understood!! I will change "unsigned int i" to "unsigned int _i";

> Finally I think you mean (1u << (order)) to be on the safe side against UB if
> order could ever reach 31. Then again - is "order" as a parameter needed
> here in the first place? Wasn't it that staticmem operations are limited to
> order-0 regions?

Yes, right now, the actual usage is limited to order-0, how about I add assertion here
and remove order parameter:

        /* Add page on the resv_page_list *after* it has been freed. */
        if ( unlikely(pg->count_info & PGC_static) )
        {
            ASSERT(!order);
            put_static_pages(d, pg);
        }

> Jan

RE: [PATCH v7 9/9] xen: retrieve reserved pages on populate_physmap

[Penny Zheng]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, June 25, 2022 3:51 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 v7 9/9] xen: retrieve reserved pages on
> populate_physmap
> 
> Hi Penny,
> 
> On 20/06/2022 03:44, Penny Zheng wrote:
> > When a static domain populates memory through populate_physmap at
> > runtime, it shall retrieve reserved pages from resv_page_list to make
> > sure that guest RAM is still restricted in statically configured memory
> regions.
> > This commit also introduces a new helper acquire_reserved_page to make
> it work.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > v7 changes:
> > - remove the lock, since we add the page to rsv_page_list after it has
> > been totally freed.
> 
> Hmmm... Adding the page after it has been totally freed doesn't mean you
> can get away with the lock. AFAICT you can still have concurrent free/allocate
> that could modify the list.
> 
> Therefore if you add/remove pages without the list, you would end up to
> corrupt the list.
> 
> If you disagree, then please point out which lock (or mechanism) will prevent
> concurrent access.
> 

Ok. Combined with the last serie comments, actually, you suggest that we need to add
two locks, right?

One is the lock for concurrent free/allocation on page_info, and we will use
heap_lock, one stays in free_staticmem_pages, one stays in its reversed function
prepare_staticmem_pages.

The other is for concurrent free/allocation on resv_page_list, and we will use
d->page_alloc_lock tp guard it. One stays in put_static_page, and another
stays in reversed function acquire_reserved_page.

> Cheers,
> 
> --
> Julien Grall

Re: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is static

[Julien Grall]

On 27/06/2022 11:03, Penny Zheng wrote:
> Hi jan
> 
>> -----Original Message-----
> put_static_pages, that is, adding pages to the reserved list, is only for freeing static
> pages on runtime. In static page initialization stage, I also use free_statimem_pages,
> and in which stage, I think the domain has not been constructed at all. So I prefer
> the freeing of staticmem pages is split into two parts: free_staticmem_pages and
> put_static_pages

AFAIU, all the pages would have to be allocated via 
acquire_domstatic_pages(). This call requires the domain to be allocated 
and setup for handling memory.

Therefore, I think the split is unnecessary. This would also have the 
advantage to remove one loop. Admittly, this is not important when the 
order 0, but it would become a problem for larger order (you may have to 
pull the struct page_info multiple time in the cache).

Cheers,

-- 
Julien Grall

Re: [PATCH v7 9/9] xen: retrieve reserved pages on populate_physmap

[Julien Grall]

Hi Penny,

On 27/06/2022 11:11, Penny Zheng wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Saturday, June 25, 2022 3:51 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 v7 9/9] xen: retrieve reserved pages on
>> populate_physmap
>>
>> Hi Penny,
>>
>> On 20/06/2022 03:44, Penny Zheng wrote:
>>> When a static domain populates memory through populate_physmap at
>>> runtime, it shall retrieve reserved pages from resv_page_list to make
>>> sure that guest RAM is still restricted in statically configured memory
>> regions.
>>> This commit also introduces a new helper acquire_reserved_page to make
>> it work.
>>>
>>> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
>>> ---
>>> v7 changes:
>>> - remove the lock, since we add the page to rsv_page_list after it has
>>> been totally freed.
>>
>> Hmmm... Adding the page after it has been totally freed doesn't mean you
>> can get away with the lock. AFAICT you can still have concurrent free/allocate
>> that could modify the list.
>>
>> Therefore if you add/remove pages without the list, you would end up to
>> corrupt the list.
>>
>> If you disagree, then please point out which lock (or mechanism) will prevent
>> concurrent access.
>>
> 
> Ok. Combined with the last serie comments, actually, you suggest that we need to add
> two locks, right?

We at least need the second lock (i.e. d->page_alloc_lock). The first 
lock (i.e.) may not be necessary if all the static memory are allocated 
for a domain. So you can guarantee ordering by adding to the resv_page_list.

Unless there are an ordering issue between the locks, I would for now 
suggest to keep both. We can refine this afterwards.

Cheers,

-- 
Julien Grall

Re: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is static

[Jan Beulich]

On 27.06.2022 12:03, Penny Zheng wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Wednesday, June 22, 2022 5:24 PM
>>
>> Furthermore careful with the local variable name used here. Consider what
>> would happen with an invocation of
>>
>>     put_static_pages(d, page, i);
>>
>> To common approach is to suffix an underscore to the variable name.
>> Such names are not supposed to be used outside of macros definitions, and
>> hence there's then no potential for such a conflict.
>>
> 
> Understood!! I will change "unsigned int i" to "unsigned int _i";

Note how I said "suffix", not "prefix".

>> Finally I think you mean (1u << (order)) to be on the safe side against UB if
>> order could ever reach 31. Then again - is "order" as a parameter needed
>> here in the first place? Wasn't it that staticmem operations are limited to
>> order-0 regions?
> 
> Yes, right now, the actual usage is limited to order-0, how about I add assertion here
> and remove order parameter:
> 
>         /* Add page on the resv_page_list *after* it has been freed. */
>         if ( unlikely(pg->count_info & PGC_static) )
>         {
>             ASSERT(!order);
>             put_static_pages(d, pg);
>         }

I don't mind an ASSERT() as long as upper layers indeed guarantee this.
What I'm worried about is that you might assert on user controlled input.

Jan

RE: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is static

[Penny Zheng]

Hi Julien and Jan

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Monday, June 27, 2022 6:19 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; Jan Beulich <jbeulich@suse.com>
> Cc: Wei Chen <Wei.Chen@arm.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is
> static
> 
> 
> 
> On 27/06/2022 11:03, Penny Zheng wrote:
> > Hi jan
> >
> >> -----Original Message-----
> > put_static_pages, that is, adding pages to the reserved list, is only
> > for freeing static pages on runtime. In static page initialization
> > stage, I also use free_statimem_pages, and in which stage, I think the
> > domain has not been constructed at all. So I prefer the freeing of
> > staticmem pages is split into two parts: free_staticmem_pages and
> > put_static_pages
> 
> AFAIU, all the pages would have to be allocated via
> acquire_domstatic_pages(). This call requires the domain to be allocated and
> setup for handling memory.
> 
> Therefore, I think the split is unnecessary. This would also have the
> advantage to remove one loop. Admittly, this is not important when the
> order 0, but it would become a problem for larger order (you may have to
> pull the struct page_info multiple time in the cache).
> 

How about this:
I create a new func free_domstatic_page, and it will be like:
"
static void free_domstatic_page(struct domain *d, struct page_info *page)
{
    unsigned int i;
    bool need_scrub;

    /* NB. May recursively lock from relinquish_memory(). */
    spin_lock_recursive(&d->page_alloc_lock);

    arch_free_heap_page(d, page);

    /*
     * Normally we expect a domain to clear pages before freeing them,
     * if it cares about the secrecy of their contents. However, after
     * a domain has died we assume responsibility for erasure. We do
     * scrub regardless if option scrub_domheap is set.
     */
    need_scrub = d->is_dying || scrub_debug || opt_scrub_domheap;

    free_staticmem_pages(page, 1, need_scrub);

    /* Add page on the resv_page_list *after* it has been freed. */
    put_static_page(d, page);

    drop_dom_ref = !domain_adjust_tot_pages(d, -1);

    spin_unlock_recursive(&d->page_alloc_lock);

    if ( drop_dom_ref )
        put_domain(d);
}
"

In free_domheap_pages, we just call free_domstatic_page:

"
@@ -2430,6 +2430,9 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)

     ASSERT_ALLOC_CONTEXT();

+    if ( unlikely(pg->count_info & PGC_static) )
+        return free_domstatic_page(d, pg);
+
     if ( unlikely(is_xen_heap_page(pg)) )
     {
         /* NB. May recursively lock from relinquish_memory(). */
@@ -2673,6 +2676,38 @@ void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
"

Then the split could be avoided and we could save the loop as much as possible.
Any suggestion? 

> Cheers,
> 
> --
> Julien Grall

Re: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is static

[Jan Beulich]

On 29.06.2022 05:12, Penny Zheng wrote:
> Hi Julien and Jan
> 
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: Monday, June 27, 2022 6:19 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>; Jan Beulich <jbeulich@suse.com>
>> Cc: Wei Chen <Wei.Chen@arm.com>; Andrew Cooper
>> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
>> Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; xen-
>> devel@lists.xenproject.org
>> Subject: Re: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is
>> static
>>
>>
>>
>> On 27/06/2022 11:03, Penny Zheng wrote:
>>> Hi jan
>>>
>>>> -----Original Message-----
>>> put_static_pages, that is, adding pages to the reserved list, is only
>>> for freeing static pages on runtime. In static page initialization
>>> stage, I also use free_statimem_pages, and in which stage, I think the
>>> domain has not been constructed at all. So I prefer the freeing of
>>> staticmem pages is split into two parts: free_staticmem_pages and
>>> put_static_pages
>>
>> AFAIU, all the pages would have to be allocated via
>> acquire_domstatic_pages(). This call requires the domain to be allocated and
>> setup for handling memory.
>>
>> Therefore, I think the split is unnecessary. This would also have the
>> advantage to remove one loop. Admittly, this is not important when the
>> order 0, but it would become a problem for larger order (you may have to
>> pull the struct page_info multiple time in the cache).
>>
> 
> How about this:
> I create a new func free_domstatic_page, and it will be like:
> "
> static void free_domstatic_page(struct domain *d, struct page_info *page)
> {
>     unsigned int i;
>     bool need_scrub;
> 
>     /* NB. May recursively lock from relinquish_memory(). */
>     spin_lock_recursive(&d->page_alloc_lock);
> 
>     arch_free_heap_page(d, page);
> 
>     /*
>      * Normally we expect a domain to clear pages before freeing them,
>      * if it cares about the secrecy of their contents. However, after
>      * a domain has died we assume responsibility for erasure. We do
>      * scrub regardless if option scrub_domheap is set.
>      */
>     need_scrub = d->is_dying || scrub_debug || opt_scrub_domheap;
> 
>     free_staticmem_pages(page, 1, need_scrub);
> 
>     /* Add page on the resv_page_list *after* it has been freed. */
>     put_static_page(d, page);
> 
>     drop_dom_ref = !domain_adjust_tot_pages(d, -1);
> 
>     spin_unlock_recursive(&d->page_alloc_lock);
> 
>     if ( drop_dom_ref )
>         put_domain(d);
> }
> "
> 
> In free_domheap_pages, we just call free_domstatic_page:
> 
> "
> @@ -2430,6 +2430,9 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
> 
>      ASSERT_ALLOC_CONTEXT();
> 
> +    if ( unlikely(pg->count_info & PGC_static) )
> +        return free_domstatic_page(d, pg);
> +
>      if ( unlikely(is_xen_heap_page(pg)) )
>      {
>          /* NB. May recursively lock from relinquish_memory(). */
> @@ -2673,6 +2676,38 @@ void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> "
> 
> Then the split could be avoided and we could save the loop as much as possible.
> Any suggestion? 

Looks reasonable at the first glance (will need to see it in proper
context for a final opinion), provided e.g. Xen heap pages can never
be static.

Jan

RE: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is static

[Penny Zheng]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, June 29, 2022 1:56 PM
> 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>;
> Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; xen-
> devel@lists.xenproject.org; Julien Grall <julien@xen.org>
> Subject: Re: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is
> static
> 
> On 29.06.2022 05:12, Penny Zheng wrote:
> > Hi Julien and Jan
> >
> >> -----Original Message-----
> >> From: Julien Grall <julien@xen.org>
> >> Sent: Monday, June 27, 2022 6:19 PM
> >> To: Penny Zheng <Penny.Zheng@arm.com>; Jan Beulich
> >> <jbeulich@suse.com>
> >> Cc: Wei Chen <Wei.Chen@arm.com>; Andrew Cooper
> >> <andrew.cooper3@citrix.com>; George Dunlap
> >> <george.dunlap@citrix.com>; Stefano Stabellini
> >> <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; xen-
> >> devel@lists.xenproject.org
> >> Subject: Re: [PATCH v7 7/9] xen/arm: unpopulate memory when domain
> is
> >> static
> >>
> >>
> >>
> >> On 27/06/2022 11:03, Penny Zheng wrote:
> >>> Hi jan
> >>>
> >>>> -----Original Message-----
> >>> put_static_pages, that is, adding pages to the reserved list, is
> >>> only for freeing static pages on runtime. In static page
> >>> initialization stage, I also use free_statimem_pages, and in which
> >>> stage, I think the domain has not been constructed at all. So I
> >>> prefer the freeing of staticmem pages is split into two parts:
> >>> free_staticmem_pages and put_static_pages
> >>
> >> AFAIU, all the pages would have to be allocated via
> >> acquire_domstatic_pages(). This call requires the domain to be
> >> allocated and setup for handling memory.
> >>
> >> Therefore, I think the split is unnecessary. This would also have the
> >> advantage to remove one loop. Admittly, this is not important when
> >> the order 0, but it would become a problem for larger order (you may
> >> have to pull the struct page_info multiple time in the cache).
> >>
> >
> > How about this:
> > I create a new func free_domstatic_page, and it will be like:
> > "
> > static void free_domstatic_page(struct domain *d, struct page_info
> > *page) {
> >     unsigned int i;
> >     bool need_scrub;
> >
> >     /* NB. May recursively lock from relinquish_memory(). */
> >     spin_lock_recursive(&d->page_alloc_lock);
> >
> >     arch_free_heap_page(d, page);
> >
> >     /*
> >      * Normally we expect a domain to clear pages before freeing them,
> >      * if it cares about the secrecy of their contents. However, after
> >      * a domain has died we assume responsibility for erasure. We do
> >      * scrub regardless if option scrub_domheap is set.
> >      */
> >     need_scrub = d->is_dying || scrub_debug || opt_scrub_domheap;
> >
> >     free_staticmem_pages(page, 1, need_scrub);
> >
> >     /* Add page on the resv_page_list *after* it has been freed. */
> >     put_static_page(d, page);
> >
> >     drop_dom_ref = !domain_adjust_tot_pages(d, -1);
> >
> >     spin_unlock_recursive(&d->page_alloc_lock);
> >
> >     if ( drop_dom_ref )
> >         put_domain(d);
> > }
> > "
> >
> > In free_domheap_pages, we just call free_domstatic_page:
> >
> > "
> > @@ -2430,6 +2430,9 @@ void free_domheap_pages(struct page_info *pg,
> > unsigned int order)
> >
> >      ASSERT_ALLOC_CONTEXT();
> >
> > +    if ( unlikely(pg->count_info & PGC_static) )
> > +        return free_domstatic_page(d, pg);
> > +
> >      if ( unlikely(is_xen_heap_page(pg)) )
> >      {
> >          /* NB. May recursively lock from relinquish_memory(). */ @@
> > -2673,6 +2676,38 @@ void free_staticmem_pages(struct page_info *pg,
> > unsigned long nr_mfns, "
> >
> > Then the split could be avoided and we could save the loop as much as
> possible.
> > Any suggestion?
> 
> Looks reasonable at the first glance (will need to see it in proper context for a
> final opinion), provided e.g. Xen heap pages can never be static.

If you don't prefer let free_domheap_pages to call free_domstatic_page, then, maybe
the if-array should happen at put_page
"
@@ -1622,6 +1622,8 @@ void put_page(struct page_info *page)

     if ( unlikely((nx & PGC_count_mask) == 0) )
     {
+        if ( unlikely(page->count_info & PGC_static) )
+            free_domstatic_page(page);
         free_domheap_page(page);
     }
 }
"
Wdyt now?
 
> 
> Jan

Re: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is static

[Jan Beulich]

On 29.06.2022 08:08, Penny Zheng wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Wednesday, June 29, 2022 1:56 PM
>>
>> On 29.06.2022 05:12, Penny Zheng wrote:
>>>> From: Julien Grall <julien@xen.org>
>>>> Sent: Monday, June 27, 2022 6:19 PM
>>>>
>>>> On 27/06/2022 11:03, Penny Zheng wrote:
>>>>>> -----Original Message-----
>>>>> put_static_pages, that is, adding pages to the reserved list, is
>>>>> only for freeing static pages on runtime. In static page
>>>>> initialization stage, I also use free_statimem_pages, and in which
>>>>> stage, I think the domain has not been constructed at all. So I
>>>>> prefer the freeing of staticmem pages is split into two parts:
>>>>> free_staticmem_pages and put_static_pages
>>>>
>>>> AFAIU, all the pages would have to be allocated via
>>>> acquire_domstatic_pages(). This call requires the domain to be
>>>> allocated and setup for handling memory.
>>>>
>>>> Therefore, I think the split is unnecessary. This would also have the
>>>> advantage to remove one loop. Admittly, this is not important when
>>>> the order 0, but it would become a problem for larger order (you may
>>>> have to pull the struct page_info multiple time in the cache).
>>>>
>>>
>>> How about this:
>>> I create a new func free_domstatic_page, and it will be like:
>>> "
>>> static void free_domstatic_page(struct domain *d, struct page_info
>>> *page) {
>>>     unsigned int i;
>>>     bool need_scrub;
>>>
>>>     /* NB. May recursively lock from relinquish_memory(). */
>>>     spin_lock_recursive(&d->page_alloc_lock);
>>>
>>>     arch_free_heap_page(d, page);
>>>
>>>     /*
>>>      * Normally we expect a domain to clear pages before freeing them,
>>>      * if it cares about the secrecy of their contents. However, after
>>>      * a domain has died we assume responsibility for erasure. We do
>>>      * scrub regardless if option scrub_domheap is set.
>>>      */
>>>     need_scrub = d->is_dying || scrub_debug || opt_scrub_domheap;
>>>
>>>     free_staticmem_pages(page, 1, need_scrub);
>>>
>>>     /* Add page on the resv_page_list *after* it has been freed. */
>>>     put_static_page(d, page);
>>>
>>>     drop_dom_ref = !domain_adjust_tot_pages(d, -1);
>>>
>>>     spin_unlock_recursive(&d->page_alloc_lock);
>>>
>>>     if ( drop_dom_ref )
>>>         put_domain(d);
>>> }
>>> "
>>>
>>> In free_domheap_pages, we just call free_domstatic_page:
>>>
>>> "
>>> @@ -2430,6 +2430,9 @@ void free_domheap_pages(struct page_info *pg,
>>> unsigned int order)
>>>
>>>      ASSERT_ALLOC_CONTEXT();
>>>
>>> +    if ( unlikely(pg->count_info & PGC_static) )
>>> +        return free_domstatic_page(d, pg);
>>> +
>>>      if ( unlikely(is_xen_heap_page(pg)) )
>>>      {
>>>          /* NB. May recursively lock from relinquish_memory(). */ @@
>>> -2673,6 +2676,38 @@ void free_staticmem_pages(struct page_info *pg,
>>> unsigned long nr_mfns, "
>>>
>>> Then the split could be avoided and we could save the loop as much as
>> possible.
>>> Any suggestion?
>>
>> Looks reasonable at the first glance (will need to see it in proper context for a
>> final opinion), provided e.g. Xen heap pages can never be static.
> 
> If you don't prefer let free_domheap_pages to call free_domstatic_page, then, maybe
> the if-array should happen at put_page
> "
> @@ -1622,6 +1622,8 @@ void put_page(struct page_info *page)
> 
>      if ( unlikely((nx & PGC_count_mask) == 0) )
>      {
> +        if ( unlikely(page->count_info & PGC_static) )
> +            free_domstatic_page(page);
>          free_domheap_page(page);
>      }
>  }
> "
> Wdyt now?

Personally I'd prefer this variant, but we'll have to see what Julien or
the other Arm maintainers think.

Jan

Re: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is static

[Julien Grall]

Hi Jan,

On 29/06/2022 07:19, Jan Beulich wrote:
> On 29.06.2022 08:08, Penny Zheng wrote:
>>> From: Jan Beulich <jbeulich@suse.com>
>>> Sent: Wednesday, June 29, 2022 1:56 PM
>>>
>>> On 29.06.2022 05:12, Penny Zheng wrote:
>>>>> From: Julien Grall <julien@xen.org>
>>>>> Sent: Monday, June 27, 2022 6:19 PM
>>>>>
>>>>> On 27/06/2022 11:03, Penny Zheng wrote:
>>>>>>> -----Original Message-----
>>>>>> put_static_pages, that is, adding pages to the reserved list, is
>>>>>> only for freeing static pages on runtime. In static page
>>>>>> initialization stage, I also use free_statimem_pages, and in which
>>>>>> stage, I think the domain has not been constructed at all. So I
>>>>>> prefer the freeing of staticmem pages is split into two parts:
>>>>>> free_staticmem_pages and put_static_pages
>>>>>
>>>>> AFAIU, all the pages would have to be allocated via
>>>>> acquire_domstatic_pages(). This call requires the domain to be
>>>>> allocated and setup for handling memory.
>>>>>
>>>>> Therefore, I think the split is unnecessary. This would also have the
>>>>> advantage to remove one loop. Admittly, this is not important when
>>>>> the order 0, but it would become a problem for larger order (you may
>>>>> have to pull the struct page_info multiple time in the cache).
>>>>>
>>>>
>>>> How about this:
>>>> I create a new func free_domstatic_page, and it will be like:
>>>> "
>>>> static void free_domstatic_page(struct domain *d, struct page_info
>>>> *page) {
>>>>      unsigned int i;
>>>>      bool need_scrub;
>>>>
>>>>      /* NB. May recursively lock from relinquish_memory(). */
>>>>      spin_lock_recursive(&d->page_alloc_lock);
>>>>
>>>>      arch_free_heap_page(d, page);
>>>>
>>>>      /*
>>>>       * Normally we expect a domain to clear pages before freeing them,
>>>>       * if it cares about the secrecy of their contents. However, after
>>>>       * a domain has died we assume responsibility for erasure. We do
>>>>       * scrub regardless if option scrub_domheap is set.
>>>>       */
>>>>      need_scrub = d->is_dying || scrub_debug || opt_scrub_domheap;
>>>>
>>>>      free_staticmem_pages(page, 1, need_scrub);
>>>>
>>>>      /* Add page on the resv_page_list *after* it has been freed. */
>>>>      put_static_page(d, page);
>>>>
>>>>      drop_dom_ref = !domain_adjust_tot_pages(d, -1);
>>>>
>>>>      spin_unlock_recursive(&d->page_alloc_lock);
>>>>
>>>>      if ( drop_dom_ref )
>>>>          put_domain(d);
>>>> }
>>>> "
>>>>
>>>> In free_domheap_pages, we just call free_domstatic_page:
>>>>
>>>> "
>>>> @@ -2430,6 +2430,9 @@ void free_domheap_pages(struct page_info *pg,
>>>> unsigned int order)
>>>>
>>>>       ASSERT_ALLOC_CONTEXT();
>>>>
>>>> +    if ( unlikely(pg->count_info & PGC_static) )
>>>> +        return free_domstatic_page(d, pg);
>>>> +
>>>>       if ( unlikely(is_xen_heap_page(pg)) )
>>>>       {
>>>>           /* NB. May recursively lock from relinquish_memory(). */ @@
>>>> -2673,6 +2676,38 @@ void free_staticmem_pages(struct page_info *pg,
>>>> unsigned long nr_mfns, "
>>>>
>>>> Then the split could be avoided and we could save the loop as much as
>>> possible.
>>>> Any suggestion?
>>>
>>> Looks reasonable at the first glance (will need to see it in proper context for a
>>> final opinion), provided e.g. Xen heap pages can never be static.
>>
>> If you don't prefer let free_domheap_pages to call free_domstatic_page, then, maybe
>> the if-array should happen at put_page
>> "
>> @@ -1622,6 +1622,8 @@ void put_page(struct page_info *page)
>>
>>       if ( unlikely((nx & PGC_count_mask) == 0) )
>>       {
>> +        if ( unlikely(page->count_info & PGC_static) )

At a first glance, this would likely need to be tested against 'nx'.

>> +            free_domstatic_page(page);
>>           free_domheap_page(page);
>>       }
>>   }
>> "
>> Wdyt now?
> 
> Personally I'd prefer this variant, but we'll have to see what Julien or
> the other Arm maintainers think.

I think this is fine so long we are not expecting more places where 
free_domheap_page() may need to be replaced with free_domstatic_page().

I can't think of any at the moment, but I would like Penny to confirm 
what Arm plans to do with static memory.

Cheers,

-- 
Julien Grall