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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.02.2026 11:30, Mykyta Poturai wrote:
> On 14.01.26 11:49, Jan Beulich wrote:
>> On 13.01.2026 09:45, Mykyta Poturai wrote:
>>> Move XEN_SYSCTL_CPU_HOTPLUG_{ONLINE,OFFLINE} handlers to common code to
>>> allow for enabling/disabling CPU cores in runtime on Arm64.
>>>
>>> SMT-disable enforcement check is moved into a separate
>>> architecture-specific function.
>>>
>>> For now this operations only support Arm64. For proper Arm32 support,
>>> there needs to be a mechanism to free per-cpu page tables, allocated in
>>> init_domheap_mappings.  Also, hotplug is not supported if ITS, FFA, or
>>> TEE is enabled, as they use non-static IRQ actions.
>>
>> For all of these "not supported" cases, what if a user nevertheless tries?
>> Wouldn't the request better be outright denied, rather leaving the system in
>> a questionable state? Hmm, I see you ...
>>
>>> --- a/xen/arch/arm/Kconfig
>>> +++ b/xen/arch/arm/Kconfig
>>> @@ -7,6 +7,7 @@ config ARM_64
>>>     def_bool y
>>>     depends on !ARM_32
>>>     select 64BIT
>>> +   select CPU_HOTPLUG if !TEE && !FFA && !HAS_ITS
>>
>> ... make the select conditional. But do TEE, FFA, and HAS_ITS each mean the
>> feature is actually in use when the hypervisor runs?
>>
> The way interrupts are requested in these modules causes Xen to crash 
> when trying to offline a cpu. It’s a fairly simple fix and I plan to 
> send them eventually. I’ve decided to omit them now and do these fixes 
> only for supported code to keep the series from ballooning with too many 
> patches.

I disagree with such an approach, but it'll be the Arm maintainers to judge 
here.

>>> +    int ret = cpu_up(cpu);
>>> +
>>> +    /* Have one more go on EBUSY. */
>>> +    if ( ret == -EBUSY )
>>> +        ret = cpu_up(cpu);
>>> +
>>> +    if ( !ret && arch_smt_cpu_disable(cpu) )
>>
>> As you validly note in a comment in do_sysctl(), SMT is an arch-specific 
>> concept
>> and perhaps even an arch-specific term. Hence using it in the name of an arch
>> hook feels inappropriate. Plus - the hook could be used for other purposes. 
>> What
>> the arch needs to indicate is whether the CPU that was brought up may 
>> actually
>> stay online. That more generic purpose is what imo the name wants to cover.
> 
> Would arch_cpu_online_allowed() be okay, or maybe you have something 
> more specific in mind?

The name is already much better, just that it gives the impression that it 
perhaps
rather would want using ahead of calling cpu_up().

>>> +        case XEN_SYSCTL_CPU_HOTPLUG_SMT_ENABLE:
>>> +        case XEN_SYSCTL_CPU_HOTPLUG_SMT_DISABLE:
>>> +            /* Use arch specific handlers as SMT is very arch-dependent */
>>> +            ret = arch_do_sysctl(op, u_sysctl);
>>> +            copyback = 0;
>>> +            goto out;
>>
>> I wonder if it wouldn't be neater for this and actually also ...
>>
>>> +        default:
>>> +            ret = -EOPNOTSUPP;
>>> +            break;
>>
>> ... this to fall through to ...
>>
>>> +        }
>>> +
>>> +        if ( !ret )
>>> +            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;
>>> +    }
>>> +#endif
>>> +
>>>       default:
>>>           ret = arch_do_sysctl(op, u_sysctl);
>>
>> ... here. (Minimally the earlier default case wants uniformly forwarding to
>> the arch handler, or else arch-specific additions would always require
>> adjustment here.)
> 
> Would it be okay to mix goto and switch like this, or is it too convoluted?

I'm not a fan of using goto, but maybe it would be acceptable here. By ...

>          case XEN_SYSCTL_CPU_HOTPLUG_OFFLINE:
>              plug = false;
>              fn = cpu_down_helper;
>              hcpu = _p(cpu);
>              break;
> 
>          default:
>              goto outer_default;
>          }
> 
>          if ( !ret )
>              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;

... wrapping everything past the switch() block in "if ( fn )" you'd already
get what is wanted.

>      }
> #endif
> 
>      default:
> outer_default:

Nit: See ./CODING_STYLE.

Jan

>          ret = arch_do_sysctl(op, u_sysctl);
>          copyback = 0;
>          break;
>      }
> 
> 
>