Re: [XEN v6 03/12] xen/arm: Introduce a wrapper for dt_device_get_address() to handle paddr_t

[Julien Grall <julien@xxxxxxx>]

Hi,

On 03/05/2023 15:06, Ayan Kumar Halder wrote:
> On 03/05/2023 12:25, Julien Grall wrote:
> > Hi Ayan,
> Hi Julien,
> > On 28/04/2023 18:55, Ayan Kumar Halder wrote:
> > > dt_device_get_address() can accept uint64_t 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
> > > uint64_t.
> > >
> > > The reason for introducing this is that in future 'paddr_t' may not
> > > always be 64-bit. Thus, we need an explicit wrapper to do the type
> > > conversion and return an error in case of truncation.
> > >
> > > With this, callers can now invoke dt_device_get_paddr().
> > >
> > > 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.
> > >
> > > v3 - 1. Modified the truncation checks as "dt_addr != (paddr_t)dt_addr".
> > > 2. Some sanity fixes.
> > >
> > > v4 - 1. Some sanity fixes.
> > > 2. Preserved the declaration of dt_device_get_address() in
> > > xen/include/xen/device_tree.h. The reason being it is currently used by
> > > ns16550.c. This driver requires some more changes as pointed by Jan in
> > > https://lore.kernel.org/xen-devel/6196e90f-752e-e61a-45ce-37e46c22b812@xxxxxxxx/
> > > which is to be addressed as a separate series.
> > >
> > > v5 - 1. Removed initialization of variables.
> > > 2. In dt_device_get_paddr(), added the check
> > > if ( !addr )
> > >      return -EINVAL;
> > >
> > >   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              |  6 ++--
> > >   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                   | 39 ++++++++++++++++++++++
> > >   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/omap-uart.c               |  4 +--
> > >   xen/drivers/char/pl011.c                   |  6 ++--
> > >   xen/drivers/char/scif-uart.c               |  4 +--
> > What about the call in xen/drivers/char/ns16550.c?
> Refer to 
> https://lore.kernel.org/xen-devel/6196e90f-752e-e61a-45ce-37e46c22b812@xxxxxxxx/ , Jan mentioned that this driver needs some prior cleanup.
> So, I decided to take it out and do it after the current series has been 
> committed.
> See 
> https://patchew.org/Xen/20230413173735.48387-1-ayan.kumar.halder@xxxxxxx/ , Jan agreed to this.
> Is this ok with you ?
I am OK with that. Can you mention it in the commit message? This would 
help the reviewer or a future reader to understand why you left out 
ns16550 (this is the only call left).
With the remarks from Michal addressed:

Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

Cheers,

--
Julien Grall