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
