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

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

[Penny Zheng]

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

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

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

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

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

-- 
2.25.1

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

[Penny Zheng]

There is a slim chance that free_heap_pages() may decide to merge a chunk
from the static region(PGC_reserved) with the about-to-be-free chunk.

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

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
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 be501582a3..1f3ad4bd28 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1483,6 +1483,7 @@ static void free_heap_pages(
             /* Merge with predecessor block? */
             if ( !mfn_valid(page_to_mfn(predecessor)) ||
                  !page_state_is(predecessor, free) ||
+                 (predecessor->count_info & PGC_reserved) ||
                  (PFN_ORDER(predecessor) != order) ||
                  (phys_to_nid(page_to_maddr(predecessor)) != node) )
                 break;
@@ -1506,6 +1507,7 @@ static void free_heap_pages(
             /* Merge with successor block? */
             if ( !mfn_valid(page_to_mfn(successor)) ||
                  !page_state_is(successor, free) ||
+                 (successor->count_info & PGC_reserved) ||
                  (PFN_ORDER(successor) != order) ||
                  (phys_to_nid(page_to_maddr(successor)) != node) )
                 break;
-- 
2.25.1

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

[Penny Zheng]

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

Another new CDF_xxx will be introduced in the next patch.

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

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

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

[Penny Zheng]

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

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

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

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

[Penny Zheng]

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

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

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

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

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

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

[Penny Zheng]

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

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
v3 changes
- 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:
- 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  | 38 ++++++++++++++++++++++++++++++++++++++
 xen/include/xen/domain.h |  4 ++++
 xen/include/xen/mm.h     |  3 +--
 4 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 69b0cd1e50..6cee51f0e3 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 retreived pages are contiguous,
+                 * so forbid non-zero-order requests here.
+                 */
+                if ( a->extent_order != 0 )
+                {
+                    gdprintk(XENLOG_INFO,
+                             "Could not allocate non-zero-order pages for static %pd.\n.",
+                             d);
+                    goto out;
+                }
+
+                mfn = acquire_reserved_page(d, a->memflags);
+                if ( mfn_eq(mfn, INVALID_MFN) )
+                {
+                    gdprintk(XENLOG_INFO,
+                             "%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 1f3ad4bd28..78cc52986c 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2769,12 +2769,50 @@ int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
 
     return 0;
 }
+
+/*
+ * Acquire a page from reserved page list(resv_page_list), when populating
+ * memory for static domain on runtime.
+ */
+mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
+{
+    struct page_info *page;
+    mfn_t smfn;
+
+    /* Acquire a page from reserved page list(resv_page_list). */
+    page = page_list_remove_head(&d->resv_page_list);
+    if ( unlikely(!page) )
+    {
+        printk(XENLOG_ERR
+               "%pd: failed to acquire a reserved page from resv_page_list.\n",
+               d);
+        return INVALID_MFN;
+    }
+
+    smfn = page_to_mfn(page);
+
+    if ( acquire_domstatic_pages(d, smfn, 1, memflags) )
+        return INVALID_MFN;
+
+    return smfn;
+}
 #else
 void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
                           bool need_scrub)
 {
     ASSERT_UNREACHABLE();
 }
+
+int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
+                                   unsigned int nr_mfns, unsigned int memflags)
+{
+    ASSERT_UNREACHABLE();
+}
+
+mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
+{
+    ASSERT_UNREACHABLE();
+}
 #endif
 
 /*
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 35dc7143a4..c613afa57e 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -38,6 +38,10 @@ void arch_get_domain_info(const struct domain *d,
 #define CDF_staticmem            (1U << 2)
 #endif
 
+#ifndef is_domain_using_staticmem
+#define is_domain_using_staticmem(d) ((void)(d), false)
+#endif
+
 /*
  * Arch-specifics.
  */
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 9fd95deaec..32b0837fa0 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -88,10 +88,9 @@ bool scrub_free_pages(void);
 /* These functions are for static memory */
 void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
                           bool need_scrub);
-#ifdef CONFIG_STATIC_MEMORY
 int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int nr_mfns,
                             unsigned int memflags);
-#endif
+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 v3 2/6] xen: do not merge reserved pages in free_heap_pages()

[Julien Grall]

Hi,

On 27/04/2022 10:27, Penny Zheng wrote:
> There is a slim chance that free_heap_pages() may decide to merge a chunk
> from the static region(PGC_reserved) with the about-to-be-free chunk.
This sentence tells me that the merge can happen but it doesn't tell me 
the cases. I think the second part is more important to know.

The code in free_heap_Pages() will try to merge with the 
successor/predecessor if they are suitably aligned. So the issue can 
only happen if the pages reserved are right next to the pages given to 
the heap allocator.

> 
> So in order to avoid the above scenario, this commit updates free_heap_pages()
> to check whether the predecessor and/or successor has PGC_reserved set,
> when trying to merge the about-to-be-freed chunk with the predecessor
> and/or successor.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> 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 be501582a3..1f3ad4bd28 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1483,6 +1483,7 @@ static void free_heap_pages(
>               /* Merge with predecessor block? */
>               if ( !mfn_valid(page_to_mfn(predecessor)) ||
>                    !page_state_is(predecessor, free) ||
> +                 (predecessor->count_info & PGC_reserved) ||
>                    (PFN_ORDER(predecessor) != order) ||
>                    (phys_to_nid(page_to_maddr(predecessor)) != node) )
>                   break;
> @@ -1506,6 +1507,7 @@ static void free_heap_pages(
>               /* Merge with successor block? */
>               if ( !mfn_valid(page_to_mfn(successor)) ||
>                    !page_state_is(successor, free) ||
> +                 (successor->count_info & PGC_reserved) ||
>                    (PFN_ORDER(successor) != order) ||
>                    (phys_to_nid(page_to_maddr(successor)) != node) )
>                   break;

Cheers,

-- 
Julien Grall

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

[Julien Grall]

Hi,

On 27/04/2022 10:27, Penny Zheng wrote:
> 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>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 4/6] xen/arm: introduce CDF_staticmem

[Julien Grall]

Hi Penny,

On 27/04/2022 10:27, Penny Zheng wrote:
> 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.
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

The reviewed-by tags should be after signed-off-by tags.

Cheers,

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

-- 
Julien Grall

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

[Julien Grall]

Hi Penny,

On 27/04/2022 10:27, 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>
> ---
> v3 changes:
> - have page_list_del() just once out of the if()
> - remove resv_pages counter
> - make arch_free_heap_page be an expression, not a compound statement.
> ---
> v2 changes:
> - put reserved pages on resv_page_list after having taken them off
> the "normal" list
> ---
>   xen/arch/arm/include/asm/mm.h | 12 ++++++++++++
>   xen/common/domain.c           |  4 ++++
>   xen/include/xen/sched.h       |  3 +++
>   3 files changed, 19 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index 424aaf2823..c6426c1705 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -358,6 +358,18 @@ void clear_and_clean_page(struct page_info *page);
>   
>   unsigned int arch_get_dma_bitsize(void);
>   
> +/*
> + * Put free pages on the resv page list after having taken them
> + * off the "normal" page list, when pages from static memory
> + */
> +#ifdef CONFIG_STATIC_MEMORY
> +#define arch_free_heap_page(d, pg) ({                   \
> +    page_list_del(pg, page_to_list(d, pg));             \
> +    if ( (pg)->count_info & PGC_reserved )              \
> +        page_list_add_tail(pg, &(d)->resv_page_list);   \
> +})
> +#endif

I am a bit puzzled how this is meant to work.

Looking at the code, arch_free_heap_page() will be called from 
free_domheap_pages(). If I am not mistaken, reserved pages are not 
considered as xen heap pages, so we would go in the else which will end 
up to call free_heap_pages().

free_heap_pages() will end up to add the page in the heap allocator and 
corrupt the d->resv_page_list because there are only one link list.

What did I miss?

Cheers,

-- 
Julien Grall

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

[Penny Zheng]

Hi julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Wednesday, April 27, 2022 6:11 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@arm.com>;
> Stefano Stabellini <sstabellini@kernel.org>; Bertrand Marquis
> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v3 5/6] xen/arm: unpopulate memory when domain is
> static
> 
> Hi Penny,
> 
> On 27/04/2022 10:27, 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>
> > ---
> > v3 changes:
> > - have page_list_del() just once out of the if()
> > - remove resv_pages counter
> > - make arch_free_heap_page be an expression, not a compound statement.
> > ---
> > v2 changes:
> > - put reserved pages on resv_page_list after having taken them off the
> > "normal" list
> > ---
> >   xen/arch/arm/include/asm/mm.h | 12 ++++++++++++
> >   xen/common/domain.c           |  4 ++++
> >   xen/include/xen/sched.h       |  3 +++
> >   3 files changed, 19 insertions(+)
> >
> > diff --git a/xen/arch/arm/include/asm/mm.h
> > b/xen/arch/arm/include/asm/mm.h index 424aaf2823..c6426c1705 100644
> > --- a/xen/arch/arm/include/asm/mm.h
> > +++ b/xen/arch/arm/include/asm/mm.h
> > @@ -358,6 +358,18 @@ void clear_and_clean_page(struct page_info
> > *page);
> >
> >   unsigned int arch_get_dma_bitsize(void);
> >
> > +/*
> > + * Put free pages on the resv page list after having taken them
> > + * off the "normal" page list, when pages from static memory  */
> > +#ifdef CONFIG_STATIC_MEMORY
> > +#define arch_free_heap_page(d, pg) ({                   \
> > +    page_list_del(pg, page_to_list(d, pg));             \
> > +    if ( (pg)->count_info & PGC_reserved )              \
> > +        page_list_add_tail(pg, &(d)->resv_page_list);   \
> > +})
> > +#endif
> 
> I am a bit puzzled how this is meant to work.
> 
> Looking at the code, arch_free_heap_page() will be called from
> free_domheap_pages(). If I am not mistaken, reserved pages are not
> considered as xen heap pages, so we would go in the else which will end up to
> call free_heap_pages().
> 
> free_heap_pages() will end up to add the page in the heap allocator and
> corrupt the d->resv_page_list because there are only one link list.
> 
> What did I miss?
> 

In my first commit "do not free reserved memory into heap", I've changed the behavior
for reserved pages in free_heap_pages()
+    if ( pg->count_info & PGC_reserved )
+        /* Reserved page shall not go back to the heap. */
+        return free_staticmem_pages(pg, 1UL << order, need_scrub);
+

> Cheers,
>
> --
> Julien Grall

Cheers,

--
Penny Zheng

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

[Julien Grall]

Hi Penny,

On 27/04/2022 11:19, Penny Zheng wrote:
>>> +/*
>>> + * Put free pages on the resv page list after having taken them
>>> + * off the "normal" page list, when pages from static memory  */
>>> +#ifdef CONFIG_STATIC_MEMORY
>>> +#define arch_free_heap_page(d, pg) ({                   \
>>> +    page_list_del(pg, page_to_list(d, pg));             \
>>> +    if ( (pg)->count_info & PGC_reserved )              \
>>> +        page_list_add_tail(pg, &(d)->resv_page_list);   \
>>> +})
>>> +#endif
>>
>> I am a bit puzzled how this is meant to work.
>>
>> Looking at the code, arch_free_heap_page() will be called from
>> free_domheap_pages(). If I am not mistaken, reserved pages are not
>> considered as xen heap pages, so we would go in the else which will end up to
>> call free_heap_pages().
>>
>> free_heap_pages() will end up to add the page in the heap allocator and
>> corrupt the d->resv_page_list because there are only one link list.
>>
>> What did I miss?
>>
> 
> In my first commit "do not free reserved memory into heap", I've changed the behavior
> for reserved pages in free_heap_pages()
> +    if ( pg->count_info & PGC_reserved )
> +        /* Reserved page shall not go back to the heap. */
> +        return free_staticmem_pages(pg, 1UL << order, need_scrub);
> +

Hmmm... somehow this e-mail is neither in my inbox nor in the archives 
on lore.kernel.org.

Cheers,
-- 
Julien Grall

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

[Penny Zheng]

Hi julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Wednesday, April 27, 2022 6:23 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@arm.com>;
> Stefano Stabellini <sstabellini@kernel.org>; Bertrand Marquis
> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH v3 5/6] xen/arm: unpopulate memory when domain is
> static
> 
> Hi Penny,
> 
> On 27/04/2022 11:19, Penny Zheng wrote:
> >>> +/*
> >>> + * Put free pages on the resv page list after having taken them
> >>> + * off the "normal" page list, when pages from static memory  */
> >>> +#ifdef CONFIG_STATIC_MEMORY
> >>> +#define arch_free_heap_page(d, pg) ({                   \
> >>> +    page_list_del(pg, page_to_list(d, pg));             \
> >>> +    if ( (pg)->count_info & PGC_reserved )              \
> >>> +        page_list_add_tail(pg, &(d)->resv_page_list);   \
> >>> +})
> >>> +#endif
> >>
> >> I am a bit puzzled how this is meant to work.
> >>
> >> Looking at the code, arch_free_heap_page() will be called from
> >> free_domheap_pages(). If I am not mistaken, reserved pages are not
> >> considered as xen heap pages, so we would go in the else which will
> >> end up to call free_heap_pages().
> >>
> >> free_heap_pages() will end up to add the page in the heap allocator
> >> and corrupt the d->resv_page_list because there are only one link list.
> >>
> >> What did I miss?
> >>
> >
> > In my first commit "do not free reserved memory into heap", I've
> > changed the behavior for reserved pages in free_heap_pages()
> > +    if ( pg->count_info & PGC_reserved )that
> > +        /* Reserved page shall not go back to the heap. */
> > +        return free_staticmem_pages(pg, 1UL << order, need_scrub);
> > +
> 
> Hmmm... somehow this e-mail is neither in my inbox nor in the archives on
> lore.kernel.org.
> 

Oh.... I just got email from tessian that they held my first commit, and needed my
confirmation to send. So sorry about that!!!

I'll re-send my first commit ASAP.

> Cheers,
> --
> Julien Grall

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

[Stefano Stabellini]

On Wed, 27 Apr 2022, Penny Zheng wrote:
> > Hi Penny,
> > 
> > On 27/04/2022 11:19, Penny Zheng wrote:
> > >>> +/*
> > >>> + * Put free pages on the resv page list after having taken them
> > >>> + * off the "normal" page list, when pages from static memory  */
> > >>> +#ifdef CONFIG_STATIC_MEMORY
> > >>> +#define arch_free_heap_page(d, pg) ({                   \
> > >>> +    page_list_del(pg, page_to_list(d, pg));             \
> > >>> +    if ( (pg)->count_info & PGC_reserved )              \
> > >>> +        page_list_add_tail(pg, &(d)->resv_page_list);   \
> > >>> +})
> > >>> +#endif
> > >>
> > >> I am a bit puzzled how this is meant to work.
> > >>
> > >> Looking at the code, arch_free_heap_page() will be called from
> > >> free_domheap_pages(). If I am not mistaken, reserved pages are not
> > >> considered as xen heap pages, so we would go in the else which will
> > >> end up to call free_heap_pages().
> > >>
> > >> free_heap_pages() will end up to add the page in the heap allocator
> > >> and corrupt the d->resv_page_list because there are only one link list.
> > >>
> > >> What did I miss?
> > >>
> > >
> > > In my first commit "do not free reserved memory into heap", I've
> > > changed the behavior for reserved pages in free_heap_pages()
> > > +    if ( pg->count_info & PGC_reserved )that
> > > +        /* Reserved page shall not go back to the heap. */
> > > +        return free_staticmem_pages(pg, 1UL << order, need_scrub);
> > > +
> > 
> > Hmmm... somehow this e-mail is neither in my inbox nor in the archives on
> > lore.kernel.org.
> > 
> 
> Oh.... I just got email from tessian that they held my first commit, and needed my
> confirmation to send. So sorry about that!!!
> 
> I'll re-send my first commit ASAP.

Just FYI I still cannot see the first patch anywhere in my inbox

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

[Penny Zheng]

Hi 

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@kernel.org>
> Sent: Thursday, April 28, 2022 6:32 AM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Julien Grall <julien@xen.org>; xen-devel@lists.xenproject.org; Wei Chen
> <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@arm.com>; Stefano
> Stabellini <sstabellini@kernel.org>; Bertrand Marquis
> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>;
> Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org>
> Subject: RE: [PATCH v3 5/6] xen/arm: unpopulate memory when domain is
> static
> 
> On Wed, 27 Apr 2022, Penny Zheng wrote:
> > > Hi Penny,
> > >
> > > On 27/04/2022 11:19, Penny Zheng wrote:
> > > >>> +/*
> > > >>> + * Put free pages on the resv page list after having taken them
> > > >>> + * off the "normal" page list, when pages from static memory
> > > >>> +*/ #ifdef CONFIG_STATIC_MEMORY
> > > >>> +#define arch_free_heap_page(d, pg) ({                   \
> > > >>> +    page_list_del(pg, page_to_list(d, pg));             \
> > > >>> +    if ( (pg)->count_info & PGC_reserved )              \
> > > >>> +        page_list_add_tail(pg, &(d)->resv_page_list);   \
> > > >>> +})
> > > >>> +#endif
> > > >>
> > > >> I am a bit puzzled how this is meant to work.
> > > >>
> > > >> Looking at the code, arch_free_heap_page() will be called from
> > > >> free_domheap_pages(). If I am not mistaken, reserved pages are
> > > >> not considered as xen heap pages, so we would go in the else
> > > >> which will end up to call free_heap_pages().
> > > >>
> > > >> free_heap_pages() will end up to add the page in the heap
> > > >> allocator and corrupt the d->resv_page_list because there are only one
> link list.
> > > >>
> > > >> What did I miss?
> > > >>
> > > >
> > > > In my first commit "do not free reserved memory into heap", I've
> > > > changed the behavior for reserved pages in free_heap_pages()
> > > > +    if ( pg->count_info & PGC_reserved )that
> > > > +        /* Reserved page shall not go back to the heap. */
> > > > +        return free_staticmem_pages(pg, 1UL << order,
> > > > + need_scrub);
> > > > +
> > >
> > > Hmmm... somehow this e-mail is neither in my inbox nor in the
> > > archives on lore.kernel.org.
> > >
> >
> > Oh.... I just got email from tessian that they held my first commit,
> > and needed my confirmation to send. So sorry about that!!!
> >
> > I'll re-send my first commit ASAP.
> 
> Just FYI I still cannot see the first patch anywhere in my inbox

So sorry about the mess again...
I've resend it just now, PLZ check https://patchwork.kernel.org/project/xen-devel/patch/20220428030127.998670-1-Penny.Zheng@arm.com/ 

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

[Jan Beulich]

On 27.04.2022 11:27, Penny Zheng wrote:
> There is a slim chance that free_heap_pages() may decide to merge a chunk
> from the static region(PGC_reserved) with the about-to-be-free chunk.
> 
> So in order to avoid the above scenario, this commit updates free_heap_pages()
> to check whether the predecessor and/or successor has PGC_reserved set,
> when trying to merge the about-to-be-freed chunk with the predecessor
> and/or successor.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>

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

I think this also wants a Suggested-by or Reported-by (iirc) Julien?

Jan

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

[Jan Beulich]

On 27.04.2022 11:27, Penny Zheng wrote:
> --- 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 retreived pages are contiguous,

Nit: retrieved

> +                 * so forbid non-zero-order requests here.
> +                 */
> +                if ( a->extent_order != 0 )
> +                {
> +                    gdprintk(XENLOG_INFO,
> +                             "Could not allocate non-zero-order pages for static %pd.\n.",

Nit: "Could not" reads as if an attempt was made, so maybe better "Cannot"?
I'd also pull "static" ahead of "non-zero-order" and, to help observers of
the message associate it with a call site, actually log the order (i.e.
"order-%u" instead of "non-zero-order").

Also please omit full stops in log messages. They serve no purpose but
consume space.

Finally, here as well as below: Is "info" log level really appropriate?
You're logging error conditions after all, so imo these want to be at
least "warn" level. An alternative would be to omit logging of messages
here altogether.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2769,12 +2769,50 @@ int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
>  
>      return 0;
>  }
> +
> +/*
> + * Acquire a page from reserved page list(resv_page_list), when populating
> + * memory for static domain on runtime.
> + */
> +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
> +{
> +    struct page_info *page;
> +    mfn_t smfn;
> +
> +    /* Acquire a page from reserved page list(resv_page_list). */
> +    page = page_list_remove_head(&d->resv_page_list);
> +    if ( unlikely(!page) )
> +    {
> +        printk(XENLOG_ERR
> +               "%pd: failed to acquire a reserved page from resv_page_list.\n",
> +               d);

A gdprintk() in the caller is acceptable. Two log messages isn't imo,
and a XENLOG_ERR message which a guest can trigger is a security concern
(log spam) anyway.

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

Don't you want to add the page back to the reserved list in case of error?

> +    return smfn;
> +}
>  #else
>  void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
>                            bool need_scrub)
>  {
>      ASSERT_UNREACHABLE();
>  }
> +
> +int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
> +                                   unsigned int nr_mfns, unsigned int memflags)
> +{
> +    ASSERT_UNREACHABLE();
> +}

I can't spot a caller of this one outside of suitable #ifdef. Also
the __init here looks wrong and you look to have missed dropping it
from the real function.

> +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
> +{
> +    ASSERT_UNREACHABLE();
> +}
>  #endif

For this one I'd again expect CSE to leave no callers, just like in the
earlier patch. Or am I overlooking anything?

Jan

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

[Penny Zheng]

Hi, 

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, May 4, 2022 9:30 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@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 v3 2/6] xen: do not merge reserved pages in
> free_heap_pages()
> 
> On 27.04.2022 11:27, Penny Zheng wrote:
> > There is a slim chance that free_heap_pages() may decide to merge a
> > chunk from the static region(PGC_reserved) with the about-to-be-free
> chunk.
> >
> > So in order to avoid the above scenario, this commit updates
> > free_heap_pages() to check whether the predecessor and/or successor
> > has PGC_reserved set, when trying to merge the about-to-be-freed chunk
> > with the predecessor and/or successor.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> I think this also wants a Suggested-by or Reported-by (iirc) Julien?
> 

Sure, I'll definitely add-in Suggested-by: Julien Grall <jgrall@amazon.com>

> Jan

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

[Penny Zheng]

Hi jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, May 4, 2022 9:45 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@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 v3 6/6] xen: retrieve reserved pages on
> populate_physmap
> 
> On 27.04.2022 11:27, Penny Zheng wrote:
> > --- 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 retreived pages are
> > + contiguous,
> 
> Nit: retrieved
> 
> > +                 * so forbid non-zero-order requests here.
> > +                 */
> > +                if ( a->extent_order != 0 )
> > +                {
> > +                    gdprintk(XENLOG_INFO,
> > +                             "Could not allocate non-zero-order pages
> > + for static %pd.\n.",
> 
> Nit: "Could not" reads as if an attempt was made, so maybe better "Cannot"?
> I'd also pull "static" ahead of "non-zero-order" and, to help observers of the
> message associate it with a call site, actually log the order (i.e.
> "order-%u" instead of "non-zero-order").
> 
> Also please omit full stops in log messages. They serve no purpose but
> consume space.
> 
> Finally, here as well as below: Is "info" log level really appropriate?
> You're logging error conditions after all, so imo these want to be at least
> "warn" level. An alternative would be to omit logging of messages here
> altogether.
> 
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2769,12 +2769,50 @@ int __init acquire_domstatic_pages(struct
> > domain *d, mfn_t smfn,
> >
> >      return 0;
> >  }
> > +
> > +/*
> > + * Acquire a page from reserved page list(resv_page_list), when
> > +populating
> > + * memory for static domain on runtime.
> > + */
> > +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
> > +{
> > +    struct page_info *page;
> > +    mfn_t smfn;
> > +
> > +    /* Acquire a page from reserved page list(resv_page_list). */
> > +    page = page_list_remove_head(&d->resv_page_list);
> > +    if ( unlikely(!page) )
> > +    {
> > +        printk(XENLOG_ERR
> > +               "%pd: failed to acquire a reserved page from resv_page_list.\n",
> > +               d);
> 
> A gdprintk() in the caller is acceptable. Two log messages isn't imo, and a
> XENLOG_ERR message which a guest can trigger is a security concern (log
> spam) anyway.
> 
> > +        return INVALID_MFN;
> > +    }
> > +
> > +    smfn = page_to_mfn(page);
> > +
> > +    if ( acquire_domstatic_pages(d, smfn, 1, memflags) )
> > +        return INVALID_MFN;
> 
> Don't you want to add the page back to the reserved list in case of error?
> 

Oh, thanks for pointing that out.

> > +    return smfn;
> > +}
> >  #else
> >  void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> >                            bool need_scrub)  {
> >      ASSERT_UNREACHABLE();
> >  }
> > +
> > +int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
> > +                                   unsigned int nr_mfns, unsigned int
> > +memflags) {
> > +    ASSERT_UNREACHABLE();
> > +}
> 
> I can't spot a caller of this one outside of suitable #ifdef. Also the __init here
> looks wrong and you look to have missed dropping it from the real function.
> 
> > +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
> > +{
> > +    ASSERT_UNREACHABLE();
> > +}
> >  #endif
> 
> For this one I'd again expect CSE to leave no callers, just like in the earlier
> patch. Or am I overlooking anything?
> 

In acquire_reserved_page, I've use a few CONFIG_STATIC_MEMORY-only variables, like
d->resv_page_list, so I'd expect to let acquire_reserved_page guarded by CONFIG_STATIC_MEMORY
too and provide the stub function here to avoid compilation error when !CONFIG_STATIC_MEMORY.


> Jan

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

[Jan Beulich]

On 05.05.2022 08:24, Penny Zheng wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Wednesday, May 4, 2022 9:45 PM
>>
>> On 27.04.2022 11:27, Penny Zheng wrote:
>>>  #else
>>>  void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
>>>                            bool need_scrub)  {
>>>      ASSERT_UNREACHABLE();
>>>  }
>>> +
>>> +int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
>>> +                                   unsigned int nr_mfns, unsigned int
>>> +memflags) {
>>> +    ASSERT_UNREACHABLE();
>>> +}
>>
>> I can't spot a caller of this one outside of suitable #ifdef. Also the __init here
>> looks wrong and you look to have missed dropping it from the real function.
>>
>>> +mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags)
>>> +{
>>> +    ASSERT_UNREACHABLE();
>>> +}
>>>  #endif
>>
>> For this one I'd again expect CSE to leave no callers, just like in the earlier
>> patch. Or am I overlooking anything?
>>
> 
> In acquire_reserved_page, I've use a few CONFIG_STATIC_MEMORY-only variables, like
> d->resv_page_list, so I'd expect to let acquire_reserved_page guarded by CONFIG_STATIC_MEMORY
> too and provide the stub function here to avoid compilation error when !CONFIG_STATIC_MEMORY.

A compilation error should only result if there's no declaration of the
function. I didn't suggest to remove that. A missing definition would
only be noticed when linking, but CSE should result in no reference to
the function in the first place. Just like was suggested for the earlier
patch. And as opposed to the call site optimization the compiler can do,
with -ffunction-sections there's no way for the linker to eliminate the
dead stub function.

Jan

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

[Penny Zheng]

Hi jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, May 5, 2022 3:47 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@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 v3 6/6] xen: retrieve reserved pages on
> populate_physmap
> 
> On 05.05.2022 08:24, Penny Zheng wrote:
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Wednesday, May 4, 2022 9:45 PM
> >>
> >> On 27.04.2022 11:27, Penny Zheng wrote:
> >>>  #else
> >>>  void free_staticmem_pages(struct page_info *pg, unsigned long
> nr_mfns,
> >>>                            bool need_scrub)  {
> >>>      ASSERT_UNREACHABLE();
> >>>  }
> >>> +
> >>> +int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
> >>> +                                   unsigned int nr_mfns, unsigned
> >>> +int
> >>> +memflags) {
> >>> +    ASSERT_UNREACHABLE();
> >>> +}
> >>
> >> I can't spot a caller of this one outside of suitable #ifdef. Also
> >> the __init here looks wrong and you look to have missed dropping it from
> the real function.
> >>
> >>> +mfn_t acquire_reserved_page(struct domain *d, unsigned int
> >>> +memflags) {
> >>> +    ASSERT_UNREACHABLE();
> >>> +}
> >>>  #endif
> >>
> >> For this one I'd again expect CSE to leave no callers, just like in
> >> the earlier patch. Or am I overlooking anything?
> >>
> >
> > In acquire_reserved_page, I've use a few CONFIG_STATIC_MEMORY-only
> > variables, like
> > d->resv_page_list, so I'd expect to let acquire_reserved_page guarded
> > d->by CONFIG_STATIC_MEMORY
> > too and provide the stub function here to avoid compilation error
> when !CONFIG_STATIC_MEMORY.
> 
> A compilation error should only result if there's no declaration of the
> function. I didn't suggest to remove that. A missing definition would only be
> noticed when linking, but CSE should result in no reference to the function in
> the first place. Just like was suggested for the earlier patch. And as opposed
> to the call site optimization the compiler can do, with -ffunction-sections
> there's no way for the linker to eliminate the dead stub function.
> 

Sure, plz correct me if I understand wrongly:
Maybe here I should use #define xxx to do the declaration, and it will also
avoid bringing dead stub function. Something like:
#define free_staticmem_pages(pg, nr_mfns, need_scrub) ((void)(pg), (void)(nr_mfns), (void)(need_scrub))
And
#define acquire_reserved_page(d, memflags) (INVALID_MFN)

> Jan

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

[Jan Beulich]

On 05.05.2022 10:44, Penny Zheng wrote:
> Hi jan
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Thursday, May 5, 2022 3:47 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>
>> Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@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 v3 6/6] xen: retrieve reserved pages on
>> populate_physmap
>>
>> On 05.05.2022 08:24, Penny Zheng wrote:
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: Wednesday, May 4, 2022 9:45 PM
>>>>
>>>> On 27.04.2022 11:27, Penny Zheng wrote:
>>>>>  #else
>>>>>  void free_staticmem_pages(struct page_info *pg, unsigned long
>> nr_mfns,
>>>>>                            bool need_scrub)  {
>>>>>      ASSERT_UNREACHABLE();
>>>>>  }
>>>>> +
>>>>> +int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
>>>>> +                                   unsigned int nr_mfns, unsigned
>>>>> +int
>>>>> +memflags) {
>>>>> +    ASSERT_UNREACHABLE();
>>>>> +}
>>>>
>>>> I can't spot a caller of this one outside of suitable #ifdef. Also
>>>> the __init here looks wrong and you look to have missed dropping it from
>> the real function.
>>>>
>>>>> +mfn_t acquire_reserved_page(struct domain *d, unsigned int
>>>>> +memflags) {
>>>>> +    ASSERT_UNREACHABLE();
>>>>> +}
>>>>>  #endif
>>>>
>>>> For this one I'd again expect CSE to leave no callers, just like in
>>>> the earlier patch. Or am I overlooking anything?
>>>>
>>>
>>> In acquire_reserved_page, I've use a few CONFIG_STATIC_MEMORY-only
>>> variables, like
>>> d->resv_page_list, so I'd expect to let acquire_reserved_page guarded
>>> d->by CONFIG_STATIC_MEMORY
>>> too and provide the stub function here to avoid compilation error
>> when !CONFIG_STATIC_MEMORY.
>>
>> A compilation error should only result if there's no declaration of the
>> function. I didn't suggest to remove that. A missing definition would only be
>> noticed when linking, but CSE should result in no reference to the function in
>> the first place. Just like was suggested for the earlier patch. And as opposed
>> to the call site optimization the compiler can do, with -ffunction-sections
>> there's no way for the linker to eliminate the dead stub function.
>>
> 
> Sure, plz correct me if I understand wrongly:
> Maybe here I should use #define xxx to do the declaration, and it will also
> avoid bringing dead stub function. Something like:
> #define free_staticmem_pages(pg, nr_mfns, need_scrub) ((void)(pg), (void)(nr_mfns), (void)(need_scrub))
> And
> #define acquire_reserved_page(d, memflags) (INVALID_MFN)

No, I don't see why you would need #define-s. You want to have normal
declarations, but no definition unless STATIC_MEMORY. If that doesn't
work, please point out why (i.e. what I am overlooking).

Jan

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

[Penny Zheng]

Hi jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, May 5, 2022 4:51 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@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 v3 6/6] xen: retrieve reserved pages on
> populate_physmap
> 
> On 05.05.2022 10:44, Penny Zheng wrote:
> > Hi jan
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Thursday, May 5, 2022 3:47 PM
> >> To: Penny Zheng <Penny.Zheng@arm.com>
> >> Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang
> <Henry.Wang@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 v3 6/6] xen: retrieve reserved pages on
> >> populate_physmap
> >>
> >> On 05.05.2022 08:24, Penny Zheng wrote:
> >>>> From: Jan Beulich <jbeulich@suse.com>
> >>>> Sent: Wednesday, May 4, 2022 9:45 PM
> >>>>
> >>>> On 27.04.2022 11:27, Penny Zheng wrote:
> >>>>>  #else
> >>>>>  void free_staticmem_pages(struct page_info *pg, unsigned long
> >> nr_mfns,
> >>>>>                            bool need_scrub)  {
> >>>>>      ASSERT_UNREACHABLE();
> >>>>>  }
> >>>>> +
> >>>>> +int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
> >>>>> +                                   unsigned int nr_mfns, unsigned
> >>>>> +int
> >>>>> +memflags) {
> >>>>> +    ASSERT_UNREACHABLE();
> >>>>> +}
> >>>>
> >>>> I can't spot a caller of this one outside of suitable #ifdef. Also
> >>>> the __init here looks wrong and you look to have missed dropping it
> >>>> from
> >> the real function.
> >>>>
> >>>>> +mfn_t acquire_reserved_page(struct domain *d, unsigned int
> >>>>> +memflags) {
> >>>>> +    ASSERT_UNREACHABLE();
> >>>>> +}
> >>>>>  #endif
> >>>>
> >>>> For this one I'd again expect CSE to leave no callers, just like in
> >>>> the earlier patch. Or am I overlooking anything?
> >>>>
> >>>
> >>> In acquire_reserved_page, I've use a few CONFIG_STATIC_MEMORY-only
> >>> variables, like
> >>> d->resv_page_list, so I'd expect to let acquire_reserved_page
> >>> d->guarded by CONFIG_STATIC_MEMORY
> >>> too and provide the stub function here to avoid compilation error
> >> when !CONFIG_STATIC_MEMORY.
> >>
> >> A compilation error should only result if there's no declaration of
> >> the function. I didn't suggest to remove that. A missing definition
> >> would only be noticed when linking, but CSE should result in no
> >> reference to the function in the first place. Just like was suggested
> >> for the earlier patch. And as opposed to the call site optimization
> >> the compiler can do, with -ffunction-sections there's no way for the linker
> to eliminate the dead stub function.
> >>
> >
> > Sure, plz correct me if I understand wrongly:
> > Maybe here I should use #define xxx to do the declaration, and it will
> > also avoid bringing dead stub function. Something like:
> > #define free_staticmem_pages(pg, nr_mfns, need_scrub) ((void)(pg),
> > (void)(nr_mfns), (void)(need_scrub)) And #define
> > acquire_reserved_page(d, memflags) (INVALID_MFN)
> 
> No, I don't see why you would need #define-s. You want to have normal
> declarations, but no definition unless STATIC_MEMORY. If that doesn't work,
> please point out why (i.e. what I am overlooking).
> 

I was trying to avoid dead stub function, and I think #define-s is the wrong path...
So, I guess If I remove the ASSERT_UNREACHABLE() part and only leave the empty
function body there, the CSE could do the optimization and result in no reference.

> Jan

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

[Jan Beulich]

On 05.05.2022 11:29, Penny Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Thursday, May 5, 2022 4:51 PM
>>
>> On 05.05.2022 10:44, Penny Zheng wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: Thursday, May 5, 2022 3:47 PM
>>>>
>>>> On 05.05.2022 08:24, Penny Zheng wrote:
>>>>>> From: Jan Beulich <jbeulich@suse.com>
>>>>>> Sent: Wednesday, May 4, 2022 9:45 PM
>>>>>>
>>>>>> On 27.04.2022 11:27, Penny Zheng wrote:
>>>>>>>  #else
>>>>>>>  void free_staticmem_pages(struct page_info *pg, unsigned long
>>>> nr_mfns,
>>>>>>>                            bool need_scrub)  {
>>>>>>>      ASSERT_UNREACHABLE();
>>>>>>>  }
>>>>>>> +
>>>>>>> +int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
>>>>>>> +                                   unsigned int nr_mfns, unsigned
>>>>>>> +int
>>>>>>> +memflags) {
>>>>>>> +    ASSERT_UNREACHABLE();
>>>>>>> +}
>>>>>>
>>>>>> I can't spot a caller of this one outside of suitable #ifdef. Also
>>>>>> the __init here looks wrong and you look to have missed dropping it
>>>>>> from
>>>> the real function.
>>>>>>
>>>>>>> +mfn_t acquire_reserved_page(struct domain *d, unsigned int
>>>>>>> +memflags) {
>>>>>>> +    ASSERT_UNREACHABLE();
>>>>>>> +}
>>>>>>>  #endif
>>>>>>
>>>>>> For this one I'd again expect CSE to leave no callers, just like in
>>>>>> the earlier patch. Or am I overlooking anything?
>>>>>>
>>>>>
>>>>> In acquire_reserved_page, I've use a few CONFIG_STATIC_MEMORY-only
>>>>> variables, like
>>>>> d->resv_page_list, so I'd expect to let acquire_reserved_page
>>>>> d->guarded by CONFIG_STATIC_MEMORY
>>>>> too and provide the stub function here to avoid compilation error
>>>> when !CONFIG_STATIC_MEMORY.
>>>>
>>>> A compilation error should only result if there's no declaration of
>>>> the function. I didn't suggest to remove that. A missing definition
>>>> would only be noticed when linking, but CSE should result in no
>>>> reference to the function in the first place. Just like was suggested
>>>> for the earlier patch. And as opposed to the call site optimization
>>>> the compiler can do, with -ffunction-sections there's no way for the linker
>> to eliminate the dead stub function.
>>>>
>>>
>>> Sure, plz correct me if I understand wrongly:
>>> Maybe here I should use #define xxx to do the declaration, and it will
>>> also avoid bringing dead stub function. Something like:
>>> #define free_staticmem_pages(pg, nr_mfns, need_scrub) ((void)(pg),
>>> (void)(nr_mfns), (void)(need_scrub)) And #define
>>> acquire_reserved_page(d, memflags) (INVALID_MFN)
>>
>> No, I don't see why you would need #define-s. You want to have normal
>> declarations, but no definition unless STATIC_MEMORY. If that doesn't work,
>> please point out why (i.e. what I am overlooking).
>>
> 
> I was trying to avoid dead stub function, and I think #define-s is the wrong path...
> So, I guess If I remove the ASSERT_UNREACHABLE() part and only leave the empty
> function body there, the CSE could do the optimization and result in no reference.

No, DCE (I'm sorry for the earlier wrong uses of CSE) cannot eliminate a
function, it can only eliminate call sites. Hence it doesn't matter whether
a function is empty. And no, if a stub function needs retaining, the
ASSERT_UNREACHABLE() should also remain there if the function indeed is
supposed to never be called.

Jan

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

[Penny Zheng]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, May 5, 2022 8:07 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@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 v3 6/6] xen: retrieve reserved pages on populate_physmap
> 
> On 05.05.2022 11:29, Penny Zheng wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Thursday, May 5, 2022 4:51 PM
> >>
> >> On 05.05.2022 10:44, Penny Zheng wrote:
> >>>> -----Original Message-----
> >>>> From: Jan Beulich <jbeulich@suse.com>
> >>>> Sent: Thursday, May 5, 2022 3:47 PM
> >>>>
> >>>> On 05.05.2022 08:24, Penny Zheng wrote:
> >>>>>> From: Jan Beulich <jbeulich@suse.com>
> >>>>>> Sent: Wednesday, May 4, 2022 9:45 PM
> >>>>>>
> >>>>>> On 27.04.2022 11:27, Penny Zheng wrote:
> >>>>>>>  #else
> >>>>>>>  void free_staticmem_pages(struct page_info *pg, unsigned long
> >>>> nr_mfns,
> >>>>>>>                            bool need_scrub)  {
> >>>>>>>      ASSERT_UNREACHABLE();
> >>>>>>>  }
> >>>>>>> +
> >>>>>>> +int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
> >>>>>>> +                                   unsigned int nr_mfns,
> >>>>>>> +unsigned int
> >>>>>>> +memflags) {
> >>>>>>> +    ASSERT_UNREACHABLE();
> >>>>>>> +}
> >>>>>>
> >>>>>> I can't spot a caller of this one outside of suitable #ifdef.
> >>>>>> Also the __init here looks wrong and you look to have missed
> >>>>>> dropping it from
> >>>> the real function.
> >>>>>>
> >>>>>>> +mfn_t acquire_reserved_page(struct domain *d, unsigned int
> >>>>>>> +memflags) {
> >>>>>>> +    ASSERT_UNREACHABLE();
> >>>>>>> +}
> >>>>>>>  #endif
> >>>>>>
> >>>>>> For this one I'd again expect CSE to leave no callers, just like
> >>>>>> in the earlier patch. Or am I overlooking anything?
> >>>>>>
> >>>>>
> >>>>> In acquire_reserved_page, I've use a few CONFIG_STATIC_MEMORY-
> only
> >>>>> variables, like
> >>>>> d->resv_page_list, so I'd expect to let acquire_reserved_page
> >>>>> d->guarded by CONFIG_STATIC_MEMORY
> >>>>> too and provide the stub function here to avoid compilation error
> >>>> when !CONFIG_STATIC_MEMORY.
> >>>>
> >>>> A compilation error should only result if there's no declaration of
> >>>> the function. I didn't suggest to remove that. A missing definition
> >>>> would only be noticed when linking, but CSE should result in no
> >>>> reference to the function in the first place. Just like was
> >>>> suggested for the earlier patch. And as opposed to the call site
> >>>> optimization the compiler can do, with -ffunction-sections there's
> >>>> no way for the linker
> >> to eliminate the dead stub function.
> >>>>
> >>>
> >>> Sure, plz correct me if I understand wrongly:
> >>> Maybe here I should use #define xxx to do the declaration, and it
> >>> will also avoid bringing dead stub function. Something like:
> >>> #define free_staticmem_pages(pg, nr_mfns, need_scrub) ((void)(pg),
> >>> (void)(nr_mfns), (void)(need_scrub)) And #define
> >>> acquire_reserved_page(d, memflags) (INVALID_MFN)
> >>
> >> No, I don't see why you would need #define-s. You want to have normal
> >> declarations, but no definition unless STATIC_MEMORY. If that doesn't
> >> work, please point out why (i.e. what I am overlooking).
> >>
> >
> > I was trying to avoid dead stub function, and I think #define-s is the wrong
> path...
> > So, I guess If I remove the ASSERT_UNREACHABLE() part and only leave
> > the empty function body there, the CSE could do the optimization and result
> in no reference.
> 
> No, DCE (I'm sorry for the earlier wrong uses of CSE) cannot eliminate a
> function, it can only eliminate call sites. Hence it doesn't matter whether a
> function is empty. And no, if a stub function needs retaining, the
> ASSERT_UNREACHABLE() should also remain there if the function indeed is
> supposed to never be called.
>

Ok. Thanks for explanation.
I misunderstand what you suggested here, I thought you were suggesting a way of stub function
which could bring some optimization.
The reason I introduced free_staticmem_pages and acquire_reserved_page here is that
we now used them in common code, and if they are not defined(using stub) on !CONFIG_STATIC_MEMORY,
we will have " hidden symbol `xxx' isn't defined " compilation error.
 
> Jan

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

[Jan Beulich]

On 05.05.2022 15:44, Penny Zheng wrote:
> I misunderstand what you suggested here, I thought you were suggesting a way of stub function
> which could bring some optimization.
> The reason I introduced free_staticmem_pages and acquire_reserved_page here is that
> we now used them in common code, and if they are not defined(using stub) on !CONFIG_STATIC_MEMORY,
> we will have " hidden symbol `xxx' isn't defined " compilation error.

This is what I've asked for clarification about: If such errors surface,
I'd like to understand why the respective call sites aren't DCE-ed by
the compiler.

Jan

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

[Penny Zheng]

Hi jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, May 5, 2022 10:23 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@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 v3 6/6] xen: retrieve reserved pages on populate_physmap
> 
> On 05.05.2022 15:44, Penny Zheng wrote:
> > I misunderstand what you suggested here, I thought you were suggesting
> > a way of stub function which could bring some optimization.
> > The reason I introduced free_staticmem_pages and acquire_reserved_page
> > here is that we now used them in common code, and if they are not
> > defined(using stub) on !CONFIG_STATIC_MEMORY, we will have " hidden
> symbol `xxx' isn't defined " compilation error.
> 
> This is what I've asked for clarification about: If such errors surface, I'd like to
> understand why the respective call sites aren't DCE-ed by the compiler.
> 

Because both definition of PGC_reserved and is_domain_using_static_memory are
not guarded by CONFIG_STATIC_MEMORY in the first place in arm-specific file.

> Jan

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

[Jan Beulich]

On 06.05.2022 04:59, Penny Zheng wrote:
> Hi jan
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Thursday, May 5, 2022 10:23 PM
>> To: Penny Zheng <Penny.Zheng@arm.com>
>> Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@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 v3 6/6] xen: retrieve reserved pages on populate_physmap
>>
>> On 05.05.2022 15:44, Penny Zheng wrote:
>>> I misunderstand what you suggested here, I thought you were suggesting
>>> a way of stub function which could bring some optimization.
>>> The reason I introduced free_staticmem_pages and acquire_reserved_page
>>> here is that we now used them in common code, and if they are not
>>> defined(using stub) on !CONFIG_STATIC_MEMORY, we will have " hidden
>> symbol `xxx' isn't defined " compilation error.
>>
>> This is what I've asked for clarification about: If such errors surface, I'd like to
>> understand why the respective call sites aren't DCE-ed by the compiler.
>>
> 
> Because both definition of PGC_reserved and is_domain_using_static_memory are
> not guarded by CONFIG_STATIC_MEMORY in the first place in arm-specific file.

So perhaps that's what wants changing (at least for PGC_reserved)?

Jan

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

[Penny Zheng]

Hi jan and julien

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, May 6, 2022 2:14 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@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 v3 6/6] xen: retrieve reserved pages on populate_physmap
> 
> On 06.05.2022 04:59, Penny Zheng wrote:
> > Hi jan
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Thursday, May 5, 2022 10:23 PM
> >> To: Penny Zheng <Penny.Zheng@arm.com>
> >> Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang
> <Henry.Wang@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 v3 6/6] xen: retrieve reserved pages on
> >> populate_physmap
> >>
> >> On 05.05.2022 15:44, Penny Zheng wrote:
> >>> I misunderstand what you suggested here, I thought you were
> >>> suggesting a way of stub function which could bring some optimization.
> >>> The reason I introduced free_staticmem_pages and
> >>> acquire_reserved_page here is that we now used them in common code,
> >>> and if they are not defined(using stub) on !CONFIG_STATIC_MEMORY, we
> >>> will have " hidden
> >> symbol `xxx' isn't defined " compilation error.
> >>
> >> This is what I've asked for clarification about: If such errors
> >> surface, I'd like to understand why the respective call sites aren't DCE-ed by
> the compiler.
> >>
> >
> > Because both definition of PGC_reserved and
> > is_domain_using_static_memory are not guarded by
> CONFIG_STATIC_MEMORY in the first place in arm-specific file.
> 
> So perhaps that's what wants changing (at least for PGC_reserved)?
> 

Hmmm, I remembered that when I firstly introduced PGC_reserved through
"Domain on static allocation", Julien commented that he may like it to be
used for other purpose, not only static memory. And one example is reserved
memory when Live Updating.(https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg97829.html
)

> Jan