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

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

[Penny Zheng]

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

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

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
This is a reissued commit for patch serie "populate/unpopulate memory when 
domain on static allocation"(
https://patchwork.kernel.org/project/xen-devel/list/?series=636094).
This commit has been held by mail server, so sorry about that and also sorry
about this commit couldn't stay in the same thread with the others.
---
v3 changes:
- fix possible racy issue in free_staticmem_pages()
- introduce a stub free_staticmem_pages() for the !CONFIG_STATIC_MEMORY case
- move the change to free_heap_pages() to cover other potential call sites
- fix the indentation
---
v2 changes:
- new commit
---
 xen/common/page_alloc.c | 17 ++++++++++++++---
 xen/include/xen/mm.h    |  2 +-
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 319029140f..5e569a48a2 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1443,6 +1443,10 @@ static void free_heap_pages(
 
     ASSERT(order <= MAX_ORDER);
 
+    if ( pg->count_info & PGC_reserved )
+        /* Reserved page shall not go back to the heap. */
+        return free_staticmem_pages(pg, 1UL << order, need_scrub);
+
     spin_lock(&heap_lock);
 
     for ( i = 0; i < (1 << order); i++ )
@@ -2636,8 +2640,8 @@ struct domain *get_pg_owner(domid_t domid)
 
 #ifdef CONFIG_STATIC_MEMORY
 /* Equivalent of free_heap_pages to free nr_mfns pages of static memory. */
-void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
-                                 bool need_scrub)
+void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
+                          bool need_scrub)
 {
     mfn_t mfn = page_to_mfn(pg);
     unsigned long i;
@@ -2653,7 +2657,8 @@ void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
         }
 
         /* In case initializing page of static memory, mark it PGC_reserved. */
-        pg[i].count_info |= PGC_reserved;
+        if ( !(pg[i].count_info & PGC_reserved) )
+            pg[i].count_info |= PGC_reserved;
     }
 }
 
@@ -2762,6 +2767,12 @@ int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
 
     return 0;
 }
+#else
+void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
+                          bool need_scrub)
+{
+    ASSERT_UNREACHABLE();
+}
 #endif
 
 /*
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 3be754da92..9fd95deaec 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -85,10 +85,10 @@ bool scrub_free_pages(void);
 } while ( false )
 #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0)
 
-#ifdef CONFIG_STATIC_MEMORY
 /* These functions are for static memory */
 void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
                           bool need_scrub);
+#ifdef CONFIG_STATIC_MEMORY
 int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int nr_mfns,
                             unsigned int memflags);
 #endif
-- 
2.25.1

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

[Jan Beulich]

On 28.04.2022 05:01, Penny Zheng wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1443,6 +1443,10 @@ static void free_heap_pages(
>  
>      ASSERT(order <= MAX_ORDER);
>  
> +    if ( pg->count_info & PGC_reserved )
> +        /* Reserved page shall not go back to the heap. */
> +        return free_staticmem_pages(pg, 1UL << order, need_scrub);

With PGC_reserved being zero, the compiler should CSE this call. Hence ...

> @@ -2762,6 +2767,12 @@ int __init acquire_domstatic_pages(struct domain *d, mfn_t smfn,
>  
>      return 0;
>  }
> +#else
> +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> +                          bool need_scrub)
> +{
> +    ASSERT_UNREACHABLE();
> +}
>  #endif

... I don't think this is needed?

Jan

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

[Penny Zheng]

Hi jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, May 4, 2022 9:27 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 v3 1/6] xen: do not free reserved memory into heap
> 
> On 28.04.2022 05:01, Penny Zheng wrote:
> > --- a/xen/common/page_alloc.c
> > +++ b/xen/common/page_alloc.c
> > @@ -1443,6 +1443,10 @@ static void free_heap_pages(
> >
> >      ASSERT(order <= MAX_ORDER);
> >
> > +    if ( pg->count_info & PGC_reserved )
> > +        /* Reserved page shall not go back to the heap. */
> > +        return free_staticmem_pages(pg, 1UL << order, need_scrub);
> 
> With PGC_reserved being zero, the compiler should CSE this call. Hence ...
> 

I assume that you suggest that we remove the stub function and just let
free_staticmem_pages not guarded by CONFIG_STATIC_MEMORY any more?
 
Hmmmm, on x86, PGC_reserved will be zero as not defined, and CSE will leave
no caller here.  but on arm, CSE could not guard this?
 
> > @@ -2762,6 +2767,12 @@ int __init acquire_domstatic_pages(struct
> > domain *d, mfn_t smfn,
> >
> >      return 0;
> >  }
> > +#else
> > +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns,
> > +                          bool need_scrub) {
> > +    ASSERT_UNREACHABLE();
> > +}
> >  #endif
> 
> ... I don't think this is needed?
> 
> Jan

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

[Jan Beulich]

On 05.05.2022 07:12, Penny Zheng wrote:
> Hi jan
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Wednesday, May 4, 2022 9:27 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 v3 1/6] xen: do not free reserved memory into heap
>>
>> On 28.04.2022 05:01, Penny Zheng wrote:
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -1443,6 +1443,10 @@ static void free_heap_pages(
>>>
>>>      ASSERT(order <= MAX_ORDER);
>>>
>>> +    if ( pg->count_info & PGC_reserved )
>>> +        /* Reserved page shall not go back to the heap. */
>>> +        return free_staticmem_pages(pg, 1UL << order, need_scrub);
>>
>> With PGC_reserved being zero, the compiler should CSE this call. Hence ...
>>
> 
> I assume that you suggest that we remove the stub function and just let
> free_staticmem_pages not guarded by CONFIG_STATIC_MEMORY any more?

No, I' not suggesting to remove the existing guard. I'm only suggesting
to avoid introducing a stub when that's not really needed.

> Hmmmm, on x86, PGC_reserved will be zero as not defined, and CSE will leave
> no caller here.  but on arm, CSE could not guard this?

Why would that be? When !CONFIG_STATIC_MEMORY, I'd expect PGC_reserved
to be zero on Arm as well.

Jan