On Thu, 13 Apr 2023 01:07:38 +0100,
Reiji Watanabe <reijiw@xxxxxxxxxx> 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@xxxxxxx> 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.
