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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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