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>
---
Based on this suggestion by Daniel:

    https://lore.kernel.org/xen-devel/a66c11c4-cfac-4934-b1f5-e07c728db8de@apertussolutions.com/

I'd be happier panicking there, seeing how DTs are by their very nature
trusted blobs. But I suspect defaulting to something will find less
resistance in review. I don't care much either way.
---
 xen/include/xen/device_tree.h | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 75017e4266..2daef8659e 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -261,10 +261,19 @@ 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]);
+    default:
+        // Nonsensical size. default to 1.
+        printk(XENLOG_WARNING "dt_read_number(%d) bad size", size);
+    };
 
-    while ( size-- )
-        r = (r << 32) | be32_to_cpu(*(cell++));
     return r;
 }
 

base-commit: 14c57887f36937c1deb9eeca852c3a7595d2d0b8
-- 
2.43.0

Hi Alejandro,

On 17/06/2025 12:07, 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>
> ---
> Based on this suggestion by Daniel:
> 
>      https://lore.kernel.org/xen-devel/a66c11c4-cfac-4934-b1f5-e07c728db8de@apertussolutions.com/
> 
> I'd be happier panicking there, seeing how DTs are by their very nature
> trusted blobs. But I suspect defaulting to something will find less
> resistance in review. I don't care much either way.
> ---
>   xen/include/xen/device_tree.h | 15 ++++++++++++---
>   1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 75017e4266..2daef8659e 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -261,10 +261,19 @@ 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]);
> +    default:
> +        // Nonsensical size. default to 1.
> +        printk(XENLOG_WARNING "dt_read_number(%d) bad size", size);

Aside what Andrew wrote. I would consider to use at least 
ASSERT_UNREACHABLE() for debug build. I am not sure what's the best 
approach for release build. But this likely want to a XENLOG_ERR.

Cheers,

-- 
Julien Grall

On Tue Jun 17, 2025 at 1:07 PM CEST, 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>
> ---
> Based on this suggestion by Daniel:
>
>     https://lore.kernel.org/xen-devel/a66c11c4-cfac-4934-b1f5-e07c728db8de@apertussolutions.com/
>
> I'd be happier panicking there, seeing how DTs are by their very nature
> trusted blobs. But I suspect defaulting to something will find less
> resistance in review. I don't care much either way.
> ---
>  xen/include/xen/device_tree.h | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 75017e4266..2daef8659e 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -261,10 +261,19 @@ 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]);

Bah, missing break. And with a printk rather than a panic CI stayed green when
it shouldn't have been.

> +    default:
> +        // Nonsensical size. default to 1.
> +        printk(XENLOG_WARNING "dt_read_number(%d) bad size", size);

I'll add an ASSERT_UNREACHABLE() here for good measure.

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

On 17/06/2025 12:07 pm, Alejandro Vallejo wrote:
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 75017e4266..2daef8659e 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -261,10 +261,19 @@ 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]);
> +    default:
> +        // Nonsensical size. default to 1.
> +        printk(XENLOG_WARNING "dt_read_number(%d) bad size", size);
> +    };
>  
> -    while ( size-- )
> -        r = (r << 32) | be32_to_cpu(*(cell++));
>      return r;
>  }

What testing has this had?

~Andrew