Re: [PATCH v11 4/6] arm/sysctl: Implement cpu hotplug ops

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Mykyta,

Mykyta Poturai <Mykyta_Poturai@xxxxxxxx> writes:

> 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 enabled,
> and partially supported FFA, or TEE is enabled, as they use non-static
> IRQ actions.
>
> Remove ifdef guards for x86 in flask, as cpu hotplug is now
> supported on more architectures.
>
> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>

Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>

> ---
> v10->v11:
> * add HAS_CPU_ONLINE_OFFLINE knob
>
> v9->v10:
> * rename arch_cpu_can_stay_online to arch_cpu_keep_online
> * drop the single-use hp_op and hcpu variables
> * make CPU_ONLINE_OFFLINE depend on !TEE, as FFA and TEE use non-static
>   IRQ actions
>
> v8->v9:
> * simplify dependencies of config CPU_ONLINE_OFFLINE again
> * rebased ove XSA-499
>
> v7->v8:
> * simplify dependencies of config CPU_ONLINE_OFFLINE
>
> v6->v7:
> * use IS_ENABLED istead of ifdef in more places
> * remove unneded variables
> * more explicit fallthrough in do_sysctl
>
> v5->v6:
> * fix style issues
> * rename arch_smt_cpu_disable -> arch_cpu_can_stay_online and invert the
> logic
> * use IS_ENABLED istead of ifdef
> * remove explicit list af arch-specific SYSCTL_CPU_HOTPLUG_* options
> from the common handler
> * fix flask issue
>
> v4->v5:
> * move handling to common code
> * rename config to CPU_HOTPUG
> * merge with "smp: Move cpu_up/down helpers to common code"
>
> v3->v4:
> * don't reimplement cpu_up/down helpers
> * add Kconfig option
> * fixup formatting
>
> v2->v3:
> * no changes
>
> v1->v2:
> * remove SMT ops
> * remove cpu == 0 checks
> * add XSM hooks
> * only implement for 64bit Arm
> ---
>  xen/arch/arm/Kconfig           |  1 +
>  xen/arch/arm/smp.c             | 11 +++++++++++
>  xen/arch/x86/Kconfig           |  1 +
>  xen/arch/x86/include/asm/smp.h |  3 ---
>  xen/arch/x86/smp.c             | 34 +++------------------------------
>  xen/arch/x86/sysctl.c          | 11 ++++-------
>  xen/common/Kconfig             |  7 +++++--
>  xen/common/smp.c               | 35 ++++++++++++++++++++++++++++++++++
>  xen/common/sysctl.c            | 32 +++++++++++++++++++++++++++++++
>  xen/include/xen/smp.h          |  6 ++++++
>  xen/xsm/flask/hooks.c          |  2 +-
>  11 files changed, 99 insertions(+), 44 deletions(-)
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index b9c5c40678..4569c76615 100644
> --- 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 HAS_CPU_ONLINE_OFFLINE if !HAS_ITS && !TEE
>       select HAS_DOMAIN_TYPE
>       select HAS_FAST_MULTIPLY
>       select HAS_VPCI_GUEST_SUPPORT if PCI_PASSTHROUGH
> diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c
> index b372472188..f84f217359 100644
> --- a/xen/arch/arm/smp.c
> +++ b/xen/arch/arm/smp.c
> @@ -44,6 +44,17 @@ void smp_send_call_function_mask(const cpumask_t *mask)
>      }
>  }
>  
> +#ifdef CONFIG_CPU_ONLINE_OFFLINE
> +/*
> + * We currently don't support SMT on ARM so we don't need any special logic 
> for
> + * CPU disabling
> + */
> +bool arch_cpu_keep_online(unsigned int cpu)
> +{
> +    return true;
> +}
> +#endif
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index e5535ac484..e5e3134022 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -17,6 +17,7 @@ config X86
>       select HAS_ALTERNATIVE
>       select HAS_COMPAT
>       select HAS_CPUFREQ
> +     select HAS_CPU_ONLINE_OFFLINE
>       select HAS_DIT
>       select HAS_EHCI
>       select HAS_EX_TABLE
> diff --git a/xen/arch/x86/include/asm/smp.h b/xen/arch/x86/include/asm/smp.h
> index 572b39b01c..6c33567cb5 100644
> --- a/xen/arch/x86/include/asm/smp.h
> +++ b/xen/arch/x86/include/asm/smp.h
> @@ -44,9 +44,6 @@ int cpu_add(uint32_t apic_id, uint32_t acpi_id, uint32_t 
> pxm);
>  
>  void __stop_this_cpu(void);
>  
> -long cf_check cpu_up_helper(void *data);
> -long cf_check cpu_down_helper(void *data);
> -
>  long cf_check core_parking_helper(void *data);
>  bool core_parking_remove(unsigned int cpu);
>  uint32_t get_cur_idle_nums(void);
> diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c
> index e3c4c0d5e0..b56d8dfd36 100644
> --- a/xen/arch/x86/smp.c
> +++ b/xen/arch/x86/smp.c
> @@ -417,37 +417,9 @@ void cf_check call_function_interrupt(void)
>  }
>  
>  #ifdef CONFIG_CPU_ONLINE_OFFLINE
> -long cf_check cpu_up_helper(void *data)
> +bool arch_cpu_keep_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;
>  }
>  #endif
> diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
> index 10d485e633..a4254beabb 100644
> --- a/xen/arch/x86/sysctl.c
> +++ b/xen/arch/x86/sysctl.c
> @@ -121,13 +121,13 @@ 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;
>          long (*fn)(void *data);
>          void *hcpu;
>  
>          if ( !IS_ENABLED(CONFIG_CPU_ONLINE_OFFLINE) )
>          {
> +            ASSERT_UNREACHABLE();
>              ret = -EOPNOTSUPP;
>              break;
>          }
> @@ -135,13 +135,10 @@ long arch_do_sysctl(
>          switch ( op )
>          {
>          case XEN_SYSCTL_CPU_HOTPLUG_ONLINE:
> -            fn = cpu_up_helper;
> -            hcpu = _p(cpu);
> -            break;
> -
>          case XEN_SYSCTL_CPU_HOTPLUG_OFFLINE:
> -            fn = cpu_down_helper;
> -            hcpu = _p(cpu);
> +            /* Handled by common code */
> +            ASSERT_UNREACHABLE();
> +            ret = -EOPNOTSUPP;
>              break;
>  
>          case XEN_SYSCTL_CPU_HOTPLUG_SMT_ENABLE:
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 5713dc1f9c..f1c78b0ac7 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -121,6 +121,9 @@ config HAS_ALTERNATIVE
>  config HAS_COMPAT
>       bool
>  
> +config HAS_CPU_ONLINE_OFFLINE
> +     bool
> +
>  config HAS_DEVICE_TREE_DISCOVERY
>       bool
>       select DEVICE_TREE_PARSE
> @@ -648,8 +651,8 @@ config SYSTEM_SUSPEND
>  
>  config CPU_ONLINE_OFFLINE
>       bool "CPU online/offline support" if EXPERT
> -     depends on X86
> -     default y
> +     depends on HAS_CPU_ONLINE_OFFLINE
> +     default X86
>       help
>         Enable support for bringing CPUs online and offline at runtime. On
>         X86 this is required for disabling SMT.
> diff --git a/xen/common/smp.c b/xen/common/smp.c
> index a011f541f1..af532d4873 100644
> --- a/xen/common/smp.c
> +++ b/xen/common/smp.c
> @@ -16,6 +16,7 @@
>   * GNU General Public License for more details.
>   */
>  
> +#include <xen/cpu.h>
>  #include <asm/hardirq.h>
>  #include <asm/processor.h>
>  #include <xen/spinlock.h>
> @@ -104,6 +105,40 @@ void smp_call_function_interrupt(void)
>      irq_exit();
>  }
>  
> +#ifdef CONFIG_CPU_ONLINE_OFFLINE
> +long cf_check cpu_up_helper(void *data)
> +{
> +    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 && !arch_cpu_keep_online(cpu) )
> +    {
> +        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)
> +{
> +    unsigned 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;
> +}
> +#endif /* CONFIG_CPU_ONLINE_OFFLINE */
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
> index 8fb5ff0af3..dd21b6ff7e 100644
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -475,6 +475,38 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>              copyback = 1;
>          break;
>  
> +    case XEN_SYSCTL_cpu_hotplug:
> +    {
> +        long (*fn)(void *data);
> +
> +        ret = -EOPNOTSUPP;
> +        if ( !IS_ENABLED(CONFIG_CPU_ONLINE_OFFLINE) )
> +            break;
> +
> +        switch ( op->u.cpu_hotplug.op )
> +        {
> +        case XEN_SYSCTL_CPU_HOTPLUG_ONLINE:
> +            fn = cpu_up_helper;
> +            break;
> +
> +        case XEN_SYSCTL_CPU_HOTPLUG_OFFLINE:
> +            fn = cpu_down_helper;
> +            break;
> +
> +        default:
> +            fn = NULL;
> +            break;
> +        }
> +
> +        if ( fn )
> +        {
> +            ret = continue_hypercall_on_cpu(0, fn, 
> _p(op->u.cpu_hotplug.cpu));
> +            break;
> +        }
> +    }
> +
> +        /* Use the arch handler for cases not handled here */
> +        fallthrough;
>      default:
>          ret = arch_do_sysctl(op, u_sysctl);
>          copyback = 0;
> diff --git a/xen/include/xen/smp.h b/xen/include/xen/smp.h
> index 2ca9ff1bfc..6c011d73ef 100644
> --- a/xen/include/xen/smp.h
> +++ b/xen/include/xen/smp.h
> @@ -76,4 +76,10 @@ extern void *stack_base[NR_CPUS];
>  void initialize_cpu_data(unsigned int cpu);
>  int setup_cpu_root_pgt(unsigned int cpu);
>  
> +#ifdef CONFIG_CPU_ONLINE_OFFLINE
> +bool arch_cpu_keep_online(unsigned int cpu);
> +#endif
> +long cf_check cpu_up_helper(void *data);
> +long cf_check cpu_down_helper(void *data);
> +
>  #endif /* __XEN_SMP_H__ */
> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index d65ba0aeae..a1eed309b4 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -957,7 +957,7 @@ static int cf_check flask_sysctl(const struct xen_sysctl 
> *op)
>      case XEN_SYSCTL_getdomaininfolist:
>          return flask_getdomaininfo(dom_xen);
>  
> -#ifdef CONFIG_X86
> +#ifdef CONFIG_CPU_ONLINE_OFFLINE
>      case XEN_SYSCTL_cpu_hotplug:
>          switch ( op->u.cpu_hotplug.op )
>          {

-- 
WBR, Volodymyr