diff mbox series

[v2,6/7] KVM: arm64: Upgrade PMU support to ARMv8.4

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

Commit Message

Marc Zyngier Jan. 25, 2021, 12:26 p.m. UTC
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(-)

Comments

Alexandru Elisei Jan. 27, 2021, 2:09 p.m. UTC | #1
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 },
Marc Zyngier Jan. 27, 2021, 2:35 p.m. UTC | #2
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.
Alexandru Elisei Jan. 27, 2021, 5 p.m. UTC | #3
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
Marc Zyngier Jan. 27, 2021, 5:23 p.m. UTC | #4
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.
Alexandru Elisei Jan. 27, 2021, 5:41 p.m. UTC | #5
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 },
Auger Eric Jan. 27, 2021, 5:53 p.m. UTC | #6
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
Marc Zyngier Feb. 3, 2021, 10:32 a.m. UTC | #7
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.
Marc Zyngier Feb. 3, 2021, 10:36 a.m. UTC | #8
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.
Auger Eric Feb. 3, 2021, 11:07 a.m. UTC | #9
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.
Marc Zyngier Feb. 3, 2021, 11:20 a.m. UTC | #10
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.
Auger Eric Feb. 3, 2021, 12:39 p.m. UTC | #11
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.
Marc Zyngier Feb. 3, 2021, 1:28 p.m. UTC | #12
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.
Alexandru Elisei Feb. 3, 2021, 5:29 p.m. UTC | #13
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
Alexandru Elisei Feb. 4, 2021, 12:32 p.m. UTC | #14
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.
Marc Zyngier Feb. 4, 2021, 2:21 p.m. UTC | #15
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 mbox series

Patch

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 },