Re: [XEN v1 2/9] xen/arm: Define translate_dt_address_size() for the translation between u64 and paddr_t

[Julien Grall <julien@xxxxxxx>]

On 23/12/2022 10:01, Ayan Kumar Halder wrote:
> Hi Julien/Stefano,
>
> I want to make sure I understand correctly.
>
> On 22/12/2022 23:20, Stefano Stabellini wrote:
> > On Sat, 17 Dec 2022, Julien Grall wrote:
> > > On 17/12/2022 00:46, Stefano Stabellini wrote:
> > > > On Fri, 16 Dec 2022, Julien Grall wrote:
> > > > > Hi Ayan,
> > > > >
> > > > > On 15/12/2022 19:32, Ayan Kumar Halder wrote:
> > > > > > paddr_t may be u64 or u32 depending of the type of architecture.
> > > > > > Thus, while translating between u64 and paddr_t, one should check 
> > > > > > that
> > > > > > the
> > > > > > truncated bits are 0. If not, then raise an appropriate error.
> > > > > I am not entirely convinced this extra helper is worth it. If the user
> > > > > can't
> > > > > provide 32-bit address in the DT, then there are also a lot of 
> > > > > other part
> > > > > that
> > > > > can go wrong.
> > > > >
> > > > > Bertrand, Stefano, what do you think?
> > > > In general, it is not Xen's job to protect itself against bugs in 
> > > > device
> > > > tree. However, if Xen spots a problem in DT and prints a helpful 
> > > > message
> > > > that is better than just crashing because it gives a hint to the
> > > > developer about what the problem is.
> > > I agree with the principle. In practice this needs to be weight out 
> > > with the
> > > long-term maintenance.
> > >
> > > > In this case, I think a BUG_ON would be sufficient.
> > > BUG_ON() is the same as panic(). They both should be used only when 
> > > there are
> > > no way to recover (see CODING_STYLE).
> > >
> > > If we parse the device-tree at boot, then BUG_ON() is ok. However, if 
> > > we need
> > > to parse it after boot (e.g. the DT overlay from Vikram), then we should
> > > definitely not call BUG_ON() for checking DT input.
> > yeah, I wasn't thinking of that series
> >
> >
> > > The correct way is like Ayan did by checking returning an error and let
> > > the caller deciding what to do.
> > >
> > > My concern with his approach is the extra amount of code in each 
> > > caller to
> > > check it (even with a new helper).
> > >
> > > I am not fully convinced that checking the addresses in the DT is 
> > > that useful.
> > I am also happy not to do it to be honest
> So are you suggesting that we do the truncation, but do not check if any 
> non zero bits have been truncated.
> As an example, currently with this patch :-
>
> -        device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, 
> &gbase);
> -        psize = dt_read_number(cells, size_cells);
> +        device_tree_get_reg(&cells, addr_cells, addr_cells, &dt_pbase, 
> &dt_gbase);
> +        ret = translate_dt_address_size(&dt_pbase, &dt_gbase, &pbase, 
> &gbase);
> +        if ( ret )
> +            return ret;
> +        dt_psize = dt_read_number(cells, size_cells);
> +        ret = translate_dt_address_size(NULL, &dt_psize, NULL, &psize);
>
>
> With your proposed change, it should be
>
> -        device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, 
> &gbase);
> -        psize = dt_read_number(cells, size_cells);
> +        device_tree_get_reg(&cells, addr_cells, addr_cells, &dt_pbase, 
> &dt_gbase);
> +        dt_psize = dt_read_number(cells, size_cells);
> +        pbase = (paddr_t) dt_pbase;
> +        gbase = (paddr_t) dt_gbase;
> +        psize = (paddr_t) dt_psize;
-ETOOMANY cast and lines (the more if this is expected to be 
duplicated). But the last one seems unnecessary as the only reason you 
need separate variable for pbase and gbase is because the function are 
taking a reference rather than returning the value.
> Because, we still need some way to convert u64 dt address/size to 
> paddr_t (u64/u32) address/size. 
How about following the approach I suggested in my previous e-mail to 
Stefano:
  - Convert device_tree_get_reg() to use paddr_t.
  - Introduce dt_device_get_address_checked() (Assuming you want to 
still add the check)
We may need an helper to wrap around dt_read_number() but I don't have a 
good idea for a name. There are only a couple of use. So I think it is 
fine to open-code. But you would not need a separate local variable. For 
the cast, I am in two mind. In one way, I don't like unnecessary 
explicit cast, but on the other way it serves as documentation.
Stefano, any opinions?

Cheers,

--
Julien Grall