(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.kuroc...@gmail.com>
> ---
> 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
