|
[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 Thu, 19 Jan 2023, Stefano Stabellini wrote:
> 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.
Actually I noticed you also add dt_device_get_paddr to
xen/arch/arm/include/asm/device_tree.h. So it sounds like it is a good
idea to introduce xen/arch/arm/include/asm/device_tree.h, and we could
also move the declarations of device_tree_get_reg, device_tree_get_u32
there.
> > ---
> >
> > 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
> >
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |