Re: [PATCH v2 08/16] xen/riscv: dt_processor_cpuid() implementation

[Jan Beulich <jbeulich@xxxxxxxx>]

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<oleksii.kurochko@xxxxxxxxx>
>>> ---
>>> 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
>