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

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

Hi Jan,

On 08/02/2023 13:33, Jan Beulich wrote:
> 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?
Actually I introduced libfdt_xen.h as I did not wish to modify the 
fdt_get_mem_rsv() (present in fdt_ro.c).
So I created libfdt_xen.h to implement fdt_get_mem_rsv_paddr(). The 
*_xen.h denotes that it is derived from * (which can be modified like 
any other file).
Let me know what name you suggest.

> > @@ -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.
Ack
> > +    /*
> > +     * 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?
Yes, makes sense.

I will use "(paddr_t)val != val" to check for truncation.

> > --- 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?
I could have use PADDR_BITS instead, but decided to use PADDR_SHIFT for 
cosmetic reasons.
> And if we need it, on what basis did you pick
> the file you've put it in?
I wanted to put in an arch independent file (where we have defined 
macros related to data types).
- Ayan

> Jan