Re: [PATCH 0/4] KVM: arm64: PMU: Fix PMUVer handling on heterogeneous PMU systems

From: Marc Zyngier <maz@kernel.org>
To: Reiji Watanabe <reijiw@google.com>
Cc: Oliver Upton <oliver.upton@linux.dev>,
	kvmarm@lists.linux.dev, kvm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Zenghui Yu <yuzenghui@huawei.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Ricardo Koller <ricarkol@google.com>,
	Jing Zhang <jingzhangos@google.com>,
	Raghavendra Rao Anata <rananta@google.com>,
	Will Deacon <will@kernel.org>
Subject: Re: [PATCH 0/4] KVM: arm64: PMU: Fix PMUVer handling on heterogeneous PMU systems
Date: Thu, 01 Jun 2023 06:02:41 +0100	[thread overview]
Message-ID: <87mt1jkc5q.wl-maz@kernel.org> (raw)
In-Reply-To: <20230530125324.ijrwrvoll2detpus@google.com>

Hey Reiji,

On Tue, 30 May 2023 13:53:24 +0100,
Reiji Watanabe <reijiw@google.com> wrote:
> 
> Hi Marc,
> 
> On Mon, May 29, 2023 at 02:39:28PM +0100, Marc Zyngier wrote:
> > On Sat, 27 May 2023 05:02:32 +0100,
> > Reiji Watanabe <reijiw@google.com> wrote:
> > > 
> > > This series fixes issues with PMUVer handling for a guest with
> > > PMU configured on heterogeneous PMU systems.
> > > Specifically, it addresses the following two issues.
> > > 
> > > [A] The default value of ID_AA64DFR0_EL1.PMUVer of the vCPU is set
> > >     to its sanitized value.  This could be inappropriate on
> > >     heterogeneous PMU systems, as arm64_ftr_bits for PMUVer is defined
> > >     as FTR_EXACT with safe_val == 0 (when ID_AA64DFR0_EL1.PMUVer of all
> > >     PEs on the host is not uniform, the sanitized value will be 0).
> > 
> > Why is this a problem? The CPUs don't implement the same version of
> > the architecture, we don't get a PMU. Why should we try to do anything
> > better? I really don't think we should go out or out way and make the
> > code more complicated for something that doesn't really exist.
> 
> Even when the CPUs don't implement the same version of the architecture,
> if one of them implement PMUv3, KVM advertises KVM_CAP_ARM_PMU_V3,
> and allows userspace to configure PMU (KVM_ARM_VCPU_PMU_V3) for vCPUs.

Ah, I see it now. The kernel will register the PMU even if it decides
that advertising it is wrong, and then we pick it up. Great :-/.

> In this case, although KVM provides PMU emulations for the guest,
> the guest's ID_AA64DFR0_EL1.PMUVer will be zero.  Also,
> KVM_SET_ONE_REG for ID_AA64DFR0_EL1 will never work for vCPUs
> with PMU configured on such systems (since KVM also doesn't allow
> userspace to set the PMUVer to 0 for the vCPUs with PMU configured).
> 
> I would think either ID_AA64DFR0_EL1.PMUVer for the guest should
> indicate PMUv3, or KVM should not allow userspace to configure PMU,
> in this case.

My vote is on the latter. Even if a PMU is available, we should rely
on the feature exposed by the kernel to decide whether exposing a PMU
or not.

To be honest, this will affect almost nobody (I only know of a single
one, an obscure ARMv8.0+ARMv8.2 system which is very unlikely to ever
use KVM). I'm happy to take the responsibility to actively break those.

> This series is a fix for the former, mainly to keep the current
> behavior of KVM_CAP_ARM_PMU_V3 and KVM_ARM_VCPU_PMU_V3 on such
> systems, since I wasn't sure if such systems don't really exist :)
> (Also, I plan to implement a similar fix for PMCR_EL0.N on top of
> those changes)
> 
> I could make a fix for the latter instead though. What do you think ?

I think this would be valuable.

Also, didn't you have patches for the EL0 side of the PMU? I've been
trying to look for a new version, but couldn't find it...

Thanks,

	M.

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