Re: [PATCH] KVM: arm64: Disable preemption in kvm_arch_hardware_enable()

From: Sean Christopherson <seanjc@google.com>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: Marc Zyngier <maz@kernel.org>,
	kvmarm@lists.linux.dev,  linux-arm-kernel@lists.infradead.org,
	kvm@vger.kernel.org,  James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	 Zenghui Yu <yuzenghui@huawei.com>,
	isaku.yamahata@intel.com, pbonzini@redhat.com,
	 Kristina Martsenko <kristina.martsenko@arm.com>,
	stable@vger.kernek.org
Subject: Re: [PATCH] KVM: arm64: Disable preemption in kvm_arch_hardware_enable()
Date: Mon, 10 Jul 2023 15:13:30 -0700	[thread overview]
Message-ID: <ZKyCimgRJThIR0pw@google.com> (raw)
In-Reply-To: <ZKxL09AW1s2uL28x@linux.dev>

On Mon, Jul 10, 2023, Oliver Upton wrote:
> On Mon, Jul 10, 2023 at 11:04:08AM -0700, Sean Christopherson wrote:
> > On Mon, Jul 03, 2023, Marc Zyngier wrote:
> > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > > index aaeae1145359..a28c4ffe4932 100644
> > > --- a/arch/arm64/kvm/arm.c
> > > +++ b/arch/arm64/kvm/arm.c
> > > @@ -1894,8 +1894,17 @@ static void _kvm_arch_hardware_enable(void *discard)
> > >  
> > >  int kvm_arch_hardware_enable(void)
> > >  {
> > > -	int was_enabled = __this_cpu_read(kvm_arm_hardware_enabled);
> > > +	int was_enabled;
> > >  
> > > +	/*
> > > +	 * Most calls to this function are made with migration
> > > +	 * disabled, but not with preemption disabled. The former is
> > > +	 * enough to ensure correctness, but most of the helpers
> > > +	 * expect the later and will throw a tantrum otherwise.
> > > +	 */
> > > +	preempt_disable();
> > > +
> > > +	was_enabled = __this_cpu_read(kvm_arm_hardware_enabled);
> > 
> > IMO, this_cpu_has_cap() is at fault.
> 
> Who ever said otherwise?

Sorry, should have phrased that as "should be fixed".

> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > index 7d7128c65161..b862477de2ce 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -3193,7 +3193,9 @@ static void __init setup_boot_cpu_capabilities(void)
> >  
> >  bool this_cpu_has_cap(unsigned int n)
> >  {
> > -       if (!WARN_ON(preemptible()) && n < ARM64_NCAPS) {
> > +       __this_cpu_preempt_check("has_cap");
> > +
> > +       if (n < ARM64_NCAPS) {
> 
> This is likely sufficient, but to Marc's point we have !preemptible()
> checks littered about, it just so happens that this_cpu_has_cap() is the
> first to get called. We need to make sure there aren't any other checks
> that'd break under hotplug.

Ah, "we" is all of arm64.  E.g. this pattern in arch/arm64/kernel/cpu_errata.c
is splattered all over the place.

  WARN_ON(scope != SCOPE_LOCAL_CPU || preemptible());

AFAICT, the only issue in KVM proper is the BUG_ON() in kvm_vgic_init_cpu_hardware().

Sadly, a hack-a-fix for stable probably does make sense :-(