Re: [PATCH v2] xen/dt: Remove loop in dt_read_number()

[Alejandro Vallejo <agarciav@xxxxxxx>]

On Wed Jun 18, 2025 at 9:06 AM CEST, Michal Orzel wrote:
>
>
> On 17/06/2025 19:13, Alejandro Vallejo wrote:
>> The DT spec declares only two number types for a property: u32 and u64,
>> as per Table 2.3 in Section 2.2.4. Remove unbounded loop and replace
>> with a switch statement. Default to a size of 1 cell in the nonsensical
>> size case, with a warning printed on the Xen console.
>> 
>> Suggested-by: Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Alejandro Vallejo <agarciav@xxxxxxx>
>> ---
>> v2:
>>   * Added missing `break` on the `case 2:` branch and added 
>> ASSERT_UNREACHABLE() to the deafult path
>> ---
>>  xen/include/xen/device_tree.h | 17 ++++++++++++++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>> 
>> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
>> index 75017e4266..2ec668b94a 100644
>> --- a/xen/include/xen/device_tree.h
>> +++ b/xen/include/xen/device_tree.h
>> @@ -261,10 +261,21 @@ void intc_dt_preinit(void);
>>  /* Helper to read a big number; size is in cells (not bytes) */
>>  static inline u64 dt_read_number(const __be32 *cell, int size)
>>  {
>> -    u64 r = 0;
>> +    u64 r = be32_to_cpu(*cell);
>> +
>> +    switch ( size )
>> +    {
>> +    case 1:
>> +        break;
>> +    case 2:
>> +        r = (r << 32) | be32_to_cpu(cell[1]);
>> +        break;
>> +    default:
>> +        // Nonsensical size. default to 1.
> I wonder why there are so many examples of device trees in Linux with
> #address-cells = <3>? Also, libfdt defines FDT_MAX_NCELLS as 4 with comment:
> "maximum value for #address-cells and #size-cells" but I guess it follows the
> IEE1275 standard and DT spec "is loosely related" to it.
>
> ~Michal

I could imagine DT's encoding CHERI 64bit capabilities as addresses, which could
require 4 cells. Needless to say, this function wouldn't even be in the top 10
biggest problems in making Xen happy running on a CHERI-capable processor.

As for #address-cells = <3>, I really can't think of a reason except testing
theoretical corner cases.  

Cheers,
Alejandro