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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.03.2026 10:39, Mykyta Poturai wrote:
> --- a/xen/arch/arm/smp.c
> +++ b/xen/arch/arm/smp.c
> @@ -44,6 +44,15 @@ void smp_send_call_function_mask(const cpumask_t *mask)
>      }
>  }
>  
> +/*
> + * We currently don't support SMT on ARM so we don't need any special logic 
> for
> + * CPU disabling
> + */
> +bool arch_cpu_can_stay_online(unsigned int cpu)
> +{
> +    return true;
> +}

Something as simple as this would be nice to be an inline function (or, less
desirably, a macro).

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

> --- a/xen/arch/x86/smp.c
> +++ b/xen/arch/x86/smp.c
> @@ -418,35 +418,8 @@ void cf_check call_function_interrupt(void)
>      smp_call_function_interrupt();
>  }
>  
> -long cf_check cpu_up_helper(void *data)
> +bool arch_cpu_can_stay_online(unsigned int cpu)
>  {
> -    unsigned int cpu = (unsigned long)data;
> -    int ret = cpu_up(cpu);
> -
> -    /* Have one more go on EBUSY. */
> -    if ( ret == -EBUSY )
> -        ret = cpu_up(cpu);
> -
> -    if ( !ret && !opt_smt &&
> -         cpu_data[cpu].compute_unit_id == INVALID_CUID &&
> -         cpumask_weight(per_cpu(cpu_sibling_mask, cpu)) > 1 )
> -    {
> -        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;
> -    int ret = cpu_down(cpu);
> -    /* Have one more go on EBUSY. */
> -    if ( ret == -EBUSY )
> -        ret = cpu_down(cpu);
> -    return ret;
> +    return opt_smt || cpu_data[cpu].compute_unit_id != INVALID_CUID ||
> +           cpumask_weight(per_cpu(cpu_sibling_mask, cpu)) <= 1;
>  }

Unlike for Arm, this may indeed better be an out-of-line function.

> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -49,6 +49,7 @@ static void cf_check l3_cache_get(void *arg)
>  
>  static long cf_check smt_up_down_helper(void *data)
>  {
> +    #ifdef CONFIG_CPU_HOTPLUG
>      bool up = (bool)data;
>      unsigned int cpu, sibling_mask = boot_cpu_data.x86_num_siblings - 1;
>      int ret = 0;
> @@ -89,6 +90,8 @@ static long cf_check smt_up_down_helper(void *data)
>                 up ? "enabled" : "disabled", CPUMASK_PR(&cpu_online_map));
>  
>      return ret;
> +    #endif /* CONFIG_CPU_HOTPLUG */
> +    return 0;
>  }

The #-es or pre-processor directives want to be in the very first column.

Sharing "return ret" would also be nice, imo. Would require ret's decl to
move ahead of the #ifdef. Actually - is there anything preventing

    if ( !IS_ENABLED(CONFIG_CPU_HOTPLUG) )
        return 0;

at the top of the function? Perhaps even with ASSERT_UNREACHABLE() added
in?

> @@ -115,24 +118,24 @@ long arch_do_sysctl(
>  
>      case XEN_SYSCTL_cpu_hotplug:
>      {
> -        unsigned int cpu = sysctl->u.cpu_hotplug.cpu;
>          unsigned int op  = sysctl->u.cpu_hotplug.op;
>          bool plug;
>          long (*fn)(void *data);
>          void *hcpu;
>  
> -        switch ( op )
> +        if ( !IS_ENABLED(CONFIG_CPU_HOTPLUG) )
>          {
> -        case XEN_SYSCTL_CPU_HOTPLUG_ONLINE:
> -            plug = true;
> -            fn = cpu_up_helper;
> -            hcpu = _p(cpu);
> +            ret = -EOPNOTSUPP;
>              break;

ASSERT_UNREACHABLE() looks to also be valid to be added here, seeing how
do_sysctl() now works.

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -637,6 +637,12 @@ config SYSTEM_SUSPEND
>  
>         If unsure, say N.
>  
> +config CPU_HOTPLUG
> +     bool "Enable CPU hotplug"

I'm not happy with this prompt. For x86 SUPPORT.md declares (ACPI) CPU
hotplug as experimental. That's physical hotplug. The code you're
fiddling with, however, is also used for soft-{off,on}lining. Which,
e.g. to disable SMT on x86, may need to be used for security purposes.

> +     depends on (X86 || ARM_64) && !FFA && !TEE && !HAS_ITS

What if on x86 FFA, TEE, or ITS gain a meaning?

> +     default y
> +
> +

Nit: No double blank lines please.

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

> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -835,7 +835,7 @@ static int cf_check flask_sysctl(int cmd)
>      case XEN_SYSCTL_getdomaininfolist:
>      case XEN_SYSCTL_page_offline_op:
>      case XEN_SYSCTL_scheduler_op:
> -#ifdef CONFIG_X86
> +#ifdef CONFIG_CPU_HOTPLUG
>      case XEN_SYSCTL_cpu_hotplug:
>  #endif
>          return 0;

Is there a reason the #ifdef can't simply be dropped?

Jan