|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN v3 2/9] xen/arm: Typecast the DT values into paddr_t
On Wed, 8 Feb 2023, Ayan Kumar Halder wrote:
> In future, we wish to support 32 bit physical address.
> However, the current dt and fdt functions can only read u64 values.
> We wish to make the DT functions read u64 as well u32 values (depending
> on the width of physical address). Also, we wish to detect if any
> truncation has occurred (ie while reading u32 physical addresses from
> u64 values read from DT).
>
> device_tree_get_reg() should now be able to return paddr_t. This is
> invoked by various callers to get DT address and size.
>
> For fdt_get_mem_rsv(), we have introduced wrapper ie
> fdt_get_mem_rsv_paddr() while will invoke fdt_get_mem_rsv() and translate
> u64 to paddr_t. The reason being we cannot modify fdt_get_mem_rsv() as
> it has been imported from external source.
>
> For dt_read_number(), we have also introduced a wrapper ie
> dt_read_paddr() to read physical addresses. We chose not to modify the
> original function as it been used in places where it needs to
> specifically u64 values from dt (For eg dt_property_read_u64()).
>
> Xen prints an error when it detects a truncation (during typecast
> between u64 and paddr_t). It is not possible to return an error in all
> scenarios. So, it is user's responsibility to check the error logs.
>
> To detect truncation, we right shift physical address by
> "PADDR_BITS - 1" and then if the resulting number is greater than 1,
> we know that truncation has occurred and an appropriate error log is
> printed.
>
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@xxxxxxx>
I just wanted to record that aside from Jan's feedback, this patch looks
good to me
> ---
>
> 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.
>
> v2 - 1. Use "( (dt_start >> (PADDR_SHIFT - 1)) > 1 )" to detect truncation.
> 2. Introduced libfdt_xen.h to implement fdt_get_mem_rsv_paddr
> 3. Logged error messages in case truncation is detected.
>
> xen/arch/arm/bootfdt.c | 38 ++++++++++++++++-----
> xen/arch/arm/domain_build.c | 2 +-
> xen/arch/arm/include/asm/setup.h | 2 +-
> xen/arch/arm/setup.c | 14 ++++----
> xen/arch/arm/smpboot.c | 2 +-
> xen/include/xen/device_tree.h | 21 ++++++++++++
> xen/include/xen/libfdt/libfdt_xen.h | 52 +++++++++++++++++++++++++++++
> xen/include/xen/types.h | 2 ++
> 8 files changed, 115 insertions(+), 18 deletions(-)
> create mode 100644 xen/include/xen/libfdt/libfdt_xen.h
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 0085c28d74..f63da3e831 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -11,7 +11,7 @@
> #include <xen/efi.h>
> #include <xen/device_tree.h>
> #include <xen/lib.h>
> -#include <xen/libfdt/libfdt.h>
> +#include <xen/libfdt/libfdt_xen.h>
> #include <xen/sort.h>
> #include <xsm/xsm.h>
> #include <asm/setup.h>
> @@ -53,10 +53,30 @@ 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);
> + u64 dt_start, dt_size;
> +
> + /*
> + * dt_next_cell will return u64 whereas paddr_t may be u64 or u32. Thus,
> + * there is an implicit cast from u64 to paddr_t.
> + */
> + dt_start = dt_next_cell(address_cells, cell);
> + dt_size = dt_next_cell(size_cells, cell);
> +
> + if ( (dt_start >> (PADDR_SHIFT - 1)) > 1 )
> + printk("Error: Physical address greater than max width supported\n");
> +
> + if ( (dt_size >> (PADDR_SHIFT - 1)) > 1 )
> + printk("Error: Physical size greater than max width supported\n");
> +
> + /*
> + * Note: It is user's responsibility to check for the error messages.
> + * Xen will sliently truncate in case if the address/size is greater than
> + * the max supported width.
> + */
> + *start = dt_start;
> + *size = dt_size;
> }
>
> static int __init device_tree_get_meminfo(const void *fdt, int node,
> @@ -326,7 +346,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 = dt_read_paddr((void *)&prop->data, dt_size_to_cells(len));
>
> prop = fdt_get_property(fdt, node, "linux,initrd-end", &len);
> if ( !prop )
> @@ -339,7 +359,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 = dt_read_paddr((void *)&prop->data, dt_size_to_cells(len));
>
> if ( start >= end )
> {
> @@ -594,9 +614,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 a798e0b256..4d7e67560f 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 = dt_read_paddr(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/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..5ade20e6e7 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -29,7 +29,7 @@
> #include <xen/virtual_region.h>
> #include <xen/vmap.h>
> #include <xen/trace.h>
> -#include <xen/libfdt/libfdt.h>
> +#include <xen/libfdt/libfdt_xen.h>
> #include <xen/acpi.h>
> #include <xen/warning.h>
> #include <asm/alternative.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..c15c177487 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 = dt_read_paddr(prop, dt_n_addr_cells(cpu));
>
> hwid = addr;
> if ( hwid != addr )
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index a28937d12a..b61bac2931 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -244,6 +244,27 @@ static inline u64 dt_read_number(const __be32 *cell, int
> size)
> return r;
> }
>
> +/* Wrapper for dt_read_number() to return paddr_t (instead of u64) */
> +static inline paddr_t dt_read_paddr(const __be32 *cell, int size)
> +{
> + u64 dt_r = 0;
> + paddr_t r;
> +
> + dt_r = dt_read_number(cell, size);
> +
> + if ( (dt_r >> (PADDR_SHIFT - 1)) > 1 )
> + printk("Error: Physical address greater than max width supported\n");
> +
> + /*
> + * Note: It is user's responsibility to check for the error messages.
> + * Xen will sliently truncate in case if the address/size is greater than
> + * the max supported width.
> + */
> + r = dt_r;
> +
> + return r;
> +}
> +
> /* Helper to convert a number of cells to bytes */
> static inline int dt_cells_to_size(int size)
> {
> diff --git a/xen/include/xen/libfdt/libfdt_xen.h
> b/xen/include/xen/libfdt/libfdt_xen.h
> new file mode 100644
> index 0000000000..a3196dd96c
> --- /dev/null
> +++ b/xen/include/xen/libfdt/libfdt_xen.h
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * xen/include/xen/libfdt/libfdt_xen.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 LIBFDT_XEN_H
> +#define LIBFDT_XEN_H
> +
> +#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);
> +
> + if ( (dt_addr >> (PADDR_SHIFT - 1)) > 1 )
> + {
> + printk("Error: Physical address greater than max width supported\n");
> + return -1;
> + }
> +
> + if ( (dt_size >> (PADDR_SHIFT - 1)) > 1 )
> + {
> + printk("Error: Physical size greater than max width supported\n");
> + return -1;
> + }
> +
> + *address = dt_addr;
> + *size = dt_size;
> +
> + return ret;
> +}
> +
> +#endif /* LIBFDT_XEN_H */
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
> index 6aba80500a..b7255c7c38 100644
> --- a/xen/include/xen/types.h
> +++ b/xen/include/xen/types.h
> @@ -71,4 +71,6 @@ typedef bool bool_t;
> #define test_and_set_bool(b) xchg(&(b), true)
> #define test_and_clear_bool(b) xchg(&(b), false)
>
> +#define PADDR_SHIFT PADDR_BITS
> +
> #endif /* __TYPES_H__ */
> --
> 2.17.1
>
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |