Re: [PATCH v2 2/2] KVM: arm64: PMU: Don't overwrite PMUSERENR with vcpu loaded

From: Marc Zyngier <maz@kernel.org>
To: Reiji Watanabe <reijiw@google.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Oliver Upton <oliver.upton@linux.dev>,
	Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	kvmarm@lists.linux.dev, kvm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Ricardo Koller <ricarkol@google.com>,
	Jing Zhang <jingzhangos@google.com>,
	Raghavendra Rao Anata <rananta@google.com>,
	Shaoqin Huang <shahuang@redhat.com>,
	Rob Herring <robh@kernel.org>
Subject: Re: [PATCH v2 2/2] KVM: arm64: PMU: Don't overwrite PMUSERENR with vcpu loaded
Date: Thu, 13 Apr 2023 09:56:55 +0100	[thread overview]
Message-ID: <86jzygkvmw.wl-maz@kernel.org> (raw)
In-Reply-To: <CAAeT=FwUZ-TTn+4tt9pcesB59b7_=_zxkeRftMh5aQDUqnMz8g@mail.gmail.com>

On Thu, 13 Apr 2023 01:07:38 +0100,
Reiji Watanabe <reijiw@google.com> wrote:
> 
> On Wed, Apr 12, 2023 at 11:22:29AM +0100, Marc Zyngier wrote:
> > On Wed, 12 Apr 2023 10:20:05 +0100,
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > >
> > > On Tue, Apr 11, 2023 at 10:14:10PM -0700, Reiji Watanabe wrote:
> > > > Uh, right, interrupts are not masked during those windows...
> > > >
> > > > What I am currently considering on this would be disabling
> > > > IRQs while manipulating the register, and introducing a new flag
> > > > to indicate whether the PMUSERENR for the guest EL0 is loaded,
> > > > and having kvm_set_pmuserenr() check the new flag.
> > > >
> > > > The code would be something like below (local_irq_save/local_irq_restore
> > > > needs to be excluded for NVHE though).
> >
> > It shouldn't need to be excluded. It should be fairly harmless, unless
> > I'm missing something really obvious?
> 
> The reason why I think local_irq_{save,restore} should be excluded
> are because they use trace_hardirqs_{on,off} (Since IRQs are
> masked here for NVHE, practically, they shouldn't be called with
> the current KVM implementation though).

Gah. Indeed, we end-up with a lot of unwanted crap, and absolutely no
way to locally override it.

> I'm looking at using "ifndef __KVM_NVHE_HYPERVISOR__" or other
> ways to organize the code for this.

I'd vote for something like the code below:

diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
index 530347cdebe3..1796fadb26cc 100644
--- a/arch/arm64/kvm/hyp/nvhe/Makefile
+++ b/arch/arm64/kvm/hyp/nvhe/Makefile
@@ -10,7 +10,7 @@ asflags-y := -D__KVM_NVHE_HYPERVISOR__ -D__DISABLE_EXPORTS
 # will explode instantly (Words of Marc Zyngier). So introduce a generic flag
 # __DISABLE_TRACE_MMIO__ to disable MMIO tracing for nVHE KVM.
 ccflags-y := -D__KVM_NVHE_HYPERVISOR__ -D__DISABLE_EXPORTS -D__DISABLE_TRACE_MMIO__
-ccflags-y += -fno-stack-protector	\
+ccflags-y += -fno-stack-protector	-DNO_TRACE_IRQFLAGS \
 	     -DDISABLE_BRANCH_PROFILING	\
 	     $(DISABLE_STACKLEAK_PLUGIN)
 
diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index 5ec0fa71399e..ab0ae58dd797 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -198,9 +198,10 @@ extern void warn_bogus_irq_restore(void);
 
 /*
  * The local_irq_*() APIs are equal to the raw_local_irq*()
- * if !TRACE_IRQFLAGS.
+ * if !TRACE_IRQFLAGS or if NO_TRACE_IRQFLAGS is localy
+ * set.
  */
-#ifdef CONFIG_TRACE_IRQFLAGS
+#if defined(CONFIG_TRACE_IRQFLAGS) && !defined(NO_TRACE_IRQFLAGS)
 
 #define local_irq_enable()				\
 	do {						\


> Since {__activate,__deactivate}_traps_common() are pretty lightweight
> functions, I'm also considering disabling IRQs in their call sites
> (i.e. activate_traps_vhe_load/deactivate_traps_vhe_put), instead of in
> __{de}activate_traps_common() (Thanks for this suggestion, Oliver).

That would work too.

Thanks,

	M.

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