Re: [PATCH v1 3/4] arm/sysctl: Implement cpu hotplug ops

[Julien Grall <julien@xxxxxxx>]

Hi Mykyta,

On 23/09/2025 14:37, Mykyta Poturai wrote:
> On 18.09.25 16:35, Julien Grall wrote:
> > Hi Mykyta,
> >
> > On 18/09/2025 13:16, Mykyta Poturai wrote:
> > > Implement XEN_SYSCTL_CPU_HOTPLUG_* calls to allow for enabling/disabling
> > > CPU cores in runtime.
> > >
> > > Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> > > ---
> > >    xen/arch/arm/sysctl.c | 67 +++++++++++++++++++++++++++++++++++++++++++
> > >    1 file changed, 67 insertions(+)
> > >
> > > diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
> > > index 32cab4feff..ca8fb550fd 100644
> > > --- a/xen/arch/arm/sysctl.c
> > > +++ b/xen/arch/arm/sysctl.c
> > > @@ -12,6 +12,7 @@
> > >    #include <xen/dt-overlay.h>
> > >    #include <xen/errno.h>
> > >    #include <xen/hypercall.h>
> > > +#include <xen/cpu.h>
> > >    #include <asm/arm64/sve.h>
> > >    #include <public/sysctl.h>
> > > @@ -23,6 +24,68 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> > >                                           
> > > XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
> > >    }
> > > +static long cpu_up_helper(void *data)
> > > +{
> > > +    unsigned long cpu = (unsigned long) data;
> > > +    return cpu_up(cpu);
> > > +}
> > > +
> > > +static long cpu_down_helper(void *data)
> > > +{
> > > +    unsigned long cpu = (unsigned long) data;
> > > +    return cpu_down(cpu);
> > > +}
> > > +
> > > +static long smt_up_down_helper(void *data)
> > Looking at the code, you will effectively disable all the CPUs but CPU0.
> > But I don't understand why. From the name is goal seems to be disable
> > SMT threading.
> Sorry I have slightly misunderstood the x86 implementation/reasoning of
> this ops. I will drop them in V2.
>
> > > +{
> > > +    bool up = (bool) data;
> > > +    unsigned int cpu;
> > > +    int ret;
> > > +
> > > +    for_each_present_cpu ( cpu )
> > > +    {
> > > +        if ( cpu == 0 )
> > > +            continue;
> > > +
> > > +        if ( up )
> > > +            ret = cpu_up(cpu);
> > > +        else
> > > +            ret = cpu_down(cpu);
> > > +
> > Regardless what I wrote above, you likely want to handle preemption.
> >
> > > +        if ( ret )
> > > +            return ret;
> >   > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +static long cpu_hotplug_sysctl(struct xen_sysctl_cpu_hotplug *hotplug)
> > > +{
> > > +    bool up;
> > > +
> > > +    switch (hotplug->op) {
> > > +        case XEN_SYSCTL_CPU_HOTPLUG_ONLINE:
> > > +            if ( hotplug->cpu == 0 )
> > I can't find a similar check on x86. Do you have any pointer?
> Jan correctly mentioned that CPU0 can't be disabled so this is a short
> circuit for clarity.
I have replied to Jan. In short, the clarify you are referring is what 
would make more difficult to support offlining CPU0. So I would rather 
prefer if they are not present.
> > > +            return continue_hypercall_on_cpu(0, cpu_up_helper,
> > > _p(hotplug->cpu));
> > > +
> > > +        case XEN_SYSCTL_CPU_HOTPLUG_OFFLINE:
> > > +            if ( hotplug->cpu == 0 )
> > > +                return -EINVAL;
> > > +            return continue_hypercall_on_cpu(0, cpu_down_helper,
> > > _p(hotplug->cpu));
> > > +
> > > +        case XEN_SYSCTL_CPU_HOTPLUG_SMT_ENABLE:
> > > +        case XEN_SYSCTL_CPU_HOTPLUG_SMT_DISABLE:
> > Why are we implementing those helpers on Arm?
> >
> > > +            if ( CONFIG_NR_CPUS <= 1 )
> > > +                return 0;
> > > +            up = hotplug->op == XEN_SYSCTL_CPU_HOTPLUG_SMT_ENABLE;
> > > +            return continue_hypercall_on_cpu(0, smt_up_down_helper,
> > > _p(up));
> > > +
> > > +        default:
> > > +            return -EINVAL;
> > > +    }
> > > +}
> > > +
> > >    long arch_do_sysctl(struct xen_sysctl *sysctl,
> > >                        XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
> > >    {
> > > @@ -34,6 +97,10 @@ long arch_do_sysctl(struct xen_sysctl *sysctl,
> > >            ret = dt_overlay_sysctl(&sysctl->u.dt_overlay);
> > >            break;
> > > +    case XEN_SYSCTL_cpu_hotplug:
> > This will also enable CPU hotplug on 32-bit Arm. Is this what you
> > intended? (I see patch #4 only mention 64-bit Arm).
> It wasn't intended. I will additionally check if it works on arm32 end
> explicitly specify it.
It will not work properly on arm32 because of the page table code. We 
have per-CPU pagetables (see init_domheap_mappings()) and they will need 
to be freed.
Note this is not a request to add support for arm32 CPU offlining.

Cheers,

--
Julien Grall