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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.02.2023 16:51, Ayan Kumar Halder wrote:
> 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.

I don't mind the new file, all I do mind is the underscore in its name
when a dash will do. Underscores should be used in place of dashes only
when dashes can't be used (e.g. in identifiers, where dash represents
the minus operator and hence can't be used in identifiers).

>>> --- 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.

I, for one, disagree with this (it's pretty unclear to me what "shift" here
is to express) as well as ...

>> 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).

... this placement.

Jan