[PATCH] KVM: arm64: Correctly handle RES0 bits PMEVTYPER<n>_EL0.evtCount

[PATCH] KVM: arm64: Correctly handle RES0 bits PMEVTYPER<n>_EL0.evtCount

[Oliver Upton]

The PMU event ID varies from 10 to 16 bits, depending on the PMU
version. If the PMU only supports 10 bits of event ID, bits [15:10] of
the evtCount field behave as RES0.

While the actual PMU emulation code gets this right (i.e. RES0 bits are
masked out when programming the perf event), the sysreg emulation writes
an unmasked value to the in-memory cpu context. The net effect is that
guest reads and writes of PMEVTYPER<n>_EL0 will see non-RES0 behavior in
the reserved bits of the field.

As it so happens, kvm_pmu_set_counter_event_type() already writes a
masked value to the in-memory context that gets overwritten by
access_pmu_evtyper(). Fix the issue by removing the unnecessary (and
incorrect) register write in access_pmu_evtyper().

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

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index bd3431823ec5..fa7597882ed8 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -986,7 +986,6 @@ static bool access_pmu_evtyper(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 
 	if (p->is_write) {
 		kvm_pmu_set_counter_event_type(vcpu, p->regval, idx);
-		__vcpu_sys_reg(vcpu, reg) = p->regval & ARMV8_PMU_EVTYPE_MASK;
 		kvm_vcpu_pmu_restore_guest(vcpu);
 	} else {
 		p->regval = __vcpu_sys_reg(vcpu, reg) & ARMV8_PMU_EVTYPE_MASK;

base-commit: dcf89d111199562fa5f31a1bb76f17bc4831f6da
-- 
2.41.0.255.g8b1d071c50-goog

Re: [PATCH] KVM: arm64: Correctly handle RES0 bits PMEVTYPER<n>_EL0.evtCount

[Marc Zyngier]

AOn Thu, 13 Jul 2023 23:16:49 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> The PMU event ID varies from 10 to 16 bits, depending on the PMU
> version. If the PMU only supports 10 bits of event ID, bits [15:10] of
> the evtCount field behave as RES0.
> 
> While the actual PMU emulation code gets this right (i.e. RES0 bits are
> masked out when programming the perf event), the sysreg emulation writes
> an unmasked value to the in-memory cpu context. The net effect is that
> guest reads and writes of PMEVTYPER<n>_EL0 will see non-RES0 behavior in
> the reserved bits of the field.
> 
> As it so happens, kvm_pmu_set_counter_event_type() already writes a
> masked value to the in-memory context that gets overwritten by
> access_pmu_evtyper(). Fix the issue by removing the unnecessary (and
> incorrect) register write in access_pmu_evtyper().
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>

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

	M.

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

Re: [PATCH] KVM: arm64: Correctly handle RES0 bits PMEVTYPER<n>_EL0.evtCount

[Reiji Watanabe]

On Thu, Jul 13, 2023 at 3:17 PM Oliver Upton <oliver.upton@linux.dev> wrote:
>
> The PMU event ID varies from 10 to 16 bits, depending on the PMU
> version. If the PMU only supports 10 bits of event ID, bits [15:10] of
> the evtCount field behave as RES0.
>
> While the actual PMU emulation code gets this right (i.e. RES0 bits are
> masked out when programming the perf event), the sysreg emulation writes
> an unmasked value to the in-memory cpu context. The net effect is that
> guest reads and writes of PMEVTYPER<n>_EL0 will see non-RES0 behavior in
> the reserved bits of the field.
>
> As it so happens, kvm_pmu_set_counter_event_type() already writes a
> masked value to the in-memory context that gets overwritten by
> access_pmu_evtyper(). Fix the issue by removing the unnecessary (and
> incorrect) register write in access_pmu_evtyper().
>
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>

Reviewed-by: Reiji Watanabe <reijiw@google.com>

FYI. Looking at their reset values, fortunately, the pseudo
"unknown" reset values used by reset_pmevtyper() has the
bits [16:11] cleared.
I would think it's a bit fragile, but not a real problem.

Thank you,
Reiji

Re: [PATCH] KVM: arm64: Correctly handle RES0 bits PMEVTYPER<n>_EL0.evtCount

[Oliver Upton]

On Thu, 13 Jul 2023 22:16:49 +0000, Oliver Upton wrote:
> The PMU event ID varies from 10 to 16 bits, depending on the PMU
> version. If the PMU only supports 10 bits of event ID, bits [15:10] of
> the evtCount field behave as RES0.
> 
> While the actual PMU emulation code gets this right (i.e. RES0 bits are
> masked out when programming the perf event), the sysreg emulation writes
> an unmasked value to the in-memory cpu context. The net effect is that
> guest reads and writes of PMEVTYPER<n>_EL0 will see non-RES0 behavior in
> the reserved bits of the field.
> 
> [...]

Applied to kvmarm/fixes, thanks!

[1/1] KVM: arm64: Correctly handle RES0 bits PMEVTYPER<n>_EL0.evtCount
      https://git.kernel.org/kvmarm/kvmarm/c/6d4f9236cd67

--
Best,
Oliver

Re: [PATCH] KVM: arm64: Correctly handle RES0 bits PMEVTYPER<n>_EL0.evtCount

[Oliver Upton]

On Fri, Jul 14, 2023 at 06:46:39AM -0700, Reiji Watanabe wrote:
> On Thu, Jul 13, 2023 at 3:17 PM Oliver Upton <oliver.upton@linux.dev> wrote:
> >
> > The PMU event ID varies from 10 to 16 bits, depending on the PMU
> > version. If the PMU only supports 10 bits of event ID, bits [15:10] of
> > the evtCount field behave as RES0.
> >
> > While the actual PMU emulation code gets this right (i.e. RES0 bits are
> > masked out when programming the perf event), the sysreg emulation writes
> > an unmasked value to the in-memory cpu context. The net effect is that
> > guest reads and writes of PMEVTYPER<n>_EL0 will see non-RES0 behavior in
> > the reserved bits of the field.
> >
> > As it so happens, kvm_pmu_set_counter_event_type() already writes a
> > masked value to the in-memory context that gets overwritten by
> > access_pmu_evtyper(). Fix the issue by removing the unnecessary (and
> > incorrect) register write in access_pmu_evtyper().
> >
> > Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> 
> Reviewed-by: Reiji Watanabe <reijiw@google.com>

Thanks!

> FYI. Looking at their reset values, fortunately, the pseudo
> "unknown" reset values used by reset_pmevtyper() has the
> bits [16:11] cleared.
> I would think it's a bit fragile, but not a real problem.

Fragile indeed. I'd like to clean up the handling a bit more in the
future, because it makes no sense why we mask the value read from
__vcpu_sys_reg() for emulated reads.

-- 
Thanks,
Oliver