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()?
~ Oleksii
