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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 23 Dec 2022, Julien Grall wrote:
> 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.

I think this is OK


>   - Introduce dt_device_get_address_checked() (Assuming you want to still add
> the check)

Let's just skip 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?

I would not add a wrapper for dt_read_number() and open-code the cast,
like you said because something non-trivial is happening and it serves
as documentation:

  psize = (paddr_t) dt_read_number(cells, size_cells);