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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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