Re: [XEN v1 3/9] xen/arm: Always use 'u64' instead of 'paddr_t' for address and size in DT

[Julien Grall <julien@xxxxxxx>]

Hi,

This patch is actually a good example to demonstrate the extra amount of 
boiler plate required to use your new boiler.
On 15/12/2022 19:32, Ayan Kumar Halder wrote:
> device_tree_get_reg(), dt_next_cell() uses u64 for address and size.
> Thus, the caller needs to be fixed to pass u64 values and then invoke
> translate_dt_address_size() to do the translation between u64 and paddr_t.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> ---
>   xen/arch/arm/bootfdt.c | 22 ++++++++++++++--------
>   1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 0085c28d74..835bb5feb9 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -14,6 +14,7 @@
>   #include <xen/libfdt/libfdt.h>
>   #include <xen/sort.h>
>   #include <xsm/xsm.h>
> +#include <asm/platform.h>
>   #include <asm/setup.h>
>   
>   static bool __init device_tree_node_matches(const void *fdt, int node,
> @@ -68,7 +69,7 @@ static int __init device_tree_get_meminfo(const void *fdt, 
> int node,
>       unsigned int i, banks;
>       const __be32 *cell;
>       u32 reg_cells = address_cells + size_cells;
> -    paddr_t start, size;
> +    u64 start, size;
>       struct meminfo *mem = data;
>   
>       if ( address_cells < 1 || size_cells < 1 )
> @@ -219,7 +220,7 @@ static void __init process_multiboot_node(const void *fdt, 
> int node,
>       const struct fdt_property *prop;
>       const __be32 *cell;
>       bootmodule_kind kind;
> -    paddr_t start, size;
> +    u64 start, size;
>       int len;
>       /* sizeof("/chosen/") + DT_MAX_NAME + '/' + DT_MAX_NAME + '/0' => 92 */
>       char path[92];
> @@ -379,7 +380,8 @@ static int __init process_shm_node(const void *fdt, int 
> node,
>   {
>       const struct fdt_property *prop, *prop_id, *prop_role;
>       const __be32 *cell;
> -    paddr_t paddr, gaddr, size;
> +    paddr_t paddr = 0, gaddr = 0, size = 0;
For a first 0 is a valid address. So we should not use is as initialization.

> +    u64 dt_paddr, dt_gaddr, dt_size;
>       struct meminfo *mem = &bootinfo.reserved_mem;
>       unsigned int i;
>       int len;
> @@ -443,10 +445,14 @@ static int __init process_shm_node(const void *fdt, int 
> node,
>       }
>   
>       cell = (const __be32 *)prop->data;
> -    device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr);
> -    size = dt_next_cell(size_cells, &cell);
> +    device_tree_get_reg(&cell, address_cells, address_cells, &dt_paddr,
> +                        &dt_gaddr);
> +    translate_dt_address_size(&dt_paddr, &dt_gaddr, &paddr, &gaddr
If we function return a value, then this should be checked. If not, then 
it should be explained.
In this case, it is not clear to me who is checking the conversion was 
successful.
Overall, I think this will increase the amount of code. So before doing 
the modification, I think we need to agree on whether this is worth it 
to check the device-tree values.
Cheers,

--
Julien Grall