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

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

[Alejandro Vallejo]

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@apertussolutions.com>
Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
---
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.
+        printk(XENLOG_WARNING "dt_read_number(%d) bad size\n", size);
+        ASSERT_UNREACHABLE();
+    };
 
-    while ( size-- )
-        r = (r << 32) | be32_to_cpu(*(cell++));
     return r;
 }
 

base-commit: 14c57887f36937c1deb9eeca852c3a7595d2d0b8
-- 
2.43.0

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

[Stefano Stabellini]

On Tue, 17 Jun 2025, 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@apertussolutions.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
> ---
> 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.

NIT: comment style is /* comment */

> +        printk(XENLOG_WARNING "dt_read_number(%d) bad size\n", size);
> +        ASSERT_UNREACHABLE();

You need a break statement even after ASSERT_UNREACHABLE() for MISRA R16.3


> +    };
>  
> -    while ( size-- )
> -        r = (r << 32) | be32_to_cpu(*(cell++));
>      return r;
>  }
>  
> 
> base-commit: 14c57887f36937c1deb9eeca852c3a7595d2d0b8
> -- 
> 2.43.0
> 

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

[Julien Grall]

Hi Alejandro,

Sorry I didn't see there was a v2.

On 17/06/2025 18: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@apertussolutions.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
> ---
> 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.
> +        printk(XENLOG_WARNING "dt_read_number(%d) bad size\n", size);

I think this needs to at least be a XENLOG_ERR.

Cheers,

-- 
Julien Grall

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

[Jan Beulich]

On 17.06.2025 19:13, Alejandro Vallejo wrote:
> --- 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.
> +        printk(XENLOG_WARNING "dt_read_number(%d) bad size\n", size);

Might be good to entirely unambiguously indicate it's the 2nd argument.
Iirc an approach we use elsewhere is to spell it dt_read_number(,%d).

Jan

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

[Orzel, Michal]

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@apertussolutions.com>
> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
> ---
> 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

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

[Julien Grall]

Hi Michal,

On 18/06/2025 08:06, Orzel, Michal 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@apertussolutions.com>
>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
>> ---
>> 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.

A lot of the use seems to be related to PCI. Below an example from [1]:

     pcie {
         #address-cells = <3>;
         #size-cells = <2>;

         pcie@0 {
             device_type = "pci";
             reg = <0x0 0x0 0x0 0x0 0x0>;
             #address-cells = <3>;
             #size-cells = <2>;
             ranges;

             wifi@0 {
                 compatible = "pci17cb,1109";
                 reg = <0x0 0x0 0x0 0x0 0x0>;

                 qcom,calibration-variant = "RDP433_1";

                 ports {
                     #address-cells = <1>;
                     #size-cells = <0>;

                     port@0 {
                         reg = <0>;

                         wifi1_wsi_tx: endpoint {
                             remote-endpoint = <&wifi2_wsi_rx>;
                         };
                     };

                     port@1 {
                         reg = <1>;

                         wifi1_wsi_rx: endpoint {
                             remote-endpoint = <&wifi3_wsi_tx>;
                         };
                     };
                 };
             };
         };

"reg" has effectively 5 cells (3 for address and 2 for size).

 From a very brief look, I couldn't find how this is interpreted. But I 
don't see how dt_read_number() can be used in this case. So I think it 
makes sense to restrict. The question though is whether it is a good 
idea to cap the value and behave differently from Linux.

Speaking of which there are another fun different behavior between Linux 
and Xen in DT. The default size of the root #address is 2 in Xen (see 
DT_ROOT_NODE_ADDR_CELLS_DEFAULT) which is spec compliant. But Linux will 
use 1 (see OF_ROOT_NODE_ADDR_CELLS_DEFAULT). I haven't seen any issue so 
far, but only notice recently when working on a separate project 
recently. I am still undecided for this one whether Xen should change 
because technically a Device-Tree should nowadays always provide 
#address-cells and #size-cells.

Cheers,

[1] devicetree/bindings/net/wireless/qcom,ath12k-wsi.yaml

-- 
Julien Grall

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

[Alejandro Vallejo]

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@apertussolutions.com>
>> Signed-off-by: Alejandro Vallejo <agarciav@amd.com>
>> ---
>> 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