Re: [PATCH v3 06/22] KVM: arm64: Simplify the CPUHP logic

[Marc Zyngier <maz@kernel.org>]

On Mon, 05 Sep 2022 08:05:09 +0100,
Yuan Yao <yuan.yao@linux.intel.com> wrote:
> 
> On Thu, Sep 01, 2022 at 07:17:41PM -0700, isaku.yamahata@intel.com wrote:
> > From: Marc Zyngier <maz@kernel.org>
> >
> > 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@kernel.org>
> > Signed-off-by: Chao Gao <chao.gao@intel.com>
> > Reviewed-by: Oliver Upton <oupton@google.com>
> > Link: https://lore.kernel.org/r/20220216031528.92558-5-chao.gao@intel.com
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> > ---
> >  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 bb24a76b4224..33fca1a691a5 100644
> > --- a/arch/arm64/kvm/arch_timer.c
> > +++ b/arch/arm64/kvm/arch_timer.c
> > @@ -811,10 +811,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);
> >  }
> 
> Should "host_vtimer_irq" be checked yet as host_ptimer_irq ?

No, because although the ptimer interrupt is optional (on older
systems, we fully emulate that timer, including the interrupt), the
vtimer interrupt is always present and can be used unconditionally.

> Because
> the host_{v,p}timer_irq is set in same function kvm_irq_init() which
> called AFTER the on_each_cpu(_kvm_arch_hardware_enable, NULL, 1) from
> init_subsystems():
> 
> kvm_init()
>   kvm_arch_init()
>     init_subsystems()
>       on_each_cpu(_kvm_arch_hardware_enable, NULL, 1);
>       kvm_timer_hyp_init()
>         kvm_irq_init()
>           host_vtimer_irq = info->virtual_irq;
>           host_ptimer_irq = info->physical_irq;
>   hardware_enable_all()

This, however, is a very nice catch. I doubt this results in anything
really bad (the interrupt enable will fail as the interrupt number
is 0, and the disable will also fail due to no prior enable), but
that's extremely ugly anyway.

The best course of action AFAICS is to differentiate between the
arm64-specific initialisation (which is a one-off) and the runtime
stuff. Something like the hack below, that I haven't tested yet:

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 32c1022eb4b3..65d03c28f32a 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -1671,23 +1671,27 @@ 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);
 	}
 }
 
 int kvm_arch_hardware_enable(void)
 {
+	int was_enabled = __this_cpu_read(kvm_arm_hardware_enabled);
+
 	_kvm_arch_hardware_enable(NULL);
+
+	if (!was_enabled) {
+		kvm_vgic_cpu_up();
+		kvm_timer_cpu_up();
+	}
+
 	return 0;
 }
 
 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);
 	}
@@ -1695,6 +1699,11 @@ static void _kvm_arch_hardware_disable(void *discard)
 
 void kvm_arch_hardware_disable(void)
 {
+	if (__this_cpu_read(kvm_arm_hardware_enabled)) {
+		kvm_timer_cpu_down();
+		kvm_vgic_cpu_down();
+	}
+
 	if (!is_protected_kvm_enabled())
 		_kvm_arch_hardware_disable(NULL);
 }

This should ensure that the init still works as it used to, and that
vgic and timers get switched as per the CPUHP notifiers.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.