Re: [XEN v4 02/11] xen/arm: domain_build: Track unallocated pages using the frame number

[Julien Grall <julien@xxxxxxx>]

Hi,

On 21/03/2023 14:03, Ayan Kumar Halder wrote:
> rangeset_{xxx}_range() functions are invoked with 'start' and 'size' as
> arguments which are either 'uint64_t' or 'paddr_t'. However, the function
> accepts 'unsigned long' for 'start' and 'size'. 'unsigned long' is 32 bits for
> ARM_32. Thus, there is an implicit downcasting from 'uint64_t'/'paddr_t' to
> 'unsigned long' when invoking rangeset_{xxx}_range().
>
> So, it may seem there is a possibility of lose of data due to truncation.
>
> In reality, 'start' and 'size' are always page aligned. And ARM_32 currently
> supports 40 bits as the width of physical address.
> So if the addresses are page aligned, the last 12 bits contain zeroes.
> Thus, we could instead pass page frame number which will contain 28 bits (40-12
> on Arm_32) and this can be represented using 'unsigned long'.
Is Arm_32 meant to refer to the config or the architecture? If the 
former, then it should be ARM_32 if the latter, it should be Arm32. I 
would prefer the latter.
> On Arm_64, this change will not induce any adverse side effect as the width of
Same here for Arm_64.

> physical address is 48 bits. Thus, the width of 'mfn' (ie 48 - 12 = 36) can be
> represented using 'unsigned long' (which is 64 bits wide).
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
> Changes from -
>
> v3 - 1. Extracted the patch from 
> https://lists.xenproject.org/archives/html/xen-devel/2023-02/msg00657.html
> and added it to this series.
> 2. Modified add_ext_regions(). This accepts a frame number instead of physical
> address.
>
>   xen/arch/arm/domain_build.c | 27 +++++++++++++++++----------
>   1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 15fa88e977..24b12b7512 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1500,10 +1500,13 @@ static int __init make_resv_memory_node(const struct 
> domain *d,
>       return res;
>   }
>   
> -static int __init add_ext_regions(unsigned long s, unsigned long e, void *data)
> +static int __init add_ext_regions(unsigned long s_pfn, unsigned long e_pfn,
We are trying to phase out any using of 'pfn' in the code. In this case, 
this is mean (see include/xen/mm.h for more details). Here, you want to 
use 'gfn' as we are looking for space in the dom0 memory address space.
> +                                  void *data)
>   {
>       struct meminfo *ext_regions = data;
>       paddr_t start, size;
> +    paddr_t s = PFN_UP(s_pfn);
> +    paddr_t e = PFN_UP(e_pfn);
PFN_UP() takes a physical address in parameter and return a page frame 
number. So this is not what you want here. You want pfn_to_paddr().
The rest looks good to me.

Cheers,

--
Julien Grall