All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v10 0/9] populate/unpopulate memory when domain on static allocation
@ 2022-08-16  2:36 Penny Zheng
  2022-08-16  2:36 ` [PATCH v10 1/9] xen/arm: rename PGC_reserved to PGC_static Penny Zheng
                   ` (9 more replies)
  0 siblings, 10 replies; 25+ messages in thread
From: Penny Zheng @ 2022-08-16  2:36 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
	Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Jan Beulich, Wei Liu

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.

---
v10 changes:
- let Arm keep #define PGC_static 0 private, with the generic fallback
remaining in page_alloc.c
- change ASSERT(d) to ASSERT_UNREACHABLE() to be more robust looking
forward, and also add a printk() to log the problem
- mention the the removal of #ifdef CONFIG_STATIC_MEMORY in commit
message
- commit message typo fix
- Do not skip the list addition in that one special case
- add lock on the fail path
- new commit "xen: rename free_staticmem_pages to unprepare_staticmem_pages"
---
v9 changes:
- move free_domheap_page into else-condition
- considering scrubbing static pages, domain dying case and opt_scrub_domheap
both do not apply to static pages.
- as unowned static pages don't make themselves to free_domstatic_page
at the moment, remove else-condition and add ASSERT(d) at the top of the
function
- remove macro helper put_static_page, and just expand its code inside
free_domstatic_page
- Use ASSERT_ALLOC_CONTEXT() in acquire_reserved_page
- Add free_staticmem_pages to undo prepare_staticmem_pages when
assign_domstatic_pages fails
- Remove redundant static in error message
---
v8 changes:
- introduce new helper free_domstatic_page
- let put_page call free_domstatic_page for static page, when last ref
drops
- #define PGC_static zero when !CONFIG_STATIC_MEMORY, as it is used
outside page_alloc.c
- #ifdef-ary around is_domain_using_staticmem() is not needed anymore
- order as a parameter is not needed here, as all staticmem operations are
limited to order-0 regions
- move d->page_alloc_lock after operation on d->resv_page_list
- As concurrent free/allocate could modify the resv_page_list, we still
need the lock
---
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: 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: unpopulate memory when domain is static
  xen: introduce prepare_staticmem_pages
  xen: retrieve reserved pages on populate_physmap
  xen: rename free_staticmem_pages to unprepare_staticmem_pages

 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     |  10 +-
 xen/arch/arm/mm.c                 |   5 +-
 xen/arch/arm/setup.c              |   3 +-
 xen/common/domain.c               |   7 ++
 xen/common/memory.c               |  23 ++++
 xen/common/page_alloc.c           | 192 ++++++++++++++++++++++--------
 xen/include/xen/domain.h          |   8 ++
 xen/include/xen/mm.h              |   8 +-
 xen/include/xen/sched.h           |   6 +
 12 files changed, 209 insertions(+), 63 deletions(-)

-- 
2.25.1



^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v10 1/9] xen/arm: rename PGC_reserved to PGC_static
  2022-08-16  2:36 [PATCH v10 0/9] populate/unpopulate memory when domain on static allocation Penny Zheng
@ 2022-08-16  2:36 ` Penny Zheng
  2022-08-16  2:36 ` [PATCH v10 2/9] xen: do not free reserved memory into heap Penny Zheng
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Penny Zheng @ 2022-08-16  2:36 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
	Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Jan Beulich, Wei Liu, Penny Zheng, Julien Grall

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>
Acked-by: Julien Grall <jgrall@amazon.com>
---
v10 changes:
- no change
---
v9 changes:
- no change
---
v8 changes:
- no change
---
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 6c0a3c789f..da25251cda 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -121,9 +121,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 8bdaffeb3d..00fa24e330 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
 
 #ifndef PGT_TYPE_INFO_INITIALIZER
@@ -2342,7 +2342,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++;
         }
@@ -2402,7 +2402,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]));
     }
@@ -2708,8 +2708,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;
     }
 }
 
@@ -2738,8 +2738,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",
@@ -2753,10 +2753,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 = PGT_TYPE_INFO_INITIALIZER;
         page_set_owner(&pg[i], NULL);
@@ -2778,7 +2778,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



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v10 2/9] xen: do not free reserved memory into heap
  2022-08-16  2:36 [PATCH v10 0/9] populate/unpopulate memory when domain on static allocation Penny Zheng
  2022-08-16  2:36 ` [PATCH v10 1/9] xen/arm: rename PGC_reserved to PGC_static Penny Zheng
@ 2022-08-16  2:36 ` Penny Zheng
  2022-08-16  6:40   ` Jan Beulich
  2022-08-16  2:36 ` [PATCH v10 3/9] xen: do not merge reserved pages in free_heap_pages() Penny Zheng
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Penny Zheng @ 2022-08-16  2:36 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
	Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Jan Beulich, Wei Liu, Penny Zheng, Julien Grall

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.

This commit introduces a new helper free_domstatic_page to free
static page in runtime, and free_staticmem_pages will be called by it
in runtime, so let's drop the __init flag.

Wrapper #ifdef CONFIG_STATIC_MEMORY around function declaration(
free_staticmem_pages, free_domstatic_page, etc) is kinds of redundant,
so we decide to remove it here.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
---
v10 changes:
- let Arm keep #define PGC_static 0 private, with the generic fallback
remaining in page_alloc.c
- change ASSERT(d) to ASSERT_UNREACHABLE() to be more robust looking
forward, and also add a printk() to log the problem
- mention the removal of #ifdef CONFIG_STATIC_MEMORY in commit message
---
v9 changes:
- move free_domheap_page into else-condition
- considering scrubbing static pages, domain dying case and opt_scrub_domheap
both donot apply to static pages.
- as unowned static pages don't make themselves to free_domstatic_page
at the moment, remove else-condition and add ASSERT(d) at the top of the
function
---
v8 changes:
- introduce new helper free_domstatic_page
- let put_page call free_domstatic_page for static page, when last ref
drops
- #define PGC_static zero when !CONFIG_STATIC_MEMORY, as it is used
outside page_alloc.c
---
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 |  6 +++++-
 xen/arch/arm/mm.c             |  5 ++++-
 xen/common/page_alloc.c       | 39 ++++++++++++++++++++++++++++++++---
 xen/include/xen/mm.h          |  3 +--
 4 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index da25251cda..749fbefa0c 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -121,9 +121,13 @@ 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)
+#else
+#define PGC_static     0
+#endif
 /* ... */
 /* Page is broken? */
 #define _PGC_broken       PG_shift(7)
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index b42cddb1b4..fbdab5598c 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1496,7 +1496,10 @@ void put_page(struct page_info *page)
 
     if ( unlikely((nx & PGC_count_mask) == 0) )
     {
-        free_domheap_page(page);
+        if ( unlikely(nx & PGC_static) )
+            free_domstatic_page(page);
+        else
+            free_domheap_page(page);
     }
 }
 
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 00fa24e330..5e97dcaa26 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2692,12 +2692,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));
@@ -2708,9 +2710,40 @@ 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);
+}
+
+void free_domstatic_page(struct page_info *page)
+{
+    struct domain *d = page_get_owner(page);
+    bool drop_dom_ref;
+
+    if ( unlikely(!d) )
+    {
+        ASSERT_UNREACHABLE();
+        printk("The about-to-free static page %"PRI_mfn" must be owned by a domain\n",
+               mfn_x(page_to_mfn(page)));
+        return;
+    }
+
+    ASSERT_ALLOC_CONTEXT();
+
+    /* NB. May recursively lock from relinquish_memory(). */
+    spin_lock_recursive(&d->page_alloc_lock);
+
+    arch_free_heap_page(d, page);
+
+    drop_dom_ref = !domain_adjust_tot_pages(d, -1);
+
+    spin_unlock_recursive(&d->page_alloc_lock);
+
+    free_staticmem_pages(page, 1, scrub_debug);
+
+    if ( drop_dom_ref )
+        put_domain(d);
 }
 
 /*
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 35b065146f..deadf4b2a1 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -85,13 +85,12 @@ 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);
+void free_domstatic_page(struct page_info *page);
 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



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v10 3/9] xen: do not merge reserved pages in free_heap_pages()
  2022-08-16  2:36 [PATCH v10 0/9] populate/unpopulate memory when domain on static allocation Penny Zheng
  2022-08-16  2:36 ` [PATCH v10 1/9] xen/arm: rename PGC_reserved to PGC_static Penny Zheng
  2022-08-16  2:36 ` [PATCH v10 2/9] xen: do not free reserved memory into heap Penny Zheng
@ 2022-08-16  2:36 ` Penny Zheng
  2022-08-16  2:36 ` [PATCH v10 4/9] xen: add field "flags" to cover all internal CDF_XXX Penny Zheng
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Penny Zheng @ 2022-08-16  2:36 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.chen, Penny Zheng, Andrew Cooper, George Dunlap, Jan Beulich,
	Julien Grall, Stefano Stabellini, Wei Liu, Julien Grall,
	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 accidentally 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_static 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>
---
v10 changes:
- commit message typo fix
---
v9 changes:
- no change
---
v8 changes:
- no change
---
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 5e97dcaa26..1be7f671dc 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_static) ||
                  (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_static) ||
                  (PFN_ORDER(successor) != order) ||
                  (phys_to_nid(page_to_maddr(successor)) != node) )
                 break;
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v10 4/9] xen: add field "flags" to cover all internal CDF_XXX
  2022-08-16  2:36 [PATCH v10 0/9] populate/unpopulate memory when domain on static allocation Penny Zheng
                   ` (2 preceding siblings ...)
  2022-08-16  2:36 ` [PATCH v10 3/9] xen: do not merge reserved pages in free_heap_pages() Penny Zheng
@ 2022-08-16  2:36 ` Penny Zheng
  2022-08-16  2:36 ` [PATCH v10 5/9] xen/arm: introduce CDF_staticmem Penny Zheng
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Penny Zheng @ 2022-08-16  2:36 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
	Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Jan Beulich, Wei Liu, Penny Zheng, Julien Grall

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>
---
v10 changes:
- no change
---
v9 changes:
- no change
---
v8 changes:
- no change
---
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 2cd481979c..a963884e81 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -712,8 +712,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 cd9ce19b4b..26a8348eed 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?
@@ -104,7 +104,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 618410e3b2..7062393e37 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 e2b3b6daa3..1cf629e7ec 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



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v10 5/9] xen/arm: introduce CDF_staticmem
  2022-08-16  2:36 [PATCH v10 0/9] populate/unpopulate memory when domain on static allocation Penny Zheng
                   ` (3 preceding siblings ...)
  2022-08-16  2:36 ` [PATCH v10 4/9] xen: add field "flags" to cover all internal CDF_XXX Penny Zheng
@ 2022-08-16  2:36 ` Penny Zheng
  2022-08-16  2:36 ` [PATCH v10 6/9] xen: unpopulate memory when domain is static Penny Zheng
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Penny Zheng @ 2022-08-16  2:36 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
	Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Jan Beulich, Wei Liu, Penny Zheng, Julien Grall

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>
Acked-by: Julien Grall <jgrall@amazon.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
v10 changes:
- no change
---
v9 changes:
- no change
---
v8 changes:
- #ifdef-ary around is_domain_using_staticmem() is not needed anymore
---
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    | 8 ++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 3fd1186b53..b76a84e8f5 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 628b14b086..2c8116afba 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -35,6 +35,14 @@ 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
+
+#define is_domain_using_staticmem(d) ((d)->cdf & CDF_staticmem)
 
 /*
  * Arch-specifics.
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v10 6/9] xen: unpopulate memory when domain is static
  2022-08-16  2:36 [PATCH v10 0/9] populate/unpopulate memory when domain on static allocation Penny Zheng
                   ` (4 preceding siblings ...)
  2022-08-16  2:36 ` [PATCH v10 5/9] xen/arm: introduce CDF_staticmem Penny Zheng
@ 2022-08-16  2:36 ` Penny Zheng
  2022-08-17  8:51   ` Jan Beulich
  2022-08-24  9:12   ` Julien Grall
  2022-08-16  2:36 ` [PATCH v10 7/9] xen: introduce prepare_staticmem_pages Penny Zheng
                   ` (3 subsequent siblings)
  9 siblings, 2 replies; 25+ messages in thread
From: Penny Zheng @ 2022-08-16  2:36 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.chen, Penny Zheng, Andrew Cooper, George Dunlap, Jan Beulich,
	Julien Grall, Stefano Stabellini, Wei Liu, 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>
---
v10 change:
- Do not skip the list addition in that one special case
---
v9 change:
- remove macro helper put_static_page, and just expand its code inside
free_domstatic_page
---
v8 changes:
- adapt this patch for newly introduced free_domstatic_page
- order as a parameter is not needed here, as all staticmem operations are
limited to order-0 regions
- move d->page_alloc_lock after operation on d->resv_page_list
---
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 | 7 +++++--
 xen/include/xen/sched.h | 3 +++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 7062393e37..c23f449451 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 1be7f671dc..25521af600 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2740,10 +2740,13 @@ void free_domstatic_page(struct page_info *page)
 
     drop_dom_ref = !domain_adjust_tot_pages(d, -1);
 
-    spin_unlock_recursive(&d->page_alloc_lock);
-
     free_staticmem_pages(page, 1, scrub_debug);
 
+    /* Add page on the resv_page_list *after* it has been freed. */
+    page_list_add_tail(page, &d->resv_page_list);
+
+    spin_unlock_recursive(&d->page_alloc_lock);
+
     if ( drop_dom_ref )
         put_domain(d);
 }
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 1cf629e7ec..956e0f9dca 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



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v10 7/9] xen: introduce prepare_staticmem_pages
  2022-08-16  2:36 [PATCH v10 0/9] populate/unpopulate memory when domain on static allocation Penny Zheng
                   ` (5 preceding siblings ...)
  2022-08-16  2:36 ` [PATCH v10 6/9] xen: unpopulate memory when domain is static Penny Zheng
@ 2022-08-16  2:36 ` Penny Zheng
  2022-08-16  2:36 ` [PATCH v10 8/9] xen: retrieve reserved pages on populate_physmap Penny Zheng
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Penny Zheng @ 2022-08-16  2:36 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.chen, Penny Zheng, Andrew Cooper, George Dunlap, Jan Beulich,
	Julien Grall, Stefano Stabellini, Wei Liu, Penny Zheng,
	Julien Grall

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>
Acked-by: Julien Grall <jgrall@amazon.com>
---
v10 changes:
- no change
---
v9 changes:
- no change
---
v8 changes:
- no change
---
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 25521af600..0ee697705c 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2751,26 +2751,13 @@ void free_domstatic_page(struct page_info *page)
         put_domain(d);
 }
 
-/*
- * 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);
 
@@ -2781,7 +2768,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;
         }
@@ -2805,6 +2792,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.
@@ -2813,14 +2832,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



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v10 8/9] xen: retrieve reserved pages on populate_physmap
  2022-08-16  2:36 [PATCH v10 0/9] populate/unpopulate memory when domain on static allocation Penny Zheng
                   ` (6 preceding siblings ...)
  2022-08-16  2:36 ` [PATCH v10 7/9] xen: introduce prepare_staticmem_pages Penny Zheng
@ 2022-08-16  2:36 ` Penny Zheng
  2022-08-17 10:04   ` Jan Beulich
  2022-08-16  2:36 ` [PATCH v10 9/9] xen: rename free_staticmem_pages to unprepare_staticmem_pages Penny Zheng
  2022-08-24  9:54 ` [PATCH v10 0/9] populate/unpopulate memory when domain on static allocation Julien Grall
  9 siblings, 1 reply; 25+ messages in thread
From: Penny Zheng @ 2022-08-16  2:36 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.chen, Penny Zheng, Andrew Cooper, George Dunlap, Jan Beulich,
	Julien Grall, Stefano Stabellini, Wei Liu, 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>
---
v10 changes:
- add lock on the fail path
---
v9 changes:
- Use ASSERT_ALLOC_CONTEXT() in acquire_reserved_page
- Add free_staticmem_pages to undo prepare_staticmem_pages when
assign_domstatic_pages
- Remove redundant static in error message
---
v8 changes:
- As concurrent free/allocate could modify the resv_page_list, we still
need the lock
---
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 | 76 ++++++++++++++++++++++++++++++++---------
 xen/include/xen/mm.h    |  1 +
 3 files changed, 83 insertions(+), 17 deletions(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index bc89442ba5..ae8163a738 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 %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 0ee697705c..9c6d369d10 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2751,9 +2751,8 @@ void free_domstatic_page(struct page_info *page)
         put_domain(d);
 }
 
-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;
@@ -2834,21 +2833,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)) )
     {
         /*
@@ -2867,6 +2854,61 @@ 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;
+
+    ASSERT_ALLOC_CONTEXT();
+
+    /* Acquire a page from reserved page list(resv_page_list). */
+    spin_lock(&d->page_alloc_lock);
+    page = page_list_remove_head(&d->resv_page_list);
+    spin_unlock(&d->page_alloc_lock);
+    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_assign;
+
+    return page_to_mfn(page);
+
+ fail_assign:
+    free_staticmem_pages(page, 1, memflags & MEMF_no_scrub);
+ fail:
+    spin_lock(&d->page_alloc_lock);
+    page_list_add_tail(page, &d->resv_page_list);
+    spin_unlock(&d->page_alloc_lock);
+    return INVALID_MFN;
+}
 #endif
 
 /*
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index deadf4b2a1..5d29aea7ad 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -198,6 +198,7 @@ struct npfec {
 #else
 #define MAX_ORDER 20 /* 2^20 contiguous pages */
 #endif
+mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags);
 
 /* Private domain structs for DOMID_XEN, DOMID_IO, etc. */
 extern struct domain *dom_xen, *dom_io;
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v10 9/9] xen: rename free_staticmem_pages to unprepare_staticmem_pages
  2022-08-16  2:36 [PATCH v10 0/9] populate/unpopulate memory when domain on static allocation Penny Zheng
                   ` (7 preceding siblings ...)
  2022-08-16  2:36 ` [PATCH v10 8/9] xen: retrieve reserved pages on populate_physmap Penny Zheng
@ 2022-08-16  2:36 ` Penny Zheng
  2022-08-17 10:07   ` Jan Beulich
  2022-08-24  9:54 ` [PATCH v10 0/9] populate/unpopulate memory when domain on static allocation Julien Grall
  9 siblings, 1 reply; 25+ messages in thread
From: Penny Zheng @ 2022-08-16  2:36 UTC (permalink / raw)
  To: xen-devel
  Cc: wei.chen, Penny Zheng, Stefano Stabellini, Julien Grall,
	Bertrand Marquis, Volodymyr Babchuk, Andrew Cooper,
	George Dunlap, Jan Beulich, Wei Liu, Penny Zheng

The name of free_staticmem_pages is inappropriate, considering it is
the opposite of function prepare_staticmem_pages.

Rename free_staticmem_pages to unprepare_staticmem_pages.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
v10 changes:
- new commit
---
 xen/arch/arm/setup.c    |  3 ++-
 xen/common/page_alloc.c | 15 +++++++++------
 xen/include/xen/mm.h    |  4 ++--
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 500307edc0..4662997c7e 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -639,7 +639,8 @@ static void __init init_staticmem_pages(void)
             if ( mfn_x(bank_end) <= mfn_x(bank_start) )
                 return;
 
-            free_staticmem_pages(mfn_to_page(bank_start), bank_pages, false);
+            unprepare_staticmem_pages(mfn_to_page(bank_start),
+                                      bank_pages, false);
         }
     }
 #endif
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 9c6d369d10..7306d69129 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2693,9 +2693,12 @@ 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 free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
-                          bool need_scrub)
+/*
+ * It is the opposite of prepare_staticmem_pages, and it aims to unprepare
+ * nr_mfns pages of static memory.
+ */
+void unprepare_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
+                               bool need_scrub)
 {
     mfn_t mfn = page_to_mfn(pg);
     unsigned long i;
@@ -2740,7 +2743,7 @@ void free_domstatic_page(struct page_info *page)
 
     drop_dom_ref = !domain_adjust_tot_pages(d, -1);
 
-    free_staticmem_pages(page, 1, scrub_debug);
+    unprepare_staticmem_pages(page, 1, scrub_debug);
 
     /* Add page on the resv_page_list *after* it has been freed. */
     page_list_add_tail(page, &d->resv_page_list);
@@ -2848,7 +2851,7 @@ static int assign_domstatic_pages(struct domain *d, struct page_info *pg,
 
     if ( assign_pages(pg, nr_mfns, d, memflags) )
     {
-        free_staticmem_pages(pg, nr_mfns, memflags & MEMF_no_scrub);
+        unprepare_staticmem_pages(pg, nr_mfns, memflags & MEMF_no_scrub);
         return -EINVAL;
     }
 
@@ -2902,7 +2905,7 @@ mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
     return page_to_mfn(page);
 
  fail_assign:
-    free_staticmem_pages(page, 1, memflags & MEMF_no_scrub);
+    unprepare_staticmem_pages(page, 1, memflags & MEMF_no_scrub);
  fail:
     spin_lock(&d->page_alloc_lock);
     page_list_add_tail(page, &d->resv_page_list);
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 5d29aea7ad..a925028ab3 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -86,8 +86,8 @@ bool scrub_free_pages(void);
 #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
 
 /* These functions are for static memory */
-void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
-                          bool need_scrub);
+void unprepare_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
+                               bool need_scrub);
 void free_domstatic_page(struct page_info *page);
 int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int nr_mfns,
                             unsigned int memflags);
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH v10 2/9] xen: do not free reserved memory into heap
  2022-08-16  2:36 ` [PATCH v10 2/9] xen: do not free reserved memory into heap Penny Zheng
@ 2022-08-16  6:40   ` Jan Beulich
  2022-08-24  9:03     ` Julien Grall
  0 siblings, 1 reply; 25+ messages in thread
From: Jan Beulich @ 2022-08-16  6:40 UTC (permalink / raw)
  To: Penny Zheng
  Cc: wei.chen, Stefano Stabellini, Julien Grall, Bertrand Marquis,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
	Julien Grall, xen-devel

On 16.08.2022 04:36, Penny Zheng wrote:
> +void free_domstatic_page(struct page_info *page)
> +{
> +    struct domain *d = page_get_owner(page);
> +    bool drop_dom_ref;
> +
> +    if ( unlikely(!d) )
> +    {
> +        ASSERT_UNREACHABLE();
> +        printk("The about-to-free static page %"PRI_mfn" must be owned by a domain\n",
> +               mfn_x(page_to_mfn(page)));
> +        return;
> +    }

For the message to be useful as a hint if the assertion triggers, it
wants printing ahead of the assertion. I also think it wants to be a
XENLOG_G_* kind of log level, so it would be rate limited by default
in release builds. Just to be on the safe side. (I'm not in favor of
the log message in the first place, but I do know that Julien had
asked for one.)

Jan


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v10 6/9] xen: unpopulate memory when domain is static
  2022-08-16  2:36 ` [PATCH v10 6/9] xen: unpopulate memory when domain is static Penny Zheng
@ 2022-08-17  8:51   ` Jan Beulich
  2022-08-24  9:12   ` Julien Grall
  1 sibling, 0 replies; 25+ messages in thread
From: Jan Beulich @ 2022-08-17  8:51 UTC (permalink / raw)
  To: Penny Zheng
  Cc: wei.chen, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu, xen-devel

On 16.08.2022 04:36, Penny Zheng wrote:
> 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.

The last part of this sentence (which afaict was backwards anyway) is
now stale and should be removed. Then ...

> Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v10 8/9] xen: retrieve reserved pages on populate_physmap
  2022-08-16  2:36 ` [PATCH v10 8/9] xen: retrieve reserved pages on populate_physmap Penny Zheng
@ 2022-08-17 10:04   ` Jan Beulich
  2022-09-05  7:08     ` Penny Zheng
  0 siblings, 1 reply; 25+ messages in thread
From: Jan Beulich @ 2022-08-17 10:04 UTC (permalink / raw)
  To: Penny Zheng
  Cc: wei.chen, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu, xen-devel

On 16.08.2022 04:36, Penny Zheng wrote:
> @@ -2867,6 +2854,61 @@ 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;
> +
> +    ASSERT_ALLOC_CONTEXT();
> +
> +    /* Acquire a page from reserved page list(resv_page_list). */
> +    spin_lock(&d->page_alloc_lock);
> +    page = page_list_remove_head(&d->resv_page_list);
> +    spin_unlock(&d->page_alloc_lock);
> +    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_assign;
> +
> +    return page_to_mfn(page);
> +
> + fail_assign:
> +    free_staticmem_pages(page, 1, memflags & MEMF_no_scrub);

Doesn't this need to be !(memflags & MEMF_no_scrub)? And then -
with assignment having failed and with it being just a single page
we're talking about, the page was not exposed to the guest at any
point afaict. So I don't see the need for scrubbing in the first
place.

Also I think the rename of the function would better be done first,
since then you wouldn't need to touch this line again right in the
next patch, and the prepare/unprepare pairing would also be visible
right here. This would then also better fit with the introduction
of prepare_*() in the previous patch (which, afaic, the name
change could also be merged into; FTAOD I don't mind it to be
separate).

Jan


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v10 9/9] xen: rename free_staticmem_pages to unprepare_staticmem_pages
  2022-08-16  2:36 ` [PATCH v10 9/9] xen: rename free_staticmem_pages to unprepare_staticmem_pages Penny Zheng
@ 2022-08-17 10:07   ` Jan Beulich
  0 siblings, 0 replies; 25+ messages in thread
From: Jan Beulich @ 2022-08-17 10:07 UTC (permalink / raw)
  To: Penny Zheng
  Cc: wei.chen, Stefano Stabellini, Julien Grall, Bertrand Marquis,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
	xen-devel

On 16.08.2022 04:36, Penny Zheng wrote:
> The name of free_staticmem_pages is inappropriate, considering it is
> the opposite of function prepare_staticmem_pages.
> 
> Rename free_staticmem_pages to unprepare_staticmem_pages.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
preferably, as said there, with it moved ahead of the previous patch.

Jan


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v10 2/9] xen: do not free reserved memory into heap
  2022-08-16  6:40   ` Jan Beulich
@ 2022-08-24  9:03     ` Julien Grall
  2022-08-24  9:27       ` Juergen Gross
  2022-08-24 10:42       ` Jan Beulich
  0 siblings, 2 replies; 25+ messages in thread
From: Julien Grall @ 2022-08-24  9:03 UTC (permalink / raw)
  To: Jan Beulich, Penny Zheng
  Cc: wei.chen, Stefano Stabellini, Bertrand Marquis,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
	Julien Grall, xen-devel

Hi,

On 16/08/2022 07:40, Jan Beulich wrote:
> On 16.08.2022 04:36, Penny Zheng wrote:
>> +void free_domstatic_page(struct page_info *page)
>> +{
>> +    struct domain *d = page_get_owner(page);
>> +    bool drop_dom_ref;
>> +
>> +    if ( unlikely(!d) )
>> +    {
>> +        ASSERT_UNREACHABLE();
>> +        printk("The about-to-free static page %"PRI_mfn" must be owned by a domain\n",
>> +               mfn_x(page_to_mfn(page)));
>> +        return;
>> +    }
> 
> For the message to be useful as a hint if the assertion triggers, it
> wants printing ahead of the assertion. I also think it wants to be a
> XENLOG_G_* kind of log level, so it would be rate limited by default
> in release builds. Just to be on the safe side.

+1

> (I'm not in favor of
> the log message in the first place, but I do know that Julien had
> asked for one.)
TBH, I think all ASSERT_UNREACHABLE() paths should be accompanied with a 
printk(). This would also allow us to catch issue in production rather 
than in only in debug.

In particular, for the page allocator, I have seen issue that only 
happen very rarely. Although, this seems unlikely in this case.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v10 6/9] xen: unpopulate memory when domain is static
  2022-08-16  2:36 ` [PATCH v10 6/9] xen: unpopulate memory when domain is static Penny Zheng
  2022-08-17  8:51   ` Jan Beulich
@ 2022-08-24  9:12   ` Julien Grall
  1 sibling, 0 replies; 25+ messages in thread
From: Julien Grall @ 2022-08-24  9:12 UTC (permalink / raw)
  To: Penny Zheng, xen-devel
  Cc: wei.chen, Andrew Cooper, George Dunlap, Jan Beulich,
	Stefano Stabellini, Wei Liu

Hi Penny,

On 16/08/2022 03:36, Penny Zheng wrote:
> 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>

Other than Jan's comment:

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

AFAICT, patch #2 needs some tweak. So I assuming this will need a 
respin. If not, I can possibly handle the change while committing if you 
provide a new version of the commit message.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v10 2/9] xen: do not free reserved memory into heap
  2022-08-24  9:03     ` Julien Grall
@ 2022-08-24  9:27       ` Juergen Gross
  2022-08-24  9:31         ` Julien Grall
  2022-08-24 10:42       ` Jan Beulich
  1 sibling, 1 reply; 25+ messages in thread
From: Juergen Gross @ 2022-08-24  9:27 UTC (permalink / raw)
  To: Julien Grall, Jan Beulich, Penny Zheng
  Cc: wei.chen, Stefano Stabellini, Bertrand Marquis,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
	Julien Grall, xen-devel


[-- Attachment #1.1.1: Type: text/plain, Size: 2206 bytes --]

On 24.08.22 11:03, Julien Grall wrote:
> Hi,
> 
> On 16/08/2022 07:40, Jan Beulich wrote:
>> On 16.08.2022 04:36, Penny Zheng wrote:
>>> +void free_domstatic_page(struct page_info *page)
>>> +{
>>> +    struct domain *d = page_get_owner(page);
>>> +    bool drop_dom_ref;
>>> +
>>> +    if ( unlikely(!d) )
>>> +    {
>>> +        ASSERT_UNREACHABLE();
>>> +        printk("The about-to-free static page %"PRI_mfn" must be owned by a 
>>> domain\n",
>>> +               mfn_x(page_to_mfn(page)));
>>> +        return;
>>> +    }
>>
>> For the message to be useful as a hint if the assertion triggers, it
>> wants printing ahead of the assertion. I also think it wants to be a
>> XENLOG_G_* kind of log level, so it would be rate limited by default
>> in release builds. Just to be on the safe side.
> 
> +1
> 
>> (I'm not in favor of
>> the log message in the first place, but I do know that Julien had
>> asked for one.)
> TBH, I think all ASSERT_UNREACHABLE() paths should be accompanied with a 
> printk(). This would also allow us to catch issue in production rather than in 
> only in debug.

What about something like the following then?

--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -40,6 +40,16 @@
      unlikely(ret_warn_on_);             \
  })

+#define WARN_ONCE() do {                \
+    static bool warned = false;         \
+                                        \
+    if ( !warned )                      \
+    {                                   \
+        warned = true;                  \
+        WARN();                         \
+    }                                   \
+} while (0)
+
  /* All clang versions supported by Xen have _Static_assert. */
  #if defined(__clang__) || \
      (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6))
@@ -63,7 +73,7 @@
  #define ASSERT_UNREACHABLE() assert_failed("unreachable")
  #else
  #define ASSERT(p) do { if ( 0 && (p) ) {} } while (0)
-#define ASSERT_UNREACHABLE() do { } while (0)
+#define ASSERT_UNREACHABLE() WARN_ONCE()
  #endif

  #define ABS(_x) ({                              \


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v10 2/9] xen: do not free reserved memory into heap
  2022-08-24  9:27       ` Juergen Gross
@ 2022-08-24  9:31         ` Julien Grall
  2022-08-24  9:38           ` Juergen Gross
  0 siblings, 1 reply; 25+ messages in thread
From: Julien Grall @ 2022-08-24  9:31 UTC (permalink / raw)
  To: Juergen Gross, Jan Beulich, Penny Zheng
  Cc: wei.chen, Stefano Stabellini, Bertrand Marquis,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
	Julien Grall, xen-devel

Hi Juergen,

On 24/08/2022 10:27, Juergen Gross wrote:
> On 24.08.22 11:03, Julien Grall wrote:
>> Hi,
>>
>> On 16/08/2022 07:40, Jan Beulich wrote:
>>> On 16.08.2022 04:36, Penny Zheng wrote:
>>>> +void free_domstatic_page(struct page_info *page)
>>>> +{
>>>> +    struct domain *d = page_get_owner(page);
>>>> +    bool drop_dom_ref;
>>>> +
>>>> +    if ( unlikely(!d) )
>>>> +    {
>>>> +        ASSERT_UNREACHABLE();
>>>> +        printk("The about-to-free static page %"PRI_mfn" must be 
>>>> owned by a domain\n",
>>>> +               mfn_x(page_to_mfn(page)));
>>>> +        return;
>>>> +    }
>>>
>>> For the message to be useful as a hint if the assertion triggers, it
>>> wants printing ahead of the assertion. I also think it wants to be a
>>> XENLOG_G_* kind of log level, so it would be rate limited by default
>>> in release builds. Just to be on the safe side.
>>
>> +1
>>
>>> (I'm not in favor of
>>> the log message in the first place, but I do know that Julien had
>>> asked for one.)
>> TBH, I think all ASSERT_UNREACHABLE() paths should be accompanied with 
>> a printk(). This would also allow us to catch issue in production 
>> rather than in only in debug.
> 
> What about something like the following then?

That could be a first step. I still think a message like Penny has added 
in the patch is useful.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v10 2/9] xen: do not free reserved memory into heap
  2022-08-24  9:31         ` Julien Grall
@ 2022-08-24  9:38           ` Juergen Gross
  0 siblings, 0 replies; 25+ messages in thread
From: Juergen Gross @ 2022-08-24  9:38 UTC (permalink / raw)
  To: Julien Grall, Jan Beulich, Penny Zheng
  Cc: wei.chen, Stefano Stabellini, Bertrand Marquis,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
	Julien Grall, xen-devel


[-- Attachment #1.1.1: Type: text/plain, Size: 1676 bytes --]

On 24.08.22 11:31, Julien Grall wrote:
> Hi Juergen,
> 
> On 24/08/2022 10:27, Juergen Gross wrote:
>> On 24.08.22 11:03, Julien Grall wrote:
>>> Hi,
>>>
>>> On 16/08/2022 07:40, Jan Beulich wrote:
>>>> On 16.08.2022 04:36, Penny Zheng wrote:
>>>>> +void free_domstatic_page(struct page_info *page)
>>>>> +{
>>>>> +    struct domain *d = page_get_owner(page);
>>>>> +    bool drop_dom_ref;
>>>>> +
>>>>> +    if ( unlikely(!d) )
>>>>> +    {
>>>>> +        ASSERT_UNREACHABLE();
>>>>> +        printk("The about-to-free static page %"PRI_mfn" must be owned by 
>>>>> a domain\n",
>>>>> +               mfn_x(page_to_mfn(page)));
>>>>> +        return;
>>>>> +    }
>>>>
>>>> For the message to be useful as a hint if the assertion triggers, it
>>>> wants printing ahead of the assertion. I also think it wants to be a
>>>> XENLOG_G_* kind of log level, so it would be rate limited by default
>>>> in release builds. Just to be on the safe side.
>>>
>>> +1
>>>
>>>> (I'm not in favor of
>>>> the log message in the first place, but I do know that Julien had
>>>> asked for one.)
>>> TBH, I think all ASSERT_UNREACHABLE() paths should be accompanied with a 
>>> printk(). This would also allow us to catch issue in production rather than 
>>> in only in debug.
>>
>> What about something like the following then?
> 
> That could be a first step. I still think a message like Penny has added in the 
> patch is useful.

As the WARN() would spit out file and line, a comment might be enough.

In the end that is something the maintainer of the related code should decide.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v10 0/9] populate/unpopulate memory when domain on static allocation
  2022-08-16  2:36 [PATCH v10 0/9] populate/unpopulate memory when domain on static allocation Penny Zheng
                   ` (8 preceding siblings ...)
  2022-08-16  2:36 ` [PATCH v10 9/9] xen: rename free_staticmem_pages to unprepare_staticmem_pages Penny Zheng
@ 2022-08-24  9:54 ` Julien Grall
  9 siblings, 0 replies; 25+ messages in thread
From: Julien Grall @ 2022-08-24  9:54 UTC (permalink / raw)
  To: Penny Zheng, xen-devel
  Cc: wei.chen, Stefano Stabellini, Bertrand Marquis,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Jan Beulich,
	Wei Liu

Hi Penny,

On 16/08/2022 03:36, Penny Zheng wrote:
> Penny Zheng (9):
>    xen/arm: rename PGC_reserved to PGC_static

This was committed by Jan a week ago. And ...


>    xen: do not merge reserved pages in free_heap_pages()
>    xen: add field "flags" to cover all internal CDF_XXX

... I have committed those two.

Cheers,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v10 2/9] xen: do not free reserved memory into heap
  2022-08-24  9:03     ` Julien Grall
  2022-08-24  9:27       ` Juergen Gross
@ 2022-08-24 10:42       ` Jan Beulich
  1 sibling, 0 replies; 25+ messages in thread
From: Jan Beulich @ 2022-08-24 10:42 UTC (permalink / raw)
  To: Julien Grall, Penny Zheng
  Cc: wei.chen, Stefano Stabellini, Bertrand Marquis,
	Volodymyr Babchuk, Andrew Cooper, George Dunlap, Wei Liu,
	Julien Grall, xen-devel

On 24.08.2022 11:03, Julien Grall wrote:
> Hi,
> 
> On 16/08/2022 07:40, Jan Beulich wrote:
>> On 16.08.2022 04:36, Penny Zheng wrote:
>>> +void free_domstatic_page(struct page_info *page)
>>> +{
>>> +    struct domain *d = page_get_owner(page);
>>> +    bool drop_dom_ref;
>>> +
>>> +    if ( unlikely(!d) )
>>> +    {
>>> +        ASSERT_UNREACHABLE();
>>> +        printk("The about-to-free static page %"PRI_mfn" must be owned by a domain\n",
>>> +               mfn_x(page_to_mfn(page)));
>>> +        return;
>>> +    }
>>
>> For the message to be useful as a hint if the assertion triggers, it
>> wants printing ahead of the assertion. I also think it wants to be a
>> XENLOG_G_* kind of log level, so it would be rate limited by default
>> in release builds. Just to be on the safe side.
> 
> +1
> 
>> (I'm not in favor of
>> the log message in the first place, but I do know that Julien had
>> asked for one.)
> TBH, I think all ASSERT_UNREACHABLE() paths should be accompanied with a 
> printk().

If you want more than just the line number, use ASSERT() with a meaningful
expression. That'll be easily a fair replacement for a separate printk().
And no, as said in reply to Jürgen's patch, ASSERT() and friends should
not leave any traces in non-debug builds. If you want such, use WARN_ON()
or BUG_ON().

Jan


^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH v10 8/9] xen: retrieve reserved pages on populate_physmap
  2022-08-17 10:04   ` Jan Beulich
@ 2022-09-05  7:08     ` Penny Zheng
  2022-09-06  6:33       ` Jan Beulich
  0 siblings, 1 reply; 25+ messages in thread
From: Penny Zheng @ 2022-09-05  7:08 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Wei Chen, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu, xen-devel

Hi jan 

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, August 17, 2022 6:05 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 v10 8/9] xen: retrieve reserved pages on
> populate_physmap
> 
> On 16.08.2022 04:36, Penny Zheng wrote:
> > @@ -2867,6 +2854,61 @@ 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;
> > +
> > +    ASSERT_ALLOC_CONTEXT();
> > +
> > +    /* Acquire a page from reserved page list(resv_page_list). */
> > +    spin_lock(&d->page_alloc_lock);
> > +    page = page_list_remove_head(&d->resv_page_list);
> > +    spin_unlock(&d->page_alloc_lock);
> > +    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_assign;
> > +
> > +    return page_to_mfn(page);
> > +
> > + fail_assign:
> > +    free_staticmem_pages(page, 1, memflags & MEMF_no_scrub);
> 
> Doesn't this need to be !(memflags & MEMF_no_scrub)? And then - with

I got a bit confused about this flag MEMF_no_scrub, does it mean no need
to scrub? 
Since I saw that in alloc_domheap_pages(...)
    if ( assign_page(pg, order, d, memflags) )
    {
        free_heap_pages(pg, order, memflags & MEMF_no_scrub);
        return NULL;
    }
It doesn't contain exclamation mark too...

> assignment having failed and with it being just a single page we're talking
> about, the page was not exposed to the guest at any point afaict. So I don't
> see the need for scrubbing in the first place.
> 
> Also I think the rename of the function would better be done first, since then
> you wouldn't need to touch this line again right in the next patch, and the
> prepare/unprepare pairing would also be visible right here. This would then
> also better fit with the introduction of prepare_*() in the previous patch
> (which, afaic, the name change could also be merged into; FTAOD I don't
> mind it to be separate).
> 
> Jan

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v10 8/9] xen: retrieve reserved pages on populate_physmap
  2022-09-05  7:08     ` Penny Zheng
@ 2022-09-06  6:33       ` Jan Beulich
  2022-09-06  7:14         ` Penny Zheng
  0 siblings, 1 reply; 25+ messages in thread
From: Jan Beulich @ 2022-09-06  6:33 UTC (permalink / raw)
  To: Penny Zheng
  Cc: Wei Chen, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu, xen-devel

On 05.09.2022 09:08, Penny Zheng wrote:
> Hi jan 
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Wednesday, August 17, 2022 6:05 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 v10 8/9] xen: retrieve reserved pages on
>> populate_physmap
>>
>> On 16.08.2022 04:36, Penny Zheng wrote:
>>> @@ -2867,6 +2854,61 @@ 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;
>>> +
>>> +    ASSERT_ALLOC_CONTEXT();
>>> +
>>> +    /* Acquire a page from reserved page list(resv_page_list). */
>>> +    spin_lock(&d->page_alloc_lock);
>>> +    page = page_list_remove_head(&d->resv_page_list);
>>> +    spin_unlock(&d->page_alloc_lock);
>>> +    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_assign;
>>> +
>>> +    return page_to_mfn(page);
>>> +
>>> + fail_assign:
>>> +    free_staticmem_pages(page, 1, memflags & MEMF_no_scrub);
>>
>> Doesn't this need to be !(memflags & MEMF_no_scrub)? And then - with
> 
> I got a bit confused about this flag MEMF_no_scrub, does it mean no need
> to scrub? 

Yes, as its name says.

> Since I saw that in alloc_domheap_pages(...)
>     if ( assign_page(pg, order, d, memflags) )
>     {
>         free_heap_pages(pg, order, memflags & MEMF_no_scrub);
>         return NULL;
>     }
> It doesn't contain exclamation mark too...

Hmm, you're right - on these error paths the scrubbing is needed if
the page wasn't previously scrubbed, as part of the set of pages may
have been transiently exposed to the guest (and by guessing it may
have been able to actually access the pages; I'm inclined to say it's
its own fault though if that way information is being leaked).

But ...

>> assignment having failed and with it being just a single page we're talking
>> about, the page was not exposed to the guest at any point afaict. So I don't
>> see the need for scrubbing in the first place.

while my comment wasn't really correct, as said - you don't need any
scrubbing here at all, I think.

Jan


^ permalink raw reply	[flat|nested] 25+ messages in thread

* RE: [PATCH v10 8/9] xen: retrieve reserved pages on populate_physmap
  2022-09-06  6:33       ` Jan Beulich
@ 2022-09-06  7:14         ` Penny Zheng
  2022-09-06  7:19           ` Jan Beulich
  0 siblings, 1 reply; 25+ messages in thread
From: Penny Zheng @ 2022-09-06  7:14 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Wei Chen, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu, xen-devel

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, September 6, 2022 2:34 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 v10 8/9] xen: retrieve reserved pages on
> populate_physmap
> 
> On 05.09.2022 09:08, Penny Zheng wrote:
> > Hi jan
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Wednesday, August 17, 2022 6:05 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 v10 8/9] xen: retrieve reserved pages on
> >> populate_physmap
> >>
> >> On 16.08.2022 04:36, Penny Zheng wrote:
> >>> @@ -2867,6 +2854,61 @@ 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;
> >>> +
> >>> +    ASSERT_ALLOC_CONTEXT();
> >>> +
> >>> +    /* Acquire a page from reserved page list(resv_page_list). */
> >>> +    spin_lock(&d->page_alloc_lock);
> >>> +    page = page_list_remove_head(&d->resv_page_list);
> >>> +    spin_unlock(&d->page_alloc_lock);
> >>> +    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_assign;
> >>> +
> >>> +    return page_to_mfn(page);
> >>> +
> >>> + fail_assign:
> >>> +    free_staticmem_pages(page, 1, memflags & MEMF_no_scrub);
> >>
> >> Doesn't this need to be !(memflags & MEMF_no_scrub)? And then - with
> >
> > I got a bit confused about this flag MEMF_no_scrub, does it mean no
> > need to scrub?
> 
> Yes, as its name says.
> 
> > Since I saw that in alloc_domheap_pages(...)
> >     if ( assign_page(pg, order, d, memflags) )
> >     {
> >         free_heap_pages(pg, order, memflags & MEMF_no_scrub);
> >         return NULL;
> >     }
> > It doesn't contain exclamation mark too...
> 
> Hmm, you're right - on these error paths the scrubbing is needed if the page
> wasn't previously scrubbed, as part of the set of pages may have been
> transiently exposed to the guest (and by guessing it may have been able to
> actually access the pages; I'm inclined to say it's its own fault though if that
> way information is being leaked).
> 

Then, the same for the acquire_domstatic_pages(...)

    if ( assign_pages(pg, nr_mfns, d, memflags) )
    {
        free_staticmem_pages(pg, nr_mfns, memflags & MEMF_no_scrub);
        return -EINVAL;
    }
On this error path, it has misused the MEMF_no_scrub too.
But IMO, as we are talking about these pages will always be reserved to the guest,
maybe here it also doesn't need scrubbing at all?
 
> But ...
> 
> >> assignment having failed and with it being just a single page we're
> >> talking about, the page was not exposed to the guest at any point
> >> afaict. So I don't see the need for scrubbing in the first place.
> 
> while my comment wasn't really correct, as said - you don't need any
> scrubbing here at all, I think.
> 
> Jan

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v10 8/9] xen: retrieve reserved pages on populate_physmap
  2022-09-06  7:14         ` Penny Zheng
@ 2022-09-06  7:19           ` Jan Beulich
  0 siblings, 0 replies; 25+ messages in thread
From: Jan Beulich @ 2022-09-06  7:19 UTC (permalink / raw)
  To: Penny Zheng
  Cc: Wei Chen, Andrew Cooper, George Dunlap, Julien Grall,
	Stefano Stabellini, Wei Liu, xen-devel

On 06.09.2022 09:14, Penny Zheng wrote:
> Hi Jan
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Tuesday, September 6, 2022 2:34 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 v10 8/9] xen: retrieve reserved pages on
>> populate_physmap
>>
>> On 05.09.2022 09:08, Penny Zheng wrote:
>>> Hi jan
>>>
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: Wednesday, August 17, 2022 6:05 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 v10 8/9] xen: retrieve reserved pages on
>>>> populate_physmap
>>>>
>>>> On 16.08.2022 04:36, Penny Zheng wrote:
>>>>> @@ -2867,6 +2854,61 @@ 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;
>>>>> +
>>>>> +    ASSERT_ALLOC_CONTEXT();
>>>>> +
>>>>> +    /* Acquire a page from reserved page list(resv_page_list). */
>>>>> +    spin_lock(&d->page_alloc_lock);
>>>>> +    page = page_list_remove_head(&d->resv_page_list);
>>>>> +    spin_unlock(&d->page_alloc_lock);
>>>>> +    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_assign;
>>>>> +
>>>>> +    return page_to_mfn(page);
>>>>> +
>>>>> + fail_assign:
>>>>> +    free_staticmem_pages(page, 1, memflags & MEMF_no_scrub);
>>>>
>>>> Doesn't this need to be !(memflags & MEMF_no_scrub)? And then - with
>>>
>>> I got a bit confused about this flag MEMF_no_scrub, does it mean no
>>> need to scrub?
>>
>> Yes, as its name says.
>>
>>> Since I saw that in alloc_domheap_pages(...)
>>>     if ( assign_page(pg, order, d, memflags) )
>>>     {
>>>         free_heap_pages(pg, order, memflags & MEMF_no_scrub);
>>>         return NULL;
>>>     }
>>> It doesn't contain exclamation mark too...
>>
>> Hmm, you're right - on these error paths the scrubbing is needed if the page
>> wasn't previously scrubbed, as part of the set of pages may have been
>> transiently exposed to the guest (and by guessing it may have been able to
>> actually access the pages; I'm inclined to say it's its own fault though if that
>> way information is being leaked).
>>
> 
> Then, the same for the acquire_domstatic_pages(...)
> 
>     if ( assign_pages(pg, nr_mfns, d, memflags) )
>     {
>         free_staticmem_pages(pg, nr_mfns, memflags & MEMF_no_scrub);
>         return -EINVAL;
>     }
> On this error path, it has misused the MEMF_no_scrub too.

Why do you say "misused"?

> But IMO, as we are talking about these pages will always be reserved to the guest,
> maybe here it also doesn't need scrubbing at all?

Perhaps. It feels as if we had been there before, quite some time ago.

Jan


^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2022-09-06  7:19 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-16  2:36 [PATCH v10 0/9] populate/unpopulate memory when domain on static allocation Penny Zheng
2022-08-16  2:36 ` [PATCH v10 1/9] xen/arm: rename PGC_reserved to PGC_static Penny Zheng
2022-08-16  2:36 ` [PATCH v10 2/9] xen: do not free reserved memory into heap Penny Zheng
2022-08-16  6:40   ` Jan Beulich
2022-08-24  9:03     ` Julien Grall
2022-08-24  9:27       ` Juergen Gross
2022-08-24  9:31         ` Julien Grall
2022-08-24  9:38           ` Juergen Gross
2022-08-24 10:42       ` Jan Beulich
2022-08-16  2:36 ` [PATCH v10 3/9] xen: do not merge reserved pages in free_heap_pages() Penny Zheng
2022-08-16  2:36 ` [PATCH v10 4/9] xen: add field "flags" to cover all internal CDF_XXX Penny Zheng
2022-08-16  2:36 ` [PATCH v10 5/9] xen/arm: introduce CDF_staticmem Penny Zheng
2022-08-16  2:36 ` [PATCH v10 6/9] xen: unpopulate memory when domain is static Penny Zheng
2022-08-17  8:51   ` Jan Beulich
2022-08-24  9:12   ` Julien Grall
2022-08-16  2:36 ` [PATCH v10 7/9] xen: introduce prepare_staticmem_pages Penny Zheng
2022-08-16  2:36 ` [PATCH v10 8/9] xen: retrieve reserved pages on populate_physmap Penny Zheng
2022-08-17 10:04   ` Jan Beulich
2022-09-05  7:08     ` Penny Zheng
2022-09-06  6:33       ` Jan Beulich
2022-09-06  7:14         ` Penny Zheng
2022-09-06  7:19           ` Jan Beulich
2022-08-16  2:36 ` [PATCH v10 9/9] xen: rename free_staticmem_pages to unprepare_staticmem_pages Penny Zheng
2022-08-17 10:07   ` Jan Beulich
2022-08-24  9:54 ` [PATCH v10 0/9] populate/unpopulate memory when domain on static allocation Julien Grall

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.