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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 28/04/2023 19: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>
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 6c9712ab7b..2163cf26d0 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -955,6 +955,45 @@ 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, dt_size;
> +    int ret;
> +
> +    ret = dt_device_get_address(dev, index, &dt_addr, &dt_size);
> +    if ( ret )
> +        return ret;
> +
> +    if ( !addr )
Because of this ...

> +        return -EINVAL;
> +
> +    if ( addr )
you should drop this.

> +    {
> +        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;
> +        }
> +
> +        *size = dt_size;
> +    }
> +
> +    return ret;
> +}
> 
>  int dt_for_each_range(const struct dt_device_node *dev,
>                        int (*cb)(const struct dt_device_node *,
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 5f8f61aec8..ce25b89c4b 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -585,6 +585,19 @@ int dt_find_node_by_gpath(XEN_GUEST_HANDLE(char) u_path, 
> uint32_t u_plen,
>   */
>  const struct dt_device_node *dt_get_parent(const struct dt_device_node 
> *node);
> 
> +/**
> + * dt_device_get_paddr - Resolve an address for a device
> + * @device: the device whose address is to be resolved
> + * @index: index of the address to resolve
> + * @addr: address filled by this function
> + * @size: size filled by this function
> + *
> + * This function resolves an address, walking the tree, for a give
s/give/given

Other than that:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal