On 11.06.2025 10:26, Oleksii Kurochko wrote:
>
> On 6/10/25 4:08 PM, Jan Beulich wrote:
>> 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).)
>
> reg property should be always evenly divisible by sizeof(*cell) according to
> device
> tree binding:
> The reg property describes the address of the device’s resources within
> the address space defined by its parent bus. Most commonly this means
> the offsets and lengths of memory-mapped IO register blocks, but may
> have a different meaning on some bus types. Addresses in the address
> space defined by the root node are CPU real addresses.
>
> The value is a <prop-encoded-array>, composed of an arbitrary number of
> pairs of address and length, <address length>. The number of <u32> cells
> required to specify the address and length are bus-specific and are
> specified by the #address-cells and #size-cells properties in the parent
> of the device node. If the parent node specifies a value of 0 for
> #size-cells, the length field in the value of reg shall be omitted. So
> it is guaranteed by DTC compiler and it would be enough to check
> overflow in suggested by you way: ac > len / sizeof(*cell)
> But considering what you noticed below ...
>
>>
>>> + 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.)
>
> ... It will be an issue for dt_read_number() which could deal only with
> uint64_t what means
> we can't have ac > 2. (UINT32_MAX / 4 it is a theoretical maximum IIUC)
>
> Thereby we could do in the following way:
> @@ -30,19 +30,18 @@ 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) {
> - printk("%s: cell count overflow (ac=%u, max=%u)\n", __func__, ac,
> - max_cells);
> + if ( !cell || !ac || (ac > len / sizeof(*cell)) )
> return ~0UL;
> - }
>
> - if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )
> - return ~0UL;
> + /*
> + * If ac > 2, the result may be truncated or meaningless unless
> + * dt_read_number() supports wider integers.
> + */
> + BUG_ON(ac > 2);
>
> return dt_read_number(cell, ac);
> }
>
> I am not sure that BUG_ON() should be in dt_get_hartid(). Probably it would
> be better move it
> to dt_read_number() as if one day support for RV128 will be needed I assume
> that it will be
> needed to change a prototype of dt_read_number() to work with address-cells =
> 3.
> What do you think? Could I go with the suggested above changes or it would be
> better to move
> BUG_ON() to dt_read_number()?
Don't know; the DT maintainers would have to judge. I don't, however, think it
should
be BUG_ON() - as said several times before, that's a check suitable to cover for
possible mistakes in Xen code. Here however you're trying to cover for a flaw
in DT.
Jan
