| Message ID | 20221028105402.2030192-11-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support | expand |
Hi Marc, On Fri, Oct 28, 2022 at 4:16 AM Marc Zyngier <maz@kernel.org> wrote: > > As further patches will enable the selection of a PMU revision > from userspace, sample the supported PMU revision at VM creation > time, rather than building each time the ID_AA64DFR0_EL1 register > is accessed. > > This shouldn't result in any change in behaviour. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/include/asm/kvm_host.h | 1 + > arch/arm64/kvm/arm.c | 6 +++++ > arch/arm64/kvm/pmu-emul.c | 11 +++++++++ > arch/arm64/kvm/sys_regs.c | 41 +++++++++++++++++++++++++------ > include/kvm/arm_pmu.h | 6 +++++ > 5 files changed, 57 insertions(+), 8 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 45e2136322ba..90c9a2dd3f26 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -163,6 +163,7 @@ struct kvm_arch { > > u8 pfr0_csv2; > u8 pfr0_csv3; > + u8 dfr0_pmuver; > > /* Hypercall features firmware registers' descriptor */ > struct kvm_smccc_features smccc_feat; > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index 94d33e296e10..6b3ed524630d 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -164,6 +164,12 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) > set_default_spectre(kvm); > kvm_arm_init_hypercalls(kvm); > > + /* > + * Initialise the default PMUver before there is a chance to > + * create an actual PMU. > + */ > + kvm->arch.dfr0_pmuver = kvm_arm_pmu_get_pmuver_limit(); > + > return ret; > out_free_stage2_pgd: > kvm_free_stage2_pgd(&kvm->arch.mmu); > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c > index 87585c12ca41..f126b45aa6c6 100644 > --- a/arch/arm64/kvm/pmu-emul.c > +++ b/arch/arm64/kvm/pmu-emul.c > @@ -1049,3 +1049,14 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr) > > return -ENXIO; > } > + > +u8 kvm_arm_pmu_get_pmuver_limit(void) > +{ > + u64 tmp; > + > + tmp = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1); > + tmp = cpuid_feature_cap_perfmon_field(tmp, > + ID_AA64DFR0_EL1_PMUVer_SHIFT, > + ID_AA64DFR0_EL1_PMUVer_V3P4); > + return FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), tmp); > +} > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index f4a7c5abcbca..7a4cd644b9c0 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1062,6 +1062,32 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, > return true; > } > > +static u8 vcpu_pmuver(const struct kvm_vcpu *vcpu) > +{ > + if (kvm_vcpu_has_pmu(vcpu)) > + return vcpu->kvm->arch.dfr0_pmuver; > + > + /* Special case for IMPDEF PMUs that KVM has exposed in the past... */ > + if (vcpu->kvm->arch.dfr0_pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF) > + return ID_AA64DFR0_EL1_PMUVer_IMP_DEF; > + > + /* The real "no PMU" */ > + return 0; > +} > + > +static u8 pmuver_to_perfmon(u8 pmuver) > +{ > + switch (pmuver) { > + case ID_AA64DFR0_EL1_PMUVer_IMP: > + return ID_DFR0_PERFMON_8_0; > + case ID_AA64DFR0_EL1_PMUVer_IMP_DEF: > + return ID_DFR0_PERFMON_IMP_DEF; > + default: > + /* Anything ARMv8.1+ has the same value. For now. */ > + return pmuver; > + } > +} > + > /* Read a sanitised cpufeature ID register by sys_reg_desc */ > static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r) > { > @@ -1111,18 +1137,17 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r > /* Limit debug to ARMv8.0 */ > val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer); > val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer), 6); > - /* Limit guests to PMUv3 for ARMv8.4 */ > - val = cpuid_feature_cap_perfmon_field(val, > - ID_AA64DFR0_EL1_PMUVer_SHIFT, > - kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_EL1_PMUVer_V3P4 : 0); > + /* Set PMUver to the required version */ > + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer); > + val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), > + vcpu_pmuver(vcpu)); > /* Hide SPE from guests */ > val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMSVer); > break; > 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 should have noticed this with the previous version... Thank you, Reiji > break; > } > > diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h > index 96b192139a23..812f729c9108 100644 > --- a/include/kvm/arm_pmu.h > +++ b/include/kvm/arm_pmu.h > @@ -89,6 +89,8 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu); > vcpu->arch.pmu.events = *kvm_get_pmu_events(); \ > } while (0) > > +u8 kvm_arm_pmu_get_pmuver_limit(void); > + > #else > struct kvm_pmu { > }; > @@ -154,6 +156,10 @@ static inline u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) > static inline void kvm_pmu_update_vcpu_events(struct kvm_vcpu *vcpu) {} > static inline void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu) {} > static inline void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu) {} > +static inline u8 kvm_arm_pmu_get_pmuver_limit(void) > +{ > + return 0; > +} > > #endif > > -- > 2.34.1 >
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.
Hi Marc, On Thu, Nov 3, 2022 at 1:44 AM Marc Zyngier <maz@kernel.org> wrote: > > 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). Oops, sorry I totally forgot about that change... > 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 believe the register will be consistent (0) even from a guest perspective with the current patch when AArch32 isn't supported because read_id_reg() checks that with sysreg_visible_as_raz() in the beginning. I withdraw my comment, and the patch looks good to me. Reviewed-by: Reiji Watanabe <reijiw@google.com> Thank you, Reiji > > 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.
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 45e2136322ba..90c9a2dd3f26 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -163,6 +163,7 @@ struct kvm_arch { u8 pfr0_csv2; u8 pfr0_csv3; + u8 dfr0_pmuver; /* Hypercall features firmware registers' descriptor */ struct kvm_smccc_features smccc_feat; diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 94d33e296e10..6b3ed524630d 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -164,6 +164,12 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) set_default_spectre(kvm); kvm_arm_init_hypercalls(kvm); + /* + * Initialise the default PMUver before there is a chance to + * create an actual PMU. + */ + kvm->arch.dfr0_pmuver = kvm_arm_pmu_get_pmuver_limit(); + return ret; out_free_stage2_pgd: kvm_free_stage2_pgd(&kvm->arch.mmu); diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c index 87585c12ca41..f126b45aa6c6 100644 --- a/arch/arm64/kvm/pmu-emul.c +++ b/arch/arm64/kvm/pmu-emul.c @@ -1049,3 +1049,14 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr) return -ENXIO; } + +u8 kvm_arm_pmu_get_pmuver_limit(void) +{ + u64 tmp; + + tmp = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1); + tmp = cpuid_feature_cap_perfmon_field(tmp, + ID_AA64DFR0_EL1_PMUVer_SHIFT, + ID_AA64DFR0_EL1_PMUVer_V3P4); + return FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), tmp); +} diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index f4a7c5abcbca..7a4cd644b9c0 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1062,6 +1062,32 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu, return true; } +static u8 vcpu_pmuver(const struct kvm_vcpu *vcpu) +{ + if (kvm_vcpu_has_pmu(vcpu)) + return vcpu->kvm->arch.dfr0_pmuver; + + /* Special case for IMPDEF PMUs that KVM has exposed in the past... */ + if (vcpu->kvm->arch.dfr0_pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF) + return ID_AA64DFR0_EL1_PMUVer_IMP_DEF; + + /* The real "no PMU" */ + return 0; +} + +static u8 pmuver_to_perfmon(u8 pmuver) +{ + switch (pmuver) { + case ID_AA64DFR0_EL1_PMUVer_IMP: + return ID_DFR0_PERFMON_8_0; + case ID_AA64DFR0_EL1_PMUVer_IMP_DEF: + return ID_DFR0_PERFMON_IMP_DEF; + default: + /* Anything ARMv8.1+ has the same value. For now. */ + return pmuver; + } +} + /* Read a sanitised cpufeature ID register by sys_reg_desc */ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r) { @@ -1111,18 +1137,17 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, struct sys_reg_desc const *r /* Limit debug to ARMv8.0 */ val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer); val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_DebugVer), 6); - /* Limit guests to PMUv3 for ARMv8.4 */ - val = cpuid_feature_cap_perfmon_field(val, - ID_AA64DFR0_EL1_PMUVer_SHIFT, - kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_EL1_PMUVer_V3P4 : 0); + /* Set PMUver to the required version */ + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer); + val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), + vcpu_pmuver(vcpu)); /* Hide SPE from guests */ val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMSVer); break; 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))); break; } diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h index 96b192139a23..812f729c9108 100644 --- a/include/kvm/arm_pmu.h +++ b/include/kvm/arm_pmu.h @@ -89,6 +89,8 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu); vcpu->arch.pmu.events = *kvm_get_pmu_events(); \ } while (0) +u8 kvm_arm_pmu_get_pmuver_limit(void); + #else struct kvm_pmu { }; @@ -154,6 +156,10 @@ static inline u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) static inline void kvm_pmu_update_vcpu_events(struct kvm_vcpu *vcpu) {} static inline void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu) {} static inline void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu) {} +static inline u8 kvm_arm_pmu_get_pmuver_limit(void) +{ + return 0; +} #endif
As further patches will enable the selection of a PMU revision from userspace, sample the supported PMU revision at VM creation time, rather than building each time the ID_AA64DFR0_EL1 register is accessed. This shouldn't result in any change in behaviour. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/include/asm/kvm_host.h | 1 + arch/arm64/kvm/arm.c | 6 +++++ arch/arm64/kvm/pmu-emul.c | 11 +++++++++ arch/arm64/kvm/sys_regs.c | 41 +++++++++++++++++++++++++------ include/kvm/arm_pmu.h | 6 +++++ 5 files changed, 57 insertions(+), 8 deletions(-)