| Message ID | 20230117174358.15344-5-ayan.kumar.halder@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for 32 bit physical address | expand |
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@amd.com> 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 >
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@amd.com> > > 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 > > >
Hi, I am answering to multiple e-mails at the same time. On 19/01/2023 23:34, Stefano Stabellini wrote: > 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 A cell in the DT is a 32-bit value. So you could read 32-bit value (address-size could be #1). It is just that our wrapper return 64-bit values because this is how we use the most. >>> typecast the values appropriately from u64 to paddr_t. C will perfectly be able to truncate a 64-bit to 32-bit value. So this is not a very good argument to explain why all of this is necessary. >>> >>> 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 Typo: s/warpper/wrapper/ >>> 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@amd.com> >> >> 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 "number" and "paddr" pretty much means the same. I think it would be better to name it "dt_read_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? Aside dt_read_number(), I would expect that most of the helper can return an error. So if you want to check the truncation, then we should propagate the error. >> >> 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. None of the helpers you mention sounds arm specific. So why should the be move an arch specific headers? [...] >>> 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); There is no need for explicit cast here. Cheers,
On 20/01/2023 10:16, Julien Grall wrote: > Hi, Hi Julien/Stefano, > > I am answering to multiple e-mails at the same time. > > On 19/01/2023 23:34, Stefano Stabellini wrote: >> 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 > > A cell in the DT is a 32-bit value. So you could read 32-bit value > (address-size could be #1). It is just that our wrapper return 64-bit > values because this is how we use the most. ok > >>>> typecast the values appropriately from u64 to paddr_t. > > C will perfectly be able to truncate a 64-bit to 32-bit value. So this > is not a very good argument to explain why all of this is necessary. I can remove this line from the commit message. However, I have a related point below... > > >>>> >>>> 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 > > Typo: s/warpper/wrapper/ ok > >>>> 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@amd.com> >>> >>> 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 > > "number" and "paddr" pretty much means the same. I think it would be > better to name it "dt_read_paddr". ok > >>> - 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? > > Aside dt_read_number(), I would expect that most of the helper can > return an error. So if you want to check the truncation, then we > should propagate the error. ok > >>> >>> 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. > > None of the helpers you mention sounds arm specific. So why should the > be move an arch specific headers? The functions (fdt_get_mem_rsv_paddr(), device_tree_get_reg(), device_tree_get_u32()) are currently used by Arm specific code only. So, I thought of implementing fdt_get_mem_rsv_paddr() in xen/arch/arm/include/asm/device_tree.h. However, I see your point as well. So the alternative approach would be :- Approach 1) Implement fdt_get_mem_rsv_paddr() in ./xen/include/xen/libfdt/libfdt.h. However libfdt is imported from external sources, so I am not sure if this is the best approach. Approach 2) Rename fdt_get_mem_rsv_paddr() to dt_get_mem_rsv_paddr() and implement it in ./xen/include/xen/device_tree.h. However, this will be an odd one as it invokes fdt_get_mem_rsv() for which we will have to include libfdt.h in device_tree.h. So, I am biased towards having xen/arch/arm/include/asm/device_tree.h in which we can implement all the non-static fdt and dt functions (which are required by xen/arch/arm/*). And then (as Stefano suggested), we can move the definitions of the following to xen/arch/arm/include/asm/device_tree.h. device_tree_get_reg() device_tree_get_u32() device_tree_for_each_node() Please let me know your thoughts. > > [...] > >>>> 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); > > There is no need for explicit cast here. Should we have it for the sake of documentation (that it casts u64 to paddr_t) ? - Ayan > > Cheers, >
Hi Ayan, On 31/01/2023 10:51, Ayan Kumar Halder wrote: > On 20/01/2023 10:16, Julien Grall wrote: >>>> 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. >> >> None of the helpers you mention sounds arm specific. So why should the >> be move an arch specific headers? > > The functions (fdt_get_mem_rsv_paddr(), device_tree_get_reg(), > device_tree_get_u32()) are currently used by Arm specific code only. > > So, I thought of implementing fdt_get_mem_rsv_paddr() in > xen/arch/arm/include/asm/device_tree.h. > > However, I see your point as well. So the alternative approach would be :- > > Approach 1) Implement fdt_get_mem_rsv_paddr() in > ./xen/include/xen/libfdt/libfdt.h. > > However libfdt is imported from external sources, so I am not sure if > this is the best approach. One way would be to introduce "libfdt_xen.h" which would contain all the implementation specific to Xen. > > Approach 2) Rename fdt_get_mem_rsv_paddr() to dt_get_mem_rsv_paddr() and > implement it in ./xen/include/xen/device_tree.h. > > However, this will be an odd one as it invokes fdt_get_mem_rsv() for > which we will have to include libfdt.h in device_tree.h. You could implement the functions in the device_tree.c but see below. > > > So, I am biased towards having xen/arch/arm/include/asm/device_tree.h in > which we can implement all the non-static fdt and dt functions (which > are required by xen/arch/arm/*). > > And then (as Stefano suggested), we can move the definitions of the > following to xen/arch/arm/include/asm/device_tree.h. > > device_tree_get_reg() > > device_tree_get_u32() > > device_tree_for_each_node() Well none of them are truly arch specific as well. I could imagine RISC-v to use it in the future if they also care about checking the truncation. I have a slight preference to introduce libfdt_xen.h over adding the implementation in device_tree.h so we can keep the unflatten API (device_tree.h) separated from the flatten API (libfdt.h). But this is not a strong preference between the two. That said, I would strongly argue against adding the helper in asm/*.h because there is nothing Arm specific in them. >> >> [...] >> >>>>> 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); >> >> There is no need for explicit cast here. > > Should we have it for the sake of documentation (that it casts u64 to > paddr_t) ? You already have a comment on top of dt_next_cell() to explain the typecast. So I would rather not add the explicit casts. Cheers,
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 )
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@amd.com> --- 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