Re: [PATCH v4 1/9] xen/riscv: dt_processor_hartid() implementation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.06.2025 17:58, Oleksii Kurochko wrote:
> @@ -14,3 +17,77 @@ void __init smp_prepare_boot_cpu(void)
>      cpumask_set_cpu(0, &cpu_possible_map);
>      cpumask_set_cpu(0, &cpu_online_map);
>  }
> +
> +/**
> + * dt_get_hartid - Get the hartid from a CPU device node
> + *
> + * @cpun: CPU number(logical index) for which device node is required
> + *
> + * Return: The hartid for the CPU node or ~0UL if not found.
> + */
> +static unsigned long dt_get_hartid(const struct dt_device_node *cpun)
> +{
> +    const __be32 *cell;
> +    unsigned int ac;
> +    uint32_t len;
> +    unsigned int max_cells = UINT32_MAX / sizeof(*cell);
> +
> +    ac = dt_n_addr_cells(cpun);
> +    cell = dt_get_property(cpun, "reg", &len);
> +
> +    if (ac > max_cells) {

Besides the (double) style issue, why's this needed? Can't you simply ...

> +        printk("%s: cell count overflow (ac=%u, max=%u)\n", __func__, ac,
> +               max_cells);
> +        return ~0UL;
> +    }
> +
> +    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )

... write the last part here in a way that there can't be overflow?
ac > len / sizeof(*cell) that is? (Remaining question then is what to
do when len isn't evenly divisible by sizeof(*cell).)

> +        return ~0UL;
> +
> +    return dt_read_number(cell, ac);

What meaning does this have for ac > 2? (As per your checking above
it can be up to UINT32_MAX / 4.)

Jan