Re: [XEN v3 2/9] xen/arm: Typecast the DT values into paddr_t

[Jan Beulich <jbeulich@xxxxxxxx>]

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.halder@xxxxxxx>
> ---
> 
> 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