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

[Julien Grall <julien@xxxxxxx>]

On 16/12/2022 11:12, Julien Grall wrote:
> On 16/12/2022 10:49, Ayan Kumar Halder wrote:
> > On 16/12/2022 09:57, Julien Grall wrote:
> > > Hi,
> > Hi Julien,
> > > 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.
> > Sorry, I missed this. The caller should have checked for the return 
> > value and "return -EINVAL" for the conversion error.
> > I am in two minds.
> >
> > I am thinking that instead of returing error from 
> > translate_dt_address_size(), the function can invoke panic() when it 
> > detects incorrect address/size.
> panic() is not an option if the function can be used after boot. But 
> even if the use were restricted to boot ...
> > Any errors related to incorrect address/size (ie providing 64 bit for 
> > PA_32) should be treated as fatal. 
> ... translate_dt_address_size() is too generic to make this decision. 
> This is up to the caller to decide whether it is fine to ignore or crash.
> > But I do not see any precedent for this (ie libfdt does not panic for 
> > an error).
> The same reasoning applies here.
>
> > We could return an error from translate_dt_address_size() as we are 
> > doing today. It means that the errors need to be checked by all the 
> > callers (which adds extra code).
> Another option is the one I mentioned before. Don't check the truncation 
> because that's not Xen job to very all the Device-Tree (in particular if 

s/very/verify/

I only noticed it after sending (sorry).

> this is intrusive).
>
> Cheers,
--
Julien Grall