| Message ID | 20210125122638.2947058-7-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: More PMU/debug ID register fixes | expand |
Hi Marc, On 1/25/21 12:26 PM, Marc Zyngier wrote: > Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be > pretty easy. All that is required is support for PMMIR_EL1, which > is read-only, and for which returning 0 is a valid option as long > as we don't advertise STALL_SLOT as an implemented event. According to ARM DDI 0487F.b, page D7-2743: "If ARMv8.4-PMU is implemented: - If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED whether the PMMIR System registers are implemented. - If STALL_SLOT is implemented, then the PMMIR System registers are implemented." I tried to come up with a reason why PMMIR is emulated instead of being left undefined, but I couldn't figure it out. Would you mind adding a comment or changing the commit message to explain that? Thanks, Alex > Let's just do that and adjust what we return to the guest. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/include/asm/sysreg.h | 3 +++ > arch/arm64/kvm/pmu-emul.c | 6 ++++++ > arch/arm64/kvm/sys_regs.c | 11 +++++++---- > 3 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > index 8b5e7e5c3cc8..2fb3f386588c 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -846,7 +846,10 @@ > > #define ID_DFR0_PERFMON_SHIFT 24 > > +#define ID_DFR0_PERFMON_8_0 0x3 > #define ID_DFR0_PERFMON_8_1 0x4 > +#define ID_DFR0_PERFMON_8_4 0x5 > +#define ID_DFR0_PERFMON_8_5 0x6 > > #define ID_ISAR4_SWP_FRAC_SHIFT 28 > #define ID_ISAR4_PSR_M_SHIFT 24 > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c > index 398f6df1bbe4..72cd704a8368 100644 > --- a/arch/arm64/kvm/pmu-emul.c > +++ b/arch/arm64/kvm/pmu-emul.c > @@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) > base = 0; > } else { > val = read_sysreg(pmceid1_el0); > + /* > + * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled > + * as RAZ > + */ > + if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4) > + val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32); > base = 32; > } > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 8f79ec1fffa7..5da536ab738d 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1051,16 +1051,16 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, > /* Limit debug to ARMv8.0 */ > val &= ~FEATURE(ID_AA64DFR0_DEBUGVER); > val |= FIELD_PREP(FEATURE(ID_AA64DFR0_DEBUGVER), 6); > - /* Limit guests to PMUv3 for ARMv8.1 */ > + /* Limit guests to PMUv3 for ARMv8.4 */ > val = cpuid_feature_cap_perfmon_field(val, > ID_AA64DFR0_PMUVER_SHIFT, > - kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_1 : 0); > + kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_4 : 0); > break; > case SYS_ID_DFR0_EL1: > - /* Limit guests to PMUv3 for ARMv8.1 */ > + /* 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_1 : 0); > + kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0); > break; > } > > @@ -1496,6 +1496,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { > > { SYS_DESC(SYS_PMINTENSET_EL1), access_pminten, reset_unknown, PMINTENSET_EL1 }, > { SYS_DESC(SYS_PMINTENCLR_EL1), access_pminten, reset_unknown, PMINTENSET_EL1 }, > + { SYS_DESC(SYS_PMMIR_EL1), trap_raz_wi }, > > { SYS_DESC(SYS_MAIR_EL1), access_vm_reg, reset_unknown, MAIR_EL1 }, > { SYS_DESC(SYS_AMAIR_EL1), access_vm_reg, reset_amair_el1, AMAIR_EL1 }, > @@ -1918,6 +1919,8 @@ static const struct sys_reg_desc cp15_regs[] = { > { Op1( 0), CRn( 9), CRm(14), Op2( 3), access_pmovs }, > { AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 4), access_pmceid }, > { AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 5), access_pmceid }, > + /* PMMIR */ > + { Op1( 0), CRn( 9), CRm(14), Op2( 6), trap_raz_wi }, > > /* PRRR/MAIR0 */ > { AA32(LO), Op1( 0), CRn(10), CRm( 2), Op2( 0), access_vm_reg, NULL, MAIR_EL1 },
Hi Alex, On 2021-01-27 14:09, Alexandru Elisei wrote: > Hi Marc, > > On 1/25/21 12:26 PM, Marc Zyngier wrote: >> Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be >> pretty easy. All that is required is support for PMMIR_EL1, which >> is read-only, and for which returning 0 is a valid option as long >> as we don't advertise STALL_SLOT as an implemented event. > > According to ARM DDI 0487F.b, page D7-2743: > > "If ARMv8.4-PMU is implemented: > - If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED > whether the PMMIR > System registers are implemented. > - If STALL_SLOT is implemented, then the PMMIR System registers are > implemented." > > I tried to come up with a reason why PMMIR is emulated instead of being > left > undefined, but I couldn't figure it out. Would you mind adding a > comment or > changing the commit message to explain that? The main reason is that PMMIR gets new fields down the line, and doing the bare minimum in term of implementation allows us to gently ease into it. We could also go for the full PMMIR reporting on homogeneous systems too, as a further improvement. What do you think? M.
Hi Marc, On 1/27/21 2:35 PM, Marc Zyngier wrote: > Hi Alex, > > On 2021-01-27 14:09, Alexandru Elisei wrote: >> Hi Marc, >> >> On 1/25/21 12:26 PM, Marc Zyngier wrote: >>> Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be >>> pretty easy. All that is required is support for PMMIR_EL1, which >>> is read-only, and for which returning 0 is a valid option as long >>> as we don't advertise STALL_SLOT as an implemented event. >> >> According to ARM DDI 0487F.b, page D7-2743: >> >> "If ARMv8.4-PMU is implemented: >> - If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED >> whether the PMMIR >> System registers are implemented. >> - If STALL_SLOT is implemented, then the PMMIR System registers are >> implemented." >> >> I tried to come up with a reason why PMMIR is emulated instead of being left >> undefined, but I couldn't figure it out. Would you mind adding a comment or >> changing the commit message to explain that? > > The main reason is that PMMIR gets new fields down the line, > and doing the bare minimum in term of implementation allows > us to gently ease into it. I think I understand what you are saying - add a bare minimum emulation of the PMMIR register now so it's less work when we do decide to support the STALL_SLOT event for a guest. > > We could also go for the full PMMIR reporting on homogeneous > systems too, as a further improvement. > > What do you think? I don't have an opinion either way. But if you do decide to add full emulation for STALL_SLOT, I would like to help with reviewing the patches (I'm curious to see how KVM will detect that it's running on a homogeneous system). Thanks, Alex
On 2021-01-27 17:00, Alexandru Elisei wrote: > Hi Marc, > > On 1/27/21 2:35 PM, Marc Zyngier wrote: >> Hi Alex, >> >> On 2021-01-27 14:09, Alexandru Elisei wrote: >>> Hi Marc, >>> >>> On 1/25/21 12:26 PM, Marc Zyngier wrote: >>>> Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be >>>> pretty easy. All that is required is support for PMMIR_EL1, which >>>> is read-only, and for which returning 0 is a valid option as long >>>> as we don't advertise STALL_SLOT as an implemented event. >>> >>> According to ARM DDI 0487F.b, page D7-2743: >>> >>> "If ARMv8.4-PMU is implemented: >>> - If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED >>> whether the PMMIR >>> System registers are implemented. >>> - If STALL_SLOT is implemented, then the PMMIR System registers are >>> implemented." >>> >>> I tried to come up with a reason why PMMIR is emulated instead of >>> being left >>> undefined, but I couldn't figure it out. Would you mind adding a >>> comment or >>> changing the commit message to explain that? >> >> The main reason is that PMMIR gets new fields down the line, >> and doing the bare minimum in term of implementation allows >> us to gently ease into it. > I think I understand what you are saying - add a bare minimum emulation > of the > PMMIR register now so it's less work when we do decide to support the > STALL_SLOT > event for a guest. >> >> We could also go for the full PMMIR reporting on homogeneous >> systems too, as a further improvement. >> >> What do you think? > > I don't have an opinion either way. But if you do decide to add full > emulation for > STALL_SLOT, I would like to help with reviewing the patches (I'm > curious to see > how KVM will detect that it's running on a homogeneous system). I'm not sure we can *detect* it. We'd need some more information from the core arch code and firmware. To be honest, PMU emulation is a joke on BL, so maybe we shouldn't really care and expose what we have. M.
Hi Marc, Had another look at the patch, comments below. On 1/25/21 12:26 PM, Marc Zyngier wrote: > Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be > pretty easy. All that is required is support for PMMIR_EL1, which > is read-only, and for which returning 0 is a valid option as long > as we don't advertise STALL_SLOT as an implemented event. > > Let's just do that and adjust what we return to the guest. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/include/asm/sysreg.h | 3 +++ > arch/arm64/kvm/pmu-emul.c | 6 ++++++ > arch/arm64/kvm/sys_regs.c | 11 +++++++---- > 3 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > index 8b5e7e5c3cc8..2fb3f386588c 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -846,7 +846,10 @@ > > #define ID_DFR0_PERFMON_SHIFT 24 > > +#define ID_DFR0_PERFMON_8_0 0x3 > #define ID_DFR0_PERFMON_8_1 0x4 > +#define ID_DFR0_PERFMON_8_4 0x5 > +#define ID_DFR0_PERFMON_8_5 0x6 > > #define ID_ISAR4_SWP_FRAC_SHIFT 28 > #define ID_ISAR4_PSR_M_SHIFT 24 > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c > index 398f6df1bbe4..72cd704a8368 100644 > --- a/arch/arm64/kvm/pmu-emul.c > +++ b/arch/arm64/kvm/pmu-emul.c > @@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) > base = 0; > } else { > val = read_sysreg(pmceid1_el0); > + /* > + * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled > + * as RAZ > + */ > + if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4) > + val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32); This is confusing the me. We have kvm->arch.pmuver set to the hardware PMU version (as set by __armv8pmu_probe_pmu()), but we ignore it when reporting the PMU version to the guest. Why do we do that? We limit the event number in kvm_pmu_event_mask() based on the hardware PMU version, so even if we advertise Armv8.4 PMU, support for all those extra events added by Arm8.1 PMU is missing (I hope I understood the code correctly). I looked at commit c854188ea010 ("KVM: arm64: limit PMU version to PMUv3 for ARMv8.1") which changed read_id_reg() to report PMUv3 for Armv8.1 unconditionally, and there's no explanation why PMUv3 for Armv8.1 was chosen instead of plain PMUv3 (PMUVer = 0b0100). Thanks, Alex > base = 32; > } > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 8f79ec1fffa7..5da536ab738d 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1051,16 +1051,16 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, > /* Limit debug to ARMv8.0 */ > val &= ~FEATURE(ID_AA64DFR0_DEBUGVER); > val |= FIELD_PREP(FEATURE(ID_AA64DFR0_DEBUGVER), 6); > - /* Limit guests to PMUv3 for ARMv8.1 */ > + /* Limit guests to PMUv3 for ARMv8.4 */ > val = cpuid_feature_cap_perfmon_field(val, > ID_AA64DFR0_PMUVER_SHIFT, > - kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_1 : 0); > + kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_4 : 0); > break; > case SYS_ID_DFR0_EL1: > - /* Limit guests to PMUv3 for ARMv8.1 */ > + /* 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_1 : 0); > + kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0); > break; > } > > @@ -1496,6 +1496,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { > > { SYS_DESC(SYS_PMINTENSET_EL1), access_pminten, reset_unknown, PMINTENSET_EL1 }, > { SYS_DESC(SYS_PMINTENCLR_EL1), access_pminten, reset_unknown, PMINTENSET_EL1 }, > + { SYS_DESC(SYS_PMMIR_EL1), trap_raz_wi }, > > { SYS_DESC(SYS_MAIR_EL1), access_vm_reg, reset_unknown, MAIR_EL1 }, > { SYS_DESC(SYS_AMAIR_EL1), access_vm_reg, reset_amair_el1, AMAIR_EL1 }, > @@ -1918,6 +1919,8 @@ static const struct sys_reg_desc cp15_regs[] = { > { Op1( 0), CRn( 9), CRm(14), Op2( 3), access_pmovs }, > { AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 4), access_pmceid }, > { AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 5), access_pmceid }, > + /* PMMIR */ > + { Op1( 0), CRn( 9), CRm(14), Op2( 6), trap_raz_wi }, > > /* PRRR/MAIR0 */ > { AA32(LO), Op1( 0), CRn(10), CRm( 2), Op2( 0), access_vm_reg, NULL, MAIR_EL1 },
Hi Marc, On 1/25/21 1:26 PM, Marc Zyngier wrote: > Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be > pretty easy. All that is required is support for PMMIR_EL1, which > is read-only, and for which returning 0 is a valid option as long > as we don't advertise STALL_SLOT as an implemented event. > > Let's just do that and adjust what we return to the guest. > > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > arch/arm64/include/asm/sysreg.h | 3 +++ > arch/arm64/kvm/pmu-emul.c | 6 ++++++ > arch/arm64/kvm/sys_regs.c | 11 +++++++---- > 3 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > index 8b5e7e5c3cc8..2fb3f386588c 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -846,7 +846,10 @@ > > #define ID_DFR0_PERFMON_SHIFT 24 > > +#define ID_DFR0_PERFMON_8_0 0x3 > #define ID_DFR0_PERFMON_8_1 0x4 > +#define ID_DFR0_PERFMON_8_4 0x5 > +#define ID_DFR0_PERFMON_8_5 0x6 > > #define ID_ISAR4_SWP_FRAC_SHIFT 28 > #define ID_ISAR4_PSR_M_SHIFT 24 > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c > index 398f6df1bbe4..72cd704a8368 100644 > --- a/arch/arm64/kvm/pmu-emul.c > +++ b/arch/arm64/kvm/pmu-emul.c > @@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) > base = 0; > } else { > val = read_sysreg(pmceid1_el0); > + /* > + * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled > + * as RAZ > + */ > + if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4) > + val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32); what about the STALL_SLOT_BACKEND and FRONTEND events then? > base = 32; > } > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 8f79ec1fffa7..5da536ab738d 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1051,16 +1051,16 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, > /* Limit debug to ARMv8.0 */ > val &= ~FEATURE(ID_AA64DFR0_DEBUGVER); > val |= FIELD_PREP(FEATURE(ID_AA64DFR0_DEBUGVER), 6); > - /* Limit guests to PMUv3 for ARMv8.1 */ > + /* Limit guests to PMUv3 for ARMv8.4 */ > val = cpuid_feature_cap_perfmon_field(val, > ID_AA64DFR0_PMUVER_SHIFT, > - kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_1 : 0); > + kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_4 : 0); > break; > case SYS_ID_DFR0_EL1: > - /* Limit guests to PMUv3 for ARMv8.1 */ > + /* 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_1 : 0); > + kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0); > break; > } > > @@ -1496,6 +1496,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { > > { SYS_DESC(SYS_PMINTENSET_EL1), access_pminten, reset_unknown, PMINTENSET_EL1 }, > { SYS_DESC(SYS_PMINTENCLR_EL1), access_pminten, reset_unknown, PMINTENSET_EL1 }, "KVM: arm64: Hide PMU registers from userspace when not available" changed the above, doesn't it? > + { SYS_DESC(SYS_PMMIR_EL1), trap_raz_wi }, > > { SYS_DESC(SYS_MAIR_EL1), access_vm_reg, reset_unknown, MAIR_EL1 }, > { SYS_DESC(SYS_AMAIR_EL1), access_vm_reg, reset_amair_el1, AMAIR_EL1 }, > @@ -1918,6 +1919,8 @@ static const struct sys_reg_desc cp15_regs[] = { > { Op1( 0), CRn( 9), CRm(14), Op2( 3), access_pmovs }, > { AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 4), access_pmceid }, > { AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 5), access_pmceid }, > + /* PMMIR */ > + { Op1( 0), CRn( 9), CRm(14), Op2( 6), trap_raz_wi }, > > /* PRRR/MAIR0 */ > { AA32(LO), Op1( 0), CRn(10), CRm( 2), Op2( 0), access_vm_reg, NULL, MAIR_EL1 }, > Thanks Eric
On 2021-01-27 17:41, Alexandru Elisei wrote: > Hi Marc, > > Had another look at the patch, comments below. > > On 1/25/21 12:26 PM, Marc Zyngier wrote: >> Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be >> pretty easy. All that is required is support for PMMIR_EL1, which >> is read-only, and for which returning 0 is a valid option as long >> as we don't advertise STALL_SLOT as an implemented event. >> >> Let's just do that and adjust what we return to the guest. >> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> arch/arm64/include/asm/sysreg.h | 3 +++ >> arch/arm64/kvm/pmu-emul.c | 6 ++++++ >> arch/arm64/kvm/sys_regs.c | 11 +++++++---- >> 3 files changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm64/include/asm/sysreg.h >> b/arch/arm64/include/asm/sysreg.h >> index 8b5e7e5c3cc8..2fb3f386588c 100644 >> --- a/arch/arm64/include/asm/sysreg.h >> +++ b/arch/arm64/include/asm/sysreg.h >> @@ -846,7 +846,10 @@ >> >> #define ID_DFR0_PERFMON_SHIFT 24 >> >> +#define ID_DFR0_PERFMON_8_0 0x3 >> #define ID_DFR0_PERFMON_8_1 0x4 >> +#define ID_DFR0_PERFMON_8_4 0x5 >> +#define ID_DFR0_PERFMON_8_5 0x6 >> >> #define ID_ISAR4_SWP_FRAC_SHIFT 28 >> #define ID_ISAR4_PSR_M_SHIFT 24 >> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c >> index 398f6df1bbe4..72cd704a8368 100644 >> --- a/arch/arm64/kvm/pmu-emul.c >> +++ b/arch/arm64/kvm/pmu-emul.c >> @@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, >> bool pmceid1) >> base = 0; >> } else { >> val = read_sysreg(pmceid1_el0); >> + /* >> + * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled >> + * as RAZ >> + */ >> + if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4) >> + val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32); > > This is confusing the me. We have kvm->arch.pmuver set to the hardware > PMU version > (as set by __armv8pmu_probe_pmu()), but we ignore it when reporting the > PMU > version to the guest. Why do we do that? We limit the event number in > kvm_pmu_event_mask() based on the hardware PMU version, so even if we > advertise > Armv8.4 PMU, support for all those extra events added by Arm8.1 PMU is > missing (I hope I understood the code correctly). That's a bit of mess-up. We obtain ID_AA64DFR0_EL1 from the sanitised regs, but do most of our handling based on kvm->arch.pmuver. They really should be the same, because that's what the sanitised registers give you. As for the events themselves, I don't get your drift. We do support all the ARMv8.1 PMU events as long as the HW supports it, and we don't lie to the guest about it either (cpuid_feature_cap_perfmon_field does *cap* the field to some value, it doesn't allow it to increase past what the HW supports). > I looked at commit c854188ea010 ("KVM: arm64: limit PMU version to > PMUv3 for > ARMv8.1") which changed read_id_reg() to report PMUv3 for Armv8.1 > unconditionally, > and there's no explanation why PMUv3 for Armv8.1 was chosen instead of > plain PMUv3 (PMUVer = 0b0100). We picked ARMv8.1 because this is the first PMU revision that gives you events in the 0x4000 range, all of which are available on common ARMv8.2 HW. Thanks, M.
Hi Eric, On 2021-01-27 17:53, Auger Eric wrote: > Hi Marc, > > On 1/25/21 1:26 PM, Marc Zyngier wrote: >> Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be >> pretty easy. All that is required is support for PMMIR_EL1, which >> is read-only, and for which returning 0 is a valid option as long >> as we don't advertise STALL_SLOT as an implemented event. >> >> Let's just do that and adjust what we return to the guest. >> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> arch/arm64/include/asm/sysreg.h | 3 +++ >> arch/arm64/kvm/pmu-emul.c | 6 ++++++ >> arch/arm64/kvm/sys_regs.c | 11 +++++++---- >> 3 files changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm64/include/asm/sysreg.h >> b/arch/arm64/include/asm/sysreg.h >> index 8b5e7e5c3cc8..2fb3f386588c 100644 >> --- a/arch/arm64/include/asm/sysreg.h >> +++ b/arch/arm64/include/asm/sysreg.h >> @@ -846,7 +846,10 @@ >> >> #define ID_DFR0_PERFMON_SHIFT 24 >> >> +#define ID_DFR0_PERFMON_8_0 0x3 >> #define ID_DFR0_PERFMON_8_1 0x4 >> +#define ID_DFR0_PERFMON_8_4 0x5 >> +#define ID_DFR0_PERFMON_8_5 0x6 >> >> #define ID_ISAR4_SWP_FRAC_SHIFT 28 >> #define ID_ISAR4_PSR_M_SHIFT 24 >> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c >> index 398f6df1bbe4..72cd704a8368 100644 >> --- a/arch/arm64/kvm/pmu-emul.c >> +++ b/arch/arm64/kvm/pmu-emul.c >> @@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, >> bool pmceid1) >> base = 0; >> } else { >> val = read_sysreg(pmceid1_el0); >> + /* >> + * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled >> + * as RAZ >> + */ >> + if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4) >> + val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32); > what about the STALL_SLOT_BACKEND and FRONTEND events then? Aren't these a mandatory ARMv8.1 feature? I don't see a reason to drop them. >> base = 32; >> } >> >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >> index 8f79ec1fffa7..5da536ab738d 100644 >> --- a/arch/arm64/kvm/sys_regs.c >> +++ b/arch/arm64/kvm/sys_regs.c >> @@ -1051,16 +1051,16 @@ static u64 read_id_reg(const struct kvm_vcpu >> *vcpu, >> /* Limit debug to ARMv8.0 */ >> val &= ~FEATURE(ID_AA64DFR0_DEBUGVER); >> val |= FIELD_PREP(FEATURE(ID_AA64DFR0_DEBUGVER), 6); >> - /* Limit guests to PMUv3 for ARMv8.1 */ >> + /* Limit guests to PMUv3 for ARMv8.4 */ >> val = cpuid_feature_cap_perfmon_field(val, >> ID_AA64DFR0_PMUVER_SHIFT, >> - kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_1 : 0); >> + kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_4 : 0); >> break; >> case SYS_ID_DFR0_EL1: >> - /* Limit guests to PMUv3 for ARMv8.1 */ >> + /* 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_1 : 0); >> + kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0); >> break; >> } >> >> @@ -1496,6 +1496,7 @@ static const struct sys_reg_desc sys_reg_descs[] >> = { >> >> { SYS_DESC(SYS_PMINTENSET_EL1), access_pminten, reset_unknown, >> PMINTENSET_EL1 }, >> { SYS_DESC(SYS_PMINTENCLR_EL1), access_pminten, reset_unknown, >> PMINTENSET_EL1 }, > "KVM: arm64: Hide PMU registers from userspace when not available" > changed the above, doesn't it? Yes, that's because the fix didn't make it in mainline before 5.11-rc5, and I based this on -rc4. I'll fix it at merge time. Thanks, M.
Hi Marc, On 2/3/21 11:36 AM, Marc Zyngier wrote: > Hi Eric, > > On 2021-01-27 17:53, Auger Eric wrote: >> Hi Marc, >> >> On 1/25/21 1:26 PM, Marc Zyngier wrote: >>> Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be >>> pretty easy. All that is required is support for PMMIR_EL1, which >>> is read-only, and for which returning 0 is a valid option as long >>> as we don't advertise STALL_SLOT as an implemented event. >>> >>> Let's just do that and adjust what we return to the guest. >>> >>> Signed-off-by: Marc Zyngier <maz@kernel.org> >>> --- >>> arch/arm64/include/asm/sysreg.h | 3 +++ >>> arch/arm64/kvm/pmu-emul.c | 6 ++++++ >>> arch/arm64/kvm/sys_regs.c | 11 +++++++---- >>> 3 files changed, 16 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/arm64/include/asm/sysreg.h >>> b/arch/arm64/include/asm/sysreg.h >>> index 8b5e7e5c3cc8..2fb3f386588c 100644 >>> --- a/arch/arm64/include/asm/sysreg.h >>> +++ b/arch/arm64/include/asm/sysreg.h >>> @@ -846,7 +846,10 @@ >>> >>> #define ID_DFR0_PERFMON_SHIFT 24 >>> >>> +#define ID_DFR0_PERFMON_8_0 0x3 >>> #define ID_DFR0_PERFMON_8_1 0x4 >>> +#define ID_DFR0_PERFMON_8_4 0x5 >>> +#define ID_DFR0_PERFMON_8_5 0x6 >>> >>> #define ID_ISAR4_SWP_FRAC_SHIFT 28 >>> #define ID_ISAR4_PSR_M_SHIFT 24 >>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c >>> index 398f6df1bbe4..72cd704a8368 100644 >>> --- a/arch/arm64/kvm/pmu-emul.c >>> +++ b/arch/arm64/kvm/pmu-emul.c >>> @@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, >>> bool pmceid1) >>> base = 0; >>> } else { >>> val = read_sysreg(pmceid1_el0); >>> + /* >>> + * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled >>> + * as RAZ >>> + */ >>> + if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4) >>> + val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32); >> what about the STALL_SLOT_BACKEND and FRONTEND events then? > > Aren't these a mandatory ARMv8.1 feature? I don't see a reason to > drop them. I understand the 3 are linked together. In D7.11 it is said " When any of the following common events are implemented, all three of them are implemented: 0x003D , STALL_SLOT_BACKEND, No operation sent for execution on a Slot due to the backend, 0x003E , STALL_SLOT_FRONTEND, No operation sent for execution on a Slot due to the frontend. 0x003F , STALL_SLOT, No operation sent for execution on a Slot. " Thanks Eric > >>> base = 32; >>> } >>> >>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >>> index 8f79ec1fffa7..5da536ab738d 100644 >>> --- a/arch/arm64/kvm/sys_regs.c >>> +++ b/arch/arm64/kvm/sys_regs.c >>> @@ -1051,16 +1051,16 @@ static u64 read_id_reg(const struct kvm_vcpu >>> *vcpu, >>> /* Limit debug to ARMv8.0 */ >>> val &= ~FEATURE(ID_AA64DFR0_DEBUGVER); >>> val |= FIELD_PREP(FEATURE(ID_AA64DFR0_DEBUGVER), 6); >>> - /* Limit guests to PMUv3 for ARMv8.1 */ >>> + /* Limit guests to PMUv3 for ARMv8.4 */ >>> val = cpuid_feature_cap_perfmon_field(val, >>> ID_AA64DFR0_PMUVER_SHIFT, >>> - kvm_vcpu_has_pmu(vcpu) ? >>> ID_AA64DFR0_PMUVER_8_1 : 0); >>> + kvm_vcpu_has_pmu(vcpu) ? >>> ID_AA64DFR0_PMUVER_8_4 : 0); >>> break; >>> case SYS_ID_DFR0_EL1: >>> - /* Limit guests to PMUv3 for ARMv8.1 */ >>> + /* 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_1 : 0); >>> + kvm_vcpu_has_pmu(vcpu) ? >>> ID_DFR0_PERFMON_8_4 : 0); >>> break; >>> } >>> >>> @@ -1496,6 +1496,7 @@ static const struct sys_reg_desc >>> sys_reg_descs[] = { >>> >>> { SYS_DESC(SYS_PMINTENSET_EL1), access_pminten, reset_unknown, >>> PMINTENSET_EL1 }, >>> { SYS_DESC(SYS_PMINTENCLR_EL1), access_pminten, reset_unknown, >>> PMINTENSET_EL1 }, >> "KVM: arm64: Hide PMU registers from userspace when not available" >> changed the above, doesn't it? > > Yes, that's because the fix didn't make it in mainline before > 5.11-rc5, and I based this on -rc4. I'll fix it at merge time. > > Thanks, > > M.
On 2021-02-03 11:07, Auger Eric wrote: > Hi Marc, > On 2/3/21 11:36 AM, Marc Zyngier wrote: >> Hi Eric, >> >> On 2021-01-27 17:53, Auger Eric wrote: >>> Hi Marc, >>> >>> On 1/25/21 1:26 PM, Marc Zyngier wrote: >>>> Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be >>>> pretty easy. All that is required is support for PMMIR_EL1, which >>>> is read-only, and for which returning 0 is a valid option as long >>>> as we don't advertise STALL_SLOT as an implemented event. >>>> >>>> Let's just do that and adjust what we return to the guest. >>>> >>>> Signed-off-by: Marc Zyngier <maz@kernel.org> >>>> --- >>>> arch/arm64/include/asm/sysreg.h | 3 +++ >>>> arch/arm64/kvm/pmu-emul.c | 6 ++++++ >>>> arch/arm64/kvm/sys_regs.c | 11 +++++++---- >>>> 3 files changed, 16 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/arch/arm64/include/asm/sysreg.h >>>> b/arch/arm64/include/asm/sysreg.h >>>> index 8b5e7e5c3cc8..2fb3f386588c 100644 >>>> --- a/arch/arm64/include/asm/sysreg.h >>>> +++ b/arch/arm64/include/asm/sysreg.h >>>> @@ -846,7 +846,10 @@ >>>> >>>> #define ID_DFR0_PERFMON_SHIFT 24 >>>> >>>> +#define ID_DFR0_PERFMON_8_0 0x3 >>>> #define ID_DFR0_PERFMON_8_1 0x4 >>>> +#define ID_DFR0_PERFMON_8_4 0x5 >>>> +#define ID_DFR0_PERFMON_8_5 0x6 >>>> >>>> #define ID_ISAR4_SWP_FRAC_SHIFT 28 >>>> #define ID_ISAR4_PSR_M_SHIFT 24 >>>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c >>>> index 398f6df1bbe4..72cd704a8368 100644 >>>> --- a/arch/arm64/kvm/pmu-emul.c >>>> +++ b/arch/arm64/kvm/pmu-emul.c >>>> @@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, >>>> bool pmceid1) >>>> base = 0; >>>> } else { >>>> val = read_sysreg(pmceid1_el0); >>>> + /* >>>> + * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled >>>> + * as RAZ >>>> + */ >>>> + if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4) >>>> + val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32); >>> what about the STALL_SLOT_BACKEND and FRONTEND events then? >> >> Aren't these a mandatory ARMv8.1 feature? I don't see a reason to >> drop them. > > I understand the 3 are linked together. > > In D7.11 it is said > " > When any of the following common events are implemented, all three of > them are implemented: > 0x003D , STALL_SLOT_BACKEND, No operation sent for execution on a Slot > due to the backend, > 0x003E , STALL_SLOT_FRONTEND, No operation sent for execution on a Slot > due to the frontend. > 0x003F , STALL_SLOT, No operation sent for execution on a Slot. > " They are linked in the sense that they report related events, but they don't have to be implemented in the same level of the architecure, if only because BACKEND/FRONTEND were introducedway before ARMv8.4. What the architecture says is: - For FEAT_PMUv3p1 (ARMv8.1): "The STALL_FRONTEND and STALL_BACKEND events are required to be implemented." (A2.4.1, DDI0487G.a) - For FEAT_PMUv3p4 (ARMv8.4): "If FEAT_PMUv3p4 is implemented: - If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED whether the PMMIR System registers are implemented. - If STALL_SLOT is implemented, then the PMMIR System registers are implemented." (D7-2873, DDI0487G.a) So while BACKEND/FRONTEND are required in an ARMv8.4 implementation by virtue of being mandatory in ARMv8.1, STALL_SLOT isn't at any point. Thanks, M.
Hi, On 2/3/21 12:20 PM, Marc Zyngier wrote: > On 2021-02-03 11:07, Auger Eric wrote: >> Hi Marc, >> On 2/3/21 11:36 AM, Marc Zyngier wrote: >>> Hi Eric, >>> >>> On 2021-01-27 17:53, Auger Eric wrote: >>>> Hi Marc, >>>> >>>> On 1/25/21 1:26 PM, Marc Zyngier wrote: >>>>> Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be >>>>> pretty easy. All that is required is support for PMMIR_EL1, which >>>>> is read-only, and for which returning 0 is a valid option as long >>>>> as we don't advertise STALL_SLOT as an implemented event. >>>>> >>>>> Let's just do that and adjust what we return to the guest. >>>>> >>>>> Signed-off-by: Marc Zyngier <maz@kernel.org> >>>>> --- >>>>> arch/arm64/include/asm/sysreg.h | 3 +++ >>>>> arch/arm64/kvm/pmu-emul.c | 6 ++++++ >>>>> arch/arm64/kvm/sys_regs.c | 11 +++++++---- >>>>> 3 files changed, 16 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/arch/arm64/include/asm/sysreg.h >>>>> b/arch/arm64/include/asm/sysreg.h >>>>> index 8b5e7e5c3cc8..2fb3f386588c 100644 >>>>> --- a/arch/arm64/include/asm/sysreg.h >>>>> +++ b/arch/arm64/include/asm/sysreg.h >>>>> @@ -846,7 +846,10 @@ >>>>> >>>>> #define ID_DFR0_PERFMON_SHIFT 24 >>>>> >>>>> +#define ID_DFR0_PERFMON_8_0 0x3 >>>>> #define ID_DFR0_PERFMON_8_1 0x4 >>>>> +#define ID_DFR0_PERFMON_8_4 0x5 >>>>> +#define ID_DFR0_PERFMON_8_5 0x6 >>>>> >>>>> #define ID_ISAR4_SWP_FRAC_SHIFT 28 >>>>> #define ID_ISAR4_PSR_M_SHIFT 24 >>>>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c >>>>> index 398f6df1bbe4..72cd704a8368 100644 >>>>> --- a/arch/arm64/kvm/pmu-emul.c >>>>> +++ b/arch/arm64/kvm/pmu-emul.c >>>>> @@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, >>>>> bool pmceid1) >>>>> base = 0; >>>>> } else { >>>>> val = read_sysreg(pmceid1_el0); >>>>> + /* >>>>> + * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled >>>>> + * as RAZ >>>>> + */ >>>>> + if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4) >>>>> + val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32); >>>> what about the STALL_SLOT_BACKEND and FRONTEND events then? >>> >>> Aren't these a mandatory ARMv8.1 feature? I don't see a reason to >>> drop them. >> >> I understand the 3 are linked together. >> >> In D7.11 it is said >> " >> When any of the following common events are implemented, all three of >> them are implemented: >> 0x003D , STALL_SLOT_BACKEND, No operation sent for execution on a Slot >> due to the backend, >> 0x003E , STALL_SLOT_FRONTEND, No operation sent for execution on a Slot >> due to the frontend. >> 0x003F , STALL_SLOT, No operation sent for execution on a Slot. >> " > > They are linked in the sense that they report related events, but they > don't have to be implemented in the same level of the architecure, if only > because BACKEND/FRONTEND were introducedway before ARMv8.4. > > What the architecture says is: > > - For FEAT_PMUv3p1 (ARMv8.1): > "The STALL_FRONTEND and STALL_BACKEND events are required to be > implemented." (A2.4.1, DDI0487G.a) OK > > - For FEAT_PMUv3p4 (ARMv8.4): > "If FEAT_PMUv3p4 is implemented: > - If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED > whether the PMMIR System registers are implemented. > - If STALL_SLOT is implemented, then the PMMIR System registers are > implemented." (D7-2873, DDI0487G.a) > > So while BACKEND/FRONTEND are required in an ARMv8.4 implementation > by virtue of being mandatory in ARMv8.1, STALL_SLOT isn't at any point. But then how do you understand "When any of the following common events are implemented, all three of them are implemented"? Eric > > Thanks, > > M.
On 2021-02-03 12:39, Auger Eric wrote: > Hi, > > On 2/3/21 12:20 PM, Marc Zyngier wrote: >> On 2021-02-03 11:07, Auger Eric wrote: >>> Hi Marc, >>> On 2/3/21 11:36 AM, Marc Zyngier wrote: >>>> Hi Eric, >>>> >>>> On 2021-01-27 17:53, Auger Eric wrote: >>>>> Hi Marc, >>>>> >>>>> On 1/25/21 1:26 PM, Marc Zyngier wrote: >>>>>> Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be >>>>>> pretty easy. All that is required is support for PMMIR_EL1, which >>>>>> is read-only, and for which returning 0 is a valid option as long >>>>>> as we don't advertise STALL_SLOT as an implemented event. >>>>>> >>>>>> Let's just do that and adjust what we return to the guest. >>>>>> >>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org> >>>>>> --- >>>>>> arch/arm64/include/asm/sysreg.h | 3 +++ >>>>>> arch/arm64/kvm/pmu-emul.c | 6 ++++++ >>>>>> arch/arm64/kvm/sys_regs.c | 11 +++++++---- >>>>>> 3 files changed, 16 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm64/include/asm/sysreg.h >>>>>> b/arch/arm64/include/asm/sysreg.h >>>>>> index 8b5e7e5c3cc8..2fb3f386588c 100644 >>>>>> --- a/arch/arm64/include/asm/sysreg.h >>>>>> +++ b/arch/arm64/include/asm/sysreg.h >>>>>> @@ -846,7 +846,10 @@ >>>>>> >>>>>> #define ID_DFR0_PERFMON_SHIFT 24 >>>>>> >>>>>> +#define ID_DFR0_PERFMON_8_0 0x3 >>>>>> #define ID_DFR0_PERFMON_8_1 0x4 >>>>>> +#define ID_DFR0_PERFMON_8_4 0x5 >>>>>> +#define ID_DFR0_PERFMON_8_5 0x6 >>>>>> >>>>>> #define ID_ISAR4_SWP_FRAC_SHIFT 28 >>>>>> #define ID_ISAR4_PSR_M_SHIFT 24 >>>>>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c >>>>>> index 398f6df1bbe4..72cd704a8368 100644 >>>>>> --- a/arch/arm64/kvm/pmu-emul.c >>>>>> +++ b/arch/arm64/kvm/pmu-emul.c >>>>>> @@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, >>>>>> bool pmceid1) >>>>>> base = 0; >>>>>> } else { >>>>>> val = read_sysreg(pmceid1_el0); >>>>>> + /* >>>>>> + * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled >>>>>> + * as RAZ >>>>>> + */ >>>>>> + if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4) >>>>>> + val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32); >>>>> what about the STALL_SLOT_BACKEND and FRONTEND events then? >>>> >>>> Aren't these a mandatory ARMv8.1 feature? I don't see a reason to >>>> drop them. >>> >>> I understand the 3 are linked together. >>> >>> In D7.11 it is said >>> " >>> When any of the following common events are implemented, all three of >>> them are implemented: >>> 0x003D , STALL_SLOT_BACKEND, No operation sent for execution on a >>> Slot >>> due to the backend, >>> 0x003E , STALL_SLOT_FRONTEND, No operation sent for execution on a >>> Slot >>> due to the frontend. >>> 0x003F , STALL_SLOT, No operation sent for execution on a Slot. >>> " >> >> They are linked in the sense that they report related events, but they >> don't have to be implemented in the same level of the architecure, if >> only >> because BACKEND/FRONTEND were introducedway before ARMv8.4. >> >> What the architecture says is: >> >> - For FEAT_PMUv3p1 (ARMv8.1): >> "The STALL_FRONTEND and STALL_BACKEND events are required to be >> implemented." (A2.4.1, DDI0487G.a) > OK >> >> - For FEAT_PMUv3p4 (ARMv8.4): >> "If FEAT_PMUv3p4 is implemented: >> - If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED >> whether the PMMIR System registers are implemented. >> - If STALL_SLOT is implemented, then the PMMIR System registers are >> implemented." (D7-2873, DDI0487G.a) >> >> So while BACKEND/FRONTEND are required in an ARMv8.4 implementation >> by virtue of being mandatory in ARMv8.1, STALL_SLOT isn't at any >> point. > But then how do you understand "When any of the following common events > are implemented, all three of them are implemented"? I think that's wholly inconsistent, because it would mean that STALL_SLOT isn't optional on ARMv8.4, and would make PMMIR mandatory. I'm starting to think that dropping this patch may be the best thing to do... Thanks, M.
Hi Marc, On 2/3/21 10:32 AM, Marc Zyngier wrote: > On 2021-01-27 17:41, Alexandru Elisei wrote: >> Hi Marc, >> >> Had another look at the patch, comments below. >> >> On 1/25/21 12:26 PM, Marc Zyngier wrote: >>> Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be >>> pretty easy. All that is required is support for PMMIR_EL1, which >>> is read-only, and for which returning 0 is a valid option as long >>> as we don't advertise STALL_SLOT as an implemented event. >>> >>> Let's just do that and adjust what we return to the guest. >>> >>> Signed-off-by: Marc Zyngier <maz@kernel.org> >>> --- >>> arch/arm64/include/asm/sysreg.h | 3 +++ >>> arch/arm64/kvm/pmu-emul.c | 6 ++++++ >>> arch/arm64/kvm/sys_regs.c | 11 +++++++---- >>> 3 files changed, 16 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h >>> index 8b5e7e5c3cc8..2fb3f386588c 100644 >>> --- a/arch/arm64/include/asm/sysreg.h >>> +++ b/arch/arm64/include/asm/sysreg.h >>> @@ -846,7 +846,10 @@ >>> >>> #define ID_DFR0_PERFMON_SHIFT 24 >>> >>> +#define ID_DFR0_PERFMON_8_0 0x3 >>> #define ID_DFR0_PERFMON_8_1 0x4 >>> +#define ID_DFR0_PERFMON_8_4 0x5 >>> +#define ID_DFR0_PERFMON_8_5 0x6 >>> >>> #define ID_ISAR4_SWP_FRAC_SHIFT 28 >>> #define ID_ISAR4_PSR_M_SHIFT 24 >>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c >>> index 398f6df1bbe4..72cd704a8368 100644 >>> --- a/arch/arm64/kvm/pmu-emul.c >>> +++ b/arch/arm64/kvm/pmu-emul.c >>> @@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) >>> base = 0; >>> } else { >>> val = read_sysreg(pmceid1_el0); >>> + /* >>> + * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled >>> + * as RAZ >>> + */ >>> + if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4) >>> + val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32); >> >> This is confusing the me. We have kvm->arch.pmuver set to the hardware >> PMU version >> (as set by __armv8pmu_probe_pmu()), but we ignore it when reporting the PMU >> version to the guest. Why do we do that? We limit the event number in >> kvm_pmu_event_mask() based on the hardware PMU version, so even if we advertise >> Armv8.4 PMU, support for all those extra events added by Arm8.1 PMU is >> missing (I hope I understood the code correctly). > > That's a bit of mess-up. We obtain ID_AA64DFR0_EL1 from the sanitised > regs, but do most of our handling based on kvm->arch.pmuver. They really > should be the same, because that's what the sanitised registers give > you. > > As for the events themselves, I don't get your drift. We do support > all the ARMv8.1 PMU events as long as the HW supports it, and we > don't lie to the guest about it either (cpuid_feature_cap_perfmon_field > does *cap* the field to some value, it doesn't allow it to increase > past what the HW supports). That's the piece that I was missing - I didn't realize that cpuid_feature_cap_perfmon_field() makes sure that the final version doesn't exceed what the hardware supports. Thanks for clearing it up! > >> I looked at commit c854188ea010 ("KVM: arm64: limit PMU version to PMUv3 for >> ARMv8.1") which changed read_id_reg() to report PMUv3 for Armv8.1 >> unconditionally, >> and there's no explanation why PMUv3 for Armv8.1 was chosen instead of >> plain PMUv3 (PMUVer = 0b0100). > > We picked ARMv8.1 because this is the first PMU revision that gives > you events in the 0x4000 range, all of which are available on > common ARMv8.2 HW. Yes, makes sense in the context of cpuid_feature_cap_perfmon_field() capping PMUVer based on the hardware supported version. Thanks, Alex
Hi, On 2/3/21 1:28 PM, Marc Zyngier wrote: > On 2021-02-03 12:39, Auger Eric wrote: >> Hi, >> >> On 2/3/21 12:20 PM, Marc Zyngier wrote: >>> On 2021-02-03 11:07, Auger Eric wrote: >>>> Hi Marc, >>>> On 2/3/21 11:36 AM, Marc Zyngier wrote: >>>>> Hi Eric, >>>>> >>>>> On 2021-01-27 17:53, Auger Eric wrote: >>>>>> Hi Marc, >>>>>> >>>>>> On 1/25/21 1:26 PM, Marc Zyngier wrote: >>>>>>> Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be >>>>>>> pretty easy. All that is required is support for PMMIR_EL1, which >>>>>>> is read-only, and for which returning 0 is a valid option as long >>>>>>> as we don't advertise STALL_SLOT as an implemented event. >>>>>>> >>>>>>> Let's just do that and adjust what we return to the guest. >>>>>>> >>>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org> >>>>>>> --- >>>>>>> arch/arm64/include/asm/sysreg.h | 3 +++ >>>>>>> arch/arm64/kvm/pmu-emul.c | 6 ++++++ >>>>>>> arch/arm64/kvm/sys_regs.c | 11 +++++++---- >>>>>>> 3 files changed, 16 insertions(+), 4 deletions(-) >>>>>>> >>>>>>> diff --git a/arch/arm64/include/asm/sysreg.h >>>>>>> b/arch/arm64/include/asm/sysreg.h >>>>>>> index 8b5e7e5c3cc8..2fb3f386588c 100644 >>>>>>> --- a/arch/arm64/include/asm/sysreg.h >>>>>>> +++ b/arch/arm64/include/asm/sysreg.h >>>>>>> @@ -846,7 +846,10 @@ >>>>>>> >>>>>>> #define ID_DFR0_PERFMON_SHIFT 24 >>>>>>> >>>>>>> +#define ID_DFR0_PERFMON_8_0 0x3 >>>>>>> #define ID_DFR0_PERFMON_8_1 0x4 >>>>>>> +#define ID_DFR0_PERFMON_8_4 0x5 >>>>>>> +#define ID_DFR0_PERFMON_8_5 0x6 >>>>>>> >>>>>>> #define ID_ISAR4_SWP_FRAC_SHIFT 28 >>>>>>> #define ID_ISAR4_PSR_M_SHIFT 24 >>>>>>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c >>>>>>> index 398f6df1bbe4..72cd704a8368 100644 >>>>>>> --- a/arch/arm64/kvm/pmu-emul.c >>>>>>> +++ b/arch/arm64/kvm/pmu-emul.c >>>>>>> @@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, >>>>>>> bool pmceid1) >>>>>>> base = 0; >>>>>>> } else { >>>>>>> val = read_sysreg(pmceid1_el0); >>>>>>> + /* >>>>>>> + * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled >>>>>>> + * as RAZ >>>>>>> + */ >>>>>>> + if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4) >>>>>>> + val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32); >>>>>> what about the STALL_SLOT_BACKEND and FRONTEND events then? >>>>> >>>>> Aren't these a mandatory ARMv8.1 feature? I don't see a reason to >>>>> drop them. >>>> >>>> I understand the 3 are linked together. >>>> >>>> In D7.11 it is said >>>> " >>>> When any of the following common events are implemented, all three of >>>> them are implemented: >>>> 0x003D , STALL_SLOT_BACKEND, No operation sent for execution on a Slot >>>> due to the backend, >>>> 0x003E , STALL_SLOT_FRONTEND, No operation sent for execution on a Slot >>>> due to the frontend. >>>> 0x003F , STALL_SLOT, No operation sent for execution on a Slot. >>>> " >>> >>> They are linked in the sense that they report related events, but they >>> don't have to be implemented in the same level of the architecure, if only >>> because BACKEND/FRONTEND were introducedway before ARMv8.4. >>> >>> What the architecture says is: >>> >>> - For FEAT_PMUv3p1 (ARMv8.1): >>> "The STALL_FRONTEND and STALL_BACKEND events are required to be >>> implemented." (A2.4.1, DDI0487G.a) >> OK >>> >>> - For FEAT_PMUv3p4 (ARMv8.4): >>> "If FEAT_PMUv3p4 is implemented: >>> - If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED >>> whether the PMMIR System registers are implemented. >>> - If STALL_SLOT is implemented, then the PMMIR System registers are >>> implemented." (D7-2873, DDI0487G.a) >>> >>> So while BACKEND/FRONTEND are required in an ARMv8.4 implementation >>> by virtue of being mandatory in ARMv8.1, STALL_SLOT isn't at any point. >> But then how do you understand "When any of the following common events >> are implemented, all three of them are implemented"? > > I think that's wholly inconsistent, because it would mean that STALL_SLOT > isn't optional on ARMv8.4, and would make PMMIR mandatory. I think there's some confusion regarding the event names. From my reading of the architecture, STALL != STALL_SLOT, STALL_BACKEND != STALL_SLOT_BACKEND, STALL_FRONTEND != STALL_SLOT_FRONTEND. STALL{, _BACKEND, _FRONTEND} count the number of CPU cycles where no instructions are being executed on the PE (page D7-2872), STALL_SLOT{, _BACKEND, _FRONTEND} count the number of slots where no instructions are being executed (page D7-2873). STALL_{BACKEND, FRONTEND} are required by ARMv8.1-PMU (pages A2-76, D7-2913); STALL is required by ARMv8.4-PMU (page D7-2914). STALL_SLOT{, _BACKEND, _FRONTEND} are optional in ARMv8.4-PMU (pages D7-2913, D7-2914), but if one of them is implemented, all of them must be implemented (page D7-2914). The problem I see with this patch is that it doesn't clear the STALL_SLOT_{BACKEND, FRONTEND} event bits along with the STALL_SLOT bit from PMCEID1_EL0. Thanks Alex > > I'm starting to think that dropping this patch may be the best thing to do... > > Thanks, > > M.
On 2021-02-04 12:32, Alexandru Elisei wrote: > Hi, > > On 2/3/21 1:28 PM, Marc Zyngier wrote: >> On 2021-02-03 12:39, Auger Eric wrote: >>> Hi, >>> >>> On 2/3/21 12:20 PM, Marc Zyngier wrote: >>>> On 2021-02-03 11:07, Auger Eric wrote: >>>>> Hi Marc, >>>>> On 2/3/21 11:36 AM, Marc Zyngier wrote: >>>>>> Hi Eric, >>>>>> >>>>>> On 2021-01-27 17:53, Auger Eric wrote: >>>>>>> Hi Marc, >>>>>>> >>>>>>> On 1/25/21 1:26 PM, Marc Zyngier wrote: >>>>>>>> Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be >>>>>>>> pretty easy. All that is required is support for PMMIR_EL1, >>>>>>>> which >>>>>>>> is read-only, and for which returning 0 is a valid option as >>>>>>>> long >>>>>>>> as we don't advertise STALL_SLOT as an implemented event. >>>>>>>> >>>>>>>> Let's just do that and adjust what we return to the guest. >>>>>>>> >>>>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org> >>>>>>>> --- >>>>>>>> arch/arm64/include/asm/sysreg.h | 3 +++ >>>>>>>> arch/arm64/kvm/pmu-emul.c | 6 ++++++ >>>>>>>> arch/arm64/kvm/sys_regs.c | 11 +++++++---- >>>>>>>> 3 files changed, 16 insertions(+), 4 deletions(-) >>>>>>>> >>>>>>>> diff --git a/arch/arm64/include/asm/sysreg.h >>>>>>>> b/arch/arm64/include/asm/sysreg.h >>>>>>>> index 8b5e7e5c3cc8..2fb3f386588c 100644 >>>>>>>> --- a/arch/arm64/include/asm/sysreg.h >>>>>>>> +++ b/arch/arm64/include/asm/sysreg.h >>>>>>>> @@ -846,7 +846,10 @@ >>>>>>>> >>>>>>>> #define ID_DFR0_PERFMON_SHIFT 24 >>>>>>>> >>>>>>>> +#define ID_DFR0_PERFMON_8_0 0x3 >>>>>>>> #define ID_DFR0_PERFMON_8_1 0x4 >>>>>>>> +#define ID_DFR0_PERFMON_8_4 0x5 >>>>>>>> +#define ID_DFR0_PERFMON_8_5 0x6 >>>>>>>> >>>>>>>> #define ID_ISAR4_SWP_FRAC_SHIFT 28 >>>>>>>> #define ID_ISAR4_PSR_M_SHIFT 24 >>>>>>>> diff --git a/arch/arm64/kvm/pmu-emul.c >>>>>>>> b/arch/arm64/kvm/pmu-emul.c >>>>>>>> index 398f6df1bbe4..72cd704a8368 100644 >>>>>>>> --- a/arch/arm64/kvm/pmu-emul.c >>>>>>>> +++ b/arch/arm64/kvm/pmu-emul.c >>>>>>>> @@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu >>>>>>>> *vcpu, >>>>>>>> bool pmceid1) >>>>>>>> base = 0; >>>>>>>> } else { >>>>>>>> val = read_sysreg(pmceid1_el0); >>>>>>>> + /* >>>>>>>> + * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled >>>>>>>> + * as RAZ >>>>>>>> + */ >>>>>>>> + if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4) >>>>>>>> + val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - >>>>>>>> 32); >>>>>>> what about the STALL_SLOT_BACKEND and FRONTEND events then? >>>>>> >>>>>> Aren't these a mandatory ARMv8.1 feature? I don't see a reason to >>>>>> drop them. >>>>> >>>>> I understand the 3 are linked together. >>>>> >>>>> In D7.11 it is said >>>>> " >>>>> When any of the following common events are implemented, all three >>>>> of >>>>> them are implemented: >>>>> 0x003D , STALL_SLOT_BACKEND, No operation sent for execution on a >>>>> Slot >>>>> due to the backend, >>>>> 0x003E , STALL_SLOT_FRONTEND, No operation sent for execution on a >>>>> Slot >>>>> due to the frontend. >>>>> 0x003F , STALL_SLOT, No operation sent for execution on a Slot. >>>>> " >>>> >>>> They are linked in the sense that they report related events, but >>>> they >>>> don't have to be implemented in the same level of the architecure, >>>> if only >>>> because BACKEND/FRONTEND were introducedway before ARMv8.4. >>>> >>>> What the architecture says is: >>>> >>>> - For FEAT_PMUv3p1 (ARMv8.1): >>>> "The STALL_FRONTEND and STALL_BACKEND events are required to be >>>> implemented." (A2.4.1, DDI0487G.a) >>> OK >>>> >>>> - For FEAT_PMUv3p4 (ARMv8.4): >>>> "If FEAT_PMUv3p4 is implemented: >>>> - If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED >>>> whether the PMMIR System registers are implemented. >>>> - If STALL_SLOT is implemented, then the PMMIR System registers >>>> are >>>> implemented." (D7-2873, DDI0487G.a) >>>> >>>> So while BACKEND/FRONTEND are required in an ARMv8.4 implementation >>>> by virtue of being mandatory in ARMv8.1, STALL_SLOT isn't at any >>>> point. >>> But then how do you understand "When any of the following common >>> events >>> are implemented, all three of them are implemented"? >> >> I think that's wholly inconsistent, because it would mean that >> STALL_SLOT >> isn't optional on ARMv8.4, and would make PMMIR mandatory. > > I think there's some confusion regarding the event names. From my > reading of the > architecture, STALL != STALL_SLOT, STALL_BACKEND != STALL_SLOT_BACKEND, > STALL_FRONTEND != STALL_SLOT_FRONTEND. Dammit, I hadn't realised that at all. Thanks for putting me right. > > STALL{, _BACKEND, _FRONTEND} count the number of CPU cycles where no > instructions > are being executed on the PE (page D7-2872), STALL_SLOT{, _BACKEND, > _FRONTEND} > count the number of slots where no instructions are being executed > (page D7-2873). > > STALL_{BACKEND, FRONTEND} are required by ARMv8.1-PMU (pages A2-76, > D7-2913); > STALL is required by ARMv8.4-PMU (page D7-2914). > > STALL_SLOT{, _BACKEND, _FRONTEND} are optional in ARMv8.4-PMU (pages > D7-2913, > D7-2914), but if one of them is implemented, all of them must be > implemented (page D7-2914). > > The problem I see with this patch is that it doesn't clear the > STALL_SLOT_{BACKEND, FRONTEND} event bits along with the STALL_SLOT bit > from > PMCEID1_EL0. OK, so Eric was right, and I'm an illiterate idiot! I'll fix the patch and repost the whole thing. Thanks, M.
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index 8b5e7e5c3cc8..2fb3f386588c 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -846,7 +846,10 @@ #define ID_DFR0_PERFMON_SHIFT 24 +#define ID_DFR0_PERFMON_8_0 0x3 #define ID_DFR0_PERFMON_8_1 0x4 +#define ID_DFR0_PERFMON_8_4 0x5 +#define ID_DFR0_PERFMON_8_5 0x6 #define ID_ISAR4_SWP_FRAC_SHIFT 28 #define ID_ISAR4_PSR_M_SHIFT 24 diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c index 398f6df1bbe4..72cd704a8368 100644 --- a/arch/arm64/kvm/pmu-emul.c +++ b/arch/arm64/kvm/pmu-emul.c @@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) base = 0; } else { val = read_sysreg(pmceid1_el0); + /* + * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled + * as RAZ + */ + if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4) + val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32); base = 32; } diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 8f79ec1fffa7..5da536ab738d 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1051,16 +1051,16 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu, /* Limit debug to ARMv8.0 */ val &= ~FEATURE(ID_AA64DFR0_DEBUGVER); val |= FIELD_PREP(FEATURE(ID_AA64DFR0_DEBUGVER), 6); - /* Limit guests to PMUv3 for ARMv8.1 */ + /* Limit guests to PMUv3 for ARMv8.4 */ val = cpuid_feature_cap_perfmon_field(val, ID_AA64DFR0_PMUVER_SHIFT, - kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_1 : 0); + kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_4 : 0); break; case SYS_ID_DFR0_EL1: - /* Limit guests to PMUv3 for ARMv8.1 */ + /* 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_1 : 0); + kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0); break; } @@ -1496,6 +1496,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { { SYS_DESC(SYS_PMINTENSET_EL1), access_pminten, reset_unknown, PMINTENSET_EL1 }, { SYS_DESC(SYS_PMINTENCLR_EL1), access_pminten, reset_unknown, PMINTENSET_EL1 }, + { SYS_DESC(SYS_PMMIR_EL1), trap_raz_wi }, { SYS_DESC(SYS_MAIR_EL1), access_vm_reg, reset_unknown, MAIR_EL1 }, { SYS_DESC(SYS_AMAIR_EL1), access_vm_reg, reset_amair_el1, AMAIR_EL1 }, @@ -1918,6 +1919,8 @@ static const struct sys_reg_desc cp15_regs[] = { { Op1( 0), CRn( 9), CRm(14), Op2( 3), access_pmovs }, { AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 4), access_pmceid }, { AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 5), access_pmceid }, + /* PMMIR */ + { Op1( 0), CRn( 9), CRm(14), Op2( 6), trap_raz_wi }, /* PRRR/MAIR0 */ { AA32(LO), Op1( 0), CRn(10), CRm( 2), Op2( 0), access_vm_reg, NULL, MAIR_EL1 },
Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be pretty easy. All that is required is support for PMMIR_EL1, which is read-only, and for which returning 0 is a valid option as long as we don't advertise STALL_SLOT as an implemented event. Let's just do that and adjust what we return to the guest. Signed-off-by: Marc Zyngier <maz@kernel.org> --- arch/arm64/include/asm/sysreg.h | 3 +++ arch/arm64/kvm/pmu-emul.c | 6 ++++++ arch/arm64/kvm/sys_regs.c | 11 +++++++---- 3 files changed, 16 insertions(+), 4 deletions(-)