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

[Jan Beulich <jbeulich@suse.com>]

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?

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -176,6 +176,9 @@ config LIBFDT
>  config MEM_ACCESS_ALWAYS_ON
>  	bool
>  
> +config CPU_HOTPLUG
> +    bool

Nit: Indentation by a single tab please. See adjacent entries.

> @@ -104,6 +105,39 @@ void smp_call_function_interrupt(void)
>      irq_exit();
>  }
>  
> +#ifdef CONFIG_CPU_HOTPLUG
> +long cf_check cpu_up_helper(void *data)
> +{
> +    unsigned int cpu = (unsigned long)data;

Note this for the first comment below on cpu_down_helper().

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

> +    {
> +        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;

Why is this left as plain int? Yes, it was like this in the original code,
but wrongly so.

> +    int ret = cpu_down(cpu);
> +    /* Have one more go on EBUSY. */

Also please add the missing blank line after the declarations.

> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -483,6 +483,51 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>              copyback = 1;
>          break;
>  
> +#ifdef CONFIG_CPU_HOTPLUG
> +    case XEN_SYSCTL_cpu_hotplug:

Please see the pretty recent
https://lists.xen.org/archives/html/xen-devel/2026-01/msg00329.html
(scroll down to the xen/arch/x86/platform_hypercall.c change).

> +    {
> +        unsigned int cpu = op->u.cpu_hotplug.cpu;
> +        unsigned int hp_op = op->u.cpu_hotplug.op;
> +        bool plug;
> +        long (*fn)(void *data);
> +        void *hcpu;
> +
> +        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;
> +
> +        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.)

Jan