xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] xen/page_alloc: Remove dead code in alloc_domheap_pages()
@ 2021-04-06 19:22 Julien Grall
  2021-04-07  9:25 ` Jan Beulich
  0 siblings, 1 reply; 3+ messages in thread
From: Julien Grall @ 2021-04-06 19:22 UTC (permalink / raw)
  To: xen-devel
  Cc: julien, Julien Grall, Andrew Cooper, George Dunlap, Ian Jackson,
	Jan Beulich, Stefano Stabellini, Wei Liu

From: Julien Grall <jgrall@amazon.com>

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 <jgrall@amazon.com>
---
 xen/common/page_alloc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

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;
-- 
2.17.1



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] xen/page_alloc: Remove dead code in alloc_domheap_pages()
  2021-04-06 19:22 [PATCH] xen/page_alloc: Remove dead code in alloc_domheap_pages() Julien Grall
@ 2021-04-07  9:25 ` Jan Beulich
  2021-04-25 13:41   ` Julien Grall
  0 siblings, 1 reply; 3+ messages in thread
From: Jan Beulich @ 2021-04-07  9:25 UTC (permalink / raw)
  To: Julien Grall
  Cc: Julien Grall, Andrew Cooper, George Dunlap, Ian Jackson,
	Stefano Stabellini, Wei Liu, xen-devel

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 <jbeulich@suse.com>

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] xen/page_alloc: Remove dead code in alloc_domheap_pages()
  2021-04-07  9:25 ` Jan Beulich
@ 2021-04-25 13:41   ` Julien Grall
  0 siblings, 0 replies; 3+ messages in thread
From: Julien Grall @ 2021-04-25 13:41 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Julien Grall, Andrew Cooper, George Dunlap, Ian Jackson,
	Stefano Stabellini, Wei Liu, xen-devel

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 <jbeulich@suse.com>
> 
> 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,

-- 
Julien Grall


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-04-25 13:41 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-06 19:22 [PATCH] xen/page_alloc: Remove dead code in alloc_domheap_pages() Julien Grall
2021-04-07  9:25 ` Jan Beulich
2021-04-25 13:41   ` Julien Grall

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).