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

In this case, I think a BUG_ON would be sufficient.


> > 
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> >   xen/arch/arm/include/asm/platform.h | 26 ++++++++++++++++++++++++++
> >   1 file changed, 26 insertions(+)
> > 
> > diff --git a/xen/arch/arm/include/asm/platform.h
> > b/xen/arch/arm/include/asm/platform.h
> > index 997eb25216..6be1549f09 100644
> > --- a/xen/arch/arm/include/asm/platform.h
> > +++ b/xen/arch/arm/include/asm/platform.h
> > @@ -42,6 +42,32 @@ struct platform_desc {
> >       unsigned int dma_bitsize;
> >   };
> >   +static inline int translate_dt_address_size(u64 *dt_addr, u64 *dt_size,
> > +                                            paddr_t *addr, paddr_t *size)
> > +{
> > +#ifdef CONFIG_ARM_PA_32
> 
> This is not yet defined. So you want to mention it in the commit message.
> 
> > +    if ( dt_addr && (*dt_addr >> PADDR_SHIFT) )
> > +    {
> > +        dprintk(XENLOG_ERR, "Error in DT. Invalid address\n");
> > +        return -ENXIO;
> > +    }
> > +
> > +    if ( dt_size && (*dt_size >> PADDR_SHIFT) )
> > +    {
> > +        dprintk(XENLOG_ERR, "Error in DT. Invalid size\n");
> > +        return -ENXIO;
> > +    }
> > +#endif
> > +
> > +    if ( dt_addr && addr )
> > +        *addr = (paddr_t) (*dt_addr);
> > +
> > +    if ( dt_size && size )
> > +        *size = (paddr_t) (*dt_size);
> > +
> > +    return 0;
> > +}
> > +
> >   /*
> >    * Quirk for platforms where device tree incorrectly reports 4K GICC
> >    * size, but actually the two GICC register ranges are placed at 64K
> 
> Cheers,
> 
> -- 
> Julien Grall
>