On 08.02.2023 13:05, 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.hal...@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.
>
> 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
Can you please avoid underscores in the names of new files, unless they're
strictly required for some reason?
> @@ -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;
No new uses of this type (and its siblings), please. We're in the process
of phasing out their use. See ./CODING_STYLE.
> + /*
> + * 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");
While I assume you wrote the checks this way to avoid "shift count too
large" compiler diagnostics, personally I think this is making it
harder to recognize what is wanted. Already (val >> (PADDR_SHIFT - 1)) >> 1
would be better imo, by why not simply (paddr_t)val != val?
> --- 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
Why do we need this alias? And if we need it, on what basis did you pick
the file you've put it in?
Jan
