Re: [PATCH v1 06/14] xen/riscv: riscv_of_processor_hartid() implementation

[Jan Beulich <jbeulich@xxxxxxxx>]

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