KVM: arm64: EL2 PMU handling fixes
Subsystems (4)
Marc Zyngier
maintainerv2 cover KVM: arm64: EL2 PMU 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
Oliver points out that setting PMCR_EL0.N from userspace by writing to the register is silly with NV, and that we need a new PMU attribute instead.
On top of that, I figured out that we had a number of little gotchas:
- It is possible for a guest to write an HPMN value that is out of bound, and it seems valuable to limit it
- PMCR_EL0.N should be the maximum number of counters when read from EL2, and MDCR_EL2.HPMN when read from EL0/EL1
- Prevent userspace from updating PMCR_EL0.N when EL2 is available
I haven't added any Cc stable, as NV is not functional upstream yet.
* From v1 [1]:
- Added patch for the new PMU attribute
- Added fix for HPMN limit
- Prevent update of PMCR_EL0.N with NV
- Fix kvm_vcpu_read_pmcr()
- Rebased on 6.15-rc1
[1] https://lore.kernel.org/r/[email protected]
Marc Zyngier (6):
KVM: arm64: Fix MDCR_EL2.HPMN reset value
KVM: arm64: Contextualise the handling of PMCR_EL0.P writes
KVM: arm64: Allow userspace to limit the number of PMU counters for
EL2 VMs
KVM: arm64: Don't let userspace write to PMCR_EL0.N when the vcpu has
EL2
KVM: arm64: Handle out-of-bound write to HDCR_EL2.HPMN
KVM: arm64: Let kvm_vcpu_read_pmcr() return an EL-dependent value for
PMCR_EL0.NDocumentation/virt/kvm/devices/vcpu.rst | 24 ++++++++++++ arch/arm64/include/uapi/asm/kvm.h | 9 +++-- arch/arm64/kvm/pmu-emul.c | 51 ++++++++++++++++++++++--- arch/arm64/kvm/sys_regs.c | 43 ++++++++++++++++----- 4 files changed, 107 insertions(+), 20 deletions(-)
Signature · 1 line
2.39.2
Marc Zyngier
maintainerPATCH v2 1/6 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 | 8 +++++++- 2 files changed, 20 insertions(+), 1 deletion(-)
arch/arm64/kvm/pmu-emul.c +13 -0
@@ -1033,6 +1033,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 +7 -1
@@ -2698,6 +2698,12 @@ static int set_imp_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r, .set_user = set_imp_id_reg, \ .reset = reset_imp_id_reg, \ .val = mask, \ + } + +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; }/*
@@ -3243,7 +3249,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 v2 1/6] KVM: arm64: Fix MDCR_EL2.HPMN reset value
Quoted 22 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."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]>--- arch/arm64/kvm/pmu-emul.c | 13 +++++++++++++ arch/arm64/kvm/sys_regs.c | 8 +++++++- 2 files changed, 20 insertions(+), 1 deletion(-)arch/arm64/kvm/pmu-emul.c +0
-0
arch/arm64/kvm/pmu-emul.c static void kvm_arm_set_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu) · line 1035
Quoted 3 lines
arch/arm64/kvm/pmu-emul.c +0
-0@@ -1033,6 +1033,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);
nit: Can we rename pmcr_n to nr_pmu_counters? The current name is misleading.
arch/arm64/kvm/pmu-emul.c static void kvm_arm_set_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu) · line 1047
Quoted 12 lines
arch/arm64/kvm/pmu-emul.c +12
-0+ +/* 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; + }
Shouldn't we be taking the vCPU mutex(es) here?
Thanks,
Oliver
Marc Zyngier
maintainerRe: [PATCH v2 1/6] KVM: arm64: Fix MDCR_EL2.HPMN reset value
Quoted 25 lines · Oliver Upton
Quoted 23 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."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]>--- arch/arm64/kvm/pmu-emul.c | 13 +++++++++++++ arch/arm64/kvm/sys_regs.c | 8 +++++++- 2 files changed, 20 insertions(+), 1 deletion(-)arch/arm64/kvm/pmu-emul.c +0
-0@@ -1033,6 +1033,19 @@ static void kvm_arm_set_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
Quoted 4 lines
arch/arm64/kvm/pmu-emul.c static void kvm_arm_set_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu) · line 1035
Quoted 2 lines
arch/arm64/kvm/pmu-emul.c +0
-0kvm->arch.arm_pmu = arm_pmu; kvm->arch.pmcr_n =kvm_arm_pmu_get_max_counters (kvm);nit: Can we rename
pmcr_ntonr_pmu_counters? The current name is misleading.
Fair enough.
Quoted 6 lines
Quoted 6 lines
arch/arm64/kvm/pmu-emul.c +6
-0+ +/* 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; +
Quoted 8 lines
arch/arm64/kvm/pmu-emul.c static void kvm_arm_set_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu) · line 1047
Quoted 6 lines
arch/arm64/kvm/pmu-emul.c +6
-0+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; + }Shouldn't we be taking the vCPU mutex(es) here?
If we needed to, it shouldn't be here. We hold the config_lock at this point, and taking a vcpu mutex would result in a locking inversion.
One option is to punt this to a request, but that makes the updated HPMN un-observable from userspace until the vcpu has run. This already affects the default PMU, btw, since it is only assigned on first run.
I'm also not convinced racing against userspace is a big problem here.
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH v2 1/6] KVM: arm64: Fix MDCR_EL2.HPMN reset value
Quoted 49 lines · Marc Zyngier
Quoted 25 lines · Oliver Upton
Quoted 23 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."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]>--- arch/arm64/kvm/pmu-emul.c | 13 +++++++++++++ arch/arm64/kvm/sys_regs.c | 8 +++++++- 2 files changed, 20 insertions(+), 1 deletion(-)arch/arm64/kvm/pmu-emul.c +0
-0@@ -1033,6 +1033,19 @@ static void kvm_arm_set_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)Quoted 4 lines
arch/arm64/kvm/pmu-emul.c static void kvm_arm_set_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu) · line 1035
Quoted 2 lines
arch/arm64/kvm/pmu-emul.c +0
-0kvm->arch.arm_pmu = arm_pmu; kvm->arch.pmcr_n =kvm_arm_pmu_get_max_counters (kvm);nit: Can we rename
pmcr_ntonr_pmu_counters? The current name is misleading.Fair enough.
Quoted 6 lines
Quoted 6 lines
arch/arm64/kvm/pmu-emul.c +6
-0+ +/* 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; +Quoted 8 lines
arch/arm64/kvm/pmu-emul.c static void kvm_arm_set_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu) · line 1047
Quoted 6 lines
arch/arm64/kvm/pmu-emul.c +6
-0+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; + }Shouldn't we be taking the vCPU mutex(es) here?
Quoted 6 lines
If we needed to, it shouldn't be here. We hold the
config_lockat this point, and taking a vcpu mutex would result in a locking inversion.One option is to punt this to a request, but that makes the updated HPMN un-observable from userspace until the vcpu has run. This already affects the default PMU, btw, since it is only assigned on first run.
Ah, right. Default PMU is set at KVM_ARM_VCPU_INIT, so any race that comes afterwards would be the fault of userspace.
Fine with it as is then.
Thanks,
Oliver
Marc Zyngier
maintainerPATCH v2 2/6 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
@@ -608,14 +608,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
Marc Zyngier
maintainerPATCH v2 3/6 KVM: arm64: Allow userspace to limit the number of PMU counters for EL2 VMs
In reply to Marc ZyngierAs long as we had purely EL1 VMs, we could easily update the number of guest-visible counters by letting userspace write to PMCR_EL0.N.
With VMs started at EL2, PMCR_EL1.N only reflects MDCR_EL2.HPMN, and we don't have a good way to limit it.
For this purpose, introduce a new PMUv3 attribute that allows limiting the maximum number of counters. This requires the explicit selection of a PMU.
--- Documentation/virt/kvm/devices/vcpu.rst | 24 ++++++++++++++++++++++++ arch/arm64/include/uapi/asm/kvm.h | 9 +++++---- arch/arm64/kvm/pmu-emul.c | 24 ++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 4 deletions(-)
Documentation/virt/kvm/devices/vcpu.rst +24 -0
@@ -137,6 +137,30 @@ exit_reason = KVM_EXIT_FAIL_ENTRY and populate the fail_entry struct by setting hardare_entry_failure_reason field to KVM_EXIT_FAIL_ENTRY_CPU_UNSUPPORTED and the cpu field to the processor id. +1.5 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_SET_PMCR_N +--------------------------------------------- + +:Parameters: in kvm_device_attr.addr the address to an unsigned int + representing the maximum value taken by PMCR_EL0.N + +:Returns: + + ======= ==================================================== + -EBUSY PMUv3 already initialized, a VCPU has already run or + an event filter has already been set + -EFAULT Error accessing the value pointed to by addr + -ENODEV PMUv3 not supported or GIC not initialized + -EINVAL No PMUv3 explicitly selected, or value of N out of + range + ======= ==================================================== + +Update the maximum value allowed in PMCR_EL0.N, defining the number of +counters visible to the guest. This mandates that a PMU has +explicitly been selected via KVM_ARM_VCPU_PMU_V3_SET_PMU, and will +fail when no PMU has been explicitly selected, or the number of +counters is out of range for the selected PMU. Selecting a new PMU +cancels the effect of setting this attribute. + 2. GROUP: KVM_ARM_VCPU_TIMER_CTRL =================================
arch/arm64/include/uapi/asm/kvm.h +5 -4
@@ -431,10 +431,11 @@ enum {/* Device Control API on vcpu fd */ #define KVM_ARM_VCPU_PMU_V3_CTRL 0 -#define KVM_ARM_VCPU_PMU_V3_IRQ 0 -#define KVM_ARM_VCPU_PMU_V3_INIT 1 -#define KVM_ARM_VCPU_PMU_V3_FILTER 2 -#define KVM_ARM_VCPU_PMU_V3_SET_PMU 3 +#define KVM_ARM_VCPU_PMU_V3_IRQ 0 +#define KVM_ARM_VCPU_PMU_V3_INIT 1 +#define KVM_ARM_VCPU_PMU_V3_FILTER 2 +#define KVM_ARM_VCPU_PMU_V3_SET_PMU 3 +#define KVM_ARM_VCPU_PMU_V3_SET_PMCR_N 4#define KVM_ARM_VCPU_TIMER_CTRL 1#define KVM_ARM_VCPU_TIMER_IRQ_VTIMER 0#define KVM_ARM_VCPU_TIMER_IRQ_PTIMER 1
arch/arm64/kvm/pmu-emul.c +24 -0
@@ -1099,6 +1099,20 @@ static int kvm_arm_pmu_v3_set_pmu(struct kvm_vcpu *vcpu, int pmu_id)return ret; } +static int kvm_arm_pmu_v3_set_pmcr_n (struct kvm_vcpu *vcpu,unsigned int n) +{ +struct kvm *kvm = vcpu->kvm; + +if (!kvm->arch.arm_pmu) +return -EINVAL ; + +if (n >kvm_arm_pmu_get_max_counters (kvm)) +return -EINVAL ; + + kvm->arch.pmcr_n = n; +return 0 ; +} +int kvm_arm_pmu_v3_set_attr (struct kvm_vcpu *vcpu,struct kvm_device_attr *attr) {struct kvm *kvm = vcpu->kvm;
@@ -1195,6 +1209,15 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)return kvm_arm_pmu_v3_set_pmu (vcpu, pmu_id); } +case KVM_ARM_VCPU_PMU_V3_SET_PMCR_N : { +unsigned int __user *uaddr = (unsigned int __user *)(long )attr->addr; +unsigned int n; + +if (get_user (n, uaddr)) +return -EFAULT ; + +return kvm_arm_pmu_v3_set_pmcr_n (vcpu, n); + }case KVM_ARM_VCPU_PMU_V3_INIT :return kvm_arm_pmu_v3_init (vcpu); }
@@ -1233,6 +1256,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)case KVM_ARM_VCPU_PMU_V3_INIT :case KVM_ARM_VCPU_PMU_V3_FILTER :case KVM_ARM_VCPU_PMU_V3_SET_PMU : +case KVM_ARM_VCPU_PMU_V3_SET_PMCR_N :if (kvm_vcpu_has_pmu (vcpu))return 0 ; }
Signature · 1 line
2.39.2
Re: [PATCH v2 3/6] KVM: arm64: Allow userspace to limit the number of PMU counters for EL2 VMs
Quoted 23 lines · Marc Zyngier
As long as we had purely EL1 VMs, we could easily update the number of guest-visible counters by letting userspace write to PMCR_EL0.N.
With VMs started at EL2, PMCR_EL1.N only reflects MDCR_EL2.HPMN, and we don't have a good way to limit it.
For this purpose, introduce a new PMUv3 attribute that allows limiting the maximum number of counters. This requires the explicit selection of a PMU.
Suggested-by Oliver Upton <[email protected]>Signed-off-by Marc Zyngier <[email protected]>--- Documentation/virt/kvm/devices/vcpu.rst | 24 ++++++++++++++++++++++++ arch/arm64/include/uapi/asm/kvm.h | 9 +++++---- arch/arm64/kvm/pmu-emul.c | 24 ++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 4 deletions(-)Documentation/virt/kvm/devices/vcpu.rst +0
-0
Documentation/virt/kvm/devices/vcpu.rst exit_reason = KVM_EXIT_FAIL_ENTRY and populate the fail_entry struct by setting · line 140
Quoted 4 lines
Documentation/virt/kvm/devices/vcpu.rst +1
-0@@ -137,6 +137,30 @@ exit_reason = KVM_EXIT_FAIL_ENTRY and populate the fail_entry struct by setting hardare_entry_failure_reason field to KVM_EXIT_FAIL_ENTRY_CPU_UNSUPPORTED and the cpu field to the processor id. +1.5 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_SET_PMCR_N
Similar to my comment on the kvm_arch field, this should be renamed to something other than PMCR_N.
Otherwise the implementation of the ioctl itself LGTM.
Quoted 8 lines
Documentation/virt/kvm/devices/vcpu.rst +8
-0+--------------------------------------------- + +:Parameters: in kvm_device_attr.addr the address to an unsigned int + representing the maximum value taken by PMCR_EL0.N + +:Returns: + + ======= ====================================================
Documentation/virt/kvm/devices/vcpu.rst exit_reason = KVM_EXIT_FAIL_ENTRY and populate the fail_entry struct by setting
Quoted 10 lines
Documentation/virt/kvm/devices/vcpu.rst +10
-0+ -EBUSY PMUv3 already initialized, a VCPU has already run or + an event filter has already been set + -EFAULT Error accessing the value pointed to by addr + -ENODEV PMUv3 not supported or GIC not initialized + -EINVAL No PMUv3 explicitly selected, or value of N out of + range + ======= ==================================================== + +Update the maximum value allowed in PMCR_EL0.N, defining the number of +counters visible to the guest.
Set the number of implemented event counters in the vPMU
Thanks,
Oliver
Marc Zyngier
maintainerPATCH v2 4/6 KVM: arm64: Don't let userspace write to PMCR_EL0.N when the vcpu has EL2
In reply to Marc ZyngierNow that userspace can provide its limit for hte maximum number of counters, prevent it from writing to PMCR_EL0.N, as the value should be derived from MDCR_EL2.HPMN in that case.
--- arch/arm64/kvm/sys_regs.c | 1 + 1 file changed, 1 insertion(+)
arch/arm64/kvm/sys_regs.c +1 -0
@@ -1216,6 +1216,7 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,* with the existing KVM behavior. */ if (!kvm_vm_has_ran_once (kvm) && + !vcpu_has_nv (vcpu) && new_n <=kvm_arm_pmu_get_max_counters (kvm)) kvm->arch.pmcr_n = new_n;
Signature · 1 line
2.39.2
Marc Zyngier
maintainerPATCH v2 5/6 KVM: arm64: Handle out-of-bound write to HDCR_EL2.HPMN
In reply to Marc ZyngierWe don't really pay attention to what gets written to MDCR_EL2.HPMN, and funky guests could play ugly games on us.
Restrict what gets written there, and limit the number of counters to what the PMU is allowed to have.
--- arch/arm64/kvm/sys_regs.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-)
arch/arm64/kvm/sys_regs.c +25 -9
@@ -2571,17 +2571,33 @@ static bool access_mdcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, const struct sys_reg_desc *r) { - u64 old = __vcpu_sys_reg(vcpu, MDCR_EL2); +if (!p->is_write) { + p->regval =__vcpu_sys_reg (vcpu,MDCR_EL2 ); + }else { +u64 hpmn =FIELD_GET (MDCR_EL2_HPMN , p->regval); +u64 old =__vcpu_sys_reg (vcpu,MDCR_EL2 ); +u64 val = p->regval; -if (!access_rw (vcpu, p, r)) -return false; +/* +* If HPMN is out of bounds, limit it to what we actually +* support. This matches the UNKNOWN definition of the field +* in that case, and keeps the emulation simple. Sort of. +*/ +if (hpmn > vcpu->kvm->arch.pmcr_n) { + hpmn = vcpu->kvm->arch.pmcr_n; +u64_replace_bits (val, hpmn,MDCR_EL2_HPMN ); + } -/* -* Request a reload of the PMU to enable/disable the counters affected -* by HPME. -*/ -if ((old ^__vcpu_sys_reg (vcpu,MDCR_EL2 )) &MDCR_EL2_HPME ) -kvm_make_request (KVM_REQ_RELOAD_PMU , vcpu); +vcpu_write_sys_reg (vcpu, val, r->reg); + +/* +* Request a reload of the PMU to enable/disable the +* counters affected by HPME. +*/ + +if ((old ^__vcpu_sys_reg (vcpu,MDCR_EL2 )) &MDCR_EL2_HPME ) +kvm_make_request (KVM_REQ_RELOAD_PMU , vcpu); + }return true; }
Signature · 1 line
2.39.2
Re: [PATCH v2 5/6] KVM: arm64: Handle out-of-bound write to HDCR_EL2.HPMN
HDCR? I thought you gave up on 32-bit a loooong time ago ;-)
Quoted 16 lines · Marc Zyngier
We don't really pay attention to what gets written to MDCR_EL2.HPMN, and funky guests could play ugly games on us.
Restrict what gets written there, and limit the number of counters to what the PMU is allowed to have.
Signed-off-by Marc Zyngier <[email protected]>--- arch/arm64/kvm/sys_regs.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-)arch/arm64/kvm/sys_regs.c +0
-0
arch/arm64/kvm/sys_regs.c static bool access_mdcr(struct kvm_vcpu *vcpu, · line 2576
Quoted 7 lines
arch/arm64/kvm/sys_regs.c +3
-1@@ -2571,17 +2571,33 @@ static bool access_mdcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p, const struct sys_reg_desc *r) { - u64 old = __vcpu_sys_reg(vcpu, MDCR_EL2); +if (!p->is_write) { + p->regval =__vcpu_sys_reg (vcpu,MDCR_EL2 ); + }else {
nit: you can do an early return for an emulated read and get rid of a level of indentation for the write case.
Quoted 38 lines
arch/arm64/kvm/sys_regs.c +22
-8+u64 hpmn =FIELD_GET (MDCR_EL2_HPMN , p->regval); +u64 old =__vcpu_sys_reg (vcpu,MDCR_EL2 ); +u64 val = p->regval; -if (!access_rw (vcpu, p, r)) -return false; +/* +* If HPMN is out of bounds, limit it to what we actually +* support. This matches the UNKNOWN definition of the field +* in that case, and keeps the emulation simple. Sort of. +*/ +if (hpmn > vcpu->kvm->arch.pmcr_n) { + hpmn = vcpu->kvm->arch.pmcr_n; +u64_replace_bits (val, hpmn,MDCR_EL2_HPMN ); + } -/* -* Request a reload of the PMU to enable/disable the counters affected -* by HPME. -*/ -if ((old ^__vcpu_sys_reg (vcpu,MDCR_EL2 )) &MDCR_EL2_HPME ) -kvm_make_request (KVM_REQ_RELOAD_PMU , vcpu); +vcpu_write_sys_reg (vcpu, val, r->reg); + +/* +* Request a reload of the PMU to enable/disable the +* counters affected by HPME. +*/ + +if ((old ^__vcpu_sys_reg (vcpu,MDCR_EL2 )) &MDCR_EL2_HPME ) +kvm_make_request (KVM_REQ_RELOAD_PMU , vcpu); + }return true; }Signature · 1 line
2.39.2
Thanks,
Oliver
Marc Zyngier
maintainerPATCH v2 6/6 KVM: arm64: Let kvm_vcpu_read_pmcr() return an EL-dependent value for PMCR_EL0.N
In reply to Marc ZyngierWhen EL2 is present, PMCR_EL0.N is the effective value of MDCR_EL2.HPMN when accessed from EL1 or EL0.
Make sure we honor this requirement.
--- arch/arm64/kvm/pmu-emul.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
arch/arm64/kvm/pmu-emul.c +5 -1
@@ -1295,8 +1295,12 @@ u8 kvm_arm_pmu_get_pmuver_limit(void)u64 kvm_vcpu_read_pmcr (struct kvm_vcpu *vcpu) {u64 pmcr =__vcpu_sys_reg (vcpu,PMCR_EL0 ); +u64 n = vcpu->kvm->arch.pmcr_n; -return u64_replace_bits (pmcr, vcpu->kvm->arch.pmcr_n,ARMV8_PMU_PMCR_N ); +if (vcpu_has_nv (vcpu) && !vcpu_is_el2 (vcpu)) + n =FIELD_GET (MDCR_EL2_HPMN ,__vcpu_sys_reg (vcpu,MDCR_EL2 )); + +return u64_replace_bits (pmcr, n,ARMV8_PMU_PMCR_N ); }void kvm_pmu_nested_transition (struct kvm_vcpu *vcpu)
Signature · 1 line
2.39.2
Quoted 16 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
Oliver points out that setting PMCR_EL0.N from userspace by writing to the register is silly with NV, and that we need a new PMU attribute instead.
On top of that, I figured out that we had a number of little gotchas:
- It is possible for a guest to write an HPMN value that is out of bound, and it seems valuable to limit it
Quoted 6 lines
- PMCR_EL0.N should be the maximum number of counters when read from EL2, and MDCR_EL2.HPMN when read from EL0/EL1
- Prevent userspace from updating PMCR_EL0.N when EL2 is available
I haven't added any Cc stable, as NV is not functional upstream yet.
Similarly, I don't see a compelling reason for this to go in as a fix for 6.15, especially since you're adding new UAPI. Do you mind grabbing it for the next merge window?
With the comments addressed:
Thanks,
Oliver
Marc Zyngier
maintainerQuoted 25 lines · Oliver Upton
Quoted 16 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
Oliver points out that setting PMCR_EL0.N from userspace by writing to the register is silly with NV, and that we need a new PMU attribute instead.
On top of that, I figured out that we had a number of little gotchas:
- It is possible for a guest to write an HPMN value that is out of bound, and it seems valuable to limit it
Quoted 6 lines
- PMCR_EL0.N should be the maximum number of counters when read from EL2, and MDCR_EL2.HPMN when read from EL0/EL1
- Prevent userspace from updating PMCR_EL0.N when EL2 is available
I haven't added any Cc stable, as NV is not functional upstream yet.
Quoted 7 lines
Similarly, I don't see a compelling reason for this to go in as a fix for 6.15, especially since you're adding new UAPI. Do you mind grabbing it for the next merge window?
With the comments addressed:
Reviewed-by Oliver Upton <[email protected]>
Thanks. I'll repost the whole thing next week and queue it for 6.16.
M.
Signature · 1 line
Without deviation from the norm, progress is not possible.