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

[Mykyta Poturai <Mykyta_Poturai@xxxxxxxx>]

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