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

[Mykyta Poturai <Mykyta_Poturai@xxxxxxxx>]

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.

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

I will add XSM checks thanks for pointing this out.

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

> 
>> +        ret = cpu_hotplug_sysctl(&sysctl->u.cpu_hotplug);
>> +        break;
>> +
>>       default:
>>           ret = -ENOSYS;
>>           break;
> 
> Cheers,
> 

-- 
Mykyta