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 switch from min_t() to min() as
bits_to_zone() cannot return a negative value. The macro is tweaked
to make it clearer.

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

Signed-off-by: Julien Grall <jgrall@amazon.com>

---
    Changes in v2:
        - Remove BUILD_BUG_ON()
        - Switch from min_t() to min()
---
 xen/common/page_alloc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index ace6333c18ea..958ba0cd9256 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -441,7 +441,7 @@ mfn_t __init alloc_boot_pages(unsigned long nr_pfns, unsigned long pfn_align)
 #define MEMZONE_XEN 0
 #define NR_ZONES    (PADDR_BITS - PAGE_SHIFT + 1)
 
-#define bits_to_zone(b) (((b) < (PAGE_SHIFT + 1)) ? 1 : ((b) - PAGE_SHIFT))
+#define bits_to_zone(b) (((b) < (PAGE_SHIFT + 1)) ? 1U : ((b) - PAGE_SHIFT))
 #define page_to_zone(pg) (is_xen_heap_page(pg) ? MEMZONE_XEN :  \
                           (flsl(mfn_x(page_to_mfn(pg))) ? : 1))
 
@@ -2336,8 +2336,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(bits_to_zone(bits), zone_hi);
+    ASSERT(zone_hi != 0);
 
     if ( memflags & MEMF_no_owner )
         memflags |= MEMF_no_refcount;
-- 
2.17.1

On 26.05.2021 18:11, Julien Grall wrote:
> 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 switch from min_t() to min() as
> bits_to_zone() cannot return a negative value. The macro is tweaked
> to make it clearer.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
>     Changes in v2:
>         - Remove BUILD_BUG_ON()
>         - Switch from min_t() to min()

Since this fulfills the dependencies put in place at the time, the
Reviewed-by: Jan Beulich <jbeulich@suse.com>
continues to apply here. Thanks for making the adjustments.

Jan

Hi Jan,

On 27/05/2021 08:15, Jan Beulich wrote:
> On 26.05.2021 18:11, Julien Grall wrote:
>> 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 switch from min_t() to min() as
>> bits_to_zone() cannot return a negative value. The macro is tweaked
>> to make it clearer.
>>
>> This bug was discovered and resolved using Coverity Static Analysis
>> Security Testing (SAST) by Synopsys, Inc.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> ---
>>      Changes in v2:
>>          - Remove BUILD_BUG_ON()
>>          - Switch from min_t() to min()
> 
> Since this fulfills the dependencies put in place at the time, the
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> continues to apply here. Thanks for making the adjustments.

Thanks for the review. It is now committed.

Cheers,

-- 
Julien Grall