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

[Jan Beulich <jbeulich@xxxxxxxx>]

(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).

> @@ -10,3 +13,66 @@ void __init smp_prepare_boot_cpu(void)
>      cpumask_set_cpu(0, &cpu_possible_map);
>      cpumask_set_cpu(0, &cpu_online_map);
>  }
> +
> +/**
> + * dt_get_cpuid - Get the cpuid from a CPU device node
> + *
> + * @cpun: CPU number(logical index) for which device node is required
> + *
> + * Return: The cpuid for the CPU node or ~0ULL if not found.
> + */
> +static unsigned long dt_get_cpuid(const struct dt_device_node *cpun)
> +{
> +    const __be32 *cell;
> +    int ac;

This is bogus (should be unsigned int afaict), but dictated by ...

> +    uint32_t len;
> +
> +    ac = dt_n_addr_cells(cpun);

... the return value here and ...

> +    cell = dt_get_property(cpun, "reg", &len);
> +    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )
> +        return ~0ULL;

(Nit: This doesn't match the return type of the function; same for
the function comment. Also, what if sizeof(*cell) * ac < len?)

> +    return dt_read_number(cell, ac);

... the function parameter type here. In fact, that function is raising
another question: If the "size" argument is outside of [0, 2], the value
returned is silently truncated.

More generally - are there any plans to make DT code signed-ness-correct?

> +/*
> + * 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?

Jan