| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <867c4yoxof.wl-maz@kernel.org>
Date: Sun, 09 Mar 2025 18:40:00 +0000
From: Marc Zyngier <maz@...nel.org>
To: Akihiko Odaki <akihiko.odaki@...nix.com>
Cc: Oliver Upton <oliver.upton@...ux.dev>,
Joey Gouly <joey.gouly@....com>,
Suzuki K Poulose <suzuki.poulose@....com>,
Zenghui Yu <yuzenghui@...wei.com>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>,
Andrew Jones <drjones@...hat.com>,
Shannon Zhao <shannon.zhao@...aro.org>,
linux-arm-kernel@...ts.infradead.org,
kvmarm@...ts.linux.dev,
linux-kernel@...r.kernel.org,
devel@...nix.com
Subject: Re: [PATCH v2 1/3] KVM: arm64: PMU: Fix SET_ONE_REG for vPMC regs
On Fri, 07 Mar 2025 10:55:28 +0000,
Akihiko Odaki <akihiko.odaki@...nix.com> wrote:
>
> Reload the perf event when setting the vPMU counter (vPMC) registers
> (PMCCNTR_EL0 and PMEVCNTR<n>_EL0). This is a change corresponding to
> commit 9228b26194d1 ("KVM: arm64: PMU: Fix GET_ONE_REG
> for vPMC regs to return the current value") but for SET_ONE_REG.
>
> Values of vPMC registers are saved in sysreg files on certain occasions.
> These saved values don't represent the current values of the vPMC
> registers if the perf events for the vPMCs count events after the save.
> The current values of those registers are the sum of the sysreg file
> value and the current perf event counter value. But, when userspace
> writes those registers (using KVM_SET_ONE_REG), KVM only updates the
> sysreg file value and leaves the current perf event counter value as is.
>
> Fix this by releasing the current perf event and trigger recreating one
> with KVM_REQ_RELOAD_PMU.
>
> Fixes: 051ff581ce70 ("arm64: KVM: Add access handler for event counter register")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@...nix.com>
> ---
> arch/arm64/kvm/pmu-emul.c | 16 ++++++++++++++++
> arch/arm64/kvm/sys_regs.c | 20 +++++++++++++++++++-
> include/kvm/arm_pmu.h | 1 +
> 3 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index e3e82b66e2268d37d5e2630e47ddf085a6846e1c..1402cce5625bffa706aabe5e6121d1f3817a0aaf 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -191,6 +191,22 @@ void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
> kvm_pmu_set_pmc_value(kvm_vcpu_idx_to_pmc(vcpu, select_idx), val, false);
> }
>
> +/**
> + * kvm_pmu_set_counter_value_user - set PMU counter value from user
> + * @vcpu: The vcpu pointer
> + * @select_idx: The counter index
> + * @val: The counter value
> + */
> +void kvm_pmu_set_counter_value_user(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
> +{
> + if (!kvm_vcpu_has_pmu(vcpu))
> + return;
How can this occur? It seems to me that we only get here from a
register that has .visibility == pmu_visibility().
Or is there another way to reach this function?
> +
> + kvm_pmu_release_perf_event(kvm_vcpu_idx_to_pmc(vcpu, select_idx));
> + __vcpu_sys_reg(vcpu, counter_index_to_reg(select_idx)) = val;
> + kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
> +}
> +
> /**
> * kvm_pmu_release_perf_event - remove the perf event
> * @pmc: The PMU counter pointer
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 42791971f75887796afab905cc12f49fead39e10..27418dac791df9a89124f867879e899db175e506 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1035,6 +1035,22 @@ static int get_pmu_evcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> return 0;
> }
>
> +static int set_pmu_evcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> + u64 val)
> +{
> + u64 idx;
> +
> + if (r->CRn == 9 && r->CRm == 13 && r->Op2 == 0)
> + /* PMCCNTR_EL0 */
> + idx = ARMV8_PMU_CYCLE_IDX;
> + else
> + /* PMEVCNTRn_EL0 */
> + idx = ((r->CRm & 3) << 3) | (r->Op2 & 7);
> +
> + kvm_pmu_set_counter_value_user(vcpu, idx, val);
> + return 0;
> +}
> +
> static bool access_pmu_evcntr(struct kvm_vcpu *vcpu,
> struct sys_reg_params *p,
> const struct sys_reg_desc *r)
> @@ -1328,6 +1344,7 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> #define PMU_PMEVCNTR_EL0(n) \
> { PMU_SYS_REG(PMEVCNTRn_EL0(n)), \
> .reset = reset_pmevcntr, .get_user = get_pmu_evcntr, \
> + .set_user = set_pmu_evcntr, \
> .access = access_pmu_evcntr, .reg = (PMEVCNTR0_EL0 + n), }
>
> /* Macro to expand the PMEVTYPERn_EL0 register */
> @@ -2682,7 +2699,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> .access = access_pmceid, .reset = NULL },
> { PMU_SYS_REG(PMCCNTR_EL0),
> .access = access_pmu_evcntr, .reset = reset_unknown,
> - .reg = PMCCNTR_EL0, .get_user = get_pmu_evcntr},
> + .reg = PMCCNTR_EL0, .get_user = get_pmu_evcntr,
> + .set_user = set_pmu_evcntr },
> { PMU_SYS_REG(PMXEVTYPER_EL0),
> .access = access_pmu_evtyper, .reset = NULL },
> { PMU_SYS_REG(PMXEVCNTR_EL0),
> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> index 28b380ad8dfa942c4275e0c7ed3535d309b81b2f..9c062756ebfad5ea555362154459ffe9f8311c6d 100644
> --- a/include/kvm/arm_pmu.h
> +++ b/include/kvm/arm_pmu.h
> @@ -41,6 +41,7 @@ bool kvm_supports_guest_pmuv3(void);
> #define kvm_arm_pmu_irq_initialized(v) ((v)->arch.pmu.irq_num >= VGIC_NR_SGIS)
> u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx);
> void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val);
> +void kvm_pmu_set_counter_value_user(struct kvm_vcpu *vcpu, u64 select_idx, u64 val);
> u64 kvm_pmu_valid_counter_mask(struct kvm_vcpu *vcpu);
> u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1);
> void kvm_pmu_vcpu_init(struct kvm_vcpu *vcpu);
Other than the nit above, looks good to me.
M.
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists