Re: [PATCH v6 3/5] arm/sysctl: Implement cpu hotplug ops

[Mykyta Poturai <Mykyta_Poturai@xxxxxxxx>]

On 3/23/26 13:09, Jan Beulich wrote:
> On 12.03.2026 10:39, Mykyta Poturai wrote:
>> --- a/xen/arch/arm/smp.c
>> +++ b/xen/arch/arm/smp.c
>> @@ -44,6 +44,15 @@ void smp_send_call_function_mask(const cpumask_t *mask)
>>       }
>>   }
>>   
>> +/*
>> + * We currently don't support SMT on ARM so we don't need any special logic 
>> for
>> + * CPU disabling
>> + */
>> +bool arch_cpu_can_stay_online(unsigned int cpu)
>> +{
>> +    return true;
>> +}
> 
> Something as simple as this would be nice to be an inline function (or, less
> desirably, a macro).
> 
>> --- a/xen/arch/x86/platform_hypercall.c
>> +++ b/xen/arch/x86/platform_hypercall.c
>> @@ -735,6 +735,12 @@ ret_t do_platform_op(
>>       {
>>           int cpu = op->u.cpu_ol.cpuid;
>>   
>> +        if ( !IS_ENABLED(CONFIG_CPU_HOTPLUG) )
>> +        {
>> +            ret = -EOPNOTSUPP;
>> +            break;
>> +        }
>> +
>>           ret = xsm_resource_plug_core(XSM_HOOK);
>>           if ( ret )
>>               break;
>> @@ -761,6 +767,12 @@ ret_t do_platform_op(
>>       {
>>           int cpu = op->u.cpu_ol.cpuid;
>>   
>> +        if ( !IS_ENABLED(CONFIG_CPU_HOTPLUG) )
>> +        {
>> +            ret = -EOPNOTSUPP;
>> +            break;
>> +        }
>> +
>>           ret = xsm_resource_unplug_core(XSM_HOOK);
>>           if ( ret )
>>               break;
> 
> I wonder whether on x86 this really should become an optional thing (and
> if so, whether that wouldn't better be a separate change with proper
> justification). See also the comment on common/Kconfig further down - by
> the name of the option, and given the support status the change above may
> be legitimate, but not some of the similar restrictions added elsewhere.
> 

Maybe force it to be always on like x86 then? I don't really have a 
justification for making it optional on x86, it just happened as side 
effect of creating a config option.

>> --- a/xen/arch/x86/smp.c
>> +++ b/xen/arch/x86/smp.c
>> @@ -418,35 +418,8 @@ void cf_check call_function_interrupt(void)
>>       smp_call_function_interrupt();
>>   }
>>   
>> -long cf_check cpu_up_helper(void *data)
>> +bool arch_cpu_can_stay_online(unsigned int cpu)
>>   {
>> -    unsigned int cpu = (unsigned long)data;
>> -    int ret = cpu_up(cpu);
>> -
>> -    /* Have one more go on EBUSY. */
>> -    if ( ret == -EBUSY )
>> -        ret = cpu_up(cpu);
>> -
>> -    if ( !ret && !opt_smt &&
>> -         cpu_data[cpu].compute_unit_id == INVALID_CUID &&
>> -         cpumask_weight(per_cpu(cpu_sibling_mask, cpu)) > 1 )
>> -    {
>> -        ret = cpu_down_helper(data);
>> -        if ( ret )
>> -            printk("Could not re-offline CPU%u (%d)\n", cpu, ret);
>> -        else
>> -            ret = -EPERM;
>> -    }
>> -
>> -    return ret;
>> -}
>> -
>> -long cf_check cpu_down_helper(void *data)
>> -{
>> -    int cpu = (unsigned long)data;
>> -    int ret = cpu_down(cpu);
>> -    /* Have one more go on EBUSY. */
>> -    if ( ret == -EBUSY )
>> -        ret = cpu_down(cpu);
>> -    return ret;
>> +    return opt_smt || cpu_data[cpu].compute_unit_id != INVALID_CUID ||
>> +           cpumask_weight(per_cpu(cpu_sibling_mask, cpu)) <= 1;
>>   }
> 
> Unlike for Arm, this may indeed better be an out-of-line function.
> 
>> --- a/xen/arch/x86/sysctl.c
>> +++ b/xen/arch/x86/sysctl.c
>> @@ -49,6 +49,7 @@ static void cf_check l3_cache_get(void *arg)
>>   
>>   static long cf_check smt_up_down_helper(void *data)
>>   {
>> +    #ifdef CONFIG_CPU_HOTPLUG
>>       bool up = (bool)data;
>>       unsigned int cpu, sibling_mask = boot_cpu_data.x86_num_siblings - 1;
>>       int ret = 0;
>> @@ -89,6 +90,8 @@ static long cf_check smt_up_down_helper(void *data)
>>                  up ? "enabled" : "disabled", CPUMASK_PR(&cpu_online_map));
>>   
>>       return ret;
>> +    #endif /* CONFIG_CPU_HOTPLUG */
>> +    return 0;
>>   }
> 
> The #-es or pre-processor directives want to be in the very first column.
> 
> Sharing "return ret" would also be nice, imo. Would require ret's decl to
> move ahead of the #ifdef. Actually - is there anything preventing
> 
>      if ( !IS_ENABLED(CONFIG_CPU_HOTPLUG) )
>          return 0;
> 
> at the top of the function? Perhaps even with ASSERT_UNREACHABLE() added
> in?
> 
>> @@ -115,24 +118,24 @@ long arch_do_sysctl(
>>   
>>       case XEN_SYSCTL_cpu_hotplug:
>>       {
>> -        unsigned int cpu = sysctl->u.cpu_hotplug.cpu;
>>           unsigned int op  = sysctl->u.cpu_hotplug.op;
>>           bool plug;
>>           long (*fn)(void *data);
>>           void *hcpu;
>>   
>> -        switch ( op )
>> +        if ( !IS_ENABLED(CONFIG_CPU_HOTPLUG) )
>>           {
>> -        case XEN_SYSCTL_CPU_HOTPLUG_ONLINE:
>> -            plug = true;
>> -            fn = cpu_up_helper;
>> -            hcpu = _p(cpu);
>> +            ret = -EOPNOTSUPP;
>>               break;
> 
> ASSERT_UNREACHABLE() looks to also be valid to be added here, seeing how
> do_sysctl() now works.
> 
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -637,6 +637,12 @@ config SYSTEM_SUSPEND
>>   
>>        If unsure, say N.
>>   
>> +config CPU_HOTPLUG
>> +    bool "Enable CPU hotplug"
> 
> I'm not happy with this prompt. For x86 SUPPORT.md declares (ACPI) CPU
> hotplug as experimental. That's physical hotplug. The code you're
> fiddling with, however, is also used for soft-{off,on}lining. Which,
> e.g. to disable SMT on x86, may need to be used for security purposes.
> 
>> +    depends on (X86 || ARM_64) && !FFA && !TEE && !HAS_ITS
> 
> What if on x86 FFA, TEE, or ITS gain a meaning?
> 
>> +    default y
>> +
>> +
> 
> Nit: No double blank lines please.
> 
>> --- a/xen/common/sysctl.c
>> +++ b/xen/common/sysctl.c
>> @@ -483,6 +483,52 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
>> u_sysctl)
>>               copyback = 1;
>>           break;
>>   
>> +    case XEN_SYSCTL_cpu_hotplug:
>> +    {
>> +        unsigned int cpu = op->u.cpu_hotplug.cpu;
> 
> I don't think this variable is very useful to keep. Instead use ...
> 
>> +        unsigned int hp_op = op->u.cpu_hotplug.op;
>> +        bool plug;
>> +        long (*fn)(void *data);
>> +        void *hcpu;
> 
>          void *hcpu = _p(op->u.cpu_hotplug.op);
> 
> right here, dropping the assignments further down.
> 
>> +        ret = -EOPNOTSUPP;
>> +        if ( !IS_ENABLED(CONFIG_CPU_HOTPLUG) )
>> +            break;
>> +
>> +        switch ( hp_op )
>> +        {
>> +        case XEN_SYSCTL_CPU_HOTPLUG_ONLINE:
>> +            plug = true;
>> +            fn = cpu_up_helper;
>> +            hcpu = _p(cpu);
>> +            break;
>> +
>> +        case XEN_SYSCTL_CPU_HOTPLUG_OFFLINE:
>> +            plug = false;
>> +            fn = cpu_down_helper;
>> +            hcpu = _p(cpu);
>> +            break;
>> +
>> +        default:
>> +            fn = NULL;
>> +            break;
>> +        }
>> +
>> +        if ( fn )
>> +        {
>> +            ret = plug ? xsm_resource_plug_core(XSM_HOOK)
>> +                       : xsm_resource_unplug_core(XSM_HOOK);
>> +
>> +            if ( !ret )
>> +                ret = continue_hypercall_on_cpu(0, fn, hcpu);
>> +
>> +            break;
>> +        }
>> +
>> +        /* Use the arch handler for cases not handled here */
>> +        fallthrough;
>> +    }
>> +
>>       default:
>>           ret = arch_do_sysctl(op, u_sysctl);
>>           copyback = 0;
> 
> This form of falling through may be a little risky, towards someone not
> looking closely enough and inserting another case label immediately ahead
> of the default one. While I don't think there's a really good solution to
> this, please consider
> 
>      }
>          /* Use the arch handler for cases not handled above */
>          fallthrough;
>      default:
> 
> instead.
> 

Just want to clarirfy if I got the idea. Is this what you meant?

     switch ( op->cmd )
     {
     ....
     case XEN_SYSCTL_cpu_hotplug:
     {
     ....
     }

         /* Use the arch handler for cases not handled here */
         fallthrough;
     default:
         ret = arch_do_sysctl(op, u_sysctl);
         copyback = 0;
         break;
     }


>> --- a/xen/xsm/flask/hooks.c
>> +++ b/xen/xsm/flask/hooks.c
>> @@ -835,7 +835,7 @@ static int cf_check flask_sysctl(int cmd)
>>       case XEN_SYSCTL_getdomaininfolist:
>>       case XEN_SYSCTL_page_offline_op:
>>       case XEN_SYSCTL_scheduler_op:
>> -#ifdef CONFIG_X86
>> +#ifdef CONFIG_CPU_HOTPLUG
>>       case XEN_SYSCTL_cpu_hotplug:
>>   #endif
>>           return 0;
> 
> Is there a reason the #ifdef can't simply be dropped?
> 
> Jan

-- 
Mykyta