Implements dt_processor_hartid() to get the hart ID of the given
device tree node and do some checks if CPU is available and given device
tree node has proper riscv,isa property.

As a helper function dt_get_hartid() is introduced to deal specifically
with reg propery of a CPU device node.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V4:
 - Sort properly inculsion of xen/types.h in riscv/smpboot.c.
 - Add overflow check of got address cell value in dt_get_hartid().
 - Use "%#lx" for loging recieved hartid from DTB.
 - s/-EINVAL/-ENODEV for recieved isa string from DTB.
---
Changes in V3:
 - s/dt_processor_cpuid/dt_processor_hartid.
 - s/dt_get_cpuid/dt_get_hartid.
 - use ~0UL in dt_get_cpuid() and in the comment above it.
 - change type for local variable ac in dt_get_cpuid() to unsigned int.
 - Update the return errors for dt_processor_cpuid().
 - Update the commit message + subject: s/cpuid/hartid.
---
Changes in V2:
 - s/of_get_cpu_hwid()/dt_get_cpu_id().
 - Update prototype of dt_get_cpu_hwid(), use pointer-to-const for cpun arg.
 - Add empty line before last return in dt_get_cpu_hwid().
 - s/riscv_of_processor_hartid/dt_processor_cpuid().
 - Use pointer-to_const for node argument of dt_processor_cpuid().
 - Use for hart_id unsigned long type as according to the spec for RV128
   mhartid register will be 128 bit long.
 - Update commit message and subject.
 - use 'CPU' instead of 'HART'.
 - Drop thread argument of dt_get_cpu_id() (of_get_cpu_hwid) as it is
   expected to be always 0 according to RISC-V's DTS binding.
---
 xen/arch/riscv/include/asm/smp.h |  4 ++
 xen/arch/riscv/smpboot.c         | 77 ++++++++++++++++++++++++++++++++
 2 files changed, 81 insertions(+)

diff --git a/xen/arch/riscv/include/asm/smp.h b/xen/arch/riscv/include/asm/smp.h
index 5e170b57b3..eb58b6576b 100644
--- a/xen/arch/riscv/include/asm/smp.h
+++ b/xen/arch/riscv/include/asm/smp.h
@@ -26,6 +26,10 @@ static inline void set_cpuid_to_hartid(unsigned long cpuid,
 
 void setup_tp(unsigned int cpuid);
 
+struct dt_device_node;
+int dt_processor_hartid(const struct dt_device_node *node,
+                        unsigned long *hartid);
+
 #endif
 
 /*
diff --git a/xen/arch/riscv/smpboot.c b/xen/arch/riscv/smpboot.c
index 0f9c2cc54a..8011876372 100644
--- a/xen/arch/riscv/smpboot.c
+++ b/xen/arch/riscv/smpboot.c
@@ -1,6 +1,9 @@
 #include <xen/cpumask.h>
+#include <xen/device_tree.h>
+#include <xen/errno.h>
 #include <xen/init.h>
 #include <xen/sections.h>
+#include <xen/types.h>
 
 #include <asm/current.h>
 
@@ -14,3 +17,77 @@ void __init smp_prepare_boot_cpu(void)
     cpumask_set_cpu(0, &cpu_possible_map);
     cpumask_set_cpu(0, &cpu_online_map);
 }
+
+/**
+ * dt_get_hartid - Get the hartid from a CPU device node
+ *
+ * @cpun: CPU number(logical index) for which device node is required
+ *
+ * Return: The hartid for the CPU node or ~0UL if not found.
+ */
+static unsigned long dt_get_hartid(const struct dt_device_node *cpun)
+{
+    const __be32 *cell;
+    unsigned int ac;
+    uint32_t len;
+    unsigned int max_cells = UINT32_MAX / sizeof(*cell);
+
+    ac = dt_n_addr_cells(cpun);
+    cell = dt_get_property(cpun, "reg", &len);
+
+    if (ac > max_cells) {
+        printk("%s: cell count overflow (ac=%u, max=%u)\n", __func__, ac,
+               max_cells);
+        return ~0UL;
+    }
+
+    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )
+        return ~0UL;
+
+    return dt_read_number(cell, ac);
+}
+
+/*
+ * Returns the hartid of the given device tree node, or -ENODEV if the node
+ * isn't an enabled and valid RISC-V hart node.
+ */
+int dt_processor_hartid(const struct dt_device_node *node,
+                        unsigned long *hartid)
+{
+    const char *isa;
+    int ret;
+
+    if ( !dt_device_is_compatible(node, "riscv") )
+    {
+        printk("Found incompatible CPU\n");
+        return -ENODEV;
+    }
+
+    *hartid = dt_get_hartid(node);
+    if ( *hartid == ~0UL )
+    {
+        printk("Found CPU without CPU ID\n");
+        return -ENODATA;
+    }
+
+    if ( !dt_device_is_available(node))
+    {
+        printk("CPU with hartid=%#lx is not available\n", *hartid);
+        return -ENODEV;
+    }
+
+    if ( (ret = dt_property_read_string(node, "riscv,isa", &isa)) )
+    {
+        printk("CPU with hartid=%#lx has no \"riscv,isa\" property\n", *hartid);
+        return ret;
+    }
+
+    if ( isa[0] != 'r' || isa[1] != 'v' )
+    {
+        printk("CPU with hartid=%#lx has an invalid ISA of \"%s\"\n", *hartid,
+               isa);
+        return -ENODEV;
+    }
+
+    return 0;
+}
-- 
2.49.0

On 05.06.2025 17:58, Oleksii Kurochko wrote:
> @@ -14,3 +17,77 @@ void __init smp_prepare_boot_cpu(void)
>      cpumask_set_cpu(0, &cpu_possible_map);
>      cpumask_set_cpu(0, &cpu_online_map);
>  }
> +
> +/**
> + * dt_get_hartid - Get the hartid from a CPU device node
> + *
> + * @cpun: CPU number(logical index) for which device node is required
> + *
> + * Return: The hartid for the CPU node or ~0UL if not found.
> + */
> +static unsigned long dt_get_hartid(const struct dt_device_node *cpun)
> +{
> +    const __be32 *cell;
> +    unsigned int ac;
> +    uint32_t len;
> +    unsigned int max_cells = UINT32_MAX / sizeof(*cell);
> +
> +    ac = dt_n_addr_cells(cpun);
> +    cell = dt_get_property(cpun, "reg", &len);
> +
> +    if (ac > max_cells) {

Besides the (double) style issue, why's this needed? Can't you simply ...

> +        printk("%s: cell count overflow (ac=%u, max=%u)\n", __func__, ac,
> +               max_cells);
> +        return ~0UL;
> +    }
> +
> +    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )

... write the last part here in a way that there can't be overflow?
ac > len / sizeof(*cell) that is? (Remaining question then is what to
do when len isn't evenly divisible by sizeof(*cell).)

> +        return ~0UL;
> +
> +    return dt_read_number(cell, ac);

What meaning does this have for ac > 2? (As per your checking above
it can be up to UINT32_MAX / 4.)

Jan

On 6/10/25 4:08 PM, Jan Beulich wrote:
> On 05.06.2025 17:58, Oleksii Kurochko wrote:
>> @@ -14,3 +17,77 @@ void __init smp_prepare_boot_cpu(void)
>>       cpumask_set_cpu(0, &cpu_possible_map);
>>       cpumask_set_cpu(0, &cpu_online_map);
>>   }
>> +
>> +/**
>> + * dt_get_hartid - Get the hartid from a CPU device node
>> + *
>> + * @cpun: CPU number(logical index) for which device node is required
>> + *
>> + * Return: The hartid for the CPU node or ~0UL if not found.
>> + */
>> +static unsigned long dt_get_hartid(const struct dt_device_node *cpun)
>> +{
>> +    const __be32 *cell;
>> +    unsigned int ac;
>> +    uint32_t len;
>> +    unsigned int max_cells = UINT32_MAX / sizeof(*cell);
>> +
>> +    ac = dt_n_addr_cells(cpun);
>> +    cell = dt_get_property(cpun, "reg", &len);
>> +
>> +    if (ac > max_cells) {
> Besides the (double) style issue, why's this needed? Can't you simply ...
>
>> +        printk("%s: cell count overflow (ac=%u, max=%u)\n", __func__, ac,
>> +               max_cells);
>> +        return ~0UL;
>> +    }
>> +
>> +    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )
> ... write the last part here in a way that there can't be overflow?
> ac > len / sizeof(*cell) that is? (Remaining question then is what to
> do when len isn't evenly divisible by sizeof(*cell).)

reg property should be always evenly divisible by sizeof(*cell) according to device
tree binding:
   The reg property describes the address of the device’s resources within 
the address space defined by its parent bus. Most commonly this means 
the offsets and lengths of memory-mapped IO register blocks, but may 
have a different meaning on some bus types. Addresses in the address 
space defined by the root node are CPU real addresses.
   
   The value is a <prop-encoded-array>, composed of an arbitrary number of 
pairs of address and length, <address length>. The number of <u32> cells 
required to specify the address and length are bus-specific and are 
specified by the #address-cells and #size-cells properties in the parent 
of the device node. If the parent node specifies a value of 0 for 
#size-cells, the length field in the value of reg shall be omitted. So 
it is guaranteed by DTC compiler and it would be enough to check 
overflow in suggested by you way: ac > len / sizeof(*cell)
But considering what you noticed below ...

>
>> +        return ~0UL;
>> +
>> +    return dt_read_number(cell, ac);
> What meaning does this have for ac > 2? (As per your checking above
> it can be up to UINT32_MAX / 4.)

... It will be an issue for dt_read_number() which could deal only with uint64_t what means
we can't have ac > 2. (UINT32_MAX / 4 it is a theoretical maximum IIUC)

Thereby we could do in the following way:
@@ -30,19 +30,18 @@ static unsigned long dt_get_hartid(const struct dt_device_node *cpun)
      const __be32 *cell;
      unsigned int ac;
      uint32_t len;
-    unsigned int max_cells = UINT32_MAX / sizeof(*cell);
  
      ac = dt_n_addr_cells(cpun);
      cell = dt_get_property(cpun, "reg", &len);
  
-    if (ac > max_cells) {
-        printk("%s: cell count overflow (ac=%u, max=%u)\n", __func__, ac,
-               max_cells);
+    if ( !cell || !ac || (ac > len / sizeof(*cell)) )
          return ~0UL;
-    }
  
-    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )
-        return ~0UL;
+    /*
+     * If ac > 2, the result may be truncated or meaningless unless
+     * dt_read_number() supports wider integers.
+     */
+    BUG_ON(ac > 2);
  
      return dt_read_number(cell, ac);
  }

I am not sure that BUG_ON() should be in dt_get_hartid(). Probably it would be better move it
to dt_read_number() as if one day support for RV128 will be needed I assume that it will be
needed to change a prototype of dt_read_number() to work with address-cells = 3.
What do you think? Could I go with the suggested above changes or it would be better to move
BUG_ON() to dt_read_number()?

~ Oleksii

On 11.06.2025 10:26, Oleksii Kurochko wrote:
> 
> On 6/10/25 4:08 PM, Jan Beulich wrote:
>> On 05.06.2025 17:58, Oleksii Kurochko wrote:
>>> @@ -14,3 +17,77 @@ void __init smp_prepare_boot_cpu(void)
>>>       cpumask_set_cpu(0, &cpu_possible_map);
>>>       cpumask_set_cpu(0, &cpu_online_map);
>>>   }
>>> +
>>> +/**
>>> + * dt_get_hartid - Get the hartid from a CPU device node
>>> + *
>>> + * @cpun: CPU number(logical index) for which device node is required
>>> + *
>>> + * Return: The hartid for the CPU node or ~0UL if not found.
>>> + */
>>> +static unsigned long dt_get_hartid(const struct dt_device_node *cpun)
>>> +{
>>> +    const __be32 *cell;
>>> +    unsigned int ac;
>>> +    uint32_t len;
>>> +    unsigned int max_cells = UINT32_MAX / sizeof(*cell);
>>> +
>>> +    ac = dt_n_addr_cells(cpun);
>>> +    cell = dt_get_property(cpun, "reg", &len);
>>> +
>>> +    if (ac > max_cells) {
>> Besides the (double) style issue, why's this needed? Can't you simply ...
>>
>>> +        printk("%s: cell count overflow (ac=%u, max=%u)\n", __func__, ac,
>>> +               max_cells);
>>> +        return ~0UL;
>>> +    }
>>> +
>>> +    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )
>> ... write the last part here in a way that there can't be overflow?
>> ac > len / sizeof(*cell) that is? (Remaining question then is what to
>> do when len isn't evenly divisible by sizeof(*cell).)
> 
> reg property should be always evenly divisible by sizeof(*cell) according to device
> tree binding:
>    The reg property describes the address of the device’s resources within 
> the address space defined by its parent bus. Most commonly this means 
> the offsets and lengths of memory-mapped IO register blocks, but may 
> have a different meaning on some bus types. Addresses in the address 
> space defined by the root node are CPU real addresses.
>    
>    The value is a <prop-encoded-array>, composed of an arbitrary number of 
> pairs of address and length, <address length>. The number of <u32> cells 
> required to specify the address and length are bus-specific and are 
> specified by the #address-cells and #size-cells properties in the parent 
> of the device node. If the parent node specifies a value of 0 for 
> #size-cells, the length field in the value of reg shall be omitted. So 
> it is guaranteed by DTC compiler and it would be enough to check 
> overflow in suggested by you way: ac > len / sizeof(*cell)
> But considering what you noticed below ...
> 
>>
>>> +        return ~0UL;
>>> +
>>> +    return dt_read_number(cell, ac);
>> What meaning does this have for ac > 2? (As per your checking above
>> it can be up to UINT32_MAX / 4.)
> 
> ... It will be an issue for dt_read_number() which could deal only with uint64_t what means
> we can't have ac > 2. (UINT32_MAX / 4 it is a theoretical maximum IIUC)
> 
> Thereby we could do in the following way:
> @@ -30,19 +30,18 @@ static unsigned long dt_get_hartid(const struct dt_device_node *cpun)
>       const __be32 *cell;
>       unsigned int ac;
>       uint32_t len;
> -    unsigned int max_cells = UINT32_MAX / sizeof(*cell);
>   
>       ac = dt_n_addr_cells(cpun);
>       cell = dt_get_property(cpun, "reg", &len);
>   
> -    if (ac > max_cells) {
> -        printk("%s: cell count overflow (ac=%u, max=%u)\n", __func__, ac,
> -               max_cells);
> +    if ( !cell || !ac || (ac > len / sizeof(*cell)) )
>           return ~0UL;
> -    }
>   
> -    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )
> -        return ~0UL;
> +    /*
> +     * If ac > 2, the result may be truncated or meaningless unless
> +     * dt_read_number() supports wider integers.
> +     */
> +    BUG_ON(ac > 2);
>   
>       return dt_read_number(cell, ac);
>   }
> 
> I am not sure that BUG_ON() should be in dt_get_hartid(). Probably it would be better move it
> to dt_read_number() as if one day support for RV128 will be needed I assume that it will be
> needed to change a prototype of dt_read_number() to work with address-cells = 3.
> What do you think? Could I go with the suggested above changes or it would be better to move
> BUG_ON() to dt_read_number()?

Don't know; the DT maintainers would have to judge. I don't, however, think it should
be BUG_ON() - as said several times before, that's a check suitable to cover for
possible mistakes in Xen code. Here however you're trying to cover for a flaw in DT.

Jan