Re: [PATCH 0/3] KVM: arm64: nv: Add EL2 PMU event filtering support

[Marc Zyngier <maz@kernel.org>]

On Mon, 26 Aug 2024 18:26:36 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Hey,
> 
> On Sun, Aug 25, 2024 at 09:16:33AM +0100, Marc Zyngier wrote:
> > On Sat, 24 Aug 2024 01:13:59 +0100,
> > Oliver Upton <oliver.upton@linux.dev> wrote:
> > > 
> > > We allow a nested VM to be configured with a vPMU, although it isn't
> > > entirely functional. We do not currently respect the EL2 event filter
> > > configuration from the guest hypervisor, which really needs to be
> > > applied to EL1 when in a hyp context.
> > > 
> > > Series to do just that. Tested on Neoverse-V2 which fortunately
> > > implements PMUv3 and NV, unlike the fruity stuff I'm typically using...
> > > 
> > > Oliver Upton (3):
> > >   KVM: arm64: Add helpers to determine if PMC counts at a given EL
> > >   KVM: arm64: nv: Honor NSH filter when in hyp context
> > >   KVM: arm64: nv: Reprogram PMU events affected by nested transition
> > > 
> > >  arch/arm64/kvm/emulate-nested.c |  4 ++
> > >  arch/arm64/kvm/pmu-emul.c       | 77 ++++++++++++++++++++++++++++-----
> > >  include/kvm/arm_pmu.h           |  3 ++
> > >  3 files changed, 72 insertions(+), 12 deletions(-)
> > > 
> > > 
> > > base-commit: 7c626ce4bae1ac14f60076d00eafe71af30450ba
> > 
> > I had a quick look, and definitely like the way it is shaping up (it
> > does the job without fuss, and NV isn't about performance anyway).
> > 
> > However, I think it needs a bit more work so that MDCR_EL2.HPMN is
> > correctly handled. This shouldn't be hard to do, and if FEAT_FGT is
> > supported in the guest, then we could even expose FEAT_HPMN0 (though I
> > haven't worked out yet why FGT is a dependency).
> 
> Right, I was hoping to spoon-feed some more PMU patches going forward,
> since we already allow the NV+PMU combination and can treat it as a 'bugfix'.
> But I'm happy to throw some more cycles at the problem.

We absolutely can. As long as we don't let userspace get in the way,
we can play the incremental game for as long as we want.

> 
> I think there are some gaps in the way the other coarse-grained PMU
> traps are handled (TPM, TPMCR), since it looks like they also apply to
> Host EL0 based on the pseudocode. I don't want to have a separate
> mechanism for describing these 'InHost' traps, so I'm gonna try and
> address this like so:
> 
>  - Indicate if CGTs apply to host EL0 in the trap_bits descriptor
> 
>  - Steal a trap_config bit to indicate if a sysreg can trap while
>    InHost, and:
> 
>  - Mark a trap_config as InHost if any of the associated CGTs are
>    InHost when inserting in the xarray
> 
> That way I can preserve the early return for is_hyp_ctxt() for all but a
> few trap configs.
> 
> Thoughts?

I see that you have already posted your proposed approach, but allow
me to say what I have in mind.

We have three possibilities:

- either we go with your approach, which has the advantage of not
  requiring any new trap description, but breaks the (unwritten)
  promise that we don't do any "local" handling at this stage

- or we perform the handling where we normally do it (in sys_regs.c),
  but we start littering the already overly complicated emulation code
  for something that is barely a trap reinjection

- or (and this is my preferred option), we treat it as a "fast" trap,
  which would match what we do for other things that we trap and that
  behave differently when 'InHost' (ERET, TLBIs).

This last option does require another bit of decoding, but has the
following advantages:

- it follows the existing model for 'InHost' trap handling

- it is close to optimal from a performance perspective

- it doesn't change the existing behaviour of the xarray handling

The way I think of it, this sort of EL0->EL2 trap reinjection is
simply the other side of the ERET coin, and I would very much like to
keep this symmetry, unless I have missed something obvious.

What do you think?

	M.

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