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

[Julien Grall <julien@xxxxxxx>]

Hi,

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.
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. However, if you both want to do it, then I think it would be 
best to introduce wrappers over the Device-Tree ones that will check the 
truncation.
For example, we could introduce dt_device_get_address_checked()
that will call dt_device_get_address() and then check for 32-bit truncation.

For the function device_tree_get_reg(), this helper was aimed to deal 
with just Physical address 'reg' very early. So we can modify the 
prototype and update the function to check for 32-bit truncation.
Note that I am suggesting a different approach for the two helpers 
because the former is generic and it is not clear to me whether this 
could be used in another context than physical address.
Cheers,

--
Julien Grall