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

[PATCH V2 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 as guest RAM for static domain shall always be
reserved to only this domain 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.

---
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/arm: do not free reserved memory into heap
  xen/arm: do not merge reserved pages in free_heap_pages()
  xen: add field "flags" to cover all internal CDF_XXX
  xen/arm: introduce CDF_staticmem
  xen/arm: unpopulate memory when domain is static
  xen/arm: 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     | 17 +++++++++++++
 xen/common/domain.c               |  7 ++++++
 xen/common/memory.c               | 29 ++++++++++++++++++++++
 xen/common/page_alloc.c           | 40 +++++++++++++++++++++++++++++--
 xen/include/xen/domain.h          |  2 ++
 xen/include/xen/mm.h              |  1 +
 xen/include/xen/sched.h           |  9 +++++++
 10 files changed, 110 insertions(+), 7 deletions(-)

-- 
2.25.1

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

[Penny Zheng]

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

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

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
v2 changes:
- new commit
---
 xen/common/page_alloc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 319029140f..9a3e9c1328 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2488,7 +2488,13 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
             scrub = 1;
         }
 
-        free_heap_pages(pg, order, scrub);
+#ifdef CONFIG_STATIC_MEMORY
+        if ( pg->count_info & PGC_reserved )
+            /* Reserved page shall not go back to the heap. */
+            free_staticmem_pages(pg, 1 << order, scrub);
+        else
+#endif
+            free_heap_pages(pg, order, scrub);
     }
 
     if ( drop_dom_ref )
@@ -2636,7 +2642,7 @@ 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,
+void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
                                  bool need_scrub)
 {
     mfn_t mfn = page_to_mfn(pg);
-- 
2.25.1

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

[Jan Beulich]

On 18.04.2022 14:22, Penny Zheng wrote:
> Pages as guest RAM for static domain, shall be reserved to this domain only.

Is there "used" missing as the 2nd word of the sentence?

> So in case reserved pages being used for other purpose, users
> shall not free them back to heap, even when last ref gets dropped.
> 
> free_staticmem_pages will be called by free_domheap_pages in runtime
> for static domain freeing memory resource, so let's drop the __init
> flag.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> v2 changes:
> - new commit
> ---
>  xen/common/page_alloc.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

With this diffstat the patch subject prefix is somewhat misleading;
I first thought I could skip this patch.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2488,7 +2488,13 @@ void free_domheap_pages(struct page_info *pg, unsigned int order)
>              scrub = 1;
>          }
>  
> -        free_heap_pages(pg, order, scrub);
> +#ifdef CONFIG_STATIC_MEMORY
> +        if ( pg->count_info & PGC_reserved )
> +            /* Reserved page shall not go back to the heap. */
> +            free_staticmem_pages(pg, 1 << order, scrub);

1UL with, in particular, the function parameter by "unsigned long".

By calling free_staticmem_pages() at runtime, you make the previous race
free (because of init-time only) update of .count_info there racy. Making
a clone of that function just for this difference would likely be
excessive, so I'd suggest to change the code there to

        /* In case initializing page of static memory, mark it PGC_reserved. */
        if ( !(pg[i].count_info & PGC_reserved) )
            pg[i].count_info |= PGC_reserved;

> +        else
> +#endif
> +            free_heap_pages(pg, order, scrub);

Of course it would be nice to avoid the #ifdef-ary here. May I ask
that you introduce a stub free_staticmem_pages() for the
!CONFIG_STATIC_MEMORY case, such that the construct can become

        if ( !(pg->count_info & PGC_reserved) )
            free_heap_pages(pg, order, scrub);
        else
            /* Reserved page shall not go back to the heap. */
            free_staticmem_pages(pg, 1 << order, scrub);

Another question is whether the distinction should be made here in
the first place. Would it perhaps better belong in free_heap_pages()
itself, thus also covering other potential call sites? Of course
this depends on where, long term, reserved pages can / will be used.
For domains to be truly static, Xen's own allocations to manage the
domain may also want to come from the reserved set ...

> @@ -2636,7 +2642,7 @@ 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,
> +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
>                                   bool need_scrub)

This line now wants its indentation adjusted.

Jan

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

[Penny Zheng]

Hi jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, April 19, 2022 4:59 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 v2 1/6] xen/arm: do not free reserved memory into heap
> 
> On 18.04.2022 14:22, Penny Zheng wrote:
> > Pages as guest RAM for static domain, shall be reserved to this domain only.
> 
> Is there "used" missing as the 2nd word of the sentence?
> 
> > So in case reserved pages being used for other purpose, users shall
> > not free them back to heap, even when last ref gets dropped.
> >
> > free_staticmem_pages will be called by free_domheap_pages in runtime
> > for static domain freeing memory resource, so let's drop the __init
> > flag.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > v2 changes:
> > - new commit
> > ---
> >  xen/common/page_alloc.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> With this diffstat the patch subject prefix is somewhat misleading; I first
> thought I could skip this patch.
> 

Oh, sorry. Will change the 'xen/arm' to 'xen'

> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -2488,7 +2488,13 @@ void free_domheap_pages(struct page_info *pg,
> unsigned int order)
> >              scrub = 1;
> >          }
> >
> > -        free_heap_pages(pg, order, scrub);
> > +#ifdef CONFIG_STATIC_MEMORY
> > +        if ( pg->count_info & PGC_reserved )
> > +            /* Reserved page shall not go back to the heap. */
> > +            free_staticmem_pages(pg, 1 << order, scrub);
> 
> 1UL with, in particular, the function parameter by "unsigned long".
> 
> By calling free_staticmem_pages() at runtime, you make the previous race free
> (because of init-time only) update of .count_info there racy. Making a clone of
> that function just for this difference would likely be excessive, so I'd suggest to
> change the code there to
> 
>         /* In case initializing page of static memory, mark it PGC_reserved. */
>         if ( !(pg[i].count_info & PGC_reserved) )
>             pg[i].count_info |= PGC_reserved;
> 

Learned!

> > +        else
> > +#endif
> > +            free_heap_pages(pg, order, scrub);
> 
> Of course it would be nice to avoid the #ifdef-ary here. May I ask that you
> introduce a stub free_staticmem_pages() for the !CONFIG_STATIC_MEMORY
> case, such that the construct can become
> 

Sure, will do.

>         if ( !(pg->count_info & PGC_reserved) )
>             free_heap_pages(pg, order, scrub);
>         else
>             /* Reserved page shall not go back to the heap. */
>             free_staticmem_pages(pg, 1 << order, scrub);
> 
> Another question is whether the distinction should be made here in the first
> place. Would it perhaps better belong in free_heap_pages() itself, thus also
> covering other potential call sites? Of course this depends on where, long term,
> reserved pages can / will be used.
> For domains to be truly static, Xen's own allocations to manage the domain
> may also want to come from the reserved set ...
> 

Yes, you're right. I'll defer the distinction to free_heap_pages. And refine the
in-code comment above free_staticmem_pages, in the first place, I was intending
to make it equivalent of free_heap_pages to free static memory.

However as you said, if letting free_heap_pages call free_staticmem_pages, it will 
cover other potential call site. We've already been trying to enable p2m pool on arm,
and in the future, maybe the pages constituting the pool shall not come from heap, but
from reserved set, if the domain is fully static

> > @@ -2636,7 +2642,7 @@ 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,
> > +void free_staticmem_pages(struct page_info *pg, unsigned long
> > +nr_mfns,
> >                                   bool need_scrub)
> 
> This line now wants its indentation adjusted.
> 
> Jan

[PATCH v2 2/6] xen/arm: 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>
---
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 9a3e9c1328..8ba38bca9a 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1479,6 +1479,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;
@@ -1502,6 +1503,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 v2 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>
---
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..36ec00e62d 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)->flags & 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 351029f8b2..859cc13d3b 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -568,6 +568,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->flags = 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..68eb08058e 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. */
+    uint32_t flags;
 };
 
 static inline struct page_list_head *page_to_list(
-- 
2.25.1

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

[Jan Beulich]

On 18.04.2022 14:22, Penny Zheng wrote:
> --- 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. */
> +    uint32_t flags;

There's no need to use a fixed width type here; unsigned int will do.
See ./CODING_STYLE.

I'd also like to ask for the field to be given a more descriptive name.
Just "flags" can mean about anything. Maybe simply "cdf"?

Jan

[PATCH v2 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_static to tell.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
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 8be01678de..4e62fd0bf1 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3191,9 +3191,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 36ec00e62d..b097433f9f 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)->flags & CDF_directmap)
 
+#define is_domain_static(d) ((d)->flags & 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

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

[Stefano Stabellini]

On Mon, 18 Apr 2022, 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_static to tell.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 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 8be01678de..4e62fd0bf1 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -3191,9 +3191,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 36ec00e62d..b097433f9f 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)->flags & CDF_directmap)
>  
> +#define is_domain_static(d) ((d)->flags & 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 v2 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>
---
v2 changes:
- put reserved pages on resv_page_list after having taken them off
the "normal" list
---
 xen/arch/arm/include/asm/mm.h | 17 +++++++++++++++++
 xen/common/domain.c           |  4 ++++
 xen/include/xen/sched.h       |  6 ++++++
 3 files changed, 27 insertions(+)

diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
index 424aaf2823..a2dde01cfa 100644
--- a/xen/arch/arm/include/asm/mm.h
+++ b/xen/arch/arm/include/asm/mm.h
@@ -358,6 +358,23 @@ 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) {                    \
+    if ( (pg)->count_info & PGC_reserved )              \
+    {                                                   \
+        page_list_del(pg, page_to_list(d, pg));         \
+        page_list_add_tail(pg, &(d)->resv_page_list);   \
+        (d)->resv_pages++;                              \
+    }                                                   \
+    else                                                \
+        page_list_del(pg, page_to_list(d, pg));         \
+}
+#endif
+
 #endif /*  __ARCH_ARM_MM__ */
 /*
  * Local variables:
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 859cc13d3b..b499cf8d2c 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -605,6 +605,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 68eb08058e..85ef378752 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()
@@ -395,6 +398,9 @@ struct domain
 #ifdef CONFIG_MEM_PAGING
     atomic_t         paged_pages;       /* paged-out pages */
 #endif
+#ifdef CONFIG_STATIC_MEMORY
+    unsigned int     resv_pages;        /* reserved pages from static region. */
+#endif
 
     /* Scheduling. */
     void            *sched_priv;    /* scheduler-specific data */
-- 
2.25.1

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

[Jan Beulich]

On 18.04.2022 14:22, Penny Zheng wrote:
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -358,6 +358,23 @@ 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) {                    \
> +    if ( (pg)->count_info & PGC_reserved )              \
> +    {                                                   \
> +        page_list_del(pg, page_to_list(d, pg));         \
> +        page_list_add_tail(pg, &(d)->resv_page_list);   \
> +        (d)->resv_pages++;                              \

There's no consumer of this counter, so I'd like to ask that it be
introduced once a consumer appears.

> +    }                                                   \
> +    else                                                \
> +        page_list_del(pg, page_to_list(d, pg));         \

Is there a particular reason to have this page_list_del() twice,
instead of just once ahead of the if()?

> +}

Also this entire construct want to be an expression, not a
(compound) statement. And it probably would better evaluate its
parameters just once.

Jan

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

[Penny Zheng]

Hi jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, April 19, 2022 5:11 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Julien Grall <julien@xen.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>;
> Wei Liu <wl@xen.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v2 5/6] xen/arm: unpopulate memory when domain is
> static
> 
> On 18.04.2022 14:22, Penny Zheng wrote:
> > --- a/xen/arch/arm/include/asm/mm.h
> > +++ b/xen/arch/arm/include/asm/mm.h
> > @@ -358,6 +358,23 @@ 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) {                    \
> > +    if ( (pg)->count_info & PGC_reserved )              \
> > +    {                                                   \
> > +        page_list_del(pg, page_to_list(d, pg));         \
> > +        page_list_add_tail(pg, &(d)->resv_page_list);   \
> > +        (d)->resv_pages++;                              \
> 
> There's no consumer of this counter, so I'd like to ask that it be introduced
> once a consumer appears.
> 
> > +    }                                                   \
> > +    else                                                \
> > +        page_list_del(pg, page_to_list(d, pg));         \
> 
> Is there a particular reason to have this page_list_del() twice, instead of just
> once ahead of the if()?
> 
> > +}
> 
> Also this entire construct want to be an expression, not a
> (compound) statement. And it probably would better evaluate its parameters
> just once.
> 

#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);   \
}

I'm trying to refine the arch_free_heap_page() here, but I'm a bit confused
about to let it be an expression, not a compound statement.  Do you mean that
you prefer to let the if-clause out of the arch_free_heap_page()?

> Jan

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

[Jan Beulich]

On 25.04.2022 08:34, Penny Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Tuesday, April 19, 2022 5:11 PM
>>
>> On 18.04.2022 14:22, Penny Zheng wrote:
>>> --- a/xen/arch/arm/include/asm/mm.h
>>> +++ b/xen/arch/arm/include/asm/mm.h
>>> @@ -358,6 +358,23 @@ 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) {                    \
>>> +    if ( (pg)->count_info & PGC_reserved )              \
>>> +    {                                                   \
>>> +        page_list_del(pg, page_to_list(d, pg));         \
>>> +        page_list_add_tail(pg, &(d)->resv_page_list);   \
>>> +        (d)->resv_pages++;                              \
>>
>> There's no consumer of this counter, so I'd like to ask that it be introduced
>> once a consumer appears.
>>
>>> +    }                                                   \
>>> +    else                                                \
>>> +        page_list_del(pg, page_to_list(d, pg));         \
>>
>> Is there a particular reason to have this page_list_del() twice, instead of just
>> once ahead of the if()?
>>
>>> +}
>>
>> Also this entire construct want to be an expression, not a
>> (compound) statement. And it probably would better evaluate its parameters
>> just once.
>>
> 
> #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);   \
> }
> 
> I'm trying to refine the arch_free_heap_page() here, but I'm a bit confused
> about to let it be an expression, not a compound statement.  Do you mean that
> you prefer to let the if-clause out of the arch_free_heap_page()?

No. You want to put parentheses around the braces, using a gcc extension
we make extensive use of throughout the code base.

Jan

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

[Penny Zheng]

Hi, jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, April 25, 2022 4:01 PM
> To: Penny Zheng <Penny.Zheng@arm.com>
> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Julien Grall <julien@xen.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>;
> Wei Liu <wl@xen.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v2 5/6] xen/arm: unpopulate memory when domain is
> static
> 
> On 25.04.2022 08:34, Penny Zheng wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Tuesday, April 19, 2022 5:11 PM
> >>
> >> On 18.04.2022 14:22, Penny Zheng wrote:
> >>> --- a/xen/arch/arm/include/asm/mm.h
> >>> +++ b/xen/arch/arm/include/asm/mm.h
> >>> @@ -358,6 +358,23 @@ 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) {                    \
> >>> +    if ( (pg)->count_info & PGC_reserved )              \
> >>> +    {                                                   \
> >>> +        page_list_del(pg, page_to_list(d, pg));         \
> >>> +        page_list_add_tail(pg, &(d)->resv_page_list);   \
> >>> +        (d)->resv_pages++;                              \
> >>
> >> There's no consumer of this counter, so I'd like to ask that it be
> >> introduced once a consumer appears.
> >>
> >>> +    }                                                   \
> >>> +    else                                                \
> >>> +        page_list_del(pg, page_to_list(d, pg));         \
> >>
> >> Is there a particular reason to have this page_list_del() twice,
> >> instead of just once ahead of the if()?
> >>
> >>> +}
> >>
> >> Also this entire construct want to be an expression, not a
> >> (compound) statement. And it probably would better evaluate its
> >> parameters just once.
> >>
> >
> > #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);   \
> > }
> >
> > I'm trying to refine the arch_free_heap_page() here, but I'm a bit
> > confused about to let it be an expression, not a compound statement.
> > Do you mean that you prefer to let the if-clause out of the
> arch_free_heap_page()?
> 
> No. You want to put parentheses around the braces, using a gcc extension we
> make extensive use of throughout the code base.
> 

Oh, oh, thanks!
put parentheses around the braces, then that's what you said about make it 
be an expression

> Jan

[PATCH v2 6/6] xen/arm: 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>
---
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     | 29 +++++++++++++++++++++++++++++
 xen/common/page_alloc.c | 28 ++++++++++++++++++++++++++++
 xen/include/xen/mm.h    |  1 +
 3 files changed, 58 insertions(+)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 69b0cd1e50..f7729dfa5c 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -35,6 +35,10 @@
 #include <asm/guest.h>
 #endif
 
+#ifndef is_domain_static
+#define is_domain_static(d) ((void)(d), false)
+#endif
+
 struct memop_args {
     /* INPUT */
     struct domain *domain;     /* Domain to be affected. */
@@ -245,6 +249,31 @@ static void populate_physmap(struct memop_args *a)
 
                 mfn = _mfn(gpfn);
             }
+#ifdef CONFIG_STATIC_MEMORY
+            else if ( is_domain_static(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;
+                }
+            }
+#endif
             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 8ba38bca9a..759d612eb8 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2770,6 +2770,34 @@ 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 %"PRI_mfn".\n",
+               d, mfn_x(page_to_mfn(page)));
+        return INVALID_MFN;
+    }
+    d->resv_pages--;
+
+    smfn = page_to_mfn(page);
+
+    if ( acquire_domstatic_pages(d, smfn, 1, memflags) )
+        return INVALID_MFN;
+
+    return smfn;
+}
 #endif
 
 /*
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 3be754da92..6ef5a6adcd 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -91,6 +91,7 @@ void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
                           bool need_scrub);
 int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int nr_mfns,
                             unsigned int memflags);
+mfn_t acquire_reserved_page(struct domain *d, unsigned int memflags);
 #endif
 
 /* Map machine page range in Xen virtual address space. */
-- 
2.25.1

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

[Jan Beulich]

On 18.04.2022 14:22, Penny Zheng wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -35,6 +35,10 @@
>  #include <asm/guest.h>
>  #endif
>  
> +#ifndef is_domain_static
> +#define is_domain_static(d) ((void)(d), false)
> +#endif

I think this might better live in a header. I wonder why you add it
though, considering ...

> @@ -245,6 +249,31 @@ static void populate_physmap(struct memop_args *a)
>  
>                  mfn = _mfn(gpfn);
>              }
> +#ifdef CONFIG_STATIC_MEMORY
> +            else if ( is_domain_static(d) )

... its use sits inside an #ifdef which ought to guarantee it's defined.
That said, even better would imo be if no new #ifdef-ary appeared here.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2770,6 +2770,34 @@ 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 %"PRI_mfn".\n",
> +               d, mfn_x(page_to_mfn(page)));

"page" is NULL, so page_to_mfn(page) is meaningless.

Jan

Re: [PATCH V2 0/6] populate/unpopulate memory when domain on static

[Jan Beulich]

On 18.04.2022 14:22, Penny Zheng wrote:
> 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 as guest RAM for static domain shall always be
> reserved to only this domain 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.
> 
> ---
> 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()"

I have reservations against this new name: This could mean far more aspects
of the domain are static than just its memory assignment. Was this intended
(or at least considered)?

Jan

RE: [PATCH V2 0/6] populate/unpopulate memory when domain on static

[Penny Zheng]

Hi jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Tuesday, April 19, 2022 4:47 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>; Bertrand Marquis <Bertrand.Marquis@arm.com>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH V2 0/6] populate/unpopulate memory when domain on
> static
> 
> On 18.04.2022 14:22, Penny Zheng wrote:
> > 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 as guest RAM for static domain
> > shall always be reserved to only this domain 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.
> >
> > ---
> > 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()"
> 
> I have reservations against this new name: This could mean far more aspects of
> the domain are static than just its memory assignment. Was this intended (or
> at least considered)?
> 

Ok. Julien gave me two suggestions back the day, maybe the other "is_domain_using_staticmem()"
is better and to the point. I'll change it in the next serie.

> Jan