Implements riscv_of_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 of_get_cpu_hwid() is introduced to deal specifically
with reg propery of a CPU device node.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
xen/arch/riscv/include/asm/smp.h | 3 ++
xen/arch/riscv/smpboot.c | 68 ++++++++++++++++++++++++++++++++
2 files changed, 71 insertions(+)
diff --git a/xen/arch/riscv/include/asm/smp.h b/xen/arch/riscv/include/asm/smp.h
index 188c033718..9b68f1e27a 100644
--- a/xen/arch/riscv/include/asm/smp.h
+++ b/xen/arch/riscv/include/asm/smp.h
@@ -26,6 +26,9 @@ static inline void set_cpuid_to_hartid(unsigned long cpuid,
void setup_tp(unsigned int cpuid);
+struct dt_device_node;
+int riscv_of_processor_hartid(struct dt_device_node *node, unsigned long *hart);
+
void smp_clear_cpu_maps(void);
#endif
diff --git a/xen/arch/riscv/smpboot.c b/xen/arch/riscv/smpboot.c
index 0f4dcc28e1..3193639f00 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>
cpumask_t cpu_online_map;
cpumask_t cpu_present_map;
@@ -13,3 +16,68 @@ void __init smp_clear_cpu_maps(void)
cpumask_set_cpu(0, &cpu_online_map);
cpumask_copy(&cpu_present_map, &cpu_possible_map);
}
+
+/**
+ * of_get_cpu_hwid - Get the hardware ID from a CPU device node
+ *
+ * @cpun: CPU number(logical index) for which device node is required
+ * @thread: The local thread number to get the hardware ID for.
+ *
+ * Return: The hardware ID for the CPU node or ~0ULL if not found.
+ */
+static uint64_t of_get_cpu_hwid(struct dt_device_node *cpun, unsigned int thread)
+{
+ const __be32 *cell;
+ int ac;
+ uint32_t len;
+
+ ac = dt_n_addr_cells(cpun);
+ cell = dt_get_property(cpun, "reg", &len);
+ if ( !cell || !ac || ((sizeof(*cell) * ac * (thread + 1)) > len) )
+ return ~0ULL;
+
+ cell += ac * thread;
+ return dt_read_number(cell, ac);
+}
+
+/*
+ * Returns the hart ID of the given device tree node, or -ENODEV if the node
+ * isn't an enabled and valid RISC-V hart node.
+ */
+int riscv_of_processor_hartid(struct dt_device_node *node, unsigned long *hart)
+{
+ const char *isa;
+
+ if ( !dt_device_is_compatible(node, "riscv") )
+ {
+ printk("Found incompatible CPU\n");
+ return -ENODEV;
+ }
+
+ *hart = (unsigned long) of_get_cpu_hwid(node, 0);
+ if ( *hart == ~0UL )
+ {
+ printk("Found CPU without hart ID\n");
+ return -ENODEV;
+ }
+
+ if ( !dt_device_is_available(node))
+ {
+ printk("CPU with hartid=%lu is not available\n", *hart);
+ return -ENODEV;
+ }
+
+ if ( dt_property_read_string(node, "riscv,isa", &isa) )
+ {
+ printk("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hart);
+ return -ENODEV;
+ }
+
+ if ( isa[0] != 'r' || isa[1] != 'v' )
+ {
+ printk("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hart, isa);
+ return -ENODEV;
+ }
+
+ return 0;
+}
--
2.49.0On 08.04.2025 17:57, Oleksii Kurochko wrote:
> @@ -13,3 +16,68 @@ void __init smp_clear_cpu_maps(void)
> cpumask_set_cpu(0, &cpu_online_map);
> cpumask_copy(&cpu_present_map, &cpu_possible_map);
> }
> +
> +/**
> + * of_get_cpu_hwid - Get the hardware ID from a CPU device node
> + *
> + * @cpun: CPU number(logical index) for which device node is required
> + * @thread: The local thread number to get the hardware ID for.
> + *
> + * Return: The hardware ID for the CPU node or ~0ULL if not found.
> + */
> +static uint64_t of_get_cpu_hwid(struct dt_device_node *cpun, unsigned int thread)
What does the "of" prefix stand for here? Looking at the function body I'm
really at a loss. (I was first guessing something like OpenFirmware, but
there's nothing here that would support that.)
As you're only fetching data - can cpun be pointer-to-const?
> +{
> + const __be32 *cell;
> + int ac;
> + uint32_t len;
> +
> + ac = dt_n_addr_cells(cpun);
> + cell = dt_get_property(cpun, "reg", &len);
> + if ( !cell || !ac || ((sizeof(*cell) * ac * (thread + 1)) > len) )
> + return ~0ULL;
I'm sorry for my lack of DT knowledge, but what's "thread" representing here?
You only pass in 0 below, so it's unclear whether it's what one might expect
(the thread number on a multi-threaded core).
> + cell += ac * thread;
> + return dt_read_number(cell, ac);
Nit (you know what)
> +/*
> + * Returns the hart ID of the given device tree node, or -ENODEV if the node
> + * isn't an enabled and valid RISC-V hart node.
> + */
> +int riscv_of_processor_hartid(struct dt_device_node *node, unsigned long *hart)
Similar question as above: What's "of" and what significance does the "riscv"
prefix have in RISC-V code?
Const-ness question again for "node".
> +{
> + const char *isa;
> +
> + if ( !dt_device_is_compatible(node, "riscv") )
> + {
> + printk("Found incompatible CPU\n");
> + return -ENODEV;
> + }
> +
> + *hart = (unsigned long) of_get_cpu_hwid(node, 0);
> + if ( *hart == ~0UL )
While for RV64 this won't matter, the difference in types (uint64_t returned,
unsigned long used) is still puzzling me. What's the deal?
Jan
On 4/10/25 5:53 PM, Jan Beulich wrote:
> On 08.04.2025 17:57, Oleksii Kurochko wrote:
>> @@ -13,3 +16,68 @@ void __init smp_clear_cpu_maps(void)
>> cpumask_set_cpu(0, &cpu_online_map);
>> cpumask_copy(&cpu_present_map, &cpu_possible_map);
>> }
>> +
>> +/**
>> + * of_get_cpu_hwid - Get the hardware ID from a CPU device node
>> + *
>> + * @cpun: CPU number(logical index) for which device node is required
>> + * @thread: The local thread number to get the hardware ID for.
>> + *
>> + * Return: The hardware ID for the CPU node or ~0ULL if not found.
>> + */
>> +static uint64_t of_get_cpu_hwid(struct dt_device_node *cpun, unsigned int thread)
> What does the "of" prefix stand for here? Looking at the function body I'm
> really at a loss. (I was first guessing something like OpenFirmware, but
> there's nothing here that would support that.)
I copy this function from Linux kernel. But you are right, "of" means OpenFirmware or
Open Firmware Device Tree and is used for the functions which work with device
tree.
I'll rename to dt_get_cpu_hwid() to follow the naming of device tree's functions
name in Xen.
>
> As you're only fetching data - can cpun be pointer-to-const?
Sure, it can be. I'll update that.
>
>> +{
>> + const __be32 *cell;
>> + int ac;
>> + uint32_t len;
>> +
>> + ac = dt_n_addr_cells(cpun);
>> + cell = dt_get_property(cpun, "reg", &len);
>> + if ( !cell || !ac || ((sizeof(*cell) * ac * (thread + 1)) > len) )
>> + return ~0ULL;
> I'm sorry for my lack of DT knowledge, but what's "thread" representing here?
> You only pass in 0 below, so it's unclear whether it's what one might expect
> (the thread number on a multi-threaded core).
Based on the DT specification alone, the|`reg`| value could refer to either a CPU or a thread ID:
```
The value of reg is a <prop-encoded-array> that defines a unique CPU/thread id for
the CPU/threads represented by the CPU node. If a CPU supports more than one thread
(i.e. multiple streams of execution) the reg prop-erty is an array with 1 element
per thread.
```
My understanding is that the term/thread/ was used in the Linux kernel to cover both
cases.
When SMT isn't supported, the CPU can be considered to have a single thread.
For example, RISC-V uses the term/hardware thread/ to describe a hart (i.e., a CPU).
Interestingly, the Linux kernel always uses|thread = 0|.
We could potentially drop this ambiguity and introduce an|ASSERT()| to check that
the|`reg`| property contains only one entry, representing the HART (CPU) ID:
```
Software can determine the number of threads by dividing the size of reg by the parent
node’s #address-cells. If `|reg`| has more than one entry, it would simply SMT support
is required.
```
Does that approach make sense, or should we stick with the current implementation?
>
>> + cell += ac * thread;
>> + return dt_read_number(cell, ac);
> Nit (you know what)
>
>> +/*
>> + * Returns the hart ID of the given device tree node, or -ENODEV if the node
>> + * isn't an enabled and valid RISC-V hart node.
>> + */
>> +int riscv_of_processor_hartid(struct dt_device_node *node, unsigned long *hart)
> Similar question as above: What's "of" and what significance does the "riscv"
> prefix have in RISC-V code?
I will drop usage of 'of' in Xen and change it to 'dt'.
>
> Const-ness question again for "node".
>
>> +{
>> + const char *isa;
>> +
>> + if ( !dt_device_is_compatible(node, "riscv") )
>> + {
>> + printk("Found incompatible CPU\n");
>> + return -ENODEV;
>> + }
>> +
>> + *hart = (unsigned long) of_get_cpu_hwid(node, 0);
>> + if ( *hart == ~0UL )
> While for RV64 this won't matter, the difference in types (uint64_t returned,
> unsigned long used) is still puzzling me. What's the deal?
No specific reason, just overlooked that moment. I think we could use just
drop this cast.
The reason for uint64_t as a return type is that dt_read_number() returns
u64.
~ Oleksii
On 15.04.2025 15:39, Oleksii Kurochko wrote:
> On 4/10/25 5:53 PM, Jan Beulich wrote:
>> On 08.04.2025 17:57, Oleksii Kurochko wrote:
>>> +{
>>> + const __be32 *cell;
>>> + int ac;
>>> + uint32_t len;
>>> +
>>> + ac = dt_n_addr_cells(cpun);
>>> + cell = dt_get_property(cpun, "reg", &len);
>>> + if ( !cell || !ac || ((sizeof(*cell) * ac * (thread + 1)) > len) )
>>> + return ~0ULL;
>> I'm sorry for my lack of DT knowledge, but what's "thread" representing here?
>> You only pass in 0 below, so it's unclear whether it's what one might expect
>> (the thread number on a multi-threaded core).
>
> Based on the DT specification alone, the|`reg`| value could refer to either a CPU or a thread ID:
> ```
> The value of reg is a <prop-encoded-array> that defines a unique CPU/thread id for
> the CPU/threads represented by the CPU node. If a CPU supports more than one thread
> (i.e. multiple streams of execution) the reg prop-erty is an array with 1 element
> per thread.
> ```
>
> My understanding is that the term/thread/ was used in the Linux kernel to cover both
> cases.
> When SMT isn't supported, the CPU can be considered to have a single thread.
> For example, RISC-V uses the term/hardware thread/ to describe a hart (i.e., a CPU).
>
> Interestingly, the Linux kernel always uses|thread = 0|.
>
> We could potentially drop this ambiguity and introduce an|ASSERT()| to check that
> the|`reg`| property contains only one entry, representing the HART (CPU) ID:
> ```
> Software can determine the number of threads by dividing the size of reg by the parent
> node’s #address-cells. If `|reg`| has more than one entry, it would simply SMT support
> is required.
> ```
>
> Does that approach make sense, or should we stick with the current implementation?
If extra enabling is required to make multi-thread CPUs work, then panic()ing
(not so much ASSERT()ing) may make sense, for the time being. Better would be
if we could use all threads in a system right away.
Jan
On 4/15/25 3:45 PM, Jan Beulich wrote:
> On 15.04.2025 15:39, Oleksii Kurochko wrote:
>> On 4/10/25 5:53 PM, Jan Beulich wrote:
>>> On 08.04.2025 17:57, Oleksii Kurochko wrote:
>>>> +{
>>>> + const __be32 *cell;
>>>> + int ac;
>>>> + uint32_t len;
>>>> +
>>>> + ac = dt_n_addr_cells(cpun);
>>>> + cell = dt_get_property(cpun, "reg", &len);
>>>> + if ( !cell || !ac || ((sizeof(*cell) * ac * (thread + 1)) > len) )
>>>> + return ~0ULL;
>>> I'm sorry for my lack of DT knowledge, but what's "thread" representing here?
>>> You only pass in 0 below, so it's unclear whether it's what one might expect
>>> (the thread number on a multi-threaded core).
>> Based on the DT specification alone, the|`reg`| value could refer to either a CPU or a thread ID:
>> ```
>> The value of reg is a <prop-encoded-array> that defines a unique CPU/thread id for
>> the CPU/threads represented by the CPU node. If a CPU supports more than one thread
>> (i.e. multiple streams of execution) the reg prop-erty is an array with 1 element
>> per thread.
>> ```
>>
>> My understanding is that the term/thread/ was used in the Linux kernel to cover both
>> cases.
>> When SMT isn't supported, the CPU can be considered to have a single thread.
>> For example, RISC-V uses the term/hardware thread/ to describe a hart (i.e., a CPU).
>>
>> Interestingly, the Linux kernel always uses|thread = 0|.
>>
>> We could potentially drop this ambiguity and introduce an|ASSERT()| to check that
>> the|`reg`| property contains only one entry, representing the HART (CPU) ID:
>> ```
>> Software can determine the number of threads by dividing the size of reg by the parent
>> node’s #address-cells. If `|reg`| has more than one entry, it would simply SMT support
>> is required.
>> ```
>>
>> Does that approach make sense, or should we stick with the current implementation?
> If extra enabling is required to make multi-thread CPUs work, then panic()ing
> (not so much ASSERT()ing) may make sense, for the time being. Better would be
> if we could use all threads in a system right away.
Actually, this function is ready to be used for multi-thread CPUs. A caller can request hardware id
by passing `thread` argument (`thread` -> the local thread number to get the hardware ID for).
So by calling:
dt_get_cpu_hwid(cpu0, 0) -> it will return hardware id of thread 0 of cpu0
dt_get_cpu_hwid(cpu0, 1) -> it will return hardware id of thread 1 of cpu0
...
In our case we assume that SMP isn't supported so that is why it is used only dt_get_cpu_hwid(cpu0, 0).
If one day, SMP will be enabled then it will be needed to change a callers of dt_get_cpu_hwid().
I will add a check in the caller.
~ Oleksii
On 25.04.2025 19:07, Oleksii Kurochko wrote:
>
> On 4/15/25 3:45 PM, Jan Beulich wrote:
>> On 15.04.2025 15:39, Oleksii Kurochko wrote:
>>> On 4/10/25 5:53 PM, Jan Beulich wrote:
>>>> On 08.04.2025 17:57, Oleksii Kurochko wrote:
>>>>> +{
>>>>> + const __be32 *cell;
>>>>> + int ac;
>>>>> + uint32_t len;
>>>>> +
>>>>> + ac = dt_n_addr_cells(cpun);
>>>>> + cell = dt_get_property(cpun, "reg", &len);
>>>>> + if ( !cell || !ac || ((sizeof(*cell) * ac * (thread + 1)) > len) )
>>>>> + return ~0ULL;
>>>> I'm sorry for my lack of DT knowledge, but what's "thread" representing here?
>>>> You only pass in 0 below, so it's unclear whether it's what one might expect
>>>> (the thread number on a multi-threaded core).
>>> Based on the DT specification alone, the|`reg`| value could refer to either a CPU or a thread ID:
>>> ```
>>> The value of reg is a <prop-encoded-array> that defines a unique CPU/thread id for
>>> the CPU/threads represented by the CPU node. If a CPU supports more than one thread
>>> (i.e. multiple streams of execution) the reg prop-erty is an array with 1 element
>>> per thread.
>>> ```
>>>
>>> My understanding is that the term/thread/ was used in the Linux kernel to cover both
>>> cases.
>>> When SMT isn't supported, the CPU can be considered to have a single thread.
>>> For example, RISC-V uses the term/hardware thread/ to describe a hart (i.e., a CPU).
Note the terminology ("CPU") you used here.
>>> Interestingly, the Linux kernel always uses|thread = 0|.
>>>
>>> We could potentially drop this ambiguity and introduce an|ASSERT()| to check that
>>> the|`reg`| property contains only one entry, representing the HART (CPU) ID:
>>> ```
>>> Software can determine the number of threads by dividing the size of reg by the parent
>>> node’s #address-cells. If `|reg`| has more than one entry, it would simply SMT support
>>> is required.
>>> ```
>>>
>>> Does that approach make sense, or should we stick with the current implementation?
>> If extra enabling is required to make multi-thread CPUs work, then panic()ing
>> (not so much ASSERT()ing) may make sense, for the time being. Better would be
>> if we could use all threads in a system right away.
>
> Actually, this function is ready to be used for multi-thread CPUs. A caller can request hardware id
> by passing `thread` argument (`thread` -> the local thread number to get the hardware ID for).
> So by calling:
> dt_get_cpu_hwid(cpu0, 0) -> it will return hardware id of thread 0 of cpu0
> dt_get_cpu_hwid(cpu0, 1) -> it will return hardware id of thread 1 of cpu0
> ...
>
> In our case we assume that SMP isn't supported so that is why it is used only dt_get_cpu_hwid(cpu0, 0).
>
> If one day, SMP will be enabled then it will be needed to change a callers of dt_get_cpu_hwid().
I assume you meant SMT in both places you wrote SMP? But my main point here is:
If enumeration gives you "thread <N> of core <M>" (using x86 terminology), you
need to be quite careful with what you call "CPU". Things need to be entirely
unambiguous, taking into account what internally in (common code) Xen we call a
"CPU". You certainly may call "CPU" what is a collection of threads / harts,
but you then need to clarify this in a prominent comment somewhere, and you
need to be entirely consistent throughout the RISC-V sub-tree.
Jan
On 4/28/25 8:31 AM, Jan Beulich wrote:
> On 25.04.2025 19:07, Oleksii Kurochko wrote:
>> On 4/15/25 3:45 PM, Jan Beulich wrote:
>>> On 15.04.2025 15:39, Oleksii Kurochko wrote:
>>>> On 4/10/25 5:53 PM, Jan Beulich wrote:
>>>>> On 08.04.2025 17:57, Oleksii Kurochko wrote:
>>>>>> +{
>>>>>> + const __be32 *cell;
>>>>>> + int ac;
>>>>>> + uint32_t len;
>>>>>> +
>>>>>> + ac = dt_n_addr_cells(cpun);
>>>>>> + cell = dt_get_property(cpun, "reg", &len);
>>>>>> + if ( !cell || !ac || ((sizeof(*cell) * ac * (thread + 1)) > len) )
>>>>>> + return ~0ULL;
>>>>> I'm sorry for my lack of DT knowledge, but what's "thread" representing here?
>>>>> You only pass in 0 below, so it's unclear whether it's what one might expect
>>>>> (the thread number on a multi-threaded core).
>>>> Based on the DT specification alone, the|`reg`| value could refer to either a CPU or a thread ID:
>>>> ```
>>>> The value of reg is a <prop-encoded-array> that defines a unique CPU/thread id for
>>>> the CPU/threads represented by the CPU node. If a CPU supports more than one thread
>>>> (i.e. multiple streams of execution) the reg prop-erty is an array with 1 element
>>>> per thread.
>>>> ```
>>>>
>>>> My understanding is that the term/thread/ was used in the Linux kernel to cover both
>>>> cases.
>>>> When SMT isn't supported, the CPU can be considered to have a single thread.
>>>> For example, RISC-V uses the term/hardware thread/ to describe a hart (i.e., a CPU).
> Note the terminology ("CPU") you used here.
>
>>>> Interestingly, the Linux kernel always uses|thread = 0|.
>>>>
>>>> We could potentially drop this ambiguity and introduce an|ASSERT()| to check that
>>>> the|`reg`| property contains only one entry, representing the HART (CPU) ID:
>>>> ```
>>>> Software can determine the number of threads by dividing the size of reg by the parent
>>>> node’s #address-cells. If `|reg`| has more than one entry, it would simply SMT support
>>>> is required.
>>>> ```
>>>>
>>>> Does that approach make sense, or should we stick with the current implementation?
>>> If extra enabling is required to make multi-thread CPUs work, then panic()ing
>>> (not so much ASSERT()ing) may make sense, for the time being. Better would be
>>> if we could use all threads in a system right away.
>> Actually, this function is ready to be used for multi-thread CPUs. A caller can request hardware id
>> by passing `thread` argument (`thread` -> the local thread number to get the hardware ID for).
>> So by calling:
>> dt_get_cpu_hwid(cpu0, 0) -> it will return hardware id of thread 0 of cpu0
>> dt_get_cpu_hwid(cpu0, 1) -> it will return hardware id of thread 1 of cpu0
>> ...
>>
>> In our case we assume that SMP isn't supported so that is why it is used only dt_get_cpu_hwid(cpu0, 0).
>>
>> If one day, SMP will be enabled then it will be needed to change a callers of dt_get_cpu_hwid().
> I assume you meant SMT in both places you wrote SMP?
Yes, it should be SMT.
> But my main point here is:
> If enumeration gives you "thread <N> of core <M>" (using x86 terminology), you
> need to be quite careful with what you call "CPU". Things need to be entirely
> unambiguous, taking into account what internally in (common code) Xen we call a
> "CPU". You certainly may call "CPU" what is a collection of threads / harts,
> but you then need to clarify this in a prominent comment somewhere, and you
> need to be entirely consistent throughout the RISC-V sub-tree.
╭────────────────────╮
│ CPU │ ← 1 physical processor (chip)
│ ┌───────┬─────────┐ │
│ │ Core 0│ Core 1 │ │ ← 2 cores (for example)
│ │ ┌──┬──┐ ┌──┬──┐ │ │
│ │Thr0 Thr1 Thr0 Thr1│ ← 2 threads on each core (SMT)
│ └───────┴─────────┘ │
╰────────────────────╯
I want to double check what Xen call a "CPU". I thought that Xen uses word
CPU to describe a core, right?
What you wrote above "thread <N> of core <M> (using x86 terminology)" is also correlated
with RISC-V terminology:
A component is termed a core if it contains an independent instruction fetch unit.
A RISC-V-compatible core might support multiple RISC-V-compatible hardware threads,
or harts, through multithreading
I checked RISC-V's DTS binding and it seems it is a little bit contradictory to DTS spec,
where it is mentioned that reg property is used to describe how many threads a cpu has
when SMP is used, but in RISC-V's dts binding they are describing a hardware execution
context:
This document uses some terminology common to the RISC-V community
that is not widely used, the definitions of which are listed here:
hart: A hardware execution context, which contains all the state
mandated by the RISC-V ISA: a PC and some registers. This
terminology is designed to disambiguate software's view of execution
contexts from any particular microarchitectural implementation
strategy. For example, an Intel laptop containing one socket with
two cores, each of which has two hyperthreads, could be described as
having four harts.
So in RISC-V's DTS binding they are describing only hardware threads what makes things more
confusing in terms what kind terminology from Xen point of view should be used.
And based on what is written in RISC-V's dts binding:
For example, an Intel laptop containing one socket with
two cores, each of which has two hyperthreads, could be described as
having four harts.
It would be more logical to drop 'thread' argument of riscv_of_get_cpu_hwid(const struct dt_device_node *cpun).
And then the question is what to do with the name of variable cpun? As it could be still confusing. Or, at least,
I can add the comment that CPUn in terms of RISC-V means hart (hardware thread). And then will it be needed to
add such comment for each usage of word "CPU"?
~ Oleksii
On 28.04.2025 12:43, Oleksii Kurochko wrote:
>
> On 4/28/25 8:31 AM, Jan Beulich wrote:
>> On 25.04.2025 19:07, Oleksii Kurochko wrote:
>>> On 4/15/25 3:45 PM, Jan Beulich wrote:
>>>> On 15.04.2025 15:39, Oleksii Kurochko wrote:
>>>>> On 4/10/25 5:53 PM, Jan Beulich wrote:
>>>>>> On 08.04.2025 17:57, Oleksii Kurochko wrote:
>>>>>>> +{
>>>>>>> + const __be32 *cell;
>>>>>>> + int ac;
>>>>>>> + uint32_t len;
>>>>>>> +
>>>>>>> + ac = dt_n_addr_cells(cpun);
>>>>>>> + cell = dt_get_property(cpun, "reg", &len);
>>>>>>> + if ( !cell || !ac || ((sizeof(*cell) * ac * (thread + 1)) > len) )
>>>>>>> + return ~0ULL;
>>>>>> I'm sorry for my lack of DT knowledge, but what's "thread" representing here?
>>>>>> You only pass in 0 below, so it's unclear whether it's what one might expect
>>>>>> (the thread number on a multi-threaded core).
>>>>> Based on the DT specification alone, the|`reg`| value could refer to either a CPU or a thread ID:
>>>>> ```
>>>>> The value of reg is a <prop-encoded-array> that defines a unique CPU/thread id for
>>>>> the CPU/threads represented by the CPU node. If a CPU supports more than one thread
>>>>> (i.e. multiple streams of execution) the reg prop-erty is an array with 1 element
>>>>> per thread.
>>>>> ```
>>>>>
>>>>> My understanding is that the term/thread/ was used in the Linux kernel to cover both
>>>>> cases.
>>>>> When SMT isn't supported, the CPU can be considered to have a single thread.
>>>>> For example, RISC-V uses the term/hardware thread/ to describe a hart (i.e., a CPU).
>> Note the terminology ("CPU") you used here.
>>
>>>>> Interestingly, the Linux kernel always uses|thread = 0|.
>>>>>
>>>>> We could potentially drop this ambiguity and introduce an|ASSERT()| to check that
>>>>> the|`reg`| property contains only one entry, representing the HART (CPU) ID:
>>>>> ```
>>>>> Software can determine the number of threads by dividing the size of reg by the parent
>>>>> node’s #address-cells. If `|reg`| has more than one entry, it would simply SMT support
>>>>> is required.
>>>>> ```
>>>>>
>>>>> Does that approach make sense, or should we stick with the current implementation?
>>>> If extra enabling is required to make multi-thread CPUs work, then panic()ing
>>>> (not so much ASSERT()ing) may make sense, for the time being. Better would be
>>>> if we could use all threads in a system right away.
>>> Actually, this function is ready to be used for multi-thread CPUs. A caller can request hardware id
>>> by passing `thread` argument (`thread` -> the local thread number to get the hardware ID for).
>>> So by calling:
>>> dt_get_cpu_hwid(cpu0, 0) -> it will return hardware id of thread 0 of cpu0
>>> dt_get_cpu_hwid(cpu0, 1) -> it will return hardware id of thread 1 of cpu0
>>> ...
>>>
>>> In our case we assume that SMP isn't supported so that is why it is used only dt_get_cpu_hwid(cpu0, 0).
>>>
>>> If one day, SMP will be enabled then it will be needed to change a callers of dt_get_cpu_hwid().
>> I assume you meant SMT in both places you wrote SMP?
>
> Yes, it should be SMT.
>
>> But my main point here is:
>> If enumeration gives you "thread <N> of core <M>" (using x86 terminology), you
>> need to be quite careful with what you call "CPU". Things need to be entirely
>> unambiguous, taking into account what internally in (common code) Xen we call a
>> "CPU". You certainly may call "CPU" what is a collection of threads / harts,
>> but you then need to clarify this in a prominent comment somewhere, and you
>> need to be entirely consistent throughout the RISC-V sub-tree.
>
> ╭────────────────────╮
> │ CPU │ ← 1 physical processor (chip)
> │ ┌───────┬─────────┐ │
> │ │ Core 0│ Core 1 │ │ ← 2 cores (for example)
> │ │ ┌──┬──┐ ┌──┬──┐ │ │
> │ │Thr0 Thr1 Thr0 Thr1│ ← 2 threads on each core (SMT)
> │ └───────┴─────────┘ │
> ╰────────────────────╯
> I want to double check what Xen call a "CPU". I thought that Xen uses word
> CPU to describe a core, right?
No, see e.g. cpumask.h - it's a hart (as per below) that we internally describe
as CPU (leaving aside potentially ambiguous comments here and there, which is
what I'd like to prevent from the start for RISC-V).
> What you wrote above "thread <N> of core <M> (using x86 terminology)" is also correlated
> with RISC-V terminology:
> A component is termed a core if it contains an independent instruction fetch unit.
> A RISC-V-compatible core might support multiple RISC-V-compatible hardware threads,
> or harts, through multithreading
>
> I checked RISC-V's DTS binding and it seems it is a little bit contradictory to DTS spec,
> where it is mentioned that reg property is used to describe how many threads a cpu has
> when SMP is used, but in RISC-V's dts binding they are describing a hardware execution
> context:
> This document uses some terminology common to the RISC-V community
> that is not widely used, the definitions of which are listed here:
>
> hart: A hardware execution context, which contains all the state
> mandated by the RISC-V ISA: a PC and some registers. This
> terminology is designed to disambiguate software's view of execution
> contexts from any particular microarchitectural implementation
> strategy. For example, an Intel laptop containing one socket with
> two cores, each of which has two hyperthreads, could be described as
> having four harts.
>
> So in RISC-V's DTS binding they are describing only hardware threads what makes things more
> confusing in terms what kind terminology from Xen point of view should be used.
>
> And based on what is written in RISC-V's dts binding:
> For example, an Intel laptop containing one socket with
> two cores, each of which has two hyperthreads, could be described as
> having four harts.
> It would be more logical to drop 'thread' argument of riscv_of_get_cpu_hwid(const struct dt_device_node *cpun).
> And then the question is what to do with the name of variable cpun? As it could be still confusing. Or, at least,
> I can add the comment that CPUn in terms of RISC-V means hart (hardware thread).
If it's the normal thing you call a CPU, I don't think much commentary is
necessary. Then please just make sure you don't call anything else "CPU",
especially in identifiers.
> And then will it be needed to
> add such comment for each usage of word "CPU"?
No, that would go too far in any event. Hence why I said "clarify this in
a prominent comment somewhere".
Jan
© 2016 - 2025 Red Hat, Inc.