Re: [XEN v3 4/9] xen/arm: Introduce a wrapper for dt_device_get_address() to handle paddr_t

[Julien Grall <julien@xxxxxxx>]

Hi,

On 11/02/2023 00:20, Stefano Stabellini wrote:
> On Wed, 8 Feb 2023, Ayan Kumar Halder wrote:
> > dt_device_get_address() can accept u64 only for address and size.
> > However, the address/size denotes physical addresses. Thus, they should
> > be represented by 'paddr_t'.
> > Consequently, we introduce a wrapper for dt_device_get_address() ie
> > dt_device_get_paddr() which accepts address/size as paddr_t and inturn
> > invokes dt_device_get_address() after converting address/size to u64.
> >
> > The reason for introducing doing this is that in future 'paddr_t' may
> > be defined as u32. Thus, we need an explicit wrapper to do the type
> > conversion and return an error in case of truncation.
> >
> > With this, callers now invoke dt_device_get_paddr().
> > dt_device_get_address() is invoked by dt_device_get_paddr() only.
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
> > ---
> > Changes from -
> >
> > v1 - 1. New patch.
> >
> > v2 - 1. Extracted part of "[XEN v2 05/11] xen/arm: Use paddr_t instead of u64 for 
> > address/size"
> > into this patch.
> >
> > 2. dt_device_get_address() callers now invoke dt_device_get_paddr() instead.
> >
> > 3. Logged error in case of truncation.
> >
> >   xen/arch/arm/domain_build.c                | 10 +++---
> >   xen/arch/arm/gic-v2.c                      | 10 +++---
> >   xen/arch/arm/gic-v3-its.c                  |  4 +--
> >   xen/arch/arm/gic-v3.c                      | 10 +++---
> >   xen/arch/arm/pci/pci-host-common.c         |  6 ++--
> >   xen/arch/arm/platforms/brcm-raspberry-pi.c |  2 +-
> >   xen/arch/arm/platforms/brcm.c              |  4 +--
> >   xen/arch/arm/platforms/exynos5.c           | 32 +++++++++----------
> >   xen/arch/arm/platforms/sunxi.c             |  2 +-
> >   xen/arch/arm/platforms/xgene-storm.c       |  2 +-
> >   xen/common/device_tree.c                   | 36 ++++++++++++++++++++--
> >   xen/drivers/char/cadence-uart.c            |  4 +--
> >   xen/drivers/char/exynos4210-uart.c         |  4 +--
> >   xen/drivers/char/imx-lpuart.c              |  4 +--
> >   xen/drivers/char/meson-uart.c              |  4 +--
> >   xen/drivers/char/mvebu-uart.c              |  4 +--
> >   xen/drivers/char/ns16550.c                 |  2 +-
> >   xen/drivers/char/omap-uart.c               |  4 +--
> >   xen/drivers/char/pl011.c                   |  6 ++--
> >   xen/drivers/char/scif-uart.c               |  4 +--
> >   xen/drivers/passthrough/arm/ipmmu-vmsa.c   |  8 ++---
> >   xen/drivers/passthrough/arm/smmu-v3.c      |  2 +-
> >   xen/drivers/passthrough/arm/smmu.c         |  8 ++---
> >   xen/include/xen/device_tree.h              |  6 ++--
> >   24 files changed, 105 insertions(+), 73 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 4d7e67560f..c7e88f5011 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1692,13 +1692,13 @@ static int __init find_memory_holes(const struct 
> > kernel_info *kinfo,
> >       dt_for_each_device_node( dt_host, np )
> >       {
> >           unsigned int naddr;
> > -        u64 addr, size;
> > +        paddr_t addr, size;
> >   
> >           naddr = dt_number_of_address(np);
> >   
> >           for ( i = 0; i < naddr; i++ )
> >           {
> > -            res = dt_device_get_address(np, i, &addr, &size);
> > +            res = dt_device_get_paddr(np, i, &addr, &size);
> One thing to be careful here is that "start" and "end" in
> find_memory_holes are defined now as paddr_t and passed to
> rangeset_add_range and rangeset_remove_range.
I am a bit puzzled why you are saying "now". Without Ayan's patch addr 
was 64-bit so...
> rangeset_add_range and rangeset_remove_range take an unsigned long as
> parameter, so in an arm32 configuration with paddr_t set to 64-bit it
> would result in truncation
... the problem you are talking about would already exist.

I would consider to store a frame number rather than the full address 
because we can only deal with page-aligned region.
Stefano (or Ayan) can you look at it?

Cheers,

--
Julien Grall