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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi,

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>
> ---
> 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/smp.c             |  9 +++++++++
>  xen/arch/ppc/stubs.c           |  5 +++++
>  xen/arch/riscv/stubs.c         |  5 +++++
>  xen/arch/x86/include/asm/smp.h |  3 ---
>  xen/arch/x86/smp.c             | 34 +++------------------------------
>  xen/arch/x86/sysctl.c          | 11 ++++-------
>  xen/common/Kconfig             |  4 ++--
>  xen/common/smp.c               | 35 ++++++++++++++++++++++++++++++++++
>  xen/common/sysctl.c            | 32 +++++++++++++++++++++++++++++++
>  xen/include/xen/smp.h          |  4 ++++
>  xen/xsm/flask/hooks.c          |  2 +-
>  11 files changed, 100 insertions(+), 44 deletions(-)
>
> diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c
> index b372472188..039577ad02 100644
> --- 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_keep_online(unsigned int cpu)
> +{
> +    return true;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/ppc/stubs.c b/xen/arch/ppc/stubs.c
> index 82a289af85..2c2e9412ad 100644
> --- a/xen/arch/ppc/stubs.c
> +++ b/xen/arch/ppc/stubs.c
> @@ -101,6 +101,11 @@ void smp_send_call_function_mask(const cpumask_t *mask)
>      BUG_ON("unimplemented");
>  }
>  
> +bool arch_cpu_keep_online(unsigned int cpu)
> +{
> +    BUG_ON("unimplemented");
> +}
> +
>  /* irq.c */
>  
>  void irq_ack_none(struct irq_desc *desc)
> diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
> index 3a7953593d..2307408c6c 100644
> --- a/xen/arch/riscv/stubs.c
> +++ b/xen/arch/riscv/stubs.c
> @@ -65,6 +65,11 @@ void smp_send_call_function_mask(const cpumask_t *mask)
>      BUG_ON("unimplemented");
>  }
>  
> +bool arch_cpu_keep_online(unsigned int cpu)
> +{
> +    BUG_ON("unimplemented");
> +}
> +
>  /* irq.c */
>  
>  void irq_ack_none(struct irq_desc *desc)
> 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..329bb07ae9 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -648,8 +648,8 @@ config SYSTEM_SUSPEND
>  
>  config CPU_ONLINE_OFFLINE
>       bool "CPU online/offline support" if EXPERT
> -     depends on X86
> -     default y
> +     depends on !ARM || (!HAS_ITS && !TEE)

I don't think this is correct, unless RISC-V also support this.

Maybe introduce HAS_CPUONLINE_OFFLINE as we do for similar features?


[...]

-- 
WBR, Volodymyr