On 16.05.2025 18:02, Oleksii Kurochko wrote:
>
> On 5/15/25 9:56 AM, Jan Beulich wrote:
>> (adding Bertrand as the one further DT maintainer, for a respective question
>> below)
>>
>> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>>> Implements dt_processor_hartid()
>> There's no such function (anymore).
>>
>>> 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 dt_get_cpuid() is introduced to deal specifically
>>> with reg propery of a CPU device node.
>>>
>>> Signed-off-by: Oleksii Kurochko<[email protected]>
>>> ---
>>> Changes in V2:
>>> - s/of_get_cpu_hwid()/dt_get_cpu_id().
>>> - Update prototype of dt_get_cpu_hwid(), use pointer-to-const for cpun
>>> arg.
>>> - Add empty line before last return in dt_get_cpu_hwid().
>>> - s/riscv_of_processor_hartid/dt_processor_cpuid().
>>> - Use pointer-to_const for node argument of dt_processor_cpuid().
>>> - Use for hart_id unsigned long type as according to the spec for RV128
>>> mhartid register will be 128 bit long.
>>> - Update commit message and subject.
>>> - use 'CPU' instead of 'HART'.
>> Was this is good move? What is returned ...
>>
>>> --- 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 dt_processor_cpuid(const struct dt_device_node *node, unsigned long
>>> *cpuid);
>> ... here isn't a number in Xen's CPU numbering space. From earlier
>> discussions I'm
>> not sure it's a hart ID either, so it may need further clarification (and I'd
>> expect RISC-V to have suitable terminology to tell apart the different
>> entities).
>
> From device tree point of view
> (https://www.kernel.org/doc/Documentation/devicetree/bindings/riscv/cpus.txt)
> it is just hart which is defined as a hardware execution context, which
> contains all the state mandated by
> the RISC-V ISA: a PC and some registers.
> And also based on this for DT binding:
> For example, my Intel laptop is
> described as having one socket with two cores, each of which has two hyper
> threads. Therefore this system has four harts.
> They are using just harts and in DT it will look like 4 node with a number in
> reg property which they
> call hart. So from DT point of view hartid is pretty fine to use.
>
> From specification point of view
> (https://riscv.github.io/riscv-isa-manual/snapshot/unprivileged/#_risc_v_hardware_platform_terminology):
> A RISC-V hardware platform can contain one or more RISC-V-compatible
> processing cores together
> with other non-RISC-V-compatible cores, fixed-function accelerators,
> various physical memory
> structures, I/O devices, and an interconnect structure to allow the
> components to communicate.
>
> 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.
> and
> (https://riscv.github.io/riscv-isa-manual/snapshot/unprivileged/#_risc_v_software_execution_environments_and_harts):
> From the perspective of software running in a given execution environment,
> a hart is a resource that
> autonomously fetches and executes RISC-V instructions within that
> execution environment. In this
> respect, a hart behaves like a hardware thread resource even if
> time-multiplexed onto real hardware by
> the execution environment. Some EEIs support the creation and destruction
> of additional harts, for
> example, via environment calls to fork new harts.
>
> DT's CPU node should be refer to core plus hardware thread (or threads in
> case of multithreading)
> in reg property to be close to the spec(?) but basically in DT they IMO just
> describe what in the sepc
> is called an independent instruction fetch unit.
>
> I still think that it is fine just to use hart_id. If to be close more to a
> spec the unit_id(?)
> but it is more confusing IMO with what is use in RISC-V's DT binding.
Based on what you quoted above I think "hart ID" is indeed appropriate here.
>>> +/*
>>> + * Returns the cpuid of the given device tree node, or -ENODEV if the node
>>> + * isn't an enabled and valid RISC-V hart node.
>>> + */
>>> +int dt_processor_cpuid(const struct dt_device_node *node, unsigned long
>>> *cpuid)
>>> +{
>>> + const char *isa;
>>> +
>>> + if ( !dt_device_is_compatible(node, "riscv") )
>>> + {
>>> + printk("Found incompatible CPU\n");
>>> + return -ENODEV;
>>> + }
>>> +
>>> + *cpuid = dt_get_cpuid(node);
>>> + if ( *cpuid == ~0UL )
>>> + {
>>> + printk("Found CPU without CPU ID\n");
>>> + return -ENODEV;
>>> + }
>>> +
>>> + if ( !dt_device_is_available(node))
>>> + {
>>> + printk("CPU with cpuid=%lu is not available\n", *cpuid);
>>> + return -ENODEV;
>>> + }
>>> +
>>> + if ( dt_property_read_string(node, "riscv,isa", &isa) )
>>> + {
>>> + printk("CPU with cpuid=%lu has no \"riscv,isa\" property\n",
>>> *cpuid);
>>> + return -ENODEV;
>>> + }
>>> +
>>> + if ( isa[0] != 'r' || isa[1] != 'v' )
>>> + {
>>> + printk("CPU with cpuid=%lu has an invalid ISA of \"%s\"\n",
>>> *cpuid, isa);
>>> + return -ENODEV;
>>> + }
>>> +
>>> + return 0;
>>> +}
>> I view it as unhelpful that all errors result in -ENODEV. Yes, there are log
>> messages for all of the cases, but surely there are errno values better
>> representing the individual failure reasons?
>
> I will update that to:
>
> @@ -46,6 +46,7 @@ static unsigned long dt_get_cpuid(const struct
> dt_device_node *cpun)
> int dt_processor_cpuid(const struct dt_device_node *node, unsigned long
> *cpuid)
> {
> const char *isa;
> + int ret;
>
> if ( !dt_device_is_compatible(node, "riscv") )
> {
> @@ -57,7 +58,7 @@ int dt_processor_cpuid(const struct dt_device_node *node,
> unsigned long *cpuid)
> if ( *cpuid == ~0UL )
> {
> printk("Found CPU without CPU ID\n");
> - return -ENODEV;
> + return -EINVAL;
EINVAL is the most commonly used error code; I'd therefore recommend to
avoid its use unless it is indeed properly describing the situation
(invalid argument, i.e. invalid value _passed in_). Here ENODATA might
be a suitable replacement, for example.
Jan
> }
>
> if ( !dt_device_is_available(node))
> @@ -66,16 +67,16 @@ int dt_processor_cpuid(const struct dt_device_node *node,
> unsigned long *cpuid)
> return -ENODEV;
> }
>
> - if ( dt_property_read_string(node, "riscv,isa", &isa) )
> + if ( (ret = dt_property_read_string(node, "riscv,isa", &isa)) )
> {
> printk("CPU with cpuid=%lu has no \"riscv,isa\" property\n",
> *cpuid);
> - return -ENODEV;
> + return ret;
> }
>
> if ( isa[0] != 'r' || isa[1] != 'v' )
> {
> printk("CPU with cpuid=%lu has an invalid ISA of \"%s\"\n", *cpuid,
> isa);
> - return -ENODEV;
> + return -EINVAL;
> }
>
> return 0;
>
> I think it's better now.
>
> Thanks.
>
> ~ Oleksii
>
