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).
I'm looking at using "ifndef __KVM_NVHE_HYPERVISOR__" or other
ways to organize the code for this.
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).
> > I'm happy with that; it doesn't change the arm_pmu side of the interface and it
> > looks good from a functional perspective.
> >
> > I'll have to leave it to Marc and Oliver to say whether they're happy with the
> > KVM side.
>
> This looks OK to me. My only ask would be to have a small comment in
> the __{de}activate_traps_common() functions to say what this protects
> against, because I'll page it out within minutes.
Thank you for the feedback!
Yes, I will add a comment for those.
Thank you,
Reiji
