Re: [PATCH v2 10/14] KVM: arm64: PMU: Move the ID_AA64DFR0_EL1.PMUver limit to VM creation

From: Marc Zyngier <maz@kernel.org>
To: Reiji Watanabe <reijiw@google.com>
Cc: linux-arm-kernel@lists.infradead.org,
	kvmarm@lists.cs.columbia.edu, kvmarm@lists.linux.dev,
	kvm@vger.kernel.org, James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Oliver Upton <oliver.upton@linux.dev>,
	Ricardo Koller <ricarkol@google.com>
Subject: Re: [PATCH v2 10/14] KVM: arm64: PMU: Move the ID_AA64DFR0_EL1.PMUver limit to VM creation
Date: Thu, 03 Nov 2022 08:44:04 +0000	[thread overview]
Message-ID: <87v8nwfmwb.wl-maz@kernel.org> (raw)
In-Reply-To: <CAAeT=FycObU5eHaR23OZ_PeR6-cQeNrmGs=Mi-VnrVuWR6ovSg@mail.gmail.com>

Hi Reiji,

On Thu, 03 Nov 2022 04:55:52 +0000,
Reiji Watanabe <reijiw@google.com> wrote:
> 
> Hi Marc,
> 
> On Fri, Oct 28, 2022 at 4:16 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> >         case SYS_ID_DFR0_EL1:
> > -               /* Limit guests to PMUv3 for ARMv8.4 */
> > -               val = cpuid_feature_cap_perfmon_field(val,
> > -                                                     ID_DFR0_PERFMON_SHIFT,
> > -                                                     kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0);
> > +               val &= ~ARM64_FEATURE_MASK(ID_DFR0_PERFMON);
> > +               val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_DFR0_PERFMON),
> > +                                 pmuver_to_perfmon(vcpu_pmuver(vcpu)));
> 
> Shouldn't KVM expose the sanitized value as it is when AArch32 is
> not supported at EL0 ? Since the register value is UNKNOWN when AArch32
> is not supported at EL0, I would think this code might change the PERFMON
> field value on such systems (could cause live migration to fail).

I'm not sure this would cause anything to fail as we now treat all
AArch32 idregs as RAZ/WI when AArch32 isn't supported (and the
visibility callback still applies here).

But it doesn't hurt to make pmuver_to_perfmon() return 0 when AArch32
isn't supported, and it will at least make the ID register consistent
from a guest perspective.

I plan to squash the following (untested) hack in:

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 8f4412cd4bf6..3b28ef48a525 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1094,6 +1094,10 @@ static u8 perfmon_to_pmuver(u8 perfmon)
 
 static u8 pmuver_to_perfmon(u8 pmuver)
 {
+       /* If no AArch32, make the field RAZ */
+       if (!kvm_supports_32bit_el0())
+               return 0;
+
        switch (pmuver) {
        case ID_AA64DFR0_EL1_PMUVer_IMP:
                return ID_DFR0_PERFMON_8_0;
@@ -1302,10 +1306,9 @@ static int set_id_dfr0_el1(struct kvm_vcpu *vcpu,
                           const struct sys_reg_desc *rd,
                           u64 val)
 {
-       u8 perfmon, host_perfmon = 0;
+       u8 perfmon, host_perfmon;
 
-       if (system_supports_32bit_el0())
-               host_perfmon = pmuver_to_perfmon(kvm_arm_pmu_get_pmuver_limit());
+       host_perfmon = pmuver_to_perfmon(kvm_arm_pmu_get_pmuver_limit());
 
        /*
         * Allow DFR0_EL1.PerfMon to be set from userspace as long as

> I should have noticed this with the previous version...

No worries, thanks a lot for having had a look!

Thanks,

	M.

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
