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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.03.2026 11:39, Mykyta Poturai wrote:
> On 3/23/26 13:09, Jan Beulich wrote:
>> On 12.03.2026 10:39, Mykyta Poturai wrote:
>>> --- 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.

Well, I would have asked for that if there wasn't a (presumed) use case for
making it optional - certification, where as much unused code as possible
(apparently) wants compiling out. You'll need to ask those doing prep work
for certification whether they'd be interested in it becoming optional.

>>> --- 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;
>      }

I can't spot anything different from what I wrote, so (presumably) yes.

Jan