xen/page_alloc: Remove dead code in alloc_domheap_pages()

Commit Message

Julien Grall April 6, 2021, 7:22 p.m. UTC

From: Julien Grall <[e-mail]>

Since commit 1aac966e24e9 "xen: support RAM at addresses 0 and 4096",
bits_to_zone() will never return 0 and it is expected that we have
minimum 2 zones.

Therefore the check in alloc_domheap_pages() is unnecessary and can
be removed. However, for sanity, it is replaced with an ASSERT().

Also take the opportunity to check atbuild time that NR_ZONES is minimum
2.

This bug was discovered and resolved using Coverity Static Analysis
Security Testing (SAST) by Synopsys, Inc.

Signed-off-by: Julien Grall <[e-mail]>
---
 xen/common/page_alloc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Jan Beulich April 7, 2021, 9:25 a.m. UTC | #1

On 06.04.2021 21:22, Julien Grall wrote:
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -457,6 +457,12 @@ static long total_avail_pages;
>  static DEFINE_SPINLOCK(heap_lock);
>  static long outstanding_claims; /* total outstanding claims by all domains */
>  
> +static void __init __maybe_unused build_assertions(void)
> +{
> +    /* Zone 0 is reserved for Xen, so we at least need two zones to function.*/
> +    BUILD_BUG_ON(NR_ZONES < 2);
> +}

With a couple of transformations this could also be

    BUILD_BUG_ON(PADDR_BITS <= PAGE_SHIFT);

i.e. you're checking that the architecture allows for at least two
pages to be addressable. Is this really a useful thing to check?

Irrespective of the usefulness, if this is to be kept I think the
function wants to live at the end of the source file, like the
majority of other files have it (another consistent place could
be at the top of the file, after all #include-s, as can be found
in two other cases).

> @@ -2340,8 +2346,9 @@ struct page_info *alloc_domheap_pages(
>  
>      bits = domain_clamp_alloc_bitsize(memflags & MEMF_no_owner ? NULL : d,
>                                        bits ? : (BITS_PER_LONG+PAGE_SHIFT));
> -    if ( (zone_hi = min_t(unsigned int, bits_to_zone(bits), zone_hi)) == 0 )
> -        return NULL;
> +
> +    zone_hi = min_t(unsigned int, bits_to_zone(bits), zone_hi);
> +    ASSERT(zone_hi != 0);

With the function above preferably dropped or at least moved,
Reviewed-by: Jan Beulich <[e-mail]>

I'd like to point out though that I think this would be a good
opportunity to eliminate the use of min_t() here, by changing
bits_to_zone()'s 1 to 1u. But I suppose you again would prefer
to not make this extra change right here, despite it being
somewhat related to bits_to_zone() only ever returning positive
values.

Jan

Julien Grall April 25, 2021, 1:41 p.m. UTC | #2

Hi Jan,

On 07/04/2021 10:25, Jan Beulich wrote:
> On 06.04.2021 21:22, Julien Grall wrote:
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -457,6 +457,12 @@ static long total_avail_pages;
>>   static DEFINE_SPINLOCK(heap_lock);
>>   static long outstanding_claims; /* total outstanding claims by all domains */
>>   
>> +static void __init __maybe_unused build_assertions(void)
>> +{
>> +    /* Zone 0 is reserved for Xen, so we at least need two zones to function.*/
>> +    BUILD_BUG_ON(NR_ZONES < 2);
>> +}
> 
> With a couple of transformations this could also be
> 
>      BUILD_BUG_ON(PADDR_BITS <= PAGE_SHIFT);
> 
> i.e. you're checking that the architecture allows for at least two
> pages to be addressable. Is this really a useful thing to check?

I saw it, but I was concerned that someone may modify the definition of 
NR_ZONES without looking at the rest of the code base.

Anyway, I guess that the ASSERT(zone_hi != 0) in the code should be 
sufficient to catch such problem. So I will drop it.

> 
> Irrespective of the usefulness, if this is to be kept I think the
> function wants to live at the end of the source file, like the
> majority of other files have it (another consistent place could
> be at the top of the file, after all #include-s, as can be found
> in two other cases).
> 
>> @@ -2340,8 +2346,9 @@ struct page_info *alloc_domheap_pages(
>>   
>>       bits = domain_clamp_alloc_bitsize(memflags & MEMF_no_owner ? NULL : d,
>>                                         bits ? : (BITS_PER_LONG+PAGE_SHIFT));
>> -    if ( (zone_hi = min_t(unsigned int, bits_to_zone(bits), zone_hi)) == 0 )
>> -        return NULL;
>> +
>> +    zone_hi = min_t(unsigned int, bits_to_zone(bits), zone_hi);
>> +    ASSERT(zone_hi != 0);
> 
> With the function above preferably dropped or at least moved,
> Reviewed-by: Jan Beulich <[e-mail]>
> 
> I'd like to point out though that I think this would be a good
> opportunity to eliminate the use of min_t() here, by changing
> bits_to_zone()'s 1 to 1u. But I suppose you again would prefer
> to not make this extra change right here, despite it being
> somewhat related to bits_to_zone() only ever returning positive
> values.

In general, I am in not in favor to modify unrelated to code (e.g. 
coding style) or things that deserved a more than a one-line explanation 
in the commit message. This is not the case here, so I will respin it 
and switch from min_t to min.

Cheers,

Patch

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 1744e6faa5c4..68e47d963842 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -457,6 +457,12 @@  static long total_avail_pages;
 static DEFINE_SPINLOCK(heap_lock);
 static long outstanding_claims; /* total outstanding claims by all domains */
 
+static void __init __maybe_unused build_assertions(void)
+{
+    /* Zone 0 is reserved for Xen, so we at least need two zones to function.*/
+    BUILD_BUG_ON(NR_ZONES < 2);
+}
+
 unsigned long domain_adjust_tot_pages(struct domain *d, long pages)
 {
     long dom_before, dom_after, dom_claimed, sys_before, sys_after;
@@ -2340,8 +2346,9 @@  struct page_info *alloc_domheap_pages(
 
     bits = domain_clamp_alloc_bitsize(memflags & MEMF_no_owner ? NULL : d,
                                       bits ? : (BITS_PER_LONG+PAGE_SHIFT));
-    if ( (zone_hi = min_t(unsigned int, bits_to_zone(bits), zone_hi)) == 0 )
-        return NULL;
+
+    zone_hi = min_t(unsigned int, bits_to_zone(bits), zone_hi);
+    ASSERT(zone_hi != 0);
 
     if ( memflags & MEMF_no_owner )
         memflags |= MEMF_no_refcount;