Re: [PATCH v3 06/14] xen/riscv: dt_processor_hartid() implementation

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/26/25 12:46 PM, Oleksii Kurochko wrote:

On 5/22/25 9:50 AM, Jan Beulich wrote:

On 21.05.2025 18:03, Oleksii Kurochko wrote:

--- a/xen/arch/riscv/smpboot.c
+++ b/xen/arch/riscv/smpboot.c
@@ -1,5 +1,8 @@
 #include <xen/cpumask.h>
+#include <xen/device_tree.h>
+#include <xen/errno.h>
 #include <xen/init.h>
+#include <xen/types.h>
 #include <xen/sections.h>

Nit: The latter insertion wants to move one line down. Yet then - isn't
xen/stdint.h sufficient here?

__be32 used in dt_get_hartid() is defined in xen/types.h.

@@ -14,3 +17,69 @@ 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;
+
+    ac = dt_n_addr_cells(cpun);
+    cell = dt_get_property(cpun, "reg", &len);
+    if ( !cell || !ac || ((sizeof(*cell) * ac) > len) )

Does DT make any guarantees for this multiplication to not overflow?

I haven't tried of DTC checks such things during compilation but considering that
ac value is uin32_t value (according to DT spec) then overflow could really happen.
I will add the following to check an overflow:
    if ( ac > ((sizeof(size_t) * BIT_PER_BYTE) / sizeof(*cell)) )
    {
        printk("%s: overflow detected\n", __func__);
        return ~0UL;
    }

Oops, I meant here size_t_max instead of (sizeof(size_t) * BIT_PER_BYTE), lost power of 2 minus 1.
Probably, SIZE_T_MAX or something similar exists. I have to check.

~ Oleksii
> > > +        return ~0UL;
> > > +
> > > +    return dt_read_number(cell, ac);
> > > +}
> > > +
> > > +/*
> > > + * Returns the hartid of the given device tree node, or -ENODEV if the node
> > > + * isn't an enabled and valid RISC-V hart node.
> > > + */
> > > +int dt_processor_hartid(const struct dt_device_node *node,
> > > +                        unsigned long *hartid)
> > > +{
> > > +    const char *isa;
> > > +    int ret;
> > > +
> > > +    if ( !dt_device_is_compatible(node, "riscv") )
> > > +    {
> > > +        printk("Found incompatible CPU\n");
> > > +        return -ENODEV;
> > > +    }
> > > +
> > > +    *hartid = dt_get_hartid(node);
> > > +    if ( *hartid == ~0UL )
> > > +    {
> > > +        printk("Found CPU without CPU ID\n");
> > > +        return -ENODATA;
> > > +    }
> > > +
> > > +    if ( !dt_device_is_available(node))
> > > +    {
> > > +        printk("CPU with hartid=%lu is not available\n", *hartid);
> > >         
> > Considering that hart ID assignment is outside of our control, would we
> > perhaps better (uniformly) log such using %#lx?
> >       
> It makes sense, DTC when generates dts from dtb also uses hex number, so it could
> help to find a failure node faster.
>
> > > +        return -ENODEV;
> > > +    }
> > > +
> > > +    if ( (ret = dt_property_read_string(node, "riscv,isa", &isa)) )
> > > +    {
> > > +        printk("CPU with hartid=%lu has no \"riscv,isa\" property\n", *hartid);
> > > +        return ret;
> > > +    }
> > > +
> > > +    if ( isa[0] != 'r' || isa[1] != 'v' )
> > > +    {
> > > +        printk("CPU with hartid=%lu has an invalid ISA of \"%s\"\n", *hartid,
> > > +               isa);
> > > +        return -EINVAL;
> > >         
> > As before -EINVAL is appropriate when input arguments have wrong values.
> > Here, however, you found an unexpected value in something the platform
> > has presented to you. While not entirely appropriate either, maybe
> > -ENODEV again (if nothing better can be found)?
> >       
> I don't see better candidate.
>
> Interesting if some reserved region exists for user
> defined errors.
>
> ~ Oleksii
>