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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Ayan,

On 13/04/2023 19:37, 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.
> 
>  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                   | 35 ++++++++++++++++++++++
>  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 +--
>  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              | 13 ++++++++
>  23 files changed, 116 insertions(+), 68 deletions(-)
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 6c9712ab7b..fdef74e7ff 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -955,6 +955,41 @@ int dt_device_get_address(const struct dt_device_node 
> *dev, unsigned int index,
>      return 0;
>  }
> 
> +int dt_device_get_paddr(const struct dt_device_node *dev, unsigned int index,
> +                        paddr_t *addr, paddr_t *size)
> +{
> +    uint64_t dt_addr = 0, dt_size = 0;
no need for these assignments

> +    int ret;
> +
> +    ret = dt_device_get_address(dev, index, &dt_addr, &dt_size);
> +    if ( ret )
> +        return ret;
> +
> +    if ( addr )
Since dt_device_get_paddr() is a wrapper for dt_device_get_address(), I think 
you should maintain the same
error logic for addr i.e. if no addr was specified (NULL), you return -EINVAL:
if ( !addr )
    return -EINVAL;

> +    {
> +        if ( dt_addr != (paddr_t)dt_addr )
> +        {
> +            printk("Error: Physical address 0x%"PRIx64" for node=%s is 
> greater than max width (%zu bytes) supported\n",
> +                   dt_addr, dev->name, sizeof(paddr_t));
> +            return -ERANGE;
> +        }
> +
> +        *addr = dt_addr;
> +    }
> +
> +    if ( size )
> +    {
> +        if ( dt_size != (paddr_t)dt_size )
> +        {
> +            printk("Error: Physical size 0x%"PRIx64" for node=%s is greater 
> than max width (%zu bytes) supported\n",
> +                   dt_size, dev->name, sizeof(paddr_t));
> +            return -ERANGE;
> +        }
add empty line

~Michal