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

[Julien Grall <julien@xxxxxxx>]

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

> +                return -EINVAL;
On x86, they seem to check for XSM permission before continuing. Can you 
explain why this is not necessary? Same questions applies below.
> +            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).
> +        ret = cpu_hotplug_sysctl(&sysctl->u.cpu_hotplug);
> +        break;
> +
>       default:
>           ret = -ENOSYS;
>           break;
Cheers,

--
Julien Grall