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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 11/02/2023 09:10, Julien Grall wrote:
> Hi,
Hi Julien/Stefano,
> 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?
I have sent out a patch to address this "[XEN v6 2/2] xen/arm: 
domain_build: Use pfn start and end address for rangeset_{xxx}_range()".
- Ayan

> Cheers,