[PATCH 0/3] KVM: arm64: PMU fixes for nVHE, protected mode

[PATCH 0/3] KVM: arm64: PMU fixes for nVHE, protected mode

[Oliver Upton]

Series to address a couple of issues I've hit using the nVHE / protected
mode configuration:

 - Host-programmed PMU events that run during guest execution do not
   work if the VM doesn't have a vPMU.

 - PMU events do not work at all for non-protected VMs on the
   protected-mode hypervisor

Oliver Upton (3):
  KVM: arm64: pkvm: Actually enable/disable PMU events when running vCPU
  KVM: arm64: Fix host-programmed guest events in nVHE
  KVM: arm64: Do not disable preemption in kvm_vcpu_pmu_restore_guest()

 arch/arm64/kvm/hyp/nvhe/hyp-main.c | 24 ++++++++++++++++++++++--
 arch/arm64/kvm/pmu.c               |  2 --
 include/kvm/arm_pmu.h              |  2 +-
 3 files changed, 23 insertions(+), 5 deletions(-)


base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
-- 
2.44.0.278.ge034bb2e1d-goog

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

[Oliver Upton]

flush_hyp_vcpu() does not carry through the PMU event context from the
host, regardless of whether the VM is protected or not. This is
problematic for two reasons:

 - The expectation for non-protected VMs is that the protected-mode
   hypervisor has feature parity with the 'normal' KVM configuration.
   However, PMU events programmed to run with the guest do not work.

 - The protected-mode hypervisor needs to prevent the host from peering
   in on the guest. Nothing stops the host from leaving an event enabled
   straight past guest entry...

Address the both of these issues by priming the hyp vCPU's PMU event
context before entering the guest. Take special care to not trust the
host in the case of pVMs by referring directly to what hardware says is
enabled.

Cc: Will Deacon <will@kernel.org>
Fixes: be66e67f1750 ("KVM: arm64: Use the pKVM hyp vCPU structure in handle___kvm_vcpu_run()")
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/hyp/nvhe/hyp-main.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
index 2385fd03ed87..8621f8383f0c 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -23,6 +23,26 @@ DEFINE_PER_CPU(struct kvm_nvhe_init_params, kvm_init_params);
 
 void __kvm_hyp_host_forward_smc(struct kvm_cpu_context *host_ctxt);
 
+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))) {
+		hyp_vcpu->vcpu.arch.pmu.events.events_host = read_sysreg(pmcntenclr_el0);
+		hyp_vcpu->vcpu.arch.pmu.events.events_guest = 0;
+	} else {
+		hyp_vcpu->vcpu.arch.pmu.events = host_vcpu->arch.pmu.events;
+	}
+}
+
 static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
 {
 	struct kvm_vcpu *host_vcpu = hyp_vcpu->host_vcpu;
@@ -35,18 +55,18 @@ static void flush_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
 	hyp_vcpu->vcpu.arch.hw_mmu	= host_vcpu->arch.hw_mmu;
 
 	hyp_vcpu->vcpu.arch.hcr_el2	= host_vcpu->arch.hcr_el2;
-	hyp_vcpu->vcpu.arch.mdcr_el2	= host_vcpu->arch.mdcr_el2;
 	hyp_vcpu->vcpu.arch.cptr_el2	= host_vcpu->arch.cptr_el2;
 
 	hyp_vcpu->vcpu.arch.iflags	= host_vcpu->arch.iflags;
 	hyp_vcpu->vcpu.arch.fp_state	= host_vcpu->arch.fp_state;
 
-	hyp_vcpu->vcpu.arch.debug_ptr	= kern_hyp_va(host_vcpu->arch.debug_ptr);
 	hyp_vcpu->vcpu.arch.host_fpsimd_state = host_vcpu->arch.host_fpsimd_state;
 
 	hyp_vcpu->vcpu.arch.vsesr_el2	= host_vcpu->arch.vsesr_el2;
 
 	hyp_vcpu->vcpu.arch.vgic_cpu.vgic_v3 = host_vcpu->arch.vgic_cpu.vgic_v3;
+
+	flush_debug_state(hyp_vcpu);
 }
 
 static void sync_hyp_vcpu(struct pkvm_hyp_vcpu *hyp_vcpu)
-- 
2.44.0.278.ge034bb2e1d-goog

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

[Marc Zyngier]

On Tue, 05 Mar 2024 18:48:38 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> flush_hyp_vcpu() does not carry through the PMU event context from the
> host, regardless of whether the VM is protected or not. This is
> problematic for two reasons:
> 
>  - The expectation for non-protected VMs is that the protected-mode
>    hypervisor has feature parity with the 'normal' KVM configuration.
>    However, PMU events programmed to run with the guest do not work.
> 
>  - The protected-mode hypervisor needs to prevent the host from peering
>    in on the guest. Nothing stops the host from leaving an event enabled
>    straight past guest entry...
> 
> Address the both of these issues by priming the hyp vCPU's PMU event
> context before entering the guest. Take special care to not trust the
> host in the case of pVMs by referring directly to what hardware says is
> enabled.
> 
> Cc: Will Deacon <will@kernel.org>
> Fixes: be66e67f1750 ("KVM: arm64: Use the pKVM hyp vCPU structure in handle___kvm_vcpu_run()")
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/hyp/nvhe/hyp-main.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> index 2385fd03ed87..8621f8383f0c 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
> @@ -23,6 +23,26 @@ DEFINE_PER_CPU(struct kvm_nvhe_init_params, kvm_init_params);
>  
>  void __kvm_hyp_host_forward_smc(struct kvm_cpu_context *host_ctxt);
>  
> +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.

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

Thanks,

	M.

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

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

[Oliver Upton]

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

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

[Fuad Tabba]

Hi,

On Wed, Mar 6, 2024 at 10:20 AM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> 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...

Like you said, this is bad/buggy placeholder code. PMU isn't supported
for protected guests, and the "real" pKVM code we plan to upstream
(soon!) doesn't have this bug.

Thanks,
/fuad

> --
> Thanks,
> Oliver
>

[PATCH 2/3] KVM: arm64: Fix host-programmed guest events in nVHE

[Oliver Upton]

Programming PMU events in the host that count during guest execution is
a feature supported by perf, e.g.

  perf stat -e cpu_cycles:G ./lkvm run

While this works for VHE, the guest/host event bitmaps are not carried
through to the hypervisor in the nVHE configuration. Make
kvm_pmu_update_vcpu_events() conditional on whether or not _hardware_
supports PMUv3 rather than if the vCPU as vPMU enabled.

Cc: stable@vger.kernel.org
Fixes: 84d751a019a9 ("KVM: arm64: Pass pmu events to hyp via vcpu")
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 include/kvm/arm_pmu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 4b9d8fb393a8..df32355e3e38 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -86,7 +86,7 @@ void kvm_vcpu_pmu_resync_el0(void);
  */
 #define kvm_pmu_update_vcpu_events(vcpu)				\
 	do {								\
-		if (!has_vhe() && kvm_vcpu_has_pmu(vcpu))		\
+		if (!has_vhe() && kvm_arm_support_pmu_v3())		\
 			vcpu->arch.pmu.events = *kvm_get_pmu_events();	\
 	} while (0)
 
-- 
2.44.0.278.ge034bb2e1d-goog

Re: [PATCH 2/3] KVM: arm64: Fix host-programmed guest events in nVHE

[Marc Zyngier]

On Tue, 05 Mar 2024 18:48:39 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Programming PMU events in the host that count during guest execution is
> a feature supported by perf, e.g.
> 
>   perf stat -e cpu_cycles:G ./lkvm run
> 
> While this works for VHE, the guest/host event bitmaps are not carried
> through to the hypervisor in the nVHE configuration. Make
> kvm_pmu_update_vcpu_events() conditional on whether or not _hardware_
> supports PMUv3 rather than if the vCPU as vPMU enabled.
> 
> Cc: stable@vger.kernel.org
> Fixes: 84d751a019a9 ("KVM: arm64: Pass pmu events to hyp via vcpu")
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  include/kvm/arm_pmu.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> index 4b9d8fb393a8..df32355e3e38 100644
> --- a/include/kvm/arm_pmu.h
> +++ b/include/kvm/arm_pmu.h
> @@ -86,7 +86,7 @@ void kvm_vcpu_pmu_resync_el0(void);
>   */
>  #define kvm_pmu_update_vcpu_events(vcpu)				\
>  	do {								\
> -		if (!has_vhe() && kvm_vcpu_has_pmu(vcpu))		\
> +		if (!has_vhe() && kvm_arm_support_pmu_v3())		\
>  			vcpu->arch.pmu.events = *kvm_get_pmu_events();	\
>  	} while (0)
>  

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

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

[PATCH 3/3] KVM: arm64: Do not disable preemption in kvm_vcpu_pmu_restore_guest()

[Oliver Upton]

The caller has already disabled preemption, no need to nest further.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/pmu.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
index a243934c5568..43da27d8087d 100644
--- a/arch/arm64/kvm/pmu.c
+++ b/arch/arm64/kvm/pmu.c
@@ -181,14 +181,12 @@ void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu)
 	if (!kvm_arm_support_pmu_v3() || !has_vhe())
 		return;
 
-	preempt_disable();
 	pmu = kvm_get_pmu_events();
 	events_guest = pmu->events_guest;
 	events_host = pmu->events_host;
 
 	kvm_vcpu_pmu_enable_el0(events_guest);
 	kvm_vcpu_pmu_disable_el0(events_host);
-	preempt_enable();
 }
 
 /*
-- 
2.44.0.278.ge034bb2e1d-goog

Re: [PATCH 3/3] KVM: arm64: Do not disable preemption in kvm_vcpu_pmu_restore_guest()

[Marc Zyngier]

On Tue, 05 Mar 2024 18:48:40 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> The caller has already disabled preemption, no need to nest further.

A quick survey indicates that it doesn't seem to be the case, I'm
afraid. access_pmu_evtyper() and access_pmcnten() both seem to be
preemptible.

Which probably means that these spots are doing the wrong thing and
that we should instead turn it into a KVM_REQ_RESYNC_PMU_EL0 request.

Then the preemption can be actually dropped, and even replaced by an
assertion.

Thanks,

	M.

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

Re: [PATCH 3/3] KVM: arm64: Do not disable preemption in kvm_vcpu_pmu_restore_guest()

[Oliver Upton]

On Wed, Mar 06, 2024 at 09:52:56AM +0000, Marc Zyngier wrote:
> On Tue, 05 Mar 2024 18:48:40 +0000,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > The caller has already disabled preemption, no need to nest further.
> 
> A quick survey indicates that it doesn't seem to be the case, I'm
> afraid. access_pmu_evtyper() and access_pmcnten() both seem to be
> preemptible.

Dammit. Someone forgot to grep...

> Which probably means that these spots are doing the wrong thing and
> that we should instead turn it into a KVM_REQ_RESYNC_PMU_EL0 request.
> 
> Then the preemption can be actually dropped, and even replaced by an
> assertion.

Agreed, thanks for the suggestion.

-- 
Best,
Oliver

Re: (subset) [PATCH 0/3] KVM: arm64: PMU fixes for nVHE, protected mode

[Oliver Upton]

On Tue, 05 Mar 2024 18:48:37 +0000, Oliver Upton wrote:
> Series to address a couple of issues I've hit using the nVHE / protected
> mode configuration:
> 
>  - Host-programmed PMU events that run during guest execution do not
>    work if the VM doesn't have a vPMU.
> 
>  - PMU events do not work at all for non-protected VMs on the
>    protected-mode hypervisor
> 
> [...]

Just applying the commit that fixes nVHE, I'm happy to ignore the
pKVM issue until the 'real' vCPU context changes appear upstream.

Apologies if this email comes through garbled, I can't access my
usual workstation right now.

Applied to kvmarm/fixes, thanks!

[2/3] KVM: arm64: Fix host-programmed guest events in nVHE
      commit: e89c928bedd77d181edc2df01cb6672184775140

-- 
Best,
Oliver