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

[PATCH 0/5] 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 domain
on static allocation.Since guest RAM for domain on static allocation is
static memory, which is pre-reserved through domain congifuration, it shall
never go back to heap.

This patch serie intends to fix this issue, by keeping page allocated and
storing it in page list when unpopulating memory, and retrieving page from page
list when populating memory.

Penny Zheng (5):
  xen/arm: field "flags" to cover all internal CDF_XXX
  xen/arm: introduce CDF_staticmem
  xen/arm: unpopulate memory when domain on static allocation
  xen/arm: retrieve reserved pages on populate_physmap
  xen/arm: no need to store pages in resv_page_list when domain is
    directly mapped

 xen/arch/arm/domain.c             |  3 ++-
 xen/arch/arm/domain_build.c       |  5 +++-
 xen/arch/arm/include/asm/domain.h |  7 +++--
 xen/common/domain.c               |  4 +++
 xen/common/memory.c               | 45 ++++++++++++++++++++++++++++++-
 xen/include/xen/domain.h          |  2 ++
 xen/include/xen/sched.h           |  6 +++++
 7 files changed, 67 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH v1 1/5] xen/arm: 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" 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>
---
 xen/arch/arm/domain.c             | 3 ++-
 xen/arch/arm/include/asm/domain.h | 5 +++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 8110c1df86..35c157d499 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -709,7 +709,8 @@ int arch_domain_create(struct domain *d,
     ioreq_domain_init(d);
 #endif
 
-    d->arch.directmap = flags & CDF_directmap;
+    /* Holding CDF_* internal flags. */
+    d->arch.flags = flags;
 
     /* p2m_init relies on some value initialized by the IOMMU subsystem */
     if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index ed63c2b6f9..95fef29111 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)->arch.flags) & CDF_directmap)
 
 /*
  * Is the domain using the host memory layout?
@@ -103,7 +103,8 @@ struct arch_domain
     void *tee;
 #endif
 
-    bool directmap;
+    /* Holding CDF_* constant. Internal flags for domain creation. */
+    uint32_t flags;
 }  __cacheline_aligned;
 
 struct arch_vcpu
-- 
2.25.1

Re: [PATCH v1 1/5] xen/arm: field "flags" to cover all internal CDF_XXX

[Julien Grall]

Hi Penny,

On 30/03/2022 10:36, Penny Zheng wrote:
> With more and more CDF_xxx internal flags in and to save the space, this
> commit introduces a new field "flags" 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>
> ---
>   xen/arch/arm/domain.c             | 3 ++-
>   xen/arch/arm/include/asm/domain.h | 5 +++--
>   2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 8110c1df86..35c157d499 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -709,7 +709,8 @@ int arch_domain_create(struct domain *d,
>       ioreq_domain_init(d);
>   #endif
>   
> -    d->arch.directmap = flags & CDF_directmap;
> +    /* Holding CDF_* internal flags. */
> +    d->arch.flags = flags;
>   
>       /* p2m_init relies on some value initialized by the IOMMU subsystem */
>       if ( (rc = iommu_domain_init(d, config->iommu_opts)) != 0 )
> diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
> index ed63c2b6f9..95fef29111 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)->arch.flags) & CDF_directmap)

The () around (d)->arch.flags are not necessary.

>   
>   /*
>    * Is the domain using the host memory layout?
> @@ -103,7 +103,8 @@ struct arch_domain
>       void *tee;
>   #endif
>   
> -    bool directmap;
> +    /* Holding CDF_* constant. Internal flags for domain creation. */
> +    uint32_t flags;

I think this wants to live in the struct domain. So other arch can take 
advantage of it in the future.

>   }  __cacheline_aligned;
>   
>   struct arch_vcpu

Cheers,

-- 
Julien Grall

[PATCH v1 2/5] xen/arm: introduce CDF_staticmem

[Penny Zheng]

In order to have an easy and quick way to find out whether this domain is
on static allocation, this commit introduces a new flag CDF_staticmem and a
new helper is_domain_on_static_allocation.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
 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 95fef29111..4379f20a12 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)->arch.flags) & CDF_directmap)
 
+#define is_domain_on_static_allocation(d) (((d)->arch.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 v1 2/5] xen/arm: introduce CDF_staticmem

[Julien Grall]

Hi Penny,

On 30/03/2022 10:36, Penny Zheng wrote:
> In order to have an easy and quick way to find out whether this domain is
> on static allocation, this commit introduces a new flag CDF_staticmem and a
> new helper is_domain_on_static_allocation.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
>   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 95fef29111..4379f20a12 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)->arch.flags) & CDF_directmap)
>   
> +#define is_domain_on_static_allocation(d) (((d)->arch.flags) & CDF_staticmem)

"on" sounds strange to me and the name is bit long. How about 
"is_domain_using_staticmem()" or maybe "is_domain_static()"?

I will have a look at the rest of the series before giving my ack on 
this patch.

> +
>   /*
>    * 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
>   
>   /*

Cheers,

-- 
Julien Grall

[PATCH v1 3/5] xen/arm: unpopulate memory when domain on static allocation

[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 domain
on static allocation.

Guest RAM for domain on static allocation is static memory, which is reserved
to only this domain, so it shall never go back to heap.

For above purpose, this commit tries to keep page allocated and store it
in page list d->resv_page_list on guest_remove_page, when domain on
static allocation.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
 xen/common/domain.c     |  4 ++++
 xen/common/memory.c     | 22 +++++++++++++++++++++-
 xen/include/xen/sched.h |  6 ++++++
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 351029f8b2..e572f27fce 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -602,6 +602,10 @@ struct domain *domain_create(domid_t domid,
     INIT_PAGE_LIST_HEAD(&d->page_list);
     INIT_PAGE_LIST_HEAD(&d->extra_page_list);
     INIT_PAGE_LIST_HEAD(&d->xenpage_list);
+#ifdef CONFIG_STATIC_MEMORY
+    INIT_PAGE_LIST_HEAD(&d->resv_page_list);
+#endif
+
 
     spin_lock_init(&d->node_affinity_lock);
     d->node_affinity = NODE_MASK_ALL;
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 69b0cd1e50..2afc3c6f10 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -35,6 +35,10 @@
 #include <asm/guest.h>
 #endif
 
+#ifndef is_domain_on_static_allocation
+#define is_domain_on_static_allocation(d) 0
+#endif
+
 struct memop_args {
     /* INPUT */
     struct domain *domain;     /* Domain to be affected. */
@@ -405,13 +409,29 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
      * device must retrieve the same pfn when the hypercall populate_physmap
      * is called.
      *
+     * When domain on static allocation, they should always get pages from the
+     * reserved static region when the hypercall populate_physmap is called.
+     *
      * For this purpose (and to match populate_physmap() behavior), the page
      * is kept allocated.
      */
-    if ( !rc && !is_domain_direct_mapped(d) )
+    if ( !rc && !(is_domain_direct_mapped(d) ||
+                  is_domain_on_static_allocation(d)) )
         put_page_alloc_ref(page);
 
     put_page(page);
+#ifdef CONFIG_STATIC_MEMORY
+    /*
+     * When domain on static allocation, we shall store pages to resv_page_list,
+     * so the hypercall populate_physmap could retrieve pages from it,
+     * rather than allocating from heap.
+     */
+    if ( is_domain_on_static_allocation(d) )
+    {
+        page_list_add_tail(page, &d->resv_page_list);
+        d->resv_pages++;
+    }
+#endif
 
 #ifdef CONFIG_X86
  out_put_gfn:
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 406d9bc610..d7e047bf36 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()
@@ -389,6 +392,9 @@ struct domain
     unsigned int     extra_pages;       /* pages not included in domain_tot_pages() */
     atomic_t         shr_pages;         /* shared pages */
     atomic_t         paged_pages;       /* paged-out pages */
+#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 v1 3/5] xen/arm: unpopulate memory when domain on static allocation

[Jan Beulich]

On 30.03.2022 11:36, 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_on_static_allocation
> +#define is_domain_on_static_allocation(d) 0

Nit: "false", not "0".

> @@ -405,13 +409,29 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
>       * device must retrieve the same pfn when the hypercall populate_physmap
>       * is called.
>       *
> +     * When domain on static allocation, they should always get pages from the
> +     * reserved static region when the hypercall populate_physmap is called.
> +     *
>       * For this purpose (and to match populate_physmap() behavior), the page
>       * is kept allocated.
>       */
> -    if ( !rc && !is_domain_direct_mapped(d) )
> +    if ( !rc && !(is_domain_direct_mapped(d) ||
> +                  is_domain_on_static_allocation(d)) )
>          put_page_alloc_ref(page);
>  
>      put_page(page);
> +#ifdef CONFIG_STATIC_MEMORY
> +    /*
> +     * When domain on static allocation, we shall store pages to resv_page_list,
> +     * so the hypercall populate_physmap could retrieve pages from it,
> +     * rather than allocating from heap.
> +     */
> +    if ( is_domain_on_static_allocation(d) )
> +    {
> +        page_list_add_tail(page, &d->resv_page_list);
> +        d->resv_pages++;
> +    }
> +#endif

I think this is wrong, as a result of not integrating with put_page().
The page should only go on that list once its last ref was dropped. I
don't recall for sure, but iirc staticmem pages are put on the
domain's page list just like other pages would be. But then you also
corrupt the list when this isn't the last ref which is put.

As a result I also think that you shouldn't need to touch the earlier
if().

Jan

RE: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on static allocation

[Penny Zheng]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, March 30, 2022 5:53 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 v1 3/5] xen/arm: unpopulate memory when domain on
> static allocation
> 
> On 30.03.2022 11:36, 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_on_static_allocation #define
> > +is_domain_on_static_allocation(d) 0
> 
> Nit: "false", not "0".
> 
> > @@ -405,13 +409,29 @@ int guest_remove_page(struct domain *d,
> unsigned long gmfn)
> >       * device must retrieve the same pfn when the hypercall
> populate_physmap
> >       * is called.
> >       *
> > +     * When domain on static allocation, they should always get pages from
> the
> > +     * reserved static region when the hypercall populate_physmap is called.
> > +     *
> >       * For this purpose (and to match populate_physmap() behavior), the page
> >       * is kept allocated.
> >       */
> > -    if ( !rc && !is_domain_direct_mapped(d) )
> > +    if ( !rc && !(is_domain_direct_mapped(d) ||
> > +                  is_domain_on_static_allocation(d)) )
> >          put_page_alloc_ref(page);
> >
> >      put_page(page);
> > +#ifdef CONFIG_STATIC_MEMORY
> > +    /*
> > +     * When domain on static allocation, we shall store pages to
> resv_page_list,
> > +     * so the hypercall populate_physmap could retrieve pages from it,
> > +     * rather than allocating from heap.
> > +     */
> > +    if ( is_domain_on_static_allocation(d) )
> > +    {
> > +        page_list_add_tail(page, &d->resv_page_list);
> > +        d->resv_pages++;
> > +    }
> > +#endif
> 
> I think this is wrong, as a result of not integrating with put_page().
> The page should only go on that list once its last ref was dropped. I don't recall
> for sure, but iirc staticmem pages are put on the domain's page list just like
> other pages would be. But then you also corrupt the list when this isn't the last
> ref which is put.

Yes, staticmem pages are put on the domain's page list.
Here, I tried to only destroy the P2M mapping, and keep the page still allocated
to this domain.
resv_page_list is just providing an easy way to track down the unpopulated memory. 
'''
But then you also corrupt the list when this isn't the last
ref which is put.
'''
I'm sorry, would you like to be more specific on this comment?
I want these pages to linked in the domain's page list, then it could be
freed properly when domain get destroyed through relinquish_memory.

> 
> As a result I also think that you shouldn't need to touch the earlier if().
> 
> Jan

Re: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on static allocation

[Jan Beulich]

On 31.03.2022 08:13, Penny Zheng wrote:
> Hi Jan
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Wednesday, March 30, 2022 5:53 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 v1 3/5] xen/arm: unpopulate memory when domain on
>> static allocation
>>
>> On 30.03.2022 11:36, 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_on_static_allocation #define
>>> +is_domain_on_static_allocation(d) 0
>>
>> Nit: "false", not "0".
>>
>>> @@ -405,13 +409,29 @@ int guest_remove_page(struct domain *d,
>> unsigned long gmfn)
>>>       * device must retrieve the same pfn when the hypercall
>> populate_physmap
>>>       * is called.
>>>       *
>>> +     * When domain on static allocation, they should always get pages from
>> the
>>> +     * reserved static region when the hypercall populate_physmap is called.
>>> +     *
>>>       * For this purpose (and to match populate_physmap() behavior), the page
>>>       * is kept allocated.
>>>       */
>>> -    if ( !rc && !is_domain_direct_mapped(d) )
>>> +    if ( !rc && !(is_domain_direct_mapped(d) ||
>>> +                  is_domain_on_static_allocation(d)) )
>>>          put_page_alloc_ref(page);
>>>
>>>      put_page(page);
>>> +#ifdef CONFIG_STATIC_MEMORY
>>> +    /*
>>> +     * When domain on static allocation, we shall store pages to
>> resv_page_list,
>>> +     * so the hypercall populate_physmap could retrieve pages from it,
>>> +     * rather than allocating from heap.
>>> +     */
>>> +    if ( is_domain_on_static_allocation(d) )
>>> +    {
>>> +        page_list_add_tail(page, &d->resv_page_list);
>>> +        d->resv_pages++;
>>> +    }
>>> +#endif
>>
>> I think this is wrong, as a result of not integrating with put_page().
>> The page should only go on that list once its last ref was dropped. I don't recall
>> for sure, but iirc staticmem pages are put on the domain's page list just like
>> other pages would be. But then you also corrupt the list when this isn't the last
>> ref which is put.
> 
> Yes, staticmem pages are put on the domain's page list.
> Here, I tried to only destroy the P2M mapping, and keep the page still allocated
> to this domain.

Well, much depends on what you call "allocated". For populate_physmap
you then take pages from resv_page_list. So I'd like to distinguish
"allocated" from "reserved": The pages are allocated to the domain
when they're on the normal page list; they're reserved when they're
on the new list you introduce. And what you want to initiate here is
an "allocated" -> "reserved" transition.

> resv_page_list is just providing an easy way to track down the unpopulated memory. 
> '''
> But then you also corrupt the list when this isn't the last
> ref which is put.
> '''
> I'm sorry, would you like to be more specific on this comment?
> I want these pages to linked in the domain's page list, then it could be
> freed properly when domain get destroyed through relinquish_memory.

Clearly they can't be on both lists. Hence you can put them on the
new list only _after_ having taken them off the "normal" list. That
"taking off the normal list" should happen when the last ref is
dropped, not here - see free_domheap_pages()'s uses of
arch_free_heap_page(), recalling that free_domheap_page() is what
put_page() calls upon dropping the last ref.

Jan

RE: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on static allocation

[Penny Zheng]

Hi Jan 

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Thursday, March 31, 2022 3:06 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 v1 3/5] xen/arm: unpopulate memory when domain on
> static allocation
> 
> On 31.03.2022 08:13, Penny Zheng wrote:
> > Hi Jan
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: Wednesday, March 30, 2022 5:53 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 v1 3/5] xen/arm: unpopulate memory when domain on
> >> static allocation
> >>
> >> On 30.03.2022 11:36, 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_on_static_allocation #define
> >>> +is_domain_on_static_allocation(d) 0
> >>
> >> Nit: "false", not "0".
> >>
> >>> @@ -405,13 +409,29 @@ int guest_remove_page(struct domain *d,
> >> unsigned long gmfn)
> >>>       * device must retrieve the same pfn when the hypercall
> >> populate_physmap
> >>>       * is called.
> >>>       *
> >>> +     * When domain on static allocation, they should always get
> >>> + pages from
> >> the
> >>> +     * reserved static region when the hypercall populate_physmap is
> called.
> >>> +     *
> >>>       * For this purpose (and to match populate_physmap() behavior), the
> page
> >>>       * is kept allocated.
> >>>       */
> >>> -    if ( !rc && !is_domain_direct_mapped(d) )
> >>> +    if ( !rc && !(is_domain_direct_mapped(d) ||
> >>> +                  is_domain_on_static_allocation(d)) )
> >>>          put_page_alloc_ref(page);
> >>>
> >>>      put_page(page);
> >>> +#ifdef CONFIG_STATIC_MEMORY
> >>> +    /*
> >>> +     * When domain on static allocation, we shall store pages to
> >> resv_page_list,
> >>> +     * so the hypercall populate_physmap could retrieve pages from it,
> >>> +     * rather than allocating from heap.
> >>> +     */
> >>> +    if ( is_domain_on_static_allocation(d) )
> >>> +    {
> >>> +        page_list_add_tail(page, &d->resv_page_list);
> >>> +        d->resv_pages++;
> >>> +    }
> >>> +#endif
> >>
> >> I think this is wrong, as a result of not integrating with put_page().
> >> The page should only go on that list once its last ref was dropped. I
> >> don't recall for sure, but iirc staticmem pages are put on the
> >> domain's page list just like other pages would be. But then you also
> >> corrupt the list when this isn't the last ref which is put.
> >
> > Yes, staticmem pages are put on the domain's page list.
> > Here, I tried to only destroy the P2M mapping, and keep the page still
> > allocated to this domain.
> 
> Well, much depends on what you call "allocated". For populate_physmap you
> then take pages from resv_page_list. So I'd like to distinguish "allocated" from
> "reserved": The pages are allocated to the domain when they're on the normal
> page list; they're reserved when they're on the new list you introduce. And
> what you want to initiate here is an "allocated" -> "reserved" transition.
> 
> > resv_page_list is just providing an easy way to track down the unpopulated
> memory.
> > '''
> > But then you also corrupt the list when this isn't the last ref which
> > is put.
> > '''
> > I'm sorry, would you like to be more specific on this comment?
> > I want these pages to linked in the domain's page list, then it could
> > be freed properly when domain get destroyed through relinquish_memory.
> 
> Clearly they can't be on both lists. Hence you can put them on the new list
> only _after_ having taken them off the "normal" list. That "taking off the
> normal list" should happen when the last ref is dropped, not here - see
> free_domheap_pages()'s uses of arch_free_heap_page(), recalling that
> free_domheap_page() is what
> put_page() calls upon dropping the last ref.
> 

Right, right, I've missed the critical point "they can't be on both lists".
Here is a thing, put_page is a very common helper that it is also beening
used when freeing memory on domain deconstruction. At that time, I
don't want to put these pages in resv_page_list, I'd like them to be
freed to the heap. This putting pages in resv_page_list thing is only for
unpopulating memory on the runtime. So I'd suggest introducing a
new helper put_pages_resv(...) to do the work.
 
About before you mentioned that "The pages are allocated to the
domain when they're on the normal page list; they're reserved when
they're on the new list you introduce. " Is there a possibility that I
still keep the pages allocated but just moving them from normal page list
to the new resv_page_list? Of course, a few extra things needed to be
covered, like domain_adjust_tot_pages, scrubing the pages. 

Another reason I want to keep page allocated is that if putting pages in
resv_page_list upon dropping the last ref, we need to do a lot things on
pages to totally let it free, like set its owner to NULL, changing page state
from in_use to free, etc. Later when populating them, we shall do the
exact in backwards, setting the owner back to the domain, changing the
state from free back to in_use, which seems a bit useless. And actually
for domain on static allocation, these staticmem pages are reserved
from the very beginning, and when it is allocated to the domain, it
forever belongs to the domain, and it could never be used in any other way.
 
Later when populating the memory, we could just move the pages from
resv_page_list back to the normal list, and also domain_adjust_tot_pages.

Another thing I'd consider to be affected is that when domain is dying, and
doing relinquish_memory, I need extra relinquishing for pages in resv_page_list.

> Jan

Re: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on static allocation

[Julien Grall]

Hi Penny,

On 31/03/2022 11:30, Penny Zheng wrote:
> Another reason I want to keep page allocated is that if putting pages in
> resv_page_list upon dropping the last ref, we need to do a lot things on
> pages to totally let it free, like set its owner to NULL, changing page state
> from in_use to free, etc.
This is not only about optimization here. Bad things can happen if you 
let a page in state free that is not meant to be used by the buddy 
allocator (e.g. it was reserved for static memory).

I discovered it earlier this year when trying to optimize 
init_heap_pages() for Live-Update. It was quite hard to debug because 
the corruption very rarely happened. So let me explain it before you 
face the same issue :).

free_heap_pages() will try to merge the about-to-be-freed chunk with the 
predecessor and/or successor. To know if the page can be merged, the 
algorithm is looking at whether the chunk is suitably aligned (e.g. same 
order) and if the page is in state free.

AFAICT, the pages belonging to the buddy allocator could be right next 
to region reserved memory. So there is a very slim chance that 
free_heap_pages() may decide to merge a chunk from the static region 
with the about-to-be-free chunk. Nothing very good will ensue.

Technically, this is already a bug in the already merged implementation 
of the static memory allocator.

I can see two ways to fix it:
   1) Update free_heap_pages() to check whether the page has 
PGC_reserved set.
   2) Use a different state for pages used by the static allocator.

So far my preference is leaning towards 1. But I would like to hear 
other opinions.

Cheers,

-- 
Julien Grall

RE: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on static allocation

[Penny Zheng]

Hi Julien and Jan

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, April 2, 2022 2:54 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; Jan Beulich <jbeulich@suse.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>; Stefano Stabellini <sstabellini@kernel.org>; Wei
> Liu <wl@xen.org>; xen-devel@lists.xenproject.org
> Subject: Re: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on
> static allocation
> 
> Hi Penny,
> 
> On 31/03/2022 11:30, Penny Zheng wrote:
> > Another reason I want to keep page allocated is that if putting pages
> > in resv_page_list upon dropping the last ref, we need to do a lot
> > things on pages to totally let it free, like set its owner to NULL,
> > changing page state from in_use to free, etc.
> This is not only about optimization here. Bad things can happen if you let a
> page in state free that is not meant to be used by the buddy allocator (e.g. it
> was reserved for static memory).
> 
> I discovered it earlier this year when trying to optimize
> init_heap_pages() for Live-Update. It was quite hard to debug because the
> corruption very rarely happened. So let me explain it before you face the same
> issue :).
> 
> free_heap_pages() will try to merge the about-to-be-freed chunk with the
> predecessor and/or successor. To know if the page can be merged, the
> algorithm is looking at whether the chunk is suitably aligned (e.g. same
> order) and if the page is in state free.
> 
> AFAICT, the pages belonging to the buddy allocator could be right next to
> region reserved memory. So there is a very slim chance that
> free_heap_pages() may decide to merge a chunk from the static region with
> the about-to-be-free chunk. Nothing very good will ensue.
> 

Oh,,, that's a thousand true.
If the free static region is the buddy of the about-to-be-free chunk, current
code will merge the static region to the heap, which is unacceptable.

I'll fix it in this patch serie too and I'm also preferring option 1~

And for unpopulating memory on runtime, I'll do what Jan suggests,
adding a new logic of moving the page from d->page_list to
d->resv_page_list in arch_free_heap_page() for reserved pages. 

> Technically, this is already a bug in the already merged implementation of the
> static memory allocator.
> 
> I can see two ways to fix it:
>    1) Update free_heap_pages() to check whether the page has PGC_reserved
> set.
>    2) Use a different state for pages used by the static allocator.
> 
> So far my preference is leaning towards 1. But I would like to hear other
> opinions.
> 
> Cheers,
> 
> --
> Julien Grall

Re: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on static allocation

[Jan Beulich]

On 31.03.2022 12:30, Penny Zheng wrote:
> Hi Jan 
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Thursday, March 31, 2022 3:06 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 v1 3/5] xen/arm: unpopulate memory when domain on
>> static allocation
>>
>> On 31.03.2022 08:13, Penny Zheng wrote:
>>> Hi Jan
>>>
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: Wednesday, March 30, 2022 5:53 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 v1 3/5] xen/arm: unpopulate memory when domain on
>>>> static allocation
>>>>
>>>> On 30.03.2022 11:36, 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_on_static_allocation #define
>>>>> +is_domain_on_static_allocation(d) 0
>>>>
>>>> Nit: "false", not "0".
>>>>
>>>>> @@ -405,13 +409,29 @@ int guest_remove_page(struct domain *d,
>>>> unsigned long gmfn)
>>>>>       * device must retrieve the same pfn when the hypercall
>>>> populate_physmap
>>>>>       * is called.
>>>>>       *
>>>>> +     * When domain on static allocation, they should always get
>>>>> + pages from
>>>> the
>>>>> +     * reserved static region when the hypercall populate_physmap is
>> called.
>>>>> +     *
>>>>>       * For this purpose (and to match populate_physmap() behavior), the
>> page
>>>>>       * is kept allocated.
>>>>>       */
>>>>> -    if ( !rc && !is_domain_direct_mapped(d) )
>>>>> +    if ( !rc && !(is_domain_direct_mapped(d) ||
>>>>> +                  is_domain_on_static_allocation(d)) )
>>>>>          put_page_alloc_ref(page);
>>>>>
>>>>>      put_page(page);
>>>>> +#ifdef CONFIG_STATIC_MEMORY
>>>>> +    /*
>>>>> +     * When domain on static allocation, we shall store pages to
>>>> resv_page_list,
>>>>> +     * so the hypercall populate_physmap could retrieve pages from it,
>>>>> +     * rather than allocating from heap.
>>>>> +     */
>>>>> +    if ( is_domain_on_static_allocation(d) )
>>>>> +    {
>>>>> +        page_list_add_tail(page, &d->resv_page_list);
>>>>> +        d->resv_pages++;
>>>>> +    }
>>>>> +#endif
>>>>
>>>> I think this is wrong, as a result of not integrating with put_page().
>>>> The page should only go on that list once its last ref was dropped. I
>>>> don't recall for sure, but iirc staticmem pages are put on the
>>>> domain's page list just like other pages would be. But then you also
>>>> corrupt the list when this isn't the last ref which is put.
>>>
>>> Yes, staticmem pages are put on the domain's page list.
>>> Here, I tried to only destroy the P2M mapping, and keep the page still
>>> allocated to this domain.
>>
>> Well, much depends on what you call "allocated". For populate_physmap you
>> then take pages from resv_page_list. So I'd like to distinguish "allocated" from
>> "reserved": The pages are allocated to the domain when they're on the normal
>> page list; they're reserved when they're on the new list you introduce. And
>> what you want to initiate here is an "allocated" -> "reserved" transition.
>>
>>> resv_page_list is just providing an easy way to track down the unpopulated
>> memory.
>>> '''
>>> But then you also corrupt the list when this isn't the last ref which
>>> is put.
>>> '''
>>> I'm sorry, would you like to be more specific on this comment?
>>> I want these pages to linked in the domain's page list, then it could
>>> be freed properly when domain get destroyed through relinquish_memory.
>>
>> Clearly they can't be on both lists. Hence you can put them on the new list
>> only _after_ having taken them off the "normal" list. That "taking off the
>> normal list" should happen when the last ref is dropped, not here - see
>> free_domheap_pages()'s uses of arch_free_heap_page(), recalling that
>> free_domheap_page() is what
>> put_page() calls upon dropping the last ref.
>>
> 
> Right, right, I've missed the critical point "they can't be on both lists".
> Here is a thing, put_page is a very common helper that it is also beening
> used when freeing memory on domain deconstruction. At that time, I
> don't want to put these pages in resv_page_list, I'd like them to be
> freed to the heap. This putting pages in resv_page_list thing is only for
> unpopulating memory on the runtime. So I'd suggest introducing a
> new helper put_pages_resv(...) to do the work.

I'd like to suggest to avoid introducing special case helpers as much
as possible. put_page() would better remain the single, common
function to use when dropping a ref. Any special treatment for certain
kinds of pages should happen without the callers needing to care.

> About before you mentioned that "The pages are allocated to the
> domain when they're on the normal page list; they're reserved when
> they're on the new list you introduce. " Is there a possibility that I
> still keep the pages allocated but just moving them from normal page list
> to the new resv_page_list? Of course, a few extra things needed to be
> covered, like domain_adjust_tot_pages, scrubing the pages. 

It's all a matter of definition. What I said (and what you quoted) was
merely based on my understanding of your intentions.

> Another reason I want to keep page allocated is that if putting pages in
> resv_page_list upon dropping the last ref, we need to do a lot things on
> pages to totally let it free, like set its owner to NULL, changing page state
> from in_use to free, etc. Later when populating them, we shall do the
> exact in backwards, setting the owner back to the domain, changing the
> state from free back to in_use, which seems a bit useless. And actually
> for domain on static allocation, these staticmem pages are reserved
> from the very beginning, and when it is allocated to the domain, it
> forever belongs to the domain, and it could never be used in any other way.
>  
> Later when populating the memory, we could just move the pages from
> resv_page_list back to the normal list, and also domain_adjust_tot_pages.

I don't mind the particular model you want to implement. What I do care
about is that the end result is consistent within itself, with as little
special casing (potentially) all over the code base as possible.

Jan

Re: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on static allocation

[Julien Grall]

Hi Jan,

On 30/03/2022 10:52, Jan Beulich wrote:
> On 30.03.2022 11:36, 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_on_static_allocation
>> +#define is_domain_on_static_allocation(d) 0
> 
> Nit: "false", not "0".

I think we also want to evaluate d so the behavior on x86 and arm is the 
same. Something like ((void)(d), false).

Cheers,

-- 
Julien Grall

[PATCH v1 4/5] xen/arm: retrieve reserved pages on populate_physmap

[Penny Zheng]

When domain on static allocation populates memory through populate_physmap,
other than allocating from heap, it shall allocate from resv_page_list to
make sure that all guest RAM are still restricted in statically configured
regions.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
 xen/common/memory.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 2afc3c6f10..2122ceeba7 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -249,6 +249,26 @@ static void populate_physmap(struct memop_args *a)
 
                 mfn = _mfn(gpfn);
             }
+#ifdef CONFIG_STATIC_MEMORY
+            else if ( is_domain_on_static_allocation(d) )
+            {
+                for ( j = 0; j < (1U << a->extent_order); j++ )
+                {
+                    page = page_list_remove_head(&d->resv_page_list);
+                    if ( unlikely(!page) )
+                    {
+                        gdprintk(XENLOG_INFO,
+                                 "Could not allocate guest page number %lx\n",
+                                 gfn_x(_gfn(gpfn)));
+                        goto out;
+                    }
+                    d->resv_pages--;
+
+                    if ( j == 0 )
+                        mfn = page_to_mfn(page);
+                }
+            }
+#endif
             else
             {
                 page = alloc_domheap_pages(d, a->extent_order, a->memflags);
-- 
2.25.1

Re: [PATCH v1 4/5] xen/arm: retrieve reserved pages on populate_physmap

[Jan Beulich]

On 30.03.2022 11:36, Penny Zheng wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -249,6 +249,26 @@ static void populate_physmap(struct memop_args *a)
>  
>                  mfn = _mfn(gpfn);
>              }
> +#ifdef CONFIG_STATIC_MEMORY
> +            else if ( is_domain_on_static_allocation(d) )
> +            {
> +                for ( j = 0; j < (1U << a->extent_order); j++ )
> +                {
> +                    page = page_list_remove_head(&d->resv_page_list);

How do you guarantee the pages are contiguous, as required by a non-zero
a->extent_order? Did you perhaps mean to forbid non-zero-order requests
in this configuration?

> +                    if ( unlikely(!page) )
> +                    {
> +                        gdprintk(XENLOG_INFO,
> +                                 "Could not allocate guest page number %lx\n",
> +                                 gfn_x(_gfn(gpfn)));
> +                        goto out;

And earlier allocated pages are simply lost / leaked?

Jan

RE: [PATCH v1 4/5] xen/arm: retrieve reserved pages on populate_physmap

[Penny Zheng]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, March 30, 2022 5:59 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 v1 4/5] xen/arm: retrieve reserved pages on
> populate_physmap
> 
> On 30.03.2022 11:36, Penny Zheng wrote:
> > --- a/xen/common/memory.c
> > +++ b/xen/common/memory.c
> > @@ -249,6 +249,26 @@ static void populate_physmap(struct memop_args
> *a)
> >
> >                  mfn = _mfn(gpfn);
> >              }
> > +#ifdef CONFIG_STATIC_MEMORY
> > +            else if ( is_domain_on_static_allocation(d) )
> > +            {
> > +                for ( j = 0; j < (1U << a->extent_order); j++ )
> > +                {
> > +                    page = page_list_remove_head(&d->resv_page_list);
> 
> How do you guarantee the pages are contiguous, as required by a non-zero
> a->extent_order? Did you perhaps mean to forbid non-zero-order requests
> in this configuration?,

True, true, thanks for pointing that out.
I would not intend to add complex algorithm here to find contiguous pages of requested
order, forbidding non-zero-order with error message shall be added here.

> 
> > +                    if ( unlikely(!page) )
> > +                    {
> > +                        gdprintk(XENLOG_INFO,
> > +                                 "Could not allocate guest page number %lx\n",
> > +                                 gfn_x(_gfn(gpfn)));
> > +                        goto out;
> 
> And earlier allocated pages are simply lost / leaked?
> 

That was unconsidered, thanks for pointing that out.
Since now we are forbidding non-zero-order requests, earlier allocated pages shall
not need to be considered.
 
> Jan

[PATCH v1 5/5] xen/arm: no need to store pages in resv_page_list when domain is directly mapped

[Penny Zheng]

When domain on static allocation and is directly mapped, in terms of
GPA == HPA(guest physical address == host physical address), we could use
mfn_to_page() to easily find the page, so there is no need to store pages
in resv_page_list.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
 xen/common/memory.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 2122ceeba7..2865e09a33 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -445,8 +445,11 @@ int guest_remove_page(struct domain *d, unsigned long gmfn)
      * When domain on static allocation, we shall store pages to resv_page_list,
      * so the hypercall populate_physmap could retrieve pages from it,
      * rather than allocating from heap.
+     * No need to store pages in resv_page_list when domain on static
+     * allocation and directly mapped, since we could use mfn_to_page() to
+     * easily find the page.
      */
-    if ( is_domain_on_static_allocation(d) )
+    if ( is_domain_on_static_allocation(d) && !is_domain_direct_mapped(d) )
     {
         page_list_add_tail(page, &d->resv_page_list);
         d->resv_pages++;
-- 
2.25.1