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.
>> + 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?
>> + 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
Would it be okay to mix goto and switch like this, or is it too convoluted?
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;
}
#endif
default:
outer_default:
ret = arch_do_sysctl(op, u_sysctl);
copyback = 0;
break;
}
--
Mykyta
