Re: [PATCH v4 2/7] arm64: KVM: Use shared area to pass PMU event state to hypervisor

[Oliver Upton]

On Mon, Feb 05, 2024 at 12:16:53PM +0000, James Clark wrote:
> > This now allows the host to program event counters for a protected
> > guest. That _might_ be a useful feature behind some debug option, but is
> > most definitely *not* something we want to do for pVMs generally.
> 
> Unless I'm missing something, using PMUs on protected guests was added
> by 722625c6f4c5b ("KVM: arm64: Reenable pmu in Protected Mode"). This
> change is just a refactor that will allow us to add the same behavior
> for a similar feature (tracing) without adding yet another copy of some
> state before the guest switch.

Ha, I had forgotten about that patch (and I had reviewed it!)

My interpretation of the intent for that change was to enable the usage
of vPMU for non-protected VMs. The situation has changed since then, as
we use the shadow state for vCPUs unconditionally in protected mode as
of commit  be66e67f1750 ("KVM: arm64: Use the pKVM hyp vCPU structure
in handle___kvm_vcpu_run()")

Protected mode is well understood at this point to be a WIP feature, and
that not all things are expected to work with it. Eventually we will
need a way to distinguish between 'normal' VMs and true pVMs (i.e. the
VMM selected full isolation) in nVHE, but right now what we have enables
testing of some isolation features.

> > I'm perfectly happy leaving these sorts of features broken for pKVM and
> > using the 'normal' way of getting percpu data to the nVHE hypervisor
> > otherwise.
> > 
> 
> I can do that. But do I also disable PMU at the same time in a new
> commit? Now that both PMU and tracing is working maybe it would be a
> waste to throw that away and hiding it behind an option is better. Or I
> can leave the PMU as it is and just keep tracing disabled in pKVM.
> 
> I don't mind either way, my main goal was to get exclude/include guest
> tracing working for normal VMs. For pKVM I don't have a strong opinion.

Unless someone has strong opinions about making this work in protected
mode, I am happy to see tracing support limited to the 'normal' nVHE
configuration. The protected feature as a whole is just baggage until
upstream support is completed.

-- 
Thanks,
Oliver