Re: [PATCH v2 1/6] xen/arm: fix math in add_ext_regions

[Julien Grall <julien@xen.org>]

Hi Stewart,

On 08/05/2025 14:20, Stewart Hildebrand wrote:
> In commit f37a59813979, the arguments to add_ext_regions() were switched
> from addresses to frame numbers. add_ext_regions() converts the frame
> numbers back to addresses, but the end address (e) is rounded down to
> page size alignment. The logic to calculate the size assumes e points to
> the last address, not page, effectively leading to the region size being
> erroneously calculated to be 2M smaller than the actual size of the
> region.
> 
> Fix by adding 1 to the frame number before converting back to address.
> 
> Fixes: f37a59813979 ("xen/arm: domain_build: Track unallocated pages using the frame number")
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Acked-by: Michal Orzel <michal.orzel@amd.com>
> ---
> v1->v2:
> * add Michal's A-b
> ---
>   xen/arch/arm/domain_build.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index df29619c4007..2f2b021dec3e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -761,7 +761,7 @@ int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
>       struct membanks *ext_regions = data;
>       paddr_t start, size;
>       paddr_t s = pfn_to_paddr(s_gfn);
> -    paddr_t e = pfn_to_paddr(e_gfn);
> +    paddr_t e = pfn_to_paddr(e_gfn + 1) - 1;

I noticed this patch. While reading the function, I noticed this would 
result to some confusing code:


     if ( start > e )
         return 0;

     [...]

     /*
      * e is actually "end-1" because it is called by rangeset functions
      * which are inclusive of the last address.
      */
     e += 1;
     size = (e - start) & ~(SZ_2M - 1);

So substract 1, but then re-add after. Is there any reason we didn't 
adjust the rest of the code?

Cheers,

-- 
Julien Grall