[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [XEN v2 04/11] xen/arm: Typecast the DT values into paddr_t



On Tue, 17 Jan 2023, Ayan Kumar Halder wrote:
> In future, we wish to support 32 bit physical address.
> However, one can only read u64 values from the DT. Thus, we need to
> typecast the values appropriately from u64 to paddr_t.
> 
> device_tree_get_reg() should now be able to return paddr_t. This is
> invoked by various callers to get DT address and size.
> Similarly, dt_read_number() is invoked as well to get DT address and
> size. The return value is typecasted to paddr_t.
> fdt_get_mem_rsv() can only accept u64 values. So, we provide a warpper
> for this called fdt_get_mem_rsv_paddr() which will do the necessary
> typecasting before invoking fdt_get_mem_rsv() and while returning.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>

I know we discussed this before and you implemented exactly what we
suggested, but now looking at this patch I think we should do the
following:

- also add a wrapper for dt_read_number, something like
  dt_read_number_paddr that returns paddr
- add a check for the top 32-bit being zero in all the wrappers
  (dt_read_number_paddr, device_tree_get_reg, fdt_get_mem_rsv_paddr)
  when paddr!=uint64_t. In case the top 32-bit are != zero I think we
  should print an error

Julien, I remember you were concerned about BUG_ON/panic/ASSERT and I
agree with you there (especially considering Vikram's device tree
overlay series). So here I am only suggesting to check truncation and
printk a message, not panic. Would you be OK with that?

Last comment, maybe we could add fdt_get_mem_rsv_paddr to setup.h
instead of introducing xen/arch/arm/include/asm/device_tree.h, given
that we already have device tree definitions there
(device_tree_get_reg). I am OK either way.


> ---
> 
> Changes from
> 
> v1 - 1. Dropped "[XEN v1 2/9] xen/arm: Define translate_dt_address_size() for 
> the translation between u64 and paddr_t" and
> "[XEN v1 4/9] xen/arm: Use translate_dt_address_size() to translate between 
> device tree addr/size and paddr_t", instead
> this approach achieves the same purpose.
> 
> 2. No need to check for truncation while converting values from u64 to 
> paddr_t.
> 
>  xen/arch/arm/bootfdt.c                 | 23 +++++++++------
>  xen/arch/arm/domain_build.c            |  2 +-
>  xen/arch/arm/include/asm/device_tree.h | 40 ++++++++++++++++++++++++++
>  xen/arch/arm/include/asm/setup.h       |  2 +-
>  xen/arch/arm/setup.c                   | 14 ++++-----
>  xen/arch/arm/smpboot.c                 |  2 +-
>  6 files changed, 65 insertions(+), 18 deletions(-)
>  create mode 100644 xen/arch/arm/include/asm/device_tree.h
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 0085c28d74..f536a3f3ab 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -11,9 +11,9 @@
>  #include <xen/efi.h>
>  #include <xen/device_tree.h>
>  #include <xen/lib.h>
> -#include <xen/libfdt/libfdt.h>
>  #include <xen/sort.h>
>  #include <xsm/xsm.h>
> +#include <asm/device_tree.h>
>  #include <asm/setup.h>
>  
>  static bool __init device_tree_node_matches(const void *fdt, int node,
> @@ -53,10 +53,15 @@ static bool __init device_tree_node_compatible(const void 
> *fdt, int node,
>  }
>  
>  void __init device_tree_get_reg(const __be32 **cell, u32 address_cells,
> -                                u32 size_cells, u64 *start, u64 *size)
> +                                u32 size_cells, paddr_t *start, paddr_t 
> *size)
>  {
> -    *start = dt_next_cell(address_cells, cell);
> -    *size = dt_next_cell(size_cells, cell);
> +    /*
> +     * dt_next_cell will return u64 whereas paddr_t may be u64 or u32. Thus, 
> one
> +     * needs to cast paddr_t to u32. Note that we do not check for 
> truncation as
> +     * it is user's responsibility to provide the correct values in the DT.
> +     */
> +    *start = (paddr_t) dt_next_cell(address_cells, cell);
> +    *size = (paddr_t) dt_next_cell(size_cells, cell);
>  }
>  
>  static int __init device_tree_get_meminfo(const void *fdt, int node,
> @@ -326,7 +331,7 @@ static int __init process_chosen_node(const void *fdt, 
> int node,
>          printk("linux,initrd-start property has invalid length %d\n", len);
>          return -EINVAL;
>      }
> -    start = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
> +    start = (paddr_t) dt_read_number((void *)&prop->data, 
> dt_size_to_cells(len));
>  
>      prop = fdt_get_property(fdt, node, "linux,initrd-end", &len);
>      if ( !prop )
> @@ -339,7 +344,7 @@ static int __init process_chosen_node(const void *fdt, 
> int node,
>          printk("linux,initrd-end property has invalid length %d\n", len);
>          return -EINVAL;
>      }
> -    end = dt_read_number((void *)&prop->data, dt_size_to_cells(len));
> +    end = (paddr_t) dt_read_number((void *)&prop->data, 
> dt_size_to_cells(len));
>  
>      if ( start >= end )
>      {
> @@ -594,9 +599,11 @@ static void __init early_print_info(void)
>      for ( i = 0; i < nr_rsvd; i++ )
>      {
>          paddr_t s, e;
> -        if ( fdt_get_mem_rsv(device_tree_flattened, i, &s, &e) < 0 )
> +
> +        if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &s, &e) < 0 )
>              continue;
> -        /* fdt_get_mem_rsv returns length */
> +
> +        /* fdt_get_mem_rsv_paddr returns length */
>          e += s;
>          printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i, s, e);
>      }
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index f904f12408..72b9afbb4c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -949,7 +949,7 @@ static int __init process_shm(struct domain *d, struct 
> kernel_info *kinfo,
>          BUG_ON(!prop);
>          cells = (const __be32 *)prop->value;
>          device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, &gbase);
> -        psize = dt_read_number(cells, size_cells);
> +        psize = (paddr_t) dt_read_number(cells, size_cells);
>          if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(gbase, PAGE_SIZE) )
>          {
>              printk("%pd: physical address 0x%"PRIpaddr", or guest address 
> 0x%"PRIpaddr" is not suitably aligned.\n",
> diff --git a/xen/arch/arm/include/asm/device_tree.h 
> b/xen/arch/arm/include/asm/device_tree.h
> new file mode 100644
> index 0000000000..51e0f0ae20
> --- /dev/null
> +++ b/xen/arch/arm/include/asm/device_tree.h
> @@ -0,0 +1,40 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * xen/arch/arm/include/asm/device_tree.h
> + * 
> + * Wrapper functions for device tree. This helps to convert dt values
> + * between u64 and paddr_t.
> + *
> + * Copyright (C) 2023, Advanced Micro Devices, Inc. All Rights Reserved.
> + */
> +
> +#ifndef __ARCH_ARM_DEVICE_TREE__
> +#define __ARCH_ARM_DEVICE_TREE__
> +
> +#include <xen/libfdt/libfdt.h>
> +
> +inline int fdt_get_mem_rsv_paddr(const void *fdt, int n,
> +                                 paddr_t *address,
> +                                 paddr_t *size)
> +{
> +    uint64_t dt_addr;
> +    uint64_t dt_size;
> +    int ret = 0;
> +
> +    ret = fdt_get_mem_rsv(fdt, n, &dt_addr, &dt_size);
> +
> +    *address = (paddr_t) dt_addr;
> +    *size = (paddr_t) dt_size;
> +
> +    return ret;
> +}
> +
> +#endif /* __ARCH_ARM_DEVICE_TREE__ */
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/include/asm/setup.h 
> b/xen/arch/arm/include/asm/setup.h
> index a926f30a2b..6105e5cae3 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -158,7 +158,7 @@ extern uint32_t hyp_traps_vector[];
>  void init_traps(void);
>  
>  void device_tree_get_reg(const __be32 **cell, u32 address_cells,
> -                         u32 size_cells, u64 *start, u64 *size);
> +                         u32 size_cells, paddr_t *start, paddr_t *size);
>  
>  u32 device_tree_get_u32(const void *fdt, int node,
>                          const char *prop_name, u32 dflt);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 1f26f67b90..da13439e62 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -29,7 +29,6 @@
>  #include <xen/virtual_region.h>
>  #include <xen/vmap.h>
>  #include <xen/trace.h>
> -#include <xen/libfdt/libfdt.h>
>  #include <xen/acpi.h>
>  #include <xen/warning.h>
>  #include <asm/alternative.h>
> @@ -39,6 +38,7 @@
>  #include <asm/gic.h>
>  #include <asm/cpuerrata.h>
>  #include <asm/cpufeature.h>
> +#include <asm/device_tree.h>
>  #include <asm/platform.h>
>  #include <asm/procinfo.h>
>  #include <asm/setup.h>
> @@ -222,11 +222,11 @@ static void __init dt_unreserved_regions(paddr_t s, 
> paddr_t e,
>      {
>          paddr_t r_s, r_e;
>  
> -        if ( fdt_get_mem_rsv(device_tree_flattened, i, &r_s, &r_e ) < 0 )
> +        if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &r_s, &r_e ) < 
> 0 )
>              /* If we can't read it, pretend it doesn't exist... */
>              continue;
>  
> -        r_e += r_s; /* fdt_get_mem_rsv returns length */
> +        r_e += r_s; /* fdt_get_mem_rsv_paddr returns length */
>  
>          if ( s < r_e && r_s < e )
>          {
> @@ -502,13 +502,13 @@ static paddr_t __init consider_modules(paddr_t s, 
> paddr_t e,
>      {
>          paddr_t mod_s, mod_e;
>  
> -        if ( fdt_get_mem_rsv(device_tree_flattened,
> -                             i - mi->nr_mods,
> -                             &mod_s, &mod_e ) < 0 )
> +        if ( fdt_get_mem_rsv_paddr(device_tree_flattened,
> +                                   i - mi->nr_mods,
> +                                   &mod_s, &mod_e ) < 0 )
>              /* If we can't read it, pretend it doesn't exist... */
>              continue;
>  
> -        /* fdt_get_mem_rsv returns length */
> +        /* fdt_get_mem_rsv_paddr returns length */
>          mod_e += mod_s;
>  
>          if ( s < mod_e && mod_s < e )
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 412ae22869..ee59b1d379 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -159,7 +159,7 @@ static void __init dt_smp_init_cpus(void)
>              continue;
>          }
>  
> -        addr = dt_read_number(prop, dt_n_addr_cells(cpu));
> +        addr = (paddr_t) dt_read_number(prop, dt_n_addr_cells(cpu));
>  
>          hwid = addr;
>          if ( hwid != addr )
> -- 
> 2.17.1
> 



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.