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 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


> 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.