[PATCH v2 1/2] Arm: drop assertion from page_is_ram_type()

Jan Beulich posted 2 patches 2 months, 1 week ago
[PATCH v2 1/2] Arm: drop assertion from page_is_ram_type()
Posted by Jan Beulich 2 months, 1 week ago
Its uses in offline_page() and query_page_offline() make it reachable on
Arm, as long as XEN_SYSCTL_page_offline_op doesn't have any Arm-specific
code added. It being reachable was even mentioned in the commit
introducing it, claiming it "clearly shouldn't be called on ARM just
yet".

However, dropping the assertion from a function of this name is deemed
problematic. Rename it to better reflect its sole purpose outside of
x86-specific code.

Fixes: 214c4cd94a80 ("xen: arm: stub page_is_ram_type")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Rename the function used in common code.
---
The new name is chosen such that, down the road, offlining of non-RAM
could in principle also become possible.

--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -64,10 +64,9 @@ int steal_page(
     return -EOPNOTSUPP;
 }
 
-int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
+bool page_is_offlinable(mfn_t mfn)
 {
-    ASSERT_UNREACHABLE();
-    return 0;
+    return false;
 }
 
 unsigned long domain_get_maximum_gpfn(struct domain *d)
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -425,6 +425,11 @@ int page_is_ram_type(unsigned long mfn,
     return 0;
 }
 
+bool page_is_offlinable(mfn_t mfn)
+{
+    return page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL);
+}
+
 unsigned int page_get_ram_type(mfn_t mfn)
 {
     uint64_t last = 0, maddr = mfn_to_maddr(mfn);
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -1635,7 +1635,7 @@ static unsigned long mark_page_offline(s
 {
     unsigned long nx, x, y = pg->count_info;
 
-    ASSERT(page_is_ram_type(mfn_x(page_to_mfn(pg)), RAM_TYPE_CONVENTIONAL));
+    ASSERT(page_is_offlinable(page_to_mfn(pg)));
     ASSERT(spin_is_locked(&heap_lock));
 
     do {
@@ -1711,7 +1711,7 @@ int offline_page(mfn_t mfn, int broken,
      * N.B. xen's txt in x86_64 is marked reserved and handled already.
      * Also kexec range is reserved.
      */
-    if ( !page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL) )
+    if ( !page_is_offlinable(mfn) )
     {
         *status = PG_OFFLINE_FAILED | PG_OFFLINE_NOT_CONV_RAM;
         return -EINVAL;
@@ -1851,7 +1851,7 @@ int query_page_offline(mfn_t mfn, uint32
 {
     struct page_info *pg;
 
-    if ( !mfn_valid(mfn) || !page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL) )
+    if ( !mfn_valid(mfn) || !page_is_offlinable(mfn) )
     {
         dprintk(XENLOG_WARNING, "call expand_pages() first\n");
         return -EINVAL;
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -144,9 +144,11 @@ unsigned long avail_domheap_pages_region
 unsigned long avail_node_heap_pages(unsigned int nodeid);
 #define alloc_domheap_page(d,f) (alloc_domheap_pages(d,0,f))
 #define free_domheap_page(p)  (free_domheap_pages(p,0))
+
 int online_page(mfn_t mfn, uint32_t *status);
 int offline_page(mfn_t mfn, int broken, uint32_t *status);
 int query_page_offline(mfn_t mfn, uint32_t *status);
+bool page_is_offlinable(mfn_t mfn);
 
 void heap_init_late(void);
Re: [PATCH v2 1/2] Arm: drop assertion from page_is_ram_type()
Posted by Orzel, Michal 2 months ago

On 18/08/2025 09:55, Jan Beulich wrote:
> Its uses in offline_page() and query_page_offline() make it reachable on
> Arm, as long as XEN_SYSCTL_page_offline_op doesn't have any Arm-specific
> code added. It being reachable was even mentioned in the commit
> introducing it, claiming it "clearly shouldn't be called on ARM just
> yet".
> 
> However, dropping the assertion from a function of this name is deemed
> problematic. Rename it to better reflect its sole purpose outside of
> x86-specific code.
> 
> Fixes: 214c4cd94a80 ("xen: arm: stub page_is_ram_type")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Michal Orzel <michal.orzel@amd.com>

~Michal
Re: [PATCH v2 1/2] Arm: drop assertion from page_is_ram_type()
Posted by Jason Andryuk 2 months, 1 week ago
On 2025-08-18 03:55, Jan Beulich wrote:
> Its uses in offline_page() and query_page_offline() make it reachable on
> Arm, as long as XEN_SYSCTL_page_offline_op doesn't have any Arm-specific
> code added. It being reachable was even mentioned in the commit
> introducing it, claiming it "clearly shouldn't be called on ARM just
> yet".
> 
> However, dropping the assertion from a function of this name is deemed
> problematic. Rename it to better reflect its sole purpose outside of
> x86-specific code.
> 
> Fixes: 214c4cd94a80 ("xen: arm: stub page_is_ram_type")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Re: [PATCH v2 1/2] Arm: drop assertion from page_is_ram_type()
Posted by Oleksii Kurochko 2 months, 1 week ago
On 8/18/25 9:55 AM, Jan Beulich wrote:
> Its uses in offline_page() and query_page_offline() make it reachable on
> Arm, as long as XEN_SYSCTL_page_offline_op doesn't have any Arm-specific
> code added. It being reachable was even mentioned in the commit
> introducing it, claiming it "clearly shouldn't be called on ARM just
> yet".
>
> However, dropping the assertion from a function of this name is deemed
> problematic. Rename it to better reflect its sole purpose outside of
> x86-specific code.
>
> Fixes: 214c4cd94a80 ("xen: arm: stub page_is_ram_type")
> Signed-off-by: Jan Beulich<jbeulich@suse.com>
> ---
> v2: Rename the function used in common code.
> ---
> The new name is chosen such that, down the road, offlining of non-RAM
> could in principle also become possible.

I think it could be useful to put in commit message.

LGTM: Reviewed-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>

~ Oleksii

>
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -64,10 +64,9 @@ int steal_page(
>       return -EOPNOTSUPP;
>   }
>   
> -int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
> +bool page_is_offlinable(mfn_t mfn)
>   {
> -    ASSERT_UNREACHABLE();
> -    return 0;
> +    return false;
>   }
>   
>   unsigned long domain_get_maximum_gpfn(struct domain *d)
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -425,6 +425,11 @@ int page_is_ram_type(unsigned long mfn,
>       return 0;
>   }
>   
> +bool page_is_offlinable(mfn_t mfn)
> +{
> +    return page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL);
> +}
> +
>   unsigned int page_get_ram_type(mfn_t mfn)
>   {
>       uint64_t last = 0, maddr = mfn_to_maddr(mfn);
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1635,7 +1635,7 @@ static unsigned long mark_page_offline(s
>   {
>       unsigned long nx, x, y = pg->count_info;
>   
> -    ASSERT(page_is_ram_type(mfn_x(page_to_mfn(pg)), RAM_TYPE_CONVENTIONAL));
> +    ASSERT(page_is_offlinable(page_to_mfn(pg)));
>       ASSERT(spin_is_locked(&heap_lock));
>   
>       do {
> @@ -1711,7 +1711,7 @@ int offline_page(mfn_t mfn, int broken,
>        * N.B. xen's txt in x86_64 is marked reserved and handled already.
>        * Also kexec range is reserved.
>        */
> -    if ( !page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL) )
> +    if ( !page_is_offlinable(mfn) )
>       {
>           *status = PG_OFFLINE_FAILED | PG_OFFLINE_NOT_CONV_RAM;
>           return -EINVAL;
> @@ -1851,7 +1851,7 @@ int query_page_offline(mfn_t mfn, uint32
>   {
>       struct page_info *pg;
>   
> -    if ( !mfn_valid(mfn) || !page_is_ram_type(mfn_x(mfn), RAM_TYPE_CONVENTIONAL) )
> +    if ( !mfn_valid(mfn) || !page_is_offlinable(mfn) )
>       {
>           dprintk(XENLOG_WARNING, "call expand_pages() first\n");
>           return -EINVAL;
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -144,9 +144,11 @@ unsigned long avail_domheap_pages_region
>   unsigned long avail_node_heap_pages(unsigned int nodeid);
>   #define alloc_domheap_page(d,f) (alloc_domheap_pages(d,0,f))
>   #define free_domheap_page(p)  (free_domheap_pages(p,0))
> +
>   int online_page(mfn_t mfn, uint32_t *status);
>   int offline_page(mfn_t mfn, int broken, uint32_t *status);
>   int query_page_offline(mfn_t mfn, uint32_t *status);
> +bool page_is_offlinable(mfn_t mfn);
>   
>   void heap_init_late(void);
>   
>