Re: [PATCH 1/3] KVM: arm64: pkvm: Actually enable/disable PMU events when running vCPU

[Oliver Upton <oliver.upton@linux.dev>]

On Wed, Mar 06, 2024 at 10:06:32AM +0000, Marc Zyngier wrote:
> On Tue, 05 Mar 2024 18:48:38 +0000, Oliver Upton <oliver.upton@linux.dev> wrote:
> > +static void flush_debug_state(struct pkvm_hyp_vcpu *hyp_vcpu)
> > +{
> > +	struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
> > +
> > +	hyp_vcpu->vcpu.arch.mdcr_el2	= host_vcpu->arch.mdcr_el2;
> > +	hyp_vcpu->vcpu.arch.debug_ptr	= kern_hyp_va(host_vcpu->arch.debug_ptr);
> > +
> > +	/*
> > +	 * The host's PMU context cannot be trusted for protected VMs. Refer to
> > +	 * hardware to determine which PMCs need to be disabled/enabled on vCPU
> > +	 * entry/exit, respectively.
> > +	 */
> > +	if (kvm_vm_is_protected(kern_hyp_va(host_vcpu->kvm))) {
> 
> This looks wrong. We should never rely on *host* controlled data to
> make a decision at EL2 in the pKVM case. Use of this helper at EL2
> should be constrained to the hyp view of the kvm structure only.

I should've spelled this out explicitly. Looking at what we have
upstream, there's no hyp-controlled view on a VM's protection (yet).

I had imagined this hilariously wrong expression was a placeholder for
pKVM folks, much like we have in early_exit_filter() and
kvm_get_exit_handler_array(). But that's no good, and I probably
should've added one myself that can be updated with the 'right' thing
later on.

> The other thing is that most of this code is going to be thrown away,
> hopefully soon... (hint, nudge...).

I was hoping Will could provide some insight on whether or not the
'real' pKVM hyp state management carries the same bug. Nothing wrong
with fixing what we already have though...

-- 
Thanks,
Oliver