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 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         | 69 ++++++++++++++++++++++++++++++++
 2 files changed, 73 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..b8d18fc3ea 100644
--- a/xen/arch/riscv/smpboot.c
+++ b/xen/arch/riscv/smpboot.c
@@ -1,5 +1,8 @@
 #include <xen/cpumask.h>
+#include <xen/device_tree.h>
+#include <xen/errno.h>
 #include <xen/init.h>
+#include <xen/types.h>
 #include <xen/sections.h>
 
 #include <asm/current.h>
@@ -14,3 +17,69 @@ 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;
+
+    ac = dt_n_addr_cells(cpun);
+    cell = dt_get_property(cpun, "reg", &len);
+    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=%lu is not available\n", *hartid);
+        return -ENODEV;
+    }
+
+    if ( (ret = dt_property_read_string(node, "riscv,isa", &isa)) )
+    {
+        printk("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hartid);
+        return ret;
+    }
+
+    if ( isa[0] != 'r' || isa[1] != 'v' )
+    {
+        printk("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hartid,
+               isa);
+        return -EINVAL;
+    }
+
+    return 0;
+}
-- 
2.49.0

On 21.05.2025 18:03, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/smpboot.c
> +++ b/xen/arch/riscv/smpboot.c
> @@ -1,5 +1,8 @@
>  #include <xen/cpumask.h>
> +#include <xen/device_tree.h>
> +#include <xen/errno.h>
>  #include <xen/init.h>
> +#include <xen/types.h>
>  #include <xen/sections.h>

Nit: The latter insertion wants to move one line down. Yet then - isn't
xen/stdint.h sufficient here?

> @@ -14,3 +17,69 @@ 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;
> +
> +    ac = dt_n_addr_cells(cpun);
> +    cell = dt_get_property(cpun, "reg", &len);
> +    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )

Does DT make any guarantees for this multiplication to not overflow?

> +        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=%lu is not available\n", *hartid);

Considering that hart ID assignment is outside of our control, would we
perhaps better (uniformly) log such using %#lx?

> +        return -ENODEV;
> +    }
> +
> +    if ( (ret = dt_property_read_string(node, "riscv,isa", &isa)) )
> +    {
> +        printk("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hartid);
> +        return ret;
> +    }
> +
> +    if ( isa[0] != 'r' || isa[1] != 'v' )
> +    {
> +        printk("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hartid,
> +               isa);
> +        return -EINVAL;

As before -EINVAL is appropriate when input arguments have wrong values.
Here, however, you found an unexpected value in something the platform
has presented to you. While not entirely appropriate either, maybe
-ENODEV again (if nothing better can be found)?

Jan

On 5/22/25 9:50 AM, Jan Beulich wrote:
> On 21.05.2025 18:03, Oleksii Kurochko wrote:
>> --- a/xen/arch/riscv/smpboot.c
>> +++ b/xen/arch/riscv/smpboot.c
>> @@ -1,5 +1,8 @@
>>   #include <xen/cpumask.h>
>> +#include <xen/device_tree.h>
>> +#include <xen/errno.h>
>>   #include <xen/init.h>
>> +#include <xen/types.h>
>>   #include <xen/sections.h>
> Nit: The latter insertion wants to move one line down. Yet then - isn't
> xen/stdint.h sufficient here?

__be32 used in dt_get_hartid() is defined in xen/types.h.

>
>> @@ -14,3 +17,69 @@ 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;
>> +
>> +    ac = dt_n_addr_cells(cpun);
>> +    cell = dt_get_property(cpun, "reg", &len);
>> +    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )
> Does DT make any guarantees for this multiplication to not overflow?

I haven't tried of DTC checks such things during compilation but considering that
ac value is uin32_t value (according to DT spec) then overflow could really happen.
I will add the following to check an overflow:
     if ( ac > ((sizeof(size_t) * BIT_PER_BYTE) / sizeof(*cell)) )
     {
         printk("%s: overflow detected\n", __func__);
         return ~0UL;
     }

>
>> +        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=%lu is not available\n", *hartid);
> Considering that hart ID assignment is outside of our control, would we
> perhaps better (uniformly) log such using %#lx?

It makes sense, DTC when generates dts from dtb also uses hex number, so it could
help to find a failure node faster.

>
>> +        return -ENODEV;
>> +    }
>> +
>> +    if ( (ret = dt_property_read_string(node, "riscv,isa", &isa)) )
>> +    {
>> +        printk("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hartid);
>> +        return ret;
>> +    }
>> +
>> +    if ( isa[0] != 'r' || isa[1] != 'v' )
>> +    {
>> +        printk("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hartid,
>> +               isa);
>> +        return -EINVAL;
> As before -EINVAL is appropriate when input arguments have wrong values.
> Here, however, you found an unexpected value in something the platform
> has presented to you. While not entirely appropriate either, maybe
> -ENODEV again (if nothing better can be found)?

I don't see better candidate.

Interesting if some reserved region exists for user
defined errors.

~ Oleksii

On 5/26/25 12:46 PM, Oleksii Kurochko wrote:
>
>
> On 5/22/25 9:50 AM, Jan Beulich wrote:
>> On 21.05.2025 18:03, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/smpboot.c
>>> +++ b/xen/arch/riscv/smpboot.c
>>> @@ -1,5 +1,8 @@
>>>   #include <xen/cpumask.h>
>>> +#include <xen/device_tree.h>
>>> +#include <xen/errno.h>
>>>   #include <xen/init.h>
>>> +#include <xen/types.h>
>>>   #include <xen/sections.h>
>> Nit: The latter insertion wants to move one line down. Yet then - isn't
>> xen/stdint.h sufficient here?
> __be32 used in dt_get_hartid() is defined in xen/types.h.
>
>>> @@ -14,3 +17,69 @@ 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;
>>> +
>>> +    ac = dt_n_addr_cells(cpun);
>>> +    cell = dt_get_property(cpun, "reg", &len);
>>> +    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )
>> Does DT make any guarantees for this multiplication to not overflow?
> I haven't tried of DTC checks such things during compilation but considering that
> ac value is uin32_t value (according to DT spec) then overflow could really happen.
> I will add the following to check an overflow:
>      if ( ac > ((sizeof(size_t) * BIT_PER_BYTE) / sizeof(*cell)) )
>      {
>          printk("%s: overflow detected\n", __func__);
>          return ~0UL;
>      }

Oops, I meant here size_t_max instead of (sizeof(size_t) * BIT_PER_BYTE), lost power of 2 minus 1.
Probably, SIZE_T_MAX or something similar exists. I have to check.

~ Oleksii

>
>>> +        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=%lu is not available\n", *hartid);
>> Considering that hart ID assignment is outside of our control, would we
>> perhaps better (uniformly) log such using %#lx?
> It makes sense, DTC when generates dts from dtb also uses hex number, so it could
> help to find a failure node faster.
>
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    if ( (ret = dt_property_read_string(node, "riscv,isa", &isa)) )
>>> +    {
>>> +        printk("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hartid);
>>> +        return ret;
>>> +    }
>>> +
>>> +    if ( isa[0] != 'r' || isa[1] != 'v' )
>>> +    {
>>> +        printk("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hartid,
>>> +               isa);
>>> +        return -EINVAL;
>> As before -EINVAL is appropriate when input arguments have wrong values.
>> Here, however, you found an unexpected value in something the platform
>> has presented to you. While not entirely appropriate either, maybe
>> -ENODEV again (if nothing better can be found)?
> I don't see better candidate.
>
> Interesting if some reserved region exists for user
> defined errors.
>
> ~ Oleksii