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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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