Re: [PATCH v1 06/27] xen/riscv: implement make_cpus_node()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.03.2026 18:08, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/Makefile
> +++ b/xen/arch/riscv/Makefile
> @@ -1,6 +1,7 @@
>  obj-y += aplic.o
>  obj-y += cpufeature.o
>  obj-y += domain.o
> +obj-y += domain-build.o
>  obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
>  obj-y += entry.o
>  obj-y += guestcopy.o

Doesn't this belong in patch 02? And then domain-build.init.o?

> --- a/xen/arch/riscv/domain-build.c
> +++ b/xen/arch/riscv/domain-build.c
> @@ -3,8 +3,10 @@
>  #include <xen/fdt-domain-build.h>
>  #include <xen/fdt-kernel.h>
>  #include <xen/init.h>
> +#include <xen/libfdt/libfdt.h>
>  #include <xen/sched.h>
>  
> +#include <asm/cpufeature.h>
>  #include <asm/current.h>
>  #include <asm/guest_access.h>
>  
> @@ -44,3 +46,109 @@ int __init construct_domain(struct domain *d, struct 
> kernel_info *kinfo)
>  
>      return 0;
>  }
> +
> +int __init make_cpus_node(const struct domain *d, void *fdt)
> +{
> +    int res;
> +    const struct dt_device_node *cpus = dt_find_node_by_path("/cpus");
> +    unsigned int cpu;
> +    u32 timebase_frequency;

uint32_t please.

> +    bool frequency_valid;
> +    uint32_t *next_phandle = &((struct domain *)d)->arch.next_phandle;

No casting away of const, please.

> +    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);

Handing through a property directly makes me wonder how that's going to
fit with migration. I understand migration may not even be a mid-term
goal, but still.

> +    for ( cpu = 0; cpu < d->max_vcpus; cpu++ )
> +    {
> +        char buf[64];
> +        uint32_t reg = cpu_to_fdt32(cpu);
> +
> +        snprintf(buf, sizeof(buf), "cpu@%u", cpu);
> +        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) + 1) >= sizeof(buf));

Nit: Overlong line. Also, why +1? The name field has to include a nul, or
else ...

> +        snprintf(buf, sizeof(buf), "riscv,%s", max_gstage_mode->name);

... you can't use plain %s here.

> +        res = fdt_property_string(fdt, "mmu-type", buf);
> +        if ( res )
> +            return res;
> +
> +        res = fdt_property_string(fdt, "riscv,isa", 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", (*next_phandle)++);
> +        if ( res )
> +            return res;
> +
> +        /* end of interrupt-controller */
> +        res = fdt_end_node(fdt);
> +        if ( res )
> +            return res;
> +
> +        res = fdt_end_node(fdt);
> +        if ( res )
> +            return res;
> +    }
> +
> +    res = fdt_end_node(fdt);
> +
> +    return res;

Simply "return fdt_end_node(fdt);"?

Jan