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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> +    {
> +        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.)

> +        res = fdt_begin_node(fdt, buf);
> +        if ( res )
> +            return res;
> +
> +        res = fdt_property(fdt, "reg", &reg, sizeof(reg));
> +        if ( res )
> +            return res;
> +
> +        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.

> +        res = fdt_end_node(fdt);
> +        if ( res )
> +            return res;
> +
> +        res = fdt_end_node(fdt);
> +        if ( res )
> +            return res;
> +    }
> +
> +    return fdt_end_node(fdt);
> +}