Re: [PATCH v4 4/6] KVM: arm64: Simplify the CPUHP logic

[Oliver Upton]

On Tue, Feb 15, 2022 at 7:16 PM Chao Gao <chao.gao@xxxxxxxxx> wrote:
>
> From: Marc Zyngier <maz@xxxxxxxxxx>
>
> For a number of historical reasons, the KVM/arm64 hotplug setup is pretty
> complicated, and we have two extra CPUHP notifiers for vGIC and timers.
>
> It looks pretty pointless, and gets in the way of further changes.
> So let's just expose some helpers that can be called from the core
> CPUHP callback, and get rid of everything else.
>
> This gives us the opportunity to drop a useless notifier entry,
> as well as tidy-up the timer enable/disable, which was a bit odd.
>
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>

Reviewed-by: Oliver Upton <oupton@xxxxxxxxxx>

> ---
>  arch/arm64/kvm/arch_timer.c     | 27 ++++++++++-----------------
>  arch/arm64/kvm/arm.c            |  4 ++++
>  arch/arm64/kvm/vgic/vgic-init.c | 19 ++-----------------
>  include/kvm/arm_arch_timer.h    |  4 ++++
>  include/kvm/arm_vgic.h          |  4 ++++
>  include/linux/cpuhotplug.h      |  3 ---
>  6 files changed, 24 insertions(+), 37 deletions(-)
>
> diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
> index 6e542e2eae32..f9d14c6dc0b4 100644
> --- a/arch/arm64/kvm/arch_timer.c
> +++ b/arch/arm64/kvm/arch_timer.c
> @@ -796,10 +796,18 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
>         ptimer->host_timer_irq_flags = host_ptimer_irq_flags;
>  }
>
> -static void kvm_timer_init_interrupt(void *info)
> +void kvm_timer_cpu_up(void)
>  {
>         enable_percpu_irq(host_vtimer_irq, host_vtimer_irq_flags);
> -       enable_percpu_irq(host_ptimer_irq, host_ptimer_irq_flags);
> +       if (host_ptimer_irq)
> +               enable_percpu_irq(host_ptimer_irq, host_ptimer_irq_flags);
> +}
> +
> +void kvm_timer_cpu_down(void)
> +{
> +       disable_percpu_irq(host_vtimer_irq);
> +       if (host_ptimer_irq)
> +               disable_percpu_irq(host_ptimer_irq);
>  }
>
>  int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value)
> @@ -961,18 +969,6 @@ void kvm_arm_timer_write_sysreg(struct kvm_vcpu *vcpu,
>         preempt_enable();
>  }
>
> -static int kvm_timer_starting_cpu(unsigned int cpu)
> -{
> -       kvm_timer_init_interrupt(NULL);
> -       return 0;
> -}
> -
> -static int kvm_timer_dying_cpu(unsigned int cpu)
> -{
> -       disable_percpu_irq(host_vtimer_irq);
> -       return 0;
> -}
> -
>  static int timer_irq_set_vcpu_affinity(struct irq_data *d, void *vcpu)
>  {
>         if (vcpu)
> @@ -1170,9 +1166,6 @@ int kvm_timer_hyp_init(bool has_gic)
>                 goto out_free_irq;
>         }
>
> -       cpuhp_setup_state(CPUHP_AP_KVM_ARM_TIMER_STARTING,
> -                         "kvm/arm/timer:starting", kvm_timer_starting_cpu,
> -                         kvm_timer_dying_cpu);
>         return 0;
>  out_free_irq:
>         free_percpu_irq(host_vtimer_irq, kvm_get_running_vcpus());
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 0165cf3aac3a..31b049e48b19 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -1658,6 +1658,8 @@ static void _kvm_arch_hardware_enable(void *discard)
>  {
>         if (!__this_cpu_read(kvm_arm_hardware_enabled)) {
>                 cpu_hyp_reinit();
> +               kvm_vgic_cpu_up();
> +               kvm_timer_cpu_up();
>                 __this_cpu_write(kvm_arm_hardware_enabled, 1);
>         }
>  }
> @@ -1671,6 +1673,8 @@ int kvm_arch_hardware_enable(void)
>  static void _kvm_arch_hardware_disable(void *discard)
>  {
>         if (__this_cpu_read(kvm_arm_hardware_enabled)) {
> +               kvm_timer_cpu_down();
> +               kvm_vgic_cpu_down();
>                 cpu_hyp_reset();
>                 __this_cpu_write(kvm_arm_hardware_enabled, 0);
>         }
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index fc00304fe7d8..60038a8516de 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -460,17 +460,15 @@ int kvm_vgic_map_resources(struct kvm *kvm)
>
>  /* GENERIC PROBE */
>
> -static int vgic_init_cpu_starting(unsigned int cpu)
> +void kvm_vgic_cpu_up(void)
>  {
>         enable_percpu_irq(kvm_vgic_global_state.maint_irq, 0);
> -       return 0;
>  }
>
>
> -static int vgic_init_cpu_dying(unsigned int cpu)
> +void kvm_vgic_cpu_down(void)
>  {
>         disable_percpu_irq(kvm_vgic_global_state.maint_irq);
> -       return 0;
>  }
>
>  static irqreturn_t vgic_maintenance_handler(int irq, void *data)
> @@ -579,19 +577,6 @@ int kvm_vgic_hyp_init(void)
>                 return ret;
>         }
>
> -       ret = cpuhp_setup_state(CPUHP_AP_KVM_ARM_VGIC_INIT_STARTING,
> -                               "kvm/arm/vgic:starting",
> -                               vgic_init_cpu_starting, vgic_init_cpu_dying);
> -       if (ret) {
> -               kvm_err("Cannot register vgic CPU notifier\n");
> -               goto out_free_irq;
> -       }
> -
>         kvm_info("vgic interrupt IRQ%d\n", kvm_vgic_global_state.maint_irq);
>         return 0;
> -
> -out_free_irq:
> -       free_percpu_irq(kvm_vgic_global_state.maint_irq,
> -                       kvm_get_running_vcpus());
> -       return ret;
>  }
> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
> index 51c19381108c..16a2f65fcfb4 100644
> --- a/include/kvm/arm_arch_timer.h
> +++ b/include/kvm/arm_arch_timer.h
> @@ -106,4 +106,8 @@ void kvm_arm_timer_write_sysreg(struct kvm_vcpu *vcpu,
>  u32 timer_get_ctl(struct arch_timer_context *ctxt);
>  u64 timer_get_cval(struct arch_timer_context *ctxt);
>
> +/* CPU HP callbacks */
> +void kvm_timer_cpu_up(void);
> +void kvm_timer_cpu_down(void);
> +
>  #endif
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index bb30a6803d9f..a2a0cca05a73 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -427,4 +427,8 @@ int vgic_v4_load(struct kvm_vcpu *vcpu);
>  void vgic_v4_commit(struct kvm_vcpu *vcpu);
>  int vgic_v4_put(struct kvm_vcpu *vcpu, bool need_db);
>
> +/* CPU HP callbacks */
> +void kvm_vgic_cpu_up(void);
> +void kvm_vgic_cpu_down(void);
> +
>  #endif /* __KVM_ARM_VGIC_H */
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index 411a428ace4d..4345b8eafc03 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -183,9 +183,6 @@ enum cpuhp_state {
>         CPUHP_AP_TI_GP_TIMER_STARTING,
>         CPUHP_AP_HYPERV_TIMER_STARTING,
>         CPUHP_AP_KVM_STARTING,
> -       CPUHP_AP_KVM_ARM_VGIC_INIT_STARTING,
> -       CPUHP_AP_KVM_ARM_VGIC_STARTING,
> -       CPUHP_AP_KVM_ARM_TIMER_STARTING,
>         /* Must be the last timer callback */
>         CPUHP_AP_DUMMY_TIMER_STARTING,
>         CPUHP_AP_ARM_XEN_STARTING,
> --
> 2.25.1
>