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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/1/26 4:11 PM, Jan Beulich wrote:
> 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?
Yes, it should be part of patch 02. Made a mistake during splitting.

> > --- 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.
Do you mean if I will set timebase-frequency = X for guest cpu node and 
then this guest will migrate to h/w where timebase-frequency is Y, so we 
will have unsynced timebase-frequency?
Migration between hosts with differing timebase-frequency would require 
either (a) restricting migration pools to frequency-matched hosts (for 
example, KVM checks if timebase-frequency isn't different here:
https://elixir.bootlin.com/linux/v6.19.11/source/arch/riscv/kvm/vcpu_timer.c#L200), 
or (b) trap-and-emulate of time CSR reads to scale the virtual timer.
I think so as unlike ARM (which has CNTFRQ_EL0 writable by EL2, letting 
the hypervisor normalize the frequency it presents), RISC-V has no 
hypervisor-controlled frequency register. The guest reads 
timebase-frequency from DT exactly once and trusts it forever. There's 
no in-guest mechanism to update it post-boot. (at least, I don't see now 
how to do that based on the spec)

> > +    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 ...
Agree, +1 looks wrong here.

Thanks.

~ Oleksii