Re: [PATCH v2 06/26] xen/riscv: implement make_cpus_node()

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/18/26 6:00 PM, Jan Beulich wrote:
> On 08.05.2026 16:43, Oleksii Kurochko wrote:
> > @@ -50,3 +52,107 @@ int __init construct_domain(struct domain *d, struct 
> > kernel_info *kinfo)
> >   
> >       return 0;
> >   }
> > +int __init make_cpus_node(const struct domain *d, struct kernel_info *kinfo)
> Nit: Blank line above here, please.
>
> > +{
> > +    int res;
> > +    const struct dt_device_node *cpus = dt_find_node_by_path("/cpus");
> > +    unsigned int cpu;
> > +    uint32_t timebase_frequency;
> > +    bool frequency_valid;
> > +    void *fdt = kinfo->fdt;
> > +
> > +    dt_dprintk("Create cpus node\n");
> > +
> > +    if ( !cpus )
> > +    {
> > +        dprintk(XENLOG_ERR, "Missing /cpus node in the device tree?\n");
> > +        return -ENOENT;
> > +    }
> > +
> > +    frequency_valid = dt_property_read_u32(cpus, "timebase-frequency",
> > +                                           &timebase_frequency);
> > +
> > +    res = fdt_begin_node(fdt, "cpus");
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property_cell(fdt, "#address-cells", 1);
> > +    if ( res )
> > +        return res;
> > +
> > +    res = fdt_property_cell(fdt, "#size-cells", 0);
> > +    if ( res )
> > +        return res;
> > +
> > +    if ( frequency_valid )
> > +        res = fdt_property_cell(fdt, "timebase-frequency", timebase_frequency);
> > +
> > +    for ( cpu = 0; cpu < d->max_vcpus; cpu++ )
> Limit cpu's scope to this loop?
I will remove 'unsigned int cpu' from the function-level declarations 
and moved it into the for loop.
> > +    {
> > +        char buf[64];
> > +        uint32_t reg = cpu_to_fdt32(cpu);
> Isn't this a byte-order adjustment? If so, how come ...
>
> > +        snprintf(buf, sizeof(buf), "cpu@%u", cpu);
> ... the result is passed to an entirely non-FDT function? (Most pre-existing 
> uses
> of the function that I can spot store something in memory, i.e. adjusting byte-
> order makes sense there.)
But here pure cpu is used instead of reg variable and reg variable is 
used here ...
> > +        res = fdt_begin_node(fdt, buf);
> > +        if ( res )
> > +            return res;
> > +
> > +        res = fdt_property(fdt, "reg", &reg, sizeof(reg));
> > +        if ( res )
> > +            return res;
... but it we could drop it and use just:
  res = fdt_property_cell(fdt, "reg", cpu);

fdt_property_cell will take care if a byte-order adjustment.

> > +
> > +        res = fdt_property_string(fdt, "status", "okay");
> > +        if ( res )
> > +            return res;
> > +
> > +        res = fdt_property_string(fdt, "compatible", "riscv");
> > +        if ( res )
> > +            return res;
> > +
> > +        BUILD_BUG_ON((sizeof("riscv,") +
> > +                      sizeof_field(struct gstage_mode_desc, name)) >= 
> > sizeof(buf));
> > +        snprintf(buf, sizeof(buf), "riscv,%s", max_gstage_mode->name);
> > +        res = fdt_property_string(fdt, "mmu-type", buf);
> > +        if ( res )
> > +            return res;
> > +
> > +        res = fdt_property_string(fdt, "riscv,isa", d->arch.guest_isa_str);
> > +        if ( res )
> > +            return res;
> > +
> > +        res = fdt_property_string(fdt, "device_type", "cpu");
> > +        if ( res )
> > +            return res;
> > +
> > +        res = fdt_begin_node(fdt, "interrupt-controller");
> > +        if ( res )
> > +            return res;
> > +
> > +        res = fdt_property_string(fdt, "compatible", "riscv,cpu-intc");
> > +        if ( res )
> > +            return res;
> > +
> > +        res = fdt_property_cell(fdt, "#interrupt-cells", 1);
> > +        if ( res )
> > +            return res;
> > +
> > +        res = fdt_property(fdt, "interrupt-controller", NULL, 0);
> > +        if ( res )
> > +            return res;
> > +
> > +        res = fdt_property_u32(fdt, "phandle", alloc_phandle(kinfo));
> > +        if ( res )
> > +            return res;
> > +
> > +        /* end of interrupt-controller */
> Nit: Comment style. Also such a comment pretty clearly calls for a counterpart
> at the start.
Sure, I will add one.

Thanks.

~ Oleksii