KVM: arm64: EL2 PMU reset handling fixes
Marc Zyngier
maintainercover KVM: arm64: EL2 PMU reset handling fixes
Joey reports that some of his PMU tests do not behave quite as expected:
- MDCR_EL2.HPMN is set to 0 out of reset
- PMCR_EL0.P should reset all the counters when written from EL2
These couple of patches attempt to remedy the situation. Note that they have only been compile-tested.
Marc Zyngier (2):
KVM: arm64: Fix MDCR_EL2.HPMN reset value
KVM: arm64: Contextualise the handling of PMCR_EL0.P writes
arch/arm64/kvm/pmu-emul.c | 21 ++++++++++++++++----- arch/arm64/kvm/sys_regs.c | 7 ++++++- 2 files changed, 22 insertions(+), 6 deletions(-)
Signature · 1 line
2.39.2
Marc Zyngier
maintainerPATCH 1/2 KVM: arm64: Fix MDCR_EL2.HPMN reset value
The MDCR_EL2 documentation indicates that the HPMN field has the following behaviour:
"On a Warm reset, this field resets to the expression NUM_PMU_COUNTERS."
However, it appears we reset it to zero, which is not very useful.
Add a reset helper for MDCR_EL2, and handle the case where userspace changes the target PMU, which may force us to change HPMN again.
--- arch/arm64/kvm/pmu-emul.c | 13 +++++++++++++ arch/arm64/kvm/sys_regs.c | 7 ++++++- 2 files changed, 19 insertions(+), 1 deletion(-)
arch/arm64/kvm/pmu-emul.c +13 -0
@@ -1007,6 +1007,19 @@ static void kvm_arm_set_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu) kvm->arch.arm_pmu = arm_pmu; kvm->arch.pmcr_n =kvm_arm_pmu_get_max_counters (kvm); + +/* Reset MDCR_EL2.HPMN behind the vcpus' back... */ +if (test_bit (KVM_ARM_VCPU_HAS_EL2 , kvm->arch.vcpu_features)) { +struct kvm_vcpu *vcpu; +unsigned long i; + +kvm_for_each_vcpu (i, vcpu, kvm) { +u64 val =__vcpu_sys_reg (vcpu,MDCR_EL2 ); + val &= ~MDCR_EL2_HPMN ; + val |=FIELD_PREP (MDCR_EL2_HPMN , kvm->arch.pmcr_n); +__vcpu_sys_reg (vcpu,MDCR_EL2 ) = val; + } + } }/**
arch/arm64/kvm/sys_regs.c +6 -1
@@ -2493,6 +2493,11 @@ static bool access_mdcr(struct kvm_vcpu *vcpu,return true; } +static u64 reset_mdcr (struct kvm_vcpu *vcpu,const struct sys_reg_desc *r) +{ +__vcpu_sys_reg (vcpu, r->reg) = vcpu->kvm->arch.pmcr_n; +return vcpu->kvm->arch.pmcr_n; +}/* * Architected system registers.
@@ -3034,7 +3039,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {EL2_REG (SCTLR_EL2 , access_rw, reset_val,SCTLR_EL2_RES1 ),EL2_REG (ACTLR_EL2 , access_rw, reset_val,0 ),EL2_REG_VNCR (HCR_EL2 , reset_hcr,0 ), -EL2_REG (MDCR_EL2 , access_mdcr, reset_val,0 ), +EL2_REG (MDCR_EL2 , access_mdcr, reset_mdcr,0 ),EL2_REG (CPTR_EL2 , access_rw, reset_val,CPTR_NVHE_EL2_RES1 ),EL2_REG_VNCR (HSTR_EL2 , reset_val,0 ), EL2_REG_VNCR(HFGRTR_EL2, reset_val, 0),
Signature · 1 line
2.39.2
Re: [PATCH 1/2] KVM: arm64: Fix MDCR_EL2.HPMN reset value
Hey,
Quoted 5 lines · Marc Zyngier
The
MDCR_EL2documentation indicates that the HPMN field has the following behaviour:"On a Warm reset, this field resets to the expression
NUM_PMU_COUNTERS."
Quoted 7 lines
However, it appears we reset it to zero, which is not very useful.
Add a reset helper for
MDCR_EL2, and handle the case where userspace changes the target PMU, which may force us to change HPMN again.Reported-by Joey Gouly <[email protected]>Signed-off-by Marc Zyngier <[email protected]>
The existing ABI expectations are that writes to PMCR_EL0.N constrain the number of counters, so that should have a similar effect on MDCR_EL2.HPMN.
At the same time, I get the feeling that we should throw out this whole behavior of writing N to change the shape of the PMU, because it complete breaks down for NV. PMCR_EL0.N is another one of those fields that change behavior based on EL and isn't a global source of truth on the shape of the PMU.
What do you think about adding a new vCPU attribute for selecting the number of counters for a VM? We can allow non-nested VMs to use the 'old' method of writing PMCR_EL0.N and force nested VMs to use the attribute.
We can then enforce ordering on the attribute and prevent it from being used after vCPU reset.
Thanks,
Oliver
Marc Zyngier
maintainerRe: [PATCH 1/2] KVM: arm64: Fix MDCR_EL2.HPMN reset value
Quoted 27 lines · Oliver Upton
Hey,
Quoted 5 lines · Marc Zyngier
The
MDCR_EL2documentation indicates that the HPMN field has the following behaviour:"On a Warm reset, this field resets to the expression
NUM_PMU_COUNTERS."Quoted 7 lines
However, it appears we reset it to zero, which is not very useful.
Add a reset helper for
MDCR_EL2, and handle the case where userspace changes the target PMU, which may force us to change HPMN again.Reported-by Joey Gouly <[email protected]>Signed-off-by Marc Zyngier <[email protected]>The existing ABI expectations are that writes to PMCR_EL0.N constrain the number of counters, so that should have a similar effect on MDCR_EL2.HPMN.
At the same time, I get the feeling that we should throw out this whole behavior of writing N to change the shape of the PMU, because it complete breaks down for NV. PMCR_EL0.N is another one of those fields that change behavior based on EL and isn't a global source of truth on the shape of the PMU.
Quoted 4 lines
What do you think about adding a new vCPU attribute for selecting the number of counters for a VM? We can allow non-nested VMs to use the 'old' method of writing PMCR_EL0.N and force nested VMs to use the attribute.
VCPU attribute? or PMU attribute? I'm really not keen on the former, but the latter is probably workable, as it is VM-wide, similar to the way we keep track of pmcr_n.
Quoted 2 lines
We can then enforce ordering on the attribute and prevent it from being used after vCPU reset.
How would that work? Do you really want to mandate the PMU selection (with its counter capping) to strictly occur between vcpu creation and init?
This would, for example, break kvmtool which has these two operations back-to-back, and sneaking new device-specific actions in the middle is a bit unpalatable (there is a split between VM-wide and per-vcpu actions).
Any idea?
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH 1/2] KVM: arm64: Fix MDCR_EL2.HPMN reset value
Quoted 9 lines · Marc Zyngier
Quoted 4 lines · Oliver Upton
What do you think about adding a new vCPU attribute for selecting the number of counters for a VM? We can allow non-nested VMs to use the 'old' method of writing PMCR_EL0.N and force nested VMs to use the attribute.
VCPU attribute? or PMU attribute? I'm really not keen on the former, but the latter is probably workable, as it is VM-wide, similar to the way we keep track of
pmcr_n.
Well the _existing_ PMU attributes are actually vCPU attributes. I do agree that accessing them as a VM attribute makes more sense, but that's the UAPI we already have...
Quoted 7 lines
Quoted 2 lines
We can then enforce ordering on the attribute and prevent it from being used after vCPU reset.
How would that work? Do you really want to mandate the PMU selection (with its counter capping) to strictly occur between vcpu creation and init?
Quoted 6 lines
This would, for example, break kvmtool which has these two operations back-to-back, and sneaking new device-specific actions in the middle is a bit unpalatable (there is a split between VM-wide and per-vcpu actions).
Any idea?
If we want to do this the 'right' way, we should provide VM attributes for selecting the PMU implementation / configuring the event filter to complement an attribute for setting the number of event counters.
I don't want to have a mix-and-match approach where vPMU attributes are scattered between the vCPU and the VM since it requires a similar amount of gymnastics in userspace to set crap up.
Thanks,
Oliver
Marc Zyngier
maintainerRe: [PATCH 1/2] KVM: arm64: Fix MDCR_EL2.HPMN reset value
Quoted 8 lines · Oliver Upton
Quoted 6 lines · Marc Zyngier
Quoted 4 lines · Oliver Upton
What do you think about adding a new vCPU attribute for selecting the number of counters for a VM? We can allow non-nested VMs to use the 'old' method of writing PMCR_EL0.N and force nested VMs to use the attribute.
Quoted 7 lines
Quoted 3 lines
VCPU attribute? or PMU attribute? I'm really not keen on the former, but the latter is probably workable, as it is VM-wide, similar to the way we keep track of
pmcr_n.Well the _existing_ PMU attributes are actually vCPU attributes. I do agree that accessing them as a VM attribute makes more sense, but that's the UAPI we already have...
Gah, I remember now. Someone please take the API to the backyard...
Quoted 15 lines
Quoted 8 lines
Quoted 3 lines
We can then enforce ordering on the attribute and prevent it from being used after vCPU reset.
How would that work? Do you really want to mandate the PMU selection (with its counter capping) to strictly occur between vcpu creation and init?
Quoted 6 lines
This would, for example, break kvmtool which has these two operations back-to-back, and sneaking new device-specific actions in the middle is a bit unpalatable (there is a split between VM-wide and per-vcpu actions).
Any idea?
Quoted 7 lines
If we want to do this the 'right' way, we should provide VM attributes for selecting the PMU implementation / configuring the event filter to complement an attribute for setting the number of event counters.
I don't want to have a mix-and-match approach where vPMU attributes are scattered between the vCPU and the VM since it requires a similar amount of gymnastics in userspace to set crap up.
I agree on not mixing vCPU and VM scoped attributes, even if that amounts to the same thing in the back-end. But freezing the number of counters on vcpu reset is something that should be considered very carefully, and I fear it breaks existing models -- specially given that this is yet another one-off.
I wonder if we should instead make use of the KVM_ARM_VCPU_FINALIZE ioctl just like we do for SVE, passing KVM_ARM_VCPU_PMU_V3 instead. This would make use of an existing mechanism and lock the PMU for good (implementation, IRQ, number of counters...).
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Marc Zyngier
maintainerPATCH 2/2 KVM: arm64: Contextualise the handling of PMCR_EL0.P writes
In reply to Marc ZyngierContrary to what the comment says in kvm_pmu_handle_pmcr(), writing PMCR_EL0.P==1 has the following effects:
<quote>
The event counters affected by this field are:
* All event counters in the first range.
* If any of the following are true, all event counters in the second range:
- EL2 is disabled or not implemented in the current Security state.
- The PE is executing at EL2 or EL3.
</quote>
where the "first range" represent the counters in the [0..HPMN-1] range, and the "second range" the counters in the [HPMN..MAX] range.
It so appears that writing P from EL2 should nuke all counters, and not just the "guest" view. Just do that, and nuke the misleading comment.
--- arch/arm64/kvm/pmu-emul.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
arch/arm64/kvm/pmu-emul.c +3 -5
@@ -617,14 +617,12 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val)kvm_pmu_set_counter_value (vcpu,ARMV8_PMU_CYCLE_IDX ,0 );if (val &ARMV8_PMU_PMCR_P ) { -/* -* Unlike other PMU sysregs, the controls in PMCR_EL0 always apply -* to the 'guest' range of counters and never the 'hyp' range. -*/ unsigned long mask =kvm_pmu_implemented_counter_mask (vcpu) & - ~kvm_pmu_hyp_counter_mask (vcpu) & ~BIT (ARMV8_PMU_CYCLE_IDX ); +if (!vcpu_is_el2 (vcpu)) + mask &= ~kvm_pmu_hyp_counter_mask (vcpu); +for_each_set_bit (i, &mask,32 )kvm_pmu_set_pmc_value (kvm_vcpu_idx_to_pmc (vcpu, i),0 , true); }
Signature · 1 line
2.39.2
Re: [PATCH 2/2] KVM: arm64: Contextualise the handling of PMCR_EL0.P writes
Quoted 15 lines · Marc Zyngier
Contrary to what the comment says in
kvm_pmu_handle_pmcr(), writing PMCR_EL0.P==1 has the following effects:<quote>
The event counters affected by this field are:
* All event counters in the first range.
* If any of the following are true, all event counters in the second range:
- EL2 is disabled or not implemented in the current Security state.
- The PE is executing at EL2 or EL3.
</quote>where the "first range" represent the counters in the [0..HPMN-1] range, and the "second range" the counters in the [HPMN..MAX] range.
Quoted 6 lines
It so appears that writing P from EL2 should nuke all counters, and not just the "guest" view. Just do that, and nuke the misleading comment.
Reported-by Joey Gouly <[email protected]>Signed-off-by Marc Zyngier <[email protected]>
Thanks,
Oliver
Quoted 10 lines · Marc Zyngier
Joey reports that some of his PMU tests do not behave quite as expected:
- MDCR_EL2.HPMN is set to 0 out of reset
- PMCR_EL0.P should reset all the counters when written from EL2
These couple of patches attempt to remedy the situation. Note that they have only been compile-tested.
Quoted 7 lines
Marc Zyngier (2):
KVM: arm64: Fix MDCR_EL2.HPMN reset value
KVM: arm64: Contextualise the handling of PMCR_EL0.P writesarch/arm64/kvm/pmu-emul.c | 21 ++++++++++++++++----- arch/arm64/kvm/sys_regs.c | 7 ++++++- 2 files changed, 22 insertions(+), 6 deletions(-)
I see you're discussing the API around this, but this does fix the PMU tests, so: