* [PATCH 1/9] KVM: arm64: PMU: Align chained counter implementation with architecture pseudocode
2022-08-05 13:58 [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support Marc Zyngier
@ 2022-08-05 13:58 ` Marc Zyngier
2022-08-10 17:21 ` Oliver Upton
2022-08-23 4:30 ` Reiji Watanabe
2022-08-05 13:58 ` [PATCH 2/9] KVM: arm64: PMU: Distinguish between 64bit counter and 64bit overflow Marc Zyngier
` (8 subsequent siblings)
9 siblings, 2 replies; 38+ messages in thread
From: Marc Zyngier @ 2022-08-05 13:58 UTC (permalink / raw)
To: linux-arm-kernel, kvmarm, kvm
Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Oliver Upton,
Ricardo Koller, kernel-team
Ricardo recently pointed out that the PMU chained counter emulation
in KVM wasn't quite behaving like the one on actual hardware, in
the sense that a chained counter would expose an overflow on
both halves of a chained counter, while KVM would only expose the
overflow on the top half.
The difference is subtle, but significant. What does the architecture
say (DDI0087 H.a):
- Before PMUv3p4, all counters but the cycle counter are 32bit
- A 32bit counter that overflows generates a CHAIN event on the
adjacent counter after exposing its own overflow status
- The CHAIN event is accounted if the counter is correctly
configured (CHAIN event selected and counter enabled)
This all means that our current implementation (which uses 64bit
perf events) prevents us from emulating this overflow on the lower half.
How to fix this? By implementing the above, to the letter.
This largly results in code deletion, removing the notions of
"counter pair", "chained counters", and "canonical counter".
The code is further restructured to make the CHAIN handling similar
to SWINC, as the two are now extremely similar in behaviour.
Reported-by: Ricardo Koller <ricarkol@google.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/kvm/pmu-emul.c | 324 +++++++++++---------------------------
include/kvm/arm_pmu.h | 2 -
2 files changed, 91 insertions(+), 235 deletions(-)
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 11c43bed5f97..4986e8b3ea6c 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -21,10 +21,6 @@ static LIST_HEAD(arm_pmus);
static DEFINE_MUTEX(arm_pmus_lock);
static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx);
-static void kvm_pmu_update_pmc_chained(struct kvm_vcpu *vcpu, u64 select_idx);
-static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc);
-
-#define PERF_ATTR_CFG1_KVM_PMU_CHAINED 0x1
static u32 kvm_pmu_event_mask(struct kvm *kvm)
{
@@ -57,6 +53,11 @@ static bool kvm_pmu_idx_is_64bit(struct kvm_vcpu *vcpu, u64 select_idx)
__vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_LC);
}
+static bool kvm_pmu_counter_can_chain(struct kvm_vcpu *vcpu, u64 idx)
+{
+ return (!(idx & 1) && (idx + 1) < ARMV8_PMU_CYCLE_IDX);
+}
+
static struct kvm_vcpu *kvm_pmc_to_vcpu(struct kvm_pmc *pmc)
{
struct kvm_pmu *pmu;
@@ -69,91 +70,22 @@ static struct kvm_vcpu *kvm_pmc_to_vcpu(struct kvm_pmc *pmc)
}
/**
- * kvm_pmu_pmc_is_chained - determine if the pmc is chained
- * @pmc: The PMU counter pointer
- */
-static bool kvm_pmu_pmc_is_chained(struct kvm_pmc *pmc)
-{
- struct kvm_vcpu *vcpu = kvm_pmc_to_vcpu(pmc);
-
- return test_bit(pmc->idx >> 1, vcpu->arch.pmu.chained);
-}
-
-/**
- * kvm_pmu_idx_is_high_counter - determine if select_idx is a high/low counter
- * @select_idx: The counter index
- */
-static bool kvm_pmu_idx_is_high_counter(u64 select_idx)
-{
- return select_idx & 0x1;
-}
-
-/**
- * kvm_pmu_get_canonical_pmc - obtain the canonical pmc
- * @pmc: The PMU counter pointer
- *
- * When a pair of PMCs are chained together we use the low counter (canonical)
- * to hold the underlying perf event.
- */
-static struct kvm_pmc *kvm_pmu_get_canonical_pmc(struct kvm_pmc *pmc)
-{
- if (kvm_pmu_pmc_is_chained(pmc) &&
- kvm_pmu_idx_is_high_counter(pmc->idx))
- return pmc - 1;
-
- return pmc;
-}
-static struct kvm_pmc *kvm_pmu_get_alternate_pmc(struct kvm_pmc *pmc)
-{
- if (kvm_pmu_idx_is_high_counter(pmc->idx))
- return pmc - 1;
- else
- return pmc + 1;
-}
-
-/**
- * kvm_pmu_idx_has_chain_evtype - determine if the event type is chain
+ * kvm_pmu_get_counter_value - get PMU counter value
* @vcpu: The vcpu pointer
* @select_idx: The counter index
*/
-static bool kvm_pmu_idx_has_chain_evtype(struct kvm_vcpu *vcpu, u64 select_idx)
-{
- u64 eventsel, reg;
-
- select_idx |= 0x1;
-
- if (select_idx == ARMV8_PMU_CYCLE_IDX)
- return false;
-
- reg = PMEVTYPER0_EL0 + select_idx;
- eventsel = __vcpu_sys_reg(vcpu, reg) & kvm_pmu_event_mask(vcpu->kvm);
-
- return eventsel == ARMV8_PMUV3_PERFCTR_CHAIN;
-}
-
-/**
- * kvm_pmu_get_pair_counter_value - get PMU counter value
- * @vcpu: The vcpu pointer
- * @pmc: The PMU counter pointer
- */
-static u64 kvm_pmu_get_pair_counter_value(struct kvm_vcpu *vcpu,
- struct kvm_pmc *pmc)
+u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx)
{
- u64 counter, counter_high, reg, enabled, running;
-
- if (kvm_pmu_pmc_is_chained(pmc)) {
- pmc = kvm_pmu_get_canonical_pmc(pmc);
- reg = PMEVCNTR0_EL0 + pmc->idx;
+ u64 counter, reg, enabled, running;
+ struct kvm_pmu *pmu = &vcpu->arch.pmu;
+ struct kvm_pmc *pmc = &pmu->pmc[select_idx];
- counter = __vcpu_sys_reg(vcpu, reg);
- counter_high = __vcpu_sys_reg(vcpu, reg + 1);
+ if (!kvm_vcpu_has_pmu(vcpu))
+ return 0;
- counter = lower_32_bits(counter) | (counter_high << 32);
- } else {
- reg = (pmc->idx == ARMV8_PMU_CYCLE_IDX)
- ? PMCCNTR_EL0 : PMEVCNTR0_EL0 + pmc->idx;
- counter = __vcpu_sys_reg(vcpu, reg);
- }
+ reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
+ ? PMCCNTR_EL0 : PMEVCNTR0_EL0 + pmc->idx;
+ counter = __vcpu_sys_reg(vcpu, reg);
/*
* The real counter value is equal to the value of counter register plus
@@ -163,29 +95,7 @@ static u64 kvm_pmu_get_pair_counter_value(struct kvm_vcpu *vcpu,
counter += perf_event_read_value(pmc->perf_event, &enabled,
&running);
- return counter;
-}
-
-/**
- * kvm_pmu_get_counter_value - get PMU counter value
- * @vcpu: The vcpu pointer
- * @select_idx: The counter index
- */
-u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx)
-{
- u64 counter;
- struct kvm_pmu *pmu = &vcpu->arch.pmu;
- struct kvm_pmc *pmc = &pmu->pmc[select_idx];
-
- if (!kvm_vcpu_has_pmu(vcpu))
- return 0;
-
- counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
-
- if (kvm_pmu_pmc_is_chained(pmc) &&
- kvm_pmu_idx_is_high_counter(select_idx))
- counter = upper_32_bits(counter);
- else if (select_idx != ARMV8_PMU_CYCLE_IDX)
+ if (select_idx != ARMV8_PMU_CYCLE_IDX)
counter = lower_32_bits(counter);
return counter;
@@ -218,7 +128,6 @@ void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
*/
static void kvm_pmu_release_perf_event(struct kvm_pmc *pmc)
{
- pmc = kvm_pmu_get_canonical_pmc(pmc);
if (pmc->perf_event) {
perf_event_disable(pmc->perf_event);
perf_event_release_kernel(pmc->perf_event);
@@ -236,11 +145,10 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc)
{
u64 counter, reg, val;
- pmc = kvm_pmu_get_canonical_pmc(pmc);
if (!pmc->perf_event)
return;
- counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
+ counter = kvm_pmu_get_counter_value(vcpu, pmc->idx);
if (pmc->idx == ARMV8_PMU_CYCLE_IDX) {
reg = PMCCNTR_EL0;
@@ -252,9 +160,6 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc)
__vcpu_sys_reg(vcpu, reg) = val;
- if (kvm_pmu_pmc_is_chained(pmc))
- __vcpu_sys_reg(vcpu, reg + 1) = upper_32_bits(counter);
-
kvm_pmu_release_perf_event(pmc);
}
@@ -285,8 +190,6 @@ void kvm_pmu_vcpu_reset(struct kvm_vcpu *vcpu)
for_each_set_bit(i, &mask, 32)
kvm_pmu_stop_counter(vcpu, &pmu->pmc[i]);
-
- bitmap_zero(vcpu->arch.pmu.chained, ARMV8_PMU_MAX_COUNTER_PAIRS);
}
/**
@@ -340,11 +243,8 @@ void kvm_pmu_enable_counter_mask(struct kvm_vcpu *vcpu, u64 val)
pmc = &pmu->pmc[i];
- /* A change in the enable state may affect the chain state */
- kvm_pmu_update_pmc_chained(vcpu, i);
kvm_pmu_create_perf_event(vcpu, i);
- /* At this point, pmc must be the canonical */
if (pmc->perf_event) {
perf_event_enable(pmc->perf_event);
if (pmc->perf_event->state != PERF_EVENT_STATE_ACTIVE)
@@ -375,11 +275,8 @@ void kvm_pmu_disable_counter_mask(struct kvm_vcpu *vcpu, u64 val)
pmc = &pmu->pmc[i];
- /* A change in the enable state may affect the chain state */
- kvm_pmu_update_pmc_chained(vcpu, i);
kvm_pmu_create_perf_event(vcpu, i);
- /* At this point, pmc must be the canonical */
if (pmc->perf_event)
perf_event_disable(pmc->perf_event);
}
@@ -484,6 +381,51 @@ static void kvm_pmu_perf_overflow_notify_vcpu(struct irq_work *work)
kvm_vcpu_kick(vcpu);
}
+/*
+ * Perform an increment on any of the counters described in @mask,
+ * generating the overflow if required, and propagate it as a chained
+ * event if possible.
+ */
+static void kvm_pmu_counter_increment(struct kvm_vcpu *vcpu,
+ unsigned long mask, u32 event)
+{
+ int i;
+
+ if (!kvm_vcpu_has_pmu(vcpu))
+ return;
+
+ if (!(__vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_E))
+ return;
+
+ /* Weed out disabled counters */
+ mask &= __vcpu_sys_reg(vcpu, PMCNTENSET_EL0);
+
+ for_each_set_bit(i, &mask, ARMV8_PMU_CYCLE_IDX) {
+ u64 type, reg;
+
+ /* Filter on event type */
+ type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i);
+ type &= kvm_pmu_event_mask(vcpu->kvm);
+ if (type != event)
+ continue;
+
+ /* Increment this counter */
+ reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1;
+ reg = lower_32_bits(reg);
+ __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = reg;
+
+ if (reg) /* No overflow? move on */
+ continue;
+
+ /* Mark overflow */
+ __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i);
+
+ if (kvm_pmu_counter_can_chain(vcpu, i))
+ kvm_pmu_counter_increment(vcpu, BIT(i + 1),
+ ARMV8_PMUV3_PERFCTR_CHAIN);
+ }
+}
+
/**
* When the perf event overflows, set the overflow status and inform the vcpu.
*/
@@ -514,6 +456,10 @@ static void kvm_pmu_perf_overflow(struct perf_event *perf_event,
__vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(idx);
+ if (kvm_pmu_counter_can_chain(vcpu, idx))
+ kvm_pmu_counter_increment(vcpu, BIT(idx + 1),
+ ARMV8_PMUV3_PERFCTR_CHAIN);
+
if (kvm_pmu_overflow_status(vcpu)) {
kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
@@ -533,50 +479,7 @@ static void kvm_pmu_perf_overflow(struct perf_event *perf_event,
*/
void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val)
{
- struct kvm_pmu *pmu = &vcpu->arch.pmu;
- int i;
-
- if (!kvm_vcpu_has_pmu(vcpu))
- return;
-
- if (!(__vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_E))
- return;
-
- /* Weed out disabled counters */
- val &= __vcpu_sys_reg(vcpu, PMCNTENSET_EL0);
-
- for (i = 0; i < ARMV8_PMU_CYCLE_IDX; i++) {
- u64 type, reg;
-
- if (!(val & BIT(i)))
- continue;
-
- /* PMSWINC only applies to ... SW_INC! */
- type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i);
- type &= kvm_pmu_event_mask(vcpu->kvm);
- if (type != ARMV8_PMUV3_PERFCTR_SW_INCR)
- continue;
-
- /* increment this even SW_INC counter */
- reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1;
- reg = lower_32_bits(reg);
- __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = reg;
-
- if (reg) /* no overflow on the low part */
- continue;
-
- if (kvm_pmu_pmc_is_chained(&pmu->pmc[i])) {
- /* increment the high counter */
- reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i + 1) + 1;
- reg = lower_32_bits(reg);
- __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i + 1) = reg;
- if (!reg) /* mark overflow on the high counter */
- __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i + 1);
- } else {
- /* mark overflow on low counter */
- __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i);
- }
- }
+ kvm_pmu_counter_increment(vcpu, val, ARMV8_PMUV3_PERFCTR_SW_INCR);
}
/**
@@ -625,30 +528,27 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
{
struct arm_pmu *arm_pmu = vcpu->kvm->arch.arm_pmu;
struct kvm_pmu *pmu = &vcpu->arch.pmu;
- struct kvm_pmc *pmc;
+ struct kvm_pmc *pmc = &pmu->pmc[select_idx];
struct perf_event *event;
struct perf_event_attr attr;
u64 eventsel, counter, reg, data;
- /*
- * For chained counters the event type and filtering attributes are
- * obtained from the low/even counter. We also use this counter to
- * determine if the event is enabled/disabled.
- */
- pmc = kvm_pmu_get_canonical_pmc(&pmu->pmc[select_idx]);
-
- reg = (pmc->idx == ARMV8_PMU_CYCLE_IDX)
+ reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + pmc->idx;
data = __vcpu_sys_reg(vcpu, reg);
kvm_pmu_stop_counter(vcpu, pmc);
- if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
+ if (select_idx == ARMV8_PMU_CYCLE_IDX)
eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
else
eventsel = data & kvm_pmu_event_mask(vcpu->kvm);
- /* Software increment event doesn't need to be backed by a perf event */
- if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
+ /*
+ * Neither SW increment nor chained events need to be backed
+ * by a perf event.
+ */
+ if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR ||
+ eventsel == ARMV8_PMUV3_PERFCTR_CHAIN)
return;
/*
@@ -663,37 +563,27 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
attr.type = arm_pmu->pmu.type;
attr.size = sizeof(attr);
attr.pinned = 1;
- attr.disabled = !kvm_pmu_counter_is_enabled(vcpu, pmc->idx);
+ attr.disabled = !kvm_pmu_counter_is_enabled(vcpu, select_idx);
attr.exclude_user = data & ARMV8_PMU_EXCLUDE_EL0 ? 1 : 0;
attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
attr.exclude_hv = 1; /* Don't count EL2 events */
attr.exclude_host = 1; /* Don't count host events */
attr.config = eventsel;
- counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
+ /* If counting a 64bit event, advertise it to the perf code */
+ if (kvm_pmu_idx_is_64bit(vcpu, select_idx))
+ attr.config1 |= 1;
- if (kvm_pmu_pmc_is_chained(pmc)) {
- /**
- * The initial sample period (overflow count) of an event. For
- * chained counters we only support overflow interrupts on the
- * high counter.
- */
- attr.sample_period = (-counter) & GENMASK(63, 0);
- attr.config1 |= PERF_ATTR_CFG1_KVM_PMU_CHAINED;
+ counter = kvm_pmu_get_counter_value(vcpu, select_idx);
- event = perf_event_create_kernel_counter(&attr, -1, current,
- kvm_pmu_perf_overflow,
- pmc + 1);
- } else {
- /* The initial sample period (overflow count) of an event. */
- if (kvm_pmu_idx_is_64bit(vcpu, pmc->idx))
- attr.sample_period = (-counter) & GENMASK(63, 0);
- else
- attr.sample_period = (-counter) & GENMASK(31, 0);
+ /* The initial sample period (overflow count) of an event. */
+ if (kvm_pmu_idx_is_64bit(vcpu, select_idx))
+ attr.sample_period = (-counter) & GENMASK(63, 0);
+ else
+ attr.sample_period = (-counter) & GENMASK(31, 0);
- event = perf_event_create_kernel_counter(&attr, -1, current,
+ event = perf_event_create_kernel_counter(&attr, -1, current,
kvm_pmu_perf_overflow, pmc);
- }
if (IS_ERR(event)) {
pr_err_once("kvm: pmu event creation failed %ld\n",
@@ -704,41 +594,6 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
pmc->perf_event = event;
}
-/**
- * kvm_pmu_update_pmc_chained - update chained bitmap
- * @vcpu: The vcpu pointer
- * @select_idx: The number of selected counter
- *
- * Update the chained bitmap based on the event type written in the
- * typer register and the enable state of the odd register.
- */
-static void kvm_pmu_update_pmc_chained(struct kvm_vcpu *vcpu, u64 select_idx)
-{
- struct kvm_pmu *pmu = &vcpu->arch.pmu;
- struct kvm_pmc *pmc = &pmu->pmc[select_idx], *canonical_pmc;
- bool new_state, old_state;
-
- old_state = kvm_pmu_pmc_is_chained(pmc);
- new_state = kvm_pmu_idx_has_chain_evtype(vcpu, pmc->idx) &&
- kvm_pmu_counter_is_enabled(vcpu, pmc->idx | 0x1);
-
- if (old_state == new_state)
- return;
-
- canonical_pmc = kvm_pmu_get_canonical_pmc(pmc);
- kvm_pmu_stop_counter(vcpu, canonical_pmc);
- if (new_state) {
- /*
- * During promotion from !chained to chained we must ensure
- * the adjacent counter is stopped and its event destroyed
- */
- kvm_pmu_stop_counter(vcpu, kvm_pmu_get_alternate_pmc(pmc));
- set_bit(pmc->idx >> 1, vcpu->arch.pmu.chained);
- return;
- }
- clear_bit(pmc->idx >> 1, vcpu->arch.pmu.chained);
-}
-
/**
* kvm_pmu_set_counter_event_type - set selected counter to monitor some event
* @vcpu: The vcpu pointer
@@ -752,11 +607,15 @@ static void kvm_pmu_update_pmc_chained(struct kvm_vcpu *vcpu, u64 select_idx)
void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
u64 select_idx)
{
+ struct kvm_pmu *pmu = &vcpu->arch.pmu;
+ struct kvm_pmc *pmc = &pmu->pmc[select_idx];
u64 reg, mask;
if (!kvm_vcpu_has_pmu(vcpu))
return;
+ kvm_pmu_stop_counter(vcpu, pmc);
+
mask = ARMV8_PMU_EVTYPE_MASK;
mask &= ~ARMV8_PMU_EVTYPE_EVENT;
mask |= kvm_pmu_event_mask(vcpu->kvm);
@@ -766,7 +625,6 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
__vcpu_sys_reg(vcpu, reg) = data & mask;
- kvm_pmu_update_pmc_chained(vcpu, select_idx);
kvm_pmu_create_perf_event(vcpu, select_idx);
}
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index c0b868ce6a8f..96b192139a23 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -11,7 +11,6 @@
#include <asm/perf_event.h>
#define ARMV8_PMU_CYCLE_IDX (ARMV8_PMU_MAX_COUNTERS - 1)
-#define ARMV8_PMU_MAX_COUNTER_PAIRS ((ARMV8_PMU_MAX_COUNTERS + 1) >> 1)
#ifdef CONFIG_HW_PERF_EVENTS
@@ -29,7 +28,6 @@ struct kvm_pmu {
struct irq_work overflow_work;
struct kvm_pmu_events events;
struct kvm_pmc pmc[ARMV8_PMU_MAX_COUNTERS];
- DECLARE_BITMAP(chained, ARMV8_PMU_MAX_COUNTER_PAIRS);
int irq_num;
bool created;
bool irq_level;
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 1/9] KVM: arm64: PMU: Align chained counter implementation with architecture pseudocode
2022-08-05 13:58 ` [PATCH 1/9] KVM: arm64: PMU: Align chained counter implementation with architecture pseudocode Marc Zyngier
@ 2022-08-10 17:21 ` Oliver Upton
2022-08-23 4:30 ` Reiji Watanabe
1 sibling, 0 replies; 38+ messages in thread
From: Oliver Upton @ 2022-08-10 17:21 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, kvmarm, kvm, James Morse, Suzuki K Poulose,
Alexandru Elisei, Ricardo Koller, kernel-team
Hi Marc,
On Fri, Aug 05, 2022 at 02:58:05PM +0100, Marc Zyngier wrote:
> Ricardo recently pointed out that the PMU chained counter emulation
> in KVM wasn't quite behaving like the one on actual hardware, in
> the sense that a chained counter would expose an overflow on
> both halves of a chained counter, while KVM would only expose the
> overflow on the top half.
>
> The difference is subtle, but significant. What does the architecture
> say (DDI0087 H.a):
>
> - Before PMUv3p4, all counters but the cycle counter are 32bit
> - A 32bit counter that overflows generates a CHAIN event on the
> adjacent counter after exposing its own overflow status
> - The CHAIN event is accounted if the counter is correctly
> configured (CHAIN event selected and counter enabled)
>
> This all means that our current implementation (which uses 64bit
> perf events) prevents us from emulating this overflow on the lower half.
>
> How to fix this? By implementing the above, to the letter.
>
> This largly results in code deletion, removing the notions of
> "counter pair", "chained counters", and "canonical counter".
> The code is further restructured to make the CHAIN handling similar
> to SWINC, as the two are now extremely similar in behaviour.
>
> Reported-by: Ricardo Koller <ricarkol@google.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/kvm/pmu-emul.c | 324 +++++++++++---------------------------
> include/kvm/arm_pmu.h | 2 -
> 2 files changed, 91 insertions(+), 235 deletions(-)
>
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 11c43bed5f97..4986e8b3ea6c 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -21,10 +21,6 @@ static LIST_HEAD(arm_pmus);
> static DEFINE_MUTEX(arm_pmus_lock);
>
> static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx);
> -static void kvm_pmu_update_pmc_chained(struct kvm_vcpu *vcpu, u64 select_idx);
> -static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc);
> -
> -#define PERF_ATTR_CFG1_KVM_PMU_CHAINED 0x1
nit: The name isn't a good fit for the config bit, but it might be nice to
keep something around.
> static u32 kvm_pmu_event_mask(struct kvm *kvm)
> {
> @@ -57,6 +53,11 @@ static bool kvm_pmu_idx_is_64bit(struct kvm_vcpu *vcpu, u64 select_idx)
> __vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_LC);
> }
>
> +static bool kvm_pmu_counter_can_chain(struct kvm_vcpu *vcpu, u64 idx)
> +{
> + return (!(idx & 1) && (idx + 1) < ARMV8_PMU_CYCLE_IDX);
> +}
> +
> static struct kvm_vcpu *kvm_pmc_to_vcpu(struct kvm_pmc *pmc)
> {
> struct kvm_pmu *pmu;
> @@ -69,91 +70,22 @@ static struct kvm_vcpu *kvm_pmc_to_vcpu(struct kvm_pmc *pmc)
> }
>
> /**
> - * kvm_pmu_pmc_is_chained - determine if the pmc is chained
> - * @pmc: The PMU counter pointer
> - */
> -static bool kvm_pmu_pmc_is_chained(struct kvm_pmc *pmc)
> -{
> - struct kvm_vcpu *vcpu = kvm_pmc_to_vcpu(pmc);
> -
> - return test_bit(pmc->idx >> 1, vcpu->arch.pmu.chained);
> -}
> -
> -/**
> - * kvm_pmu_idx_is_high_counter - determine if select_idx is a high/low counter
> - * @select_idx: The counter index
> - */
> -static bool kvm_pmu_idx_is_high_counter(u64 select_idx)
> -{
> - return select_idx & 0x1;
> -}
> -
> -/**
> - * kvm_pmu_get_canonical_pmc - obtain the canonical pmc
> - * @pmc: The PMU counter pointer
> - *
> - * When a pair of PMCs are chained together we use the low counter (canonical)
> - * to hold the underlying perf event.
> - */
> -static struct kvm_pmc *kvm_pmu_get_canonical_pmc(struct kvm_pmc *pmc)
> -{
> - if (kvm_pmu_pmc_is_chained(pmc) &&
> - kvm_pmu_idx_is_high_counter(pmc->idx))
> - return pmc - 1;
> -
> - return pmc;
> -}
> -static struct kvm_pmc *kvm_pmu_get_alternate_pmc(struct kvm_pmc *pmc)
> -{
> - if (kvm_pmu_idx_is_high_counter(pmc->idx))
> - return pmc - 1;
> - else
> - return pmc + 1;
> -}
> -
> -/**
> - * kvm_pmu_idx_has_chain_evtype - determine if the event type is chain
> + * kvm_pmu_get_counter_value - get PMU counter value
> * @vcpu: The vcpu pointer
> * @select_idx: The counter index
> */
> -static bool kvm_pmu_idx_has_chain_evtype(struct kvm_vcpu *vcpu, u64 select_idx)
> -{
> - u64 eventsel, reg;
> -
> - select_idx |= 0x1;
> -
> - if (select_idx == ARMV8_PMU_CYCLE_IDX)
> - return false;
> -
> - reg = PMEVTYPER0_EL0 + select_idx;
> - eventsel = __vcpu_sys_reg(vcpu, reg) & kvm_pmu_event_mask(vcpu->kvm);
> -
> - return eventsel == ARMV8_PMUV3_PERFCTR_CHAIN;
> -}
> -
> -/**
> - * kvm_pmu_get_pair_counter_value - get PMU counter value
> - * @vcpu: The vcpu pointer
> - * @pmc: The PMU counter pointer
> - */
> -static u64 kvm_pmu_get_pair_counter_value(struct kvm_vcpu *vcpu,
> - struct kvm_pmc *pmc)
> +u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx)
> {
> - u64 counter, counter_high, reg, enabled, running;
> -
> - if (kvm_pmu_pmc_is_chained(pmc)) {
> - pmc = kvm_pmu_get_canonical_pmc(pmc);
> - reg = PMEVCNTR0_EL0 + pmc->idx;
> + u64 counter, reg, enabled, running;
> + struct kvm_pmu *pmu = &vcpu->arch.pmu;
> + struct kvm_pmc *pmc = &pmu->pmc[select_idx];
>
> - counter = __vcpu_sys_reg(vcpu, reg);
> - counter_high = __vcpu_sys_reg(vcpu, reg + 1);
> + if (!kvm_vcpu_has_pmu(vcpu))
> + return 0;
>
> - counter = lower_32_bits(counter) | (counter_high << 32);
> - } else {
> - reg = (pmc->idx == ARMV8_PMU_CYCLE_IDX)
> - ? PMCCNTR_EL0 : PMEVCNTR0_EL0 + pmc->idx;
> - counter = __vcpu_sys_reg(vcpu, reg);
> - }
> + reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
> + ? PMCCNTR_EL0 : PMEVCNTR0_EL0 + pmc->idx;
> + counter = __vcpu_sys_reg(vcpu, reg);
>
> /*
> * The real counter value is equal to the value of counter register plus
> @@ -163,29 +95,7 @@ static u64 kvm_pmu_get_pair_counter_value(struct kvm_vcpu *vcpu,
> counter += perf_event_read_value(pmc->perf_event, &enabled,
> &running);
>
> - return counter;
> -}
> -
> -/**
> - * kvm_pmu_get_counter_value - get PMU counter value
> - * @vcpu: The vcpu pointer
> - * @select_idx: The counter index
> - */
> -u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx)
> -{
> - u64 counter;
> - struct kvm_pmu *pmu = &vcpu->arch.pmu;
> - struct kvm_pmc *pmc = &pmu->pmc[select_idx];
> -
> - if (!kvm_vcpu_has_pmu(vcpu))
> - return 0;
> -
> - counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
> -
> - if (kvm_pmu_pmc_is_chained(pmc) &&
> - kvm_pmu_idx_is_high_counter(select_idx))
> - counter = upper_32_bits(counter);
> - else if (select_idx != ARMV8_PMU_CYCLE_IDX)
> + if (select_idx != ARMV8_PMU_CYCLE_IDX)
> counter = lower_32_bits(counter);
>
> return counter;
> @@ -218,7 +128,6 @@ void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
> */
> static void kvm_pmu_release_perf_event(struct kvm_pmc *pmc)
> {
> - pmc = kvm_pmu_get_canonical_pmc(pmc);
> if (pmc->perf_event) {
> perf_event_disable(pmc->perf_event);
> perf_event_release_kernel(pmc->perf_event);
> @@ -236,11 +145,10 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc)
> {
> u64 counter, reg, val;
>
> - pmc = kvm_pmu_get_canonical_pmc(pmc);
> if (!pmc->perf_event)
> return;
>
> - counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
> + counter = kvm_pmu_get_counter_value(vcpu, pmc->idx);
>
> if (pmc->idx == ARMV8_PMU_CYCLE_IDX) {
> reg = PMCCNTR_EL0;
> @@ -252,9 +160,6 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc)
>
> __vcpu_sys_reg(vcpu, reg) = val;
>
> - if (kvm_pmu_pmc_is_chained(pmc))
> - __vcpu_sys_reg(vcpu, reg + 1) = upper_32_bits(counter);
> -
> kvm_pmu_release_perf_event(pmc);
> }
>
> @@ -285,8 +190,6 @@ void kvm_pmu_vcpu_reset(struct kvm_vcpu *vcpu)
>
> for_each_set_bit(i, &mask, 32)
> kvm_pmu_stop_counter(vcpu, &pmu->pmc[i]);
> -
> - bitmap_zero(vcpu->arch.pmu.chained, ARMV8_PMU_MAX_COUNTER_PAIRS);
> }
>
> /**
> @@ -340,11 +243,8 @@ void kvm_pmu_enable_counter_mask(struct kvm_vcpu *vcpu, u64 val)
>
> pmc = &pmu->pmc[i];
>
> - /* A change in the enable state may affect the chain state */
> - kvm_pmu_update_pmc_chained(vcpu, i);
> kvm_pmu_create_perf_event(vcpu, i);
>
> - /* At this point, pmc must be the canonical */
> if (pmc->perf_event) {
> perf_event_enable(pmc->perf_event);
> if (pmc->perf_event->state != PERF_EVENT_STATE_ACTIVE)
> @@ -375,11 +275,8 @@ void kvm_pmu_disable_counter_mask(struct kvm_vcpu *vcpu, u64 val)
>
> pmc = &pmu->pmc[i];
>
> - /* A change in the enable state may affect the chain state */
> - kvm_pmu_update_pmc_chained(vcpu, i);
> kvm_pmu_create_perf_event(vcpu, i);
>
> - /* At this point, pmc must be the canonical */
> if (pmc->perf_event)
> perf_event_disable(pmc->perf_event);
> }
> @@ -484,6 +381,51 @@ static void kvm_pmu_perf_overflow_notify_vcpu(struct irq_work *work)
> kvm_vcpu_kick(vcpu);
> }
>
> +/*
> + * Perform an increment on any of the counters described in @mask,
> + * generating the overflow if required, and propagate it as a chained
> + * event if possible.
> + */
> +static void kvm_pmu_counter_increment(struct kvm_vcpu *vcpu,
> + unsigned long mask, u32 event)
> +{
> + int i;
> +
> + if (!kvm_vcpu_has_pmu(vcpu))
> + return;
> +
> + if (!(__vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_E))
> + return;
> +
> + /* Weed out disabled counters */
> + mask &= __vcpu_sys_reg(vcpu, PMCNTENSET_EL0);
> +
> + for_each_set_bit(i, &mask, ARMV8_PMU_CYCLE_IDX) {
> + u64 type, reg;
nit: replace 'reg' with 'counter' or 'val'. I think it might read better
as it avoids a collision with counter_index_to_reg()
It feels like this patch could be broken down a bit as I found myself
skipping around a bit. The s/pmc->idx/select_idx/ doesn't seem strictly
necessary to bake in with this patch, either. Nonetheless, the end
result looks good.
--
Thanks,
Oliver
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/9] KVM: arm64: PMU: Align chained counter implementation with architecture pseudocode
2022-08-05 13:58 ` [PATCH 1/9] KVM: arm64: PMU: Align chained counter implementation with architecture pseudocode Marc Zyngier
2022-08-10 17:21 ` Oliver Upton
@ 2022-08-23 4:30 ` Reiji Watanabe
2022-10-24 10:29 ` Marc Zyngier
1 sibling, 1 reply; 38+ messages in thread
From: Reiji Watanabe @ 2022-08-23 4:30 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Linux ARM, kvmarm, kvm, kernel-team
Hi Marc,
On Fri, Aug 5, 2022 at 6:58 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Ricardo recently pointed out that the PMU chained counter emulation
> in KVM wasn't quite behaving like the one on actual hardware, in
> the sense that a chained counter would expose an overflow on
> both halves of a chained counter, while KVM would only expose the
> overflow on the top half.
>
> The difference is subtle, but significant. What does the architecture
> say (DDI0087 H.a):
>
> - Before PMUv3p4, all counters but the cycle counter are 32bit
> - A 32bit counter that overflows generates a CHAIN event on the
> adjacent counter after exposing its own overflow status
> - The CHAIN event is accounted if the counter is correctly
> configured (CHAIN event selected and counter enabled)
>
> This all means that our current implementation (which uses 64bit
> perf events) prevents us from emulating this overflow on the lower half.
>
> How to fix this? By implementing the above, to the letter.
>
> This largly results in code deletion, removing the notions of
> "counter pair", "chained counters", and "canonical counter".
> The code is further restructured to make the CHAIN handling similar
> to SWINC, as the two are now extremely similar in behaviour.
>
> Reported-by: Ricardo Koller <ricarkol@google.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/kvm/pmu-emul.c | 324 +++++++++++---------------------------
> include/kvm/arm_pmu.h | 2 -
> 2 files changed, 91 insertions(+), 235 deletions(-)
>
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 11c43bed5f97..4986e8b3ea6c 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -21,10 +21,6 @@ static LIST_HEAD(arm_pmus);
> static DEFINE_MUTEX(arm_pmus_lock);
>
> static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx);
> -static void kvm_pmu_update_pmc_chained(struct kvm_vcpu *vcpu, u64 select_idx);
> -static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc);
> -
> -#define PERF_ATTR_CFG1_KVM_PMU_CHAINED 0x1
>
> static u32 kvm_pmu_event_mask(struct kvm *kvm)
> {
> @@ -57,6 +53,11 @@ static bool kvm_pmu_idx_is_64bit(struct kvm_vcpu *vcpu, u64 select_idx)
> __vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_LC);
> }
>
> +static bool kvm_pmu_counter_can_chain(struct kvm_vcpu *vcpu, u64 idx)
> +{
> + return (!(idx & 1) && (idx + 1) < ARMV8_PMU_CYCLE_IDX);
> +}
> +
> static struct kvm_vcpu *kvm_pmc_to_vcpu(struct kvm_pmc *pmc)
> {
> struct kvm_pmu *pmu;
> @@ -69,91 +70,22 @@ static struct kvm_vcpu *kvm_pmc_to_vcpu(struct kvm_pmc *pmc)
> }
>
> /**
> - * kvm_pmu_pmc_is_chained - determine if the pmc is chained
> - * @pmc: The PMU counter pointer
> - */
> -static bool kvm_pmu_pmc_is_chained(struct kvm_pmc *pmc)
> -{
> - struct kvm_vcpu *vcpu = kvm_pmc_to_vcpu(pmc);
> -
> - return test_bit(pmc->idx >> 1, vcpu->arch.pmu.chained);
> -}
> -
> -/**
> - * kvm_pmu_idx_is_high_counter - determine if select_idx is a high/low counter
> - * @select_idx: The counter index
> - */
> -static bool kvm_pmu_idx_is_high_counter(u64 select_idx)
> -{
> - return select_idx & 0x1;
> -}
> -
> -/**
> - * kvm_pmu_get_canonical_pmc - obtain the canonical pmc
> - * @pmc: The PMU counter pointer
> - *
> - * When a pair of PMCs are chained together we use the low counter (canonical)
> - * to hold the underlying perf event.
> - */
> -static struct kvm_pmc *kvm_pmu_get_canonical_pmc(struct kvm_pmc *pmc)
> -{
> - if (kvm_pmu_pmc_is_chained(pmc) &&
> - kvm_pmu_idx_is_high_counter(pmc->idx))
> - return pmc - 1;
> -
> - return pmc;
> -}
> -static struct kvm_pmc *kvm_pmu_get_alternate_pmc(struct kvm_pmc *pmc)
> -{
> - if (kvm_pmu_idx_is_high_counter(pmc->idx))
> - return pmc - 1;
> - else
> - return pmc + 1;
> -}
> -
> -/**
> - * kvm_pmu_idx_has_chain_evtype - determine if the event type is chain
> + * kvm_pmu_get_counter_value - get PMU counter value
> * @vcpu: The vcpu pointer
> * @select_idx: The counter index
> */
> -static bool kvm_pmu_idx_has_chain_evtype(struct kvm_vcpu *vcpu, u64 select_idx)
> -{
> - u64 eventsel, reg;
> -
> - select_idx |= 0x1;
> -
> - if (select_idx == ARMV8_PMU_CYCLE_IDX)
> - return false;
> -
> - reg = PMEVTYPER0_EL0 + select_idx;
> - eventsel = __vcpu_sys_reg(vcpu, reg) & kvm_pmu_event_mask(vcpu->kvm);
> -
> - return eventsel == ARMV8_PMUV3_PERFCTR_CHAIN;
> -}
> -
> -/**
> - * kvm_pmu_get_pair_counter_value - get PMU counter value
> - * @vcpu: The vcpu pointer
> - * @pmc: The PMU counter pointer
> - */
> -static u64 kvm_pmu_get_pair_counter_value(struct kvm_vcpu *vcpu,
> - struct kvm_pmc *pmc)
> +u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx)
> {
> - u64 counter, counter_high, reg, enabled, running;
> -
> - if (kvm_pmu_pmc_is_chained(pmc)) {
> - pmc = kvm_pmu_get_canonical_pmc(pmc);
> - reg = PMEVCNTR0_EL0 + pmc->idx;
> + u64 counter, reg, enabled, running;
> + struct kvm_pmu *pmu = &vcpu->arch.pmu;
> + struct kvm_pmc *pmc = &pmu->pmc[select_idx];
>
> - counter = __vcpu_sys_reg(vcpu, reg);
> - counter_high = __vcpu_sys_reg(vcpu, reg + 1);
> + if (!kvm_vcpu_has_pmu(vcpu))
> + return 0;
>
> - counter = lower_32_bits(counter) | (counter_high << 32);
> - } else {
> - reg = (pmc->idx == ARMV8_PMU_CYCLE_IDX)
> - ? PMCCNTR_EL0 : PMEVCNTR0_EL0 + pmc->idx;
> - counter = __vcpu_sys_reg(vcpu, reg);
> - }
> + reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
> + ? PMCCNTR_EL0 : PMEVCNTR0_EL0 + pmc->idx;
> + counter = __vcpu_sys_reg(vcpu, reg);
>
> /*
> * The real counter value is equal to the value of counter register plus
> @@ -163,29 +95,7 @@ static u64 kvm_pmu_get_pair_counter_value(struct kvm_vcpu *vcpu,
> counter += perf_event_read_value(pmc->perf_event, &enabled,
> &running);
>
> - return counter;
> -}
> -
> -/**
> - * kvm_pmu_get_counter_value - get PMU counter value
> - * @vcpu: The vcpu pointer
> - * @select_idx: The counter index
> - */
> -u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx)
> -{
> - u64 counter;
> - struct kvm_pmu *pmu = &vcpu->arch.pmu;
> - struct kvm_pmc *pmc = &pmu->pmc[select_idx];
> -
> - if (!kvm_vcpu_has_pmu(vcpu))
> - return 0;
> -
> - counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
> -
> - if (kvm_pmu_pmc_is_chained(pmc) &&
> - kvm_pmu_idx_is_high_counter(select_idx))
> - counter = upper_32_bits(counter);
> - else if (select_idx != ARMV8_PMU_CYCLE_IDX)
> + if (select_idx != ARMV8_PMU_CYCLE_IDX)
> counter = lower_32_bits(counter);
>
> return counter;
> @@ -218,7 +128,6 @@ void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
> */
> static void kvm_pmu_release_perf_event(struct kvm_pmc *pmc)
> {
> - pmc = kvm_pmu_get_canonical_pmc(pmc);
> if (pmc->perf_event) {
> perf_event_disable(pmc->perf_event);
> perf_event_release_kernel(pmc->perf_event);
> @@ -236,11 +145,10 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc)
> {
> u64 counter, reg, val;
>
> - pmc = kvm_pmu_get_canonical_pmc(pmc);
> if (!pmc->perf_event)
> return;
>
> - counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
> + counter = kvm_pmu_get_counter_value(vcpu, pmc->idx);
>
> if (pmc->idx == ARMV8_PMU_CYCLE_IDX) {
> reg = PMCCNTR_EL0;
> @@ -252,9 +160,6 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc)
>
> __vcpu_sys_reg(vcpu, reg) = val;
>
> - if (kvm_pmu_pmc_is_chained(pmc))
> - __vcpu_sys_reg(vcpu, reg + 1) = upper_32_bits(counter);
> -
> kvm_pmu_release_perf_event(pmc);
> }
>
> @@ -285,8 +190,6 @@ void kvm_pmu_vcpu_reset(struct kvm_vcpu *vcpu)
>
> for_each_set_bit(i, &mask, 32)
> kvm_pmu_stop_counter(vcpu, &pmu->pmc[i]);
> -
> - bitmap_zero(vcpu->arch.pmu.chained, ARMV8_PMU_MAX_COUNTER_PAIRS);
> }
>
> /**
> @@ -340,11 +243,8 @@ void kvm_pmu_enable_counter_mask(struct kvm_vcpu *vcpu, u64 val)
>
> pmc = &pmu->pmc[i];
>
> - /* A change in the enable state may affect the chain state */
> - kvm_pmu_update_pmc_chained(vcpu, i);
> kvm_pmu_create_perf_event(vcpu, i);
>
> - /* At this point, pmc must be the canonical */
> if (pmc->perf_event) {
> perf_event_enable(pmc->perf_event);
> if (pmc->perf_event->state != PERF_EVENT_STATE_ACTIVE)
> @@ -375,11 +275,8 @@ void kvm_pmu_disable_counter_mask(struct kvm_vcpu *vcpu, u64 val)
>
> pmc = &pmu->pmc[i];
>
> - /* A change in the enable state may affect the chain state */
> - kvm_pmu_update_pmc_chained(vcpu, i);
> kvm_pmu_create_perf_event(vcpu, i);
>
> - /* At this point, pmc must be the canonical */
> if (pmc->perf_event)
> perf_event_disable(pmc->perf_event);
> }
> @@ -484,6 +381,51 @@ static void kvm_pmu_perf_overflow_notify_vcpu(struct irq_work *work)
> kvm_vcpu_kick(vcpu);
> }
>
> +/*
> + * Perform an increment on any of the counters described in @mask,
> + * generating the overflow if required, and propagate it as a chained
> + * event if possible.
> + */
> +static void kvm_pmu_counter_increment(struct kvm_vcpu *vcpu,
> + unsigned long mask, u32 event)
> +{
> + int i;
> +
> + if (!kvm_vcpu_has_pmu(vcpu))
> + return;
> +
> + if (!(__vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_E))
> + return;
> +
> + /* Weed out disabled counters */
> + mask &= __vcpu_sys_reg(vcpu, PMCNTENSET_EL0);
> +
> + for_each_set_bit(i, &mask, ARMV8_PMU_CYCLE_IDX) {
> + u64 type, reg;
> +
> + /* Filter on event type */
> + type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i);
> + type &= kvm_pmu_event_mask(vcpu->kvm);
> + if (type != event)
> + continue;
> +
> + /* Increment this counter */
> + reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1;
> + reg = lower_32_bits(reg);
> + __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = reg;
> +
> + if (reg) /* No overflow? move on */
> + continue;
> +
> + /* Mark overflow */
> + __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i);
Perhaps it might be useful to create another helper that takes
care of just one counter (it would essentially do the code above
in the loop). The helper could be used (in addition to the above
loop) from the code below for the CHAIN event case and from
kvm_pmu_perf_overflow(). Then unnecessary execution of
for_each_set_bit() could be avoided for these two cases.
> +
> + if (kvm_pmu_counter_can_chain(vcpu, i))
> + kvm_pmu_counter_increment(vcpu, BIT(i + 1),
> + ARMV8_PMUV3_PERFCTR_CHAIN);
> + }
> +}
> +
> /**
> * When the perf event overflows, set the overflow status and inform the vcpu.
> */
> @@ -514,6 +456,10 @@ static void kvm_pmu_perf_overflow(struct perf_event *perf_event,
>
> __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(idx);
>
> + if (kvm_pmu_counter_can_chain(vcpu, idx))
> + kvm_pmu_counter_increment(vcpu, BIT(idx + 1),
> + ARMV8_PMUV3_PERFCTR_CHAIN);
> +
> if (kvm_pmu_overflow_status(vcpu)) {
> kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
>
> @@ -533,50 +479,7 @@ static void kvm_pmu_perf_overflow(struct perf_event *perf_event,
> */
> void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val)
> {
> - struct kvm_pmu *pmu = &vcpu->arch.pmu;
> - int i;
> -
> - if (!kvm_vcpu_has_pmu(vcpu))
> - return;
> -
> - if (!(__vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_E))
> - return;
> -
> - /* Weed out disabled counters */
> - val &= __vcpu_sys_reg(vcpu, PMCNTENSET_EL0);
> -
> - for (i = 0; i < ARMV8_PMU_CYCLE_IDX; i++) {
> - u64 type, reg;
> -
> - if (!(val & BIT(i)))
> - continue;
> -
> - /* PMSWINC only applies to ... SW_INC! */
> - type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i);
> - type &= kvm_pmu_event_mask(vcpu->kvm);
> - if (type != ARMV8_PMUV3_PERFCTR_SW_INCR)
> - continue;
> -
> - /* increment this even SW_INC counter */
> - reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1;
> - reg = lower_32_bits(reg);
> - __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = reg;
> -
> - if (reg) /* no overflow on the low part */
> - continue;
> -
> - if (kvm_pmu_pmc_is_chained(&pmu->pmc[i])) {
> - /* increment the high counter */
> - reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i + 1) + 1;
> - reg = lower_32_bits(reg);
> - __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i + 1) = reg;
> - if (!reg) /* mark overflow on the high counter */
> - __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i + 1);
> - } else {
> - /* mark overflow on low counter */
> - __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i);
> - }
> - }
> + kvm_pmu_counter_increment(vcpu, val, ARMV8_PMUV3_PERFCTR_SW_INCR);
> }
>
> /**
> @@ -625,30 +528,27 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
> {
> struct arm_pmu *arm_pmu = vcpu->kvm->arch.arm_pmu;
> struct kvm_pmu *pmu = &vcpu->arch.pmu;
> - struct kvm_pmc *pmc;
> + struct kvm_pmc *pmc = &pmu->pmc[select_idx];
> struct perf_event *event;
> struct perf_event_attr attr;
> u64 eventsel, counter, reg, data;
>
> - /*
> - * For chained counters the event type and filtering attributes are
> - * obtained from the low/even counter. We also use this counter to
> - * determine if the event is enabled/disabled.
> - */
> - pmc = kvm_pmu_get_canonical_pmc(&pmu->pmc[select_idx]);
> -
> - reg = (pmc->idx == ARMV8_PMU_CYCLE_IDX)
> + reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
> ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + pmc->idx;
You may want to use select_idx instead of pmc->id for consistency ?
> data = __vcpu_sys_reg(vcpu, reg);
>
> kvm_pmu_stop_counter(vcpu, pmc);
> - if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
> + if (select_idx == ARMV8_PMU_CYCLE_IDX)
> eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
> else
> eventsel = data & kvm_pmu_event_mask(vcpu->kvm);
>
> - /* Software increment event doesn't need to be backed by a perf event */
> - if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
> + /*
> + * Neither SW increment nor chained events need to be backed
> + * by a perf event.
> + */
> + if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR ||
> + eventsel == ARMV8_PMUV3_PERFCTR_CHAIN)
> return;
>
> /*
> @@ -663,37 +563,27 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
> attr.type = arm_pmu->pmu.type;
> attr.size = sizeof(attr);
> attr.pinned = 1;
> - attr.disabled = !kvm_pmu_counter_is_enabled(vcpu, pmc->idx);
> + attr.disabled = !kvm_pmu_counter_is_enabled(vcpu, select_idx);
> attr.exclude_user = data & ARMV8_PMU_EXCLUDE_EL0 ? 1 : 0;
> attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
> attr.exclude_hv = 1; /* Don't count EL2 events */
> attr.exclude_host = 1; /* Don't count host events */
> attr.config = eventsel;
>
> - counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
> + /* If counting a 64bit event, advertise it to the perf code */
> + if (kvm_pmu_idx_is_64bit(vcpu, select_idx))
> + attr.config1 |= 1;
>
> - if (kvm_pmu_pmc_is_chained(pmc)) {
> - /**
> - * The initial sample period (overflow count) of an event. For
> - * chained counters we only support overflow interrupts on the
> - * high counter.
> - */
> - attr.sample_period = (-counter) & GENMASK(63, 0);
> - attr.config1 |= PERF_ATTR_CFG1_KVM_PMU_CHAINED;
> + counter = kvm_pmu_get_counter_value(vcpu, select_idx);
>
> - event = perf_event_create_kernel_counter(&attr, -1, current,
> - kvm_pmu_perf_overflow,
> - pmc + 1);
> - } else {
> - /* The initial sample period (overflow count) of an event. */
> - if (kvm_pmu_idx_is_64bit(vcpu, pmc->idx))
> - attr.sample_period = (-counter) & GENMASK(63, 0);
> - else
> - attr.sample_period = (-counter) & GENMASK(31, 0);
> + /* The initial sample period (overflow count) of an event. */
> + if (kvm_pmu_idx_is_64bit(vcpu, select_idx))
> + attr.sample_period = (-counter) & GENMASK(63, 0);
> + else
> + attr.sample_period = (-counter) & GENMASK(31, 0);
>
> - event = perf_event_create_kernel_counter(&attr, -1, current,
> + event = perf_event_create_kernel_counter(&attr, -1, current,
> kvm_pmu_perf_overflow, pmc);
> - }
>
> if (IS_ERR(event)) {
> pr_err_once("kvm: pmu event creation failed %ld\n",
> @@ -704,41 +594,6 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
> pmc->perf_event = event;
> }
>
> -/**
> - * kvm_pmu_update_pmc_chained - update chained bitmap
> - * @vcpu: The vcpu pointer
> - * @select_idx: The number of selected counter
> - *
> - * Update the chained bitmap based on the event type written in the
> - * typer register and the enable state of the odd register.
> - */
> -static void kvm_pmu_update_pmc_chained(struct kvm_vcpu *vcpu, u64 select_idx)
> -{
> - struct kvm_pmu *pmu = &vcpu->arch.pmu;
> - struct kvm_pmc *pmc = &pmu->pmc[select_idx], *canonical_pmc;
> - bool new_state, old_state;
> -
> - old_state = kvm_pmu_pmc_is_chained(pmc);
> - new_state = kvm_pmu_idx_has_chain_evtype(vcpu, pmc->idx) &&
> - kvm_pmu_counter_is_enabled(vcpu, pmc->idx | 0x1);
> -
> - if (old_state == new_state)
> - return;
> -
> - canonical_pmc = kvm_pmu_get_canonical_pmc(pmc);
> - kvm_pmu_stop_counter(vcpu, canonical_pmc);
> - if (new_state) {
> - /*
> - * During promotion from !chained to chained we must ensure
> - * the adjacent counter is stopped and its event destroyed
> - */
> - kvm_pmu_stop_counter(vcpu, kvm_pmu_get_alternate_pmc(pmc));
> - set_bit(pmc->idx >> 1, vcpu->arch.pmu.chained);
> - return;
> - }
> - clear_bit(pmc->idx >> 1, vcpu->arch.pmu.chained);
> -}
> -
> /**
> * kvm_pmu_set_counter_event_type - set selected counter to monitor some event
> * @vcpu: The vcpu pointer
> @@ -752,11 +607,15 @@ static void kvm_pmu_update_pmc_chained(struct kvm_vcpu *vcpu, u64 select_idx)
> void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
> u64 select_idx)
> {
> + struct kvm_pmu *pmu = &vcpu->arch.pmu;
> + struct kvm_pmc *pmc = &pmu->pmc[select_idx];
> u64 reg, mask;
>
> if (!kvm_vcpu_has_pmu(vcpu))
> return;
>
> + kvm_pmu_stop_counter(vcpu, pmc);
It appears that kvm_pmu_stop_counter() doesn't have to be called here
because it is called in the beginning of kvm_pmu_create_perf_event().
Thank you,
Reiji
> +
> mask = ARMV8_PMU_EVTYPE_MASK;
> mask &= ~ARMV8_PMU_EVTYPE_EVENT;
> mask |= kvm_pmu_event_mask(vcpu->kvm);
> @@ -766,7 +625,6 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
>
> __vcpu_sys_reg(vcpu, reg) = data & mask;
>
> - kvm_pmu_update_pmc_chained(vcpu, select_idx);
> kvm_pmu_create_perf_event(vcpu, select_idx);
> }
>
> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> index c0b868ce6a8f..96b192139a23 100644
> --- a/include/kvm/arm_pmu.h
> +++ b/include/kvm/arm_pmu.h
> @@ -11,7 +11,6 @@
> #include <asm/perf_event.h>
>
> #define ARMV8_PMU_CYCLE_IDX (ARMV8_PMU_MAX_COUNTERS - 1)
> -#define ARMV8_PMU_MAX_COUNTER_PAIRS ((ARMV8_PMU_MAX_COUNTERS + 1) >> 1)
>
> #ifdef CONFIG_HW_PERF_EVENTS
>
> @@ -29,7 +28,6 @@ struct kvm_pmu {
> struct irq_work overflow_work;
> struct kvm_pmu_events events;
> struct kvm_pmc pmc[ARMV8_PMU_MAX_COUNTERS];
> - DECLARE_BITMAP(chained, ARMV8_PMU_MAX_COUNTER_PAIRS);
> int irq_num;
> bool created;
> bool irq_level;
> --
> 2.34.1
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/9] KVM: arm64: PMU: Align chained counter implementation with architecture pseudocode
2022-08-23 4:30 ` Reiji Watanabe
@ 2022-10-24 10:29 ` Marc Zyngier
2022-10-27 14:33 ` Reiji Watanabe
0 siblings, 1 reply; 38+ messages in thread
From: Marc Zyngier @ 2022-10-24 10:29 UTC (permalink / raw)
To: Reiji Watanabe; +Cc: Linux ARM, kvmarm, kvm, kernel-team
Hi Reiji,
Catching up on this.
On Tue, 23 Aug 2022 05:30:21 +0100,
Reiji Watanabe <reijiw@google.com> wrote:
>
> Hi Marc,
>
> On Fri, Aug 5, 2022 at 6:58 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > Ricardo recently pointed out that the PMU chained counter emulation
> > in KVM wasn't quite behaving like the one on actual hardware, in
> > the sense that a chained counter would expose an overflow on
> > both halves of a chained counter, while KVM would only expose the
> > overflow on the top half.
> >
> > The difference is subtle, but significant. What does the architecture
> > say (DDI0087 H.a):
> >
> > - Before PMUv3p4, all counters but the cycle counter are 32bit
> > - A 32bit counter that overflows generates a CHAIN event on the
> > adjacent counter after exposing its own overflow status
> > - The CHAIN event is accounted if the counter is correctly
> > configured (CHAIN event selected and counter enabled)
> >
> > This all means that our current implementation (which uses 64bit
> > perf events) prevents us from emulating this overflow on the lower half.
> >
> > How to fix this? By implementing the above, to the letter.
> >
> > This largly results in code deletion, removing the notions of
> > "counter pair", "chained counters", and "canonical counter".
> > The code is further restructured to make the CHAIN handling similar
> > to SWINC, as the two are now extremely similar in behaviour.
> >
> > Reported-by: Ricardo Koller <ricarkol@google.com>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> > arch/arm64/kvm/pmu-emul.c | 324 +++++++++++---------------------------
> > include/kvm/arm_pmu.h | 2 -
> > 2 files changed, 91 insertions(+), 235 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > index 11c43bed5f97..4986e8b3ea6c 100644
> > --- a/arch/arm64/kvm/pmu-emul.c
> > +++ b/arch/arm64/kvm/pmu-emul.c
[...]
> > +/*
> > + * Perform an increment on any of the counters described in @mask,
> > + * generating the overflow if required, and propagate it as a chained
> > + * event if possible.
> > + */
> > +static void kvm_pmu_counter_increment(struct kvm_vcpu *vcpu,
> > + unsigned long mask, u32 event)
> > +{
> > + int i;
> > +
> > + if (!kvm_vcpu_has_pmu(vcpu))
> > + return;
> > +
> > + if (!(__vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_E))
> > + return;
> > +
> > + /* Weed out disabled counters */
> > + mask &= __vcpu_sys_reg(vcpu, PMCNTENSET_EL0);
> > +
> > + for_each_set_bit(i, &mask, ARMV8_PMU_CYCLE_IDX) {
> > + u64 type, reg;
> > +
> > + /* Filter on event type */
> > + type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i);
> > + type &= kvm_pmu_event_mask(vcpu->kvm);
> > + if (type != event)
> > + continue;
> > +
> > + /* Increment this counter */
> > + reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1;
> > + reg = lower_32_bits(reg);
> > + __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = reg;
> > +
> > + if (reg) /* No overflow? move on */
> > + continue;
> > +
> > + /* Mark overflow */
> > + __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i);
>
> Perhaps it might be useful to create another helper that takes
> care of just one counter (it would essentially do the code above
> in the loop). The helper could be used (in addition to the above
> loop) from the code below for the CHAIN event case and from
> kvm_pmu_perf_overflow(). Then unnecessary execution of
> for_each_set_bit() could be avoided for these two cases.
I'm not sure it really helps. We would still need to check whether the
counter is enabled, and we'd need to bring that into the helper
instead of keeping it outside of the loop.
[...]
> > @@ -625,30 +528,27 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
> > {
> > struct arm_pmu *arm_pmu = vcpu->kvm->arch.arm_pmu;
> > struct kvm_pmu *pmu = &vcpu->arch.pmu;
> > - struct kvm_pmc *pmc;
> > + struct kvm_pmc *pmc = &pmu->pmc[select_idx];
> > struct perf_event *event;
> > struct perf_event_attr attr;
> > u64 eventsel, counter, reg, data;
> >
> > - /*
> > - * For chained counters the event type and filtering attributes are
> > - * obtained from the low/even counter. We also use this counter to
> > - * determine if the event is enabled/disabled.
> > - */
> > - pmc = kvm_pmu_get_canonical_pmc(&pmu->pmc[select_idx]);
> > -
> > - reg = (pmc->idx == ARMV8_PMU_CYCLE_IDX)
> > + reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
> > ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + pmc->idx;
>
> You may want to use select_idx instead of pmc->id for consistency ?
Yes. Although Oliver had a point in saying that these pmc->idx vs
select_idx conversions were not strictly necessary and cluttered the
patch.
[...]
> > @@ -752,11 +607,15 @@ static void kvm_pmu_update_pmc_chained(struct kvm_vcpu *vcpu, u64 select_idx)
> > void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
> > u64 select_idx)
> > {
> > + struct kvm_pmu *pmu = &vcpu->arch.pmu;
> > + struct kvm_pmc *pmc = &pmu->pmc[select_idx];
> > u64 reg, mask;
> >
> > if (!kvm_vcpu_has_pmu(vcpu))
> > return;
> >
> > + kvm_pmu_stop_counter(vcpu, pmc);
>
> It appears that kvm_pmu_stop_counter() doesn't have to be called here
> because it is called in the beginning of kvm_pmu_create_perf_event().
It feels a bit odd to change the event type without stopping the
counter first, but I can't see anything going wrong if we omit it.
I'll drop it.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/9] KVM: arm64: PMU: Align chained counter implementation with architecture pseudocode
2022-10-24 10:29 ` Marc Zyngier
@ 2022-10-27 14:33 ` Reiji Watanabe
2022-10-27 15:21 ` Marc Zyngier
0 siblings, 1 reply; 38+ messages in thread
From: Reiji Watanabe @ 2022-10-27 14:33 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Linux ARM, kvmarm, kvm, kernel-team
Hi Marc,
> > > +static void kvm_pmu_counter_increment(struct kvm_vcpu *vcpu,
> > > + unsigned long mask, u32 event)
> > > +{
> > > + int i;
> > > +
> > > + if (!kvm_vcpu_has_pmu(vcpu))
> > > + return;
> > > +
> > > + if (!(__vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_E))
> > > + return;
> > > +
> > > + /* Weed out disabled counters */
> > > + mask &= __vcpu_sys_reg(vcpu, PMCNTENSET_EL0);
> > > +
> > > + for_each_set_bit(i, &mask, ARMV8_PMU_CYCLE_IDX) {
> > > + u64 type, reg;
> > > +
> > > + /* Filter on event type */
> > > + type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i);
> > > + type &= kvm_pmu_event_mask(vcpu->kvm);
> > > + if (type != event)
> > > + continue;
> > > +
> > > + /* Increment this counter */
> > > + reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1;
> > > + reg = lower_32_bits(reg);
> > > + __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = reg;
> > > +
> > > + if (reg) /* No overflow? move on */
> > > + continue;
> > > +
> > > + /* Mark overflow */
> > > + __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i);
> >
> > Perhaps it might be useful to create another helper that takes
> > care of just one counter (it would essentially do the code above
> > in the loop). The helper could be used (in addition to the above
> > loop) from the code below for the CHAIN event case and from
> > kvm_pmu_perf_overflow(). Then unnecessary execution of
> > for_each_set_bit() could be avoided for these two cases.
>
> I'm not sure it really helps. We would still need to check whether the
> counter is enabled, and we'd need to bring that into the helper
> instead of keeping it outside of the loop.
That's true. It seems that I overlooked that.
Although it appears checking with kvm_vcpu_has_pmu() is unnecessary
(redundant), the check with PMCR_EL0.E is necessary.
Thank you,
Reiji
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 1/9] KVM: arm64: PMU: Align chained counter implementation with architecture pseudocode
2022-10-27 14:33 ` Reiji Watanabe
@ 2022-10-27 15:21 ` Marc Zyngier
0 siblings, 0 replies; 38+ messages in thread
From: Marc Zyngier @ 2022-10-27 15:21 UTC (permalink / raw)
To: Reiji Watanabe; +Cc: Linux ARM, kvmarm, kvm, kernel-team
Hi Reiji,
On 2022-10-27 15:33, Reiji Watanabe wrote:
> Hi Marc,
>
>> > > +static void kvm_pmu_counter_increment(struct kvm_vcpu *vcpu,
>> > > + unsigned long mask, u32 event)
>> > > +{
>> > > + int i;
>> > > +
>> > > + if (!kvm_vcpu_has_pmu(vcpu))
>> > > + return;
>> > > +
>> > > + if (!(__vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_E))
>> > > + return;
>> > > +
>> > > + /* Weed out disabled counters */
>> > > + mask &= __vcpu_sys_reg(vcpu, PMCNTENSET_EL0);
>> > > +
>> > > + for_each_set_bit(i, &mask, ARMV8_PMU_CYCLE_IDX) {
>> > > + u64 type, reg;
>> > > +
>> > > + /* Filter on event type */
>> > > + type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i);
>> > > + type &= kvm_pmu_event_mask(vcpu->kvm);
>> > > + if (type != event)
>> > > + continue;
>> > > +
>> > > + /* Increment this counter */
>> > > + reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1;
>> > > + reg = lower_32_bits(reg);
>> > > + __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = reg;
>> > > +
>> > > + if (reg) /* No overflow? move on */
>> > > + continue;
>> > > +
>> > > + /* Mark overflow */
>> > > + __vcpu_sys_reg(vcpu, PMOVSSET_EL0) |= BIT(i);
>> >
>> > Perhaps it might be useful to create another helper that takes
>> > care of just one counter (it would essentially do the code above
>> > in the loop). The helper could be used (in addition to the above
>> > loop) from the code below for the CHAIN event case and from
>> > kvm_pmu_perf_overflow(). Then unnecessary execution of
>> > for_each_set_bit() could be avoided for these two cases.
>>
>> I'm not sure it really helps. We would still need to check whether the
>> counter is enabled, and we'd need to bring that into the helper
>> instead of keeping it outside of the loop.
>
> That's true. It seems that I overlooked that.
> Although it appears checking with kvm_vcpu_has_pmu() is unnecessary
> (redundant), the check with PMCR_EL0.E is necessary.
Ah indeed, I'll drop the kvm_vcpu_has_pmu() check.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 2/9] KVM: arm64: PMU: Distinguish between 64bit counter and 64bit overflow
2022-08-05 13:58 [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support Marc Zyngier
2022-08-05 13:58 ` [PATCH 1/9] KVM: arm64: PMU: Align chained counter implementation with architecture pseudocode Marc Zyngier
@ 2022-08-05 13:58 ` Marc Zyngier
2022-08-05 13:58 ` [PATCH 3/9] KVM: arm64: PMU: Only narrow counters that are not 64bit wide Marc Zyngier
` (7 subsequent siblings)
9 siblings, 0 replies; 38+ messages in thread
From: Marc Zyngier @ 2022-08-05 13:58 UTC (permalink / raw)
To: linux-arm-kernel, kvmarm, kvm
Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Oliver Upton,
Ricardo Koller, kernel-team
The PMU architecture makes a subtle difference between a 64bit
counter and a counter that has a 64bit overflow. This is for example
the case of the cycle counter, which can generate an overflow on
a 32bit boundary if PMCR_EL0.LC==0 despite the accumulation being
done on 64 bits.
Use this distinction in the few cases where it matters in the code,
as we will reuse this with PMUv3p5 long counters.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/kvm/pmu-emul.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 4986e8b3ea6c..9040d3c80096 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -48,6 +48,11 @@ static u32 kvm_pmu_event_mask(struct kvm *kvm)
* @select_idx: The counter index
*/
static bool kvm_pmu_idx_is_64bit(struct kvm_vcpu *vcpu, u64 select_idx)
+{
+ return (select_idx == ARMV8_PMU_CYCLE_IDX);
+}
+
+static bool kvm_pmu_idx_has_64bit_overflow(struct kvm_vcpu *vcpu, u64 select_idx)
{
return (select_idx == ARMV8_PMU_CYCLE_IDX &&
__vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_LC);
@@ -55,7 +60,8 @@ static bool kvm_pmu_idx_is_64bit(struct kvm_vcpu *vcpu, u64 select_idx)
static bool kvm_pmu_counter_can_chain(struct kvm_vcpu *vcpu, u64 idx)
{
- return (!(idx & 1) && (idx + 1) < ARMV8_PMU_CYCLE_IDX);
+ return (!(idx & 1) && (idx + 1) < ARMV8_PMU_CYCLE_IDX &&
+ !kvm_pmu_idx_has_64bit_overflow(vcpu, idx));
}
static struct kvm_vcpu *kvm_pmc_to_vcpu(struct kvm_pmc *pmc)
@@ -95,7 +101,7 @@ u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx)
counter += perf_event_read_value(pmc->perf_event, &enabled,
&running);
- if (select_idx != ARMV8_PMU_CYCLE_IDX)
+ if (!kvm_pmu_idx_is_64bit(vcpu, select_idx))
counter = lower_32_bits(counter);
return counter;
@@ -447,7 +453,7 @@ static void kvm_pmu_perf_overflow(struct perf_event *perf_event,
*/
period = -(local64_read(&perf_event->count));
- if (!kvm_pmu_idx_is_64bit(vcpu, pmc->idx))
+ if (!kvm_pmu_idx_has_64bit_overflow(vcpu, pmc->idx))
period &= GENMASK(31, 0);
local64_set(&perf_event->hw.period_left, 0);
@@ -577,7 +583,7 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
counter = kvm_pmu_get_counter_value(vcpu, select_idx);
/* The initial sample period (overflow count) of an event. */
- if (kvm_pmu_idx_is_64bit(vcpu, select_idx))
+ if (kvm_pmu_idx_has_64bit_overflow(vcpu, select_idx))
attr.sample_period = (-counter) & GENMASK(63, 0);
else
attr.sample_period = (-counter) & GENMASK(31, 0);
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH 3/9] KVM: arm64: PMU: Only narrow counters that are not 64bit wide
2022-08-05 13:58 [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support Marc Zyngier
2022-08-05 13:58 ` [PATCH 1/9] KVM: arm64: PMU: Align chained counter implementation with architecture pseudocode Marc Zyngier
2022-08-05 13:58 ` [PATCH 2/9] KVM: arm64: PMU: Distinguish between 64bit counter and 64bit overflow Marc Zyngier
@ 2022-08-05 13:58 ` Marc Zyngier
2022-08-24 4:07 ` Reiji Watanabe
2022-08-05 13:58 ` [PATCH 4/9] KVM: arm64: PMU: Add counter_index_to_*reg() helpers Marc Zyngier
` (6 subsequent siblings)
9 siblings, 1 reply; 38+ messages in thread
From: Marc Zyngier @ 2022-08-05 13:58 UTC (permalink / raw)
To: linux-arm-kernel, kvmarm, kvm
Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Oliver Upton,
Ricardo Koller, kernel-team
The current PMU emulation sometimes narrows counters to 32bit
if the counter isn't the cycle counter. As this is going to
change with PMUv3p5 where the counters are all 64bit, only
perform the narrowing on 32bit counters.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/kvm/pmu-emul.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 9040d3c80096..0ab6f59f433c 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -149,22 +149,22 @@ static void kvm_pmu_release_perf_event(struct kvm_pmc *pmc)
*/
static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc)
{
- u64 counter, reg, val;
+ u64 counter, reg;
if (!pmc->perf_event)
return;
counter = kvm_pmu_get_counter_value(vcpu, pmc->idx);
- if (pmc->idx == ARMV8_PMU_CYCLE_IDX) {
+ if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
reg = PMCCNTR_EL0;
- val = counter;
- } else {
+ else
reg = PMEVCNTR0_EL0 + pmc->idx;
- val = lower_32_bits(counter);
- }
- __vcpu_sys_reg(vcpu, reg) = val;
+ if (!kvm_pmu_idx_is_64bit(vcpu, pmc->idx))
+ counter = lower_32_bits(counter);
+
+ __vcpu_sys_reg(vcpu, reg) = counter;
kvm_pmu_release_perf_event(pmc);
}
@@ -417,7 +417,8 @@ static void kvm_pmu_counter_increment(struct kvm_vcpu *vcpu,
/* Increment this counter */
reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1;
- reg = lower_32_bits(reg);
+ if (!kvm_pmu_idx_is_64bit(vcpu, i))
+ reg = lower_32_bits(reg);
__vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = reg;
if (reg) /* No overflow? move on */
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 3/9] KVM: arm64: PMU: Only narrow counters that are not 64bit wide
2022-08-05 13:58 ` [PATCH 3/9] KVM: arm64: PMU: Only narrow counters that are not 64bit wide Marc Zyngier
@ 2022-08-24 4:07 ` Reiji Watanabe
0 siblings, 0 replies; 38+ messages in thread
From: Reiji Watanabe @ 2022-08-24 4:07 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Linux ARM, kvmarm, kvm, kernel-team
Hi Marc,
On Fri, Aug 5, 2022 at 6:58 AM Marc Zyngier <maz@kernel.org> wrote:
>
> The current PMU emulation sometimes narrows counters to 32bit
> if the counter isn't the cycle counter. As this is going to
> change with PMUv3p5 where the counters are all 64bit, only
> perform the narrowing on 32bit counters.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/kvm/pmu-emul.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 9040d3c80096..0ab6f59f433c 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -149,22 +149,22 @@ static void kvm_pmu_release_perf_event(struct kvm_pmc *pmc)
> */
> static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc)
> {
> - u64 counter, reg, val;
> + u64 counter, reg;
>
> if (!pmc->perf_event)
> return;
>
> counter = kvm_pmu_get_counter_value(vcpu, pmc->idx);
>
> - if (pmc->idx == ARMV8_PMU_CYCLE_IDX) {
> + if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
> reg = PMCCNTR_EL0;
> - val = counter;
> - } else {
> + else
> reg = PMEVCNTR0_EL0 + pmc->idx;
> - val = lower_32_bits(counter);
> - }
>
> - __vcpu_sys_reg(vcpu, reg) = val;
> + if (!kvm_pmu_idx_is_64bit(vcpu, pmc->idx))
> + counter = lower_32_bits(counter);
It appears that narrowing the counter to 32bit here is unnecessary
because it is already done by kvm_pmu_get_counter_value().
Thank you,
Reiji
> +
> + __vcpu_sys_reg(vcpu, reg) = counter;
>
> kvm_pmu_release_perf_event(pmc);
> }
> @@ -417,7 +417,8 @@ static void kvm_pmu_counter_increment(struct kvm_vcpu *vcpu,
>
> /* Increment this counter */
> reg = __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1;
> - reg = lower_32_bits(reg);
> + if (!kvm_pmu_idx_is_64bit(vcpu, i))
> + reg = lower_32_bits(reg);
> __vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = reg;
>
> if (reg) /* No overflow? move on */
> --
> 2.34.1
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 4/9] KVM: arm64: PMU: Add counter_index_to_*reg() helpers
2022-08-05 13:58 [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support Marc Zyngier
` (2 preceding siblings ...)
2022-08-05 13:58 ` [PATCH 3/9] KVM: arm64: PMU: Only narrow counters that are not 64bit wide Marc Zyngier
@ 2022-08-05 13:58 ` Marc Zyngier
2022-08-10 7:17 ` Oliver Upton
2022-08-24 4:27 ` Reiji Watanabe
2022-08-05 13:58 ` [PATCH 5/9] KVM: arm64: PMU: Simplify setting a counter to a specific value Marc Zyngier
` (5 subsequent siblings)
9 siblings, 2 replies; 38+ messages in thread
From: Marc Zyngier @ 2022-08-05 13:58 UTC (permalink / raw)
To: linux-arm-kernel, kvmarm, kvm
Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Oliver Upton,
Ricardo Koller, kernel-team
In order to reduce the boilerplate code, add two helpers returning
the counter register index (resp. the event register) in the vcpu
register file from the counter index.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/kvm/pmu-emul.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 0ab6f59f433c..9be485d23416 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -75,6 +75,16 @@ static struct kvm_vcpu *kvm_pmc_to_vcpu(struct kvm_pmc *pmc)
return container_of(vcpu_arch, struct kvm_vcpu, arch);
}
+static u32 counter_index_to_reg(u64 idx)
+{
+ return (idx == ARMV8_PMU_CYCLE_IDX) ? PMCCNTR_EL0 : PMEVCNTR0_EL0 + idx;
+}
+
+static u32 counter_index_to_evtreg(u64 idx)
+{
+ return (idx == ARMV8_PMU_CYCLE_IDX) ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + idx;
+}
+
/**
* kvm_pmu_get_counter_value - get PMU counter value
* @vcpu: The vcpu pointer
@@ -89,8 +99,7 @@ u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx)
if (!kvm_vcpu_has_pmu(vcpu))
return 0;
- reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
- ? PMCCNTR_EL0 : PMEVCNTR0_EL0 + pmc->idx;
+ reg = counter_index_to_reg(select_idx);
counter = __vcpu_sys_reg(vcpu, reg);
/*
@@ -120,8 +129,7 @@ void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
if (!kvm_vcpu_has_pmu(vcpu))
return;
- reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
- ? PMCCNTR_EL0 : PMEVCNTR0_EL0 + select_idx;
+ reg = counter_index_to_reg(select_idx);
__vcpu_sys_reg(vcpu, reg) += (s64)val - kvm_pmu_get_counter_value(vcpu, select_idx);
/* Recreate the perf event to reflect the updated sample_period */
@@ -156,10 +164,7 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc)
counter = kvm_pmu_get_counter_value(vcpu, pmc->idx);
- if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
- reg = PMCCNTR_EL0;
- else
- reg = PMEVCNTR0_EL0 + pmc->idx;
+ reg = counter_index_to_reg(pmc->idx);
if (!kvm_pmu_idx_is_64bit(vcpu, pmc->idx))
counter = lower_32_bits(counter);
@@ -540,8 +545,7 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
struct perf_event_attr attr;
u64 eventsel, counter, reg, data;
- reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
- ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + pmc->idx;
+ reg = counter_index_to_evtreg(select_idx);
data = __vcpu_sys_reg(vcpu, reg);
kvm_pmu_stop_counter(vcpu, pmc);
@@ -627,8 +631,7 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
mask &= ~ARMV8_PMU_EVTYPE_EVENT;
mask |= kvm_pmu_event_mask(vcpu->kvm);
- reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
- ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + select_idx;
+ reg = counter_index_to_evtreg(select_idx);
__vcpu_sys_reg(vcpu, reg) = data & mask;
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 4/9] KVM: arm64: PMU: Add counter_index_to_*reg() helpers
2022-08-05 13:58 ` [PATCH 4/9] KVM: arm64: PMU: Add counter_index_to_*reg() helpers Marc Zyngier
@ 2022-08-10 7:17 ` Oliver Upton
2022-08-10 17:23 ` Oliver Upton
2022-08-24 4:27 ` Reiji Watanabe
1 sibling, 1 reply; 38+ messages in thread
From: Oliver Upton @ 2022-08-10 7:17 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, kvmarm, kvm, James Morse, Suzuki K Poulose,
Alexandru Elisei, Ricardo Koller, kernel-team
On Fri, Aug 05, 2022 at 02:58:08PM +0100, Marc Zyngier wrote:
> In order to reduce the boilerplate code, add two helpers returning
> the counter register index (resp. the event register) in the vcpu
> register file from the counter index.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
Reviewed-by: Oliver Upton <oliver.upton@linux.dev>
> ---
> arch/arm64/kvm/pmu-emul.c | 27 +++++++++++++++------------
> 1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 0ab6f59f433c..9be485d23416 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -75,6 +75,16 @@ static struct kvm_vcpu *kvm_pmc_to_vcpu(struct kvm_pmc *pmc)
> return container_of(vcpu_arch, struct kvm_vcpu, arch);
> }
>
> +static u32 counter_index_to_reg(u64 idx)
> +{
> + return (idx == ARMV8_PMU_CYCLE_IDX) ? PMCCNTR_EL0 : PMEVCNTR0_EL0 + idx;
> +}
> +
> +static u32 counter_index_to_evtreg(u64 idx)
> +{
> + return (idx == ARMV8_PMU_CYCLE_IDX) ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + idx;
> +}
> +
> /**
> * kvm_pmu_get_counter_value - get PMU counter value
> * @vcpu: The vcpu pointer
> @@ -89,8 +99,7 @@ u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx)
> if (!kvm_vcpu_has_pmu(vcpu))
> return 0;
>
> - reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
> - ? PMCCNTR_EL0 : PMEVCNTR0_EL0 + pmc->idx;
> + reg = counter_index_to_reg(select_idx);
> counter = __vcpu_sys_reg(vcpu, reg);
>
> /*
> @@ -120,8 +129,7 @@ void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
> if (!kvm_vcpu_has_pmu(vcpu))
> return;
>
> - reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
> - ? PMCCNTR_EL0 : PMEVCNTR0_EL0 + select_idx;
> + reg = counter_index_to_reg(select_idx);
> __vcpu_sys_reg(vcpu, reg) += (s64)val - kvm_pmu_get_counter_value(vcpu, select_idx);
>
> /* Recreate the perf event to reflect the updated sample_period */
> @@ -156,10 +164,7 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc)
>
> counter = kvm_pmu_get_counter_value(vcpu, pmc->idx);
>
> - if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
> - reg = PMCCNTR_EL0;
> - else
> - reg = PMEVCNTR0_EL0 + pmc->idx;
> + reg = counter_index_to_reg(pmc->idx);
>
> if (!kvm_pmu_idx_is_64bit(vcpu, pmc->idx))
> counter = lower_32_bits(counter);
> @@ -540,8 +545,7 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
> struct perf_event_attr attr;
> u64 eventsel, counter, reg, data;
>
> - reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
> - ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + pmc->idx;
> + reg = counter_index_to_evtreg(select_idx);
> data = __vcpu_sys_reg(vcpu, reg);
>
> kvm_pmu_stop_counter(vcpu, pmc);
> @@ -627,8 +631,7 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
> mask &= ~ARMV8_PMU_EVTYPE_EVENT;
> mask |= kvm_pmu_event_mask(vcpu->kvm);
>
> - reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
> - ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + select_idx;
> + reg = counter_index_to_evtreg(select_idx);
>
> __vcpu_sys_reg(vcpu, reg) = data & mask;
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 4/9] KVM: arm64: PMU: Add counter_index_to_*reg() helpers
2022-08-10 7:17 ` Oliver Upton
@ 2022-08-10 17:23 ` Oliver Upton
0 siblings, 0 replies; 38+ messages in thread
From: Oliver Upton @ 2022-08-10 17:23 UTC (permalink / raw)
To: Marc Zyngier; +Cc: kvm, kernel-team, kvmarm, linux-arm-kernel
On Wed, Aug 10, 2022 at 02:17:52AM -0500, Oliver Upton wrote:
> On Fri, Aug 05, 2022 at 02:58:08PM +0100, Marc Zyngier wrote:
> > In order to reduce the boilerplate code, add two helpers returning
> > the counter register index (resp. the event register) in the vcpu
> > register file from the counter index.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
>
> Reviewed-by: Oliver Upton <oliver.upton@linux.dev>
>
> > ---
> > arch/arm64/kvm/pmu-emul.c | 27 +++++++++++++++------------
> > 1 file changed, 15 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > index 0ab6f59f433c..9be485d23416 100644
> > --- a/arch/arm64/kvm/pmu-emul.c
> > +++ b/arch/arm64/kvm/pmu-emul.c
> > @@ -75,6 +75,16 @@ static struct kvm_vcpu *kvm_pmc_to_vcpu(struct kvm_pmc *pmc)
> > return container_of(vcpu_arch, struct kvm_vcpu, arch);
> > }
> >
> > +static u32 counter_index_to_reg(u64 idx)
> > +{
> > + return (idx == ARMV8_PMU_CYCLE_IDX) ? PMCCNTR_EL0 : PMEVCNTR0_EL0 + idx;
> > +}
> > +
> > +static u32 counter_index_to_evtreg(u64 idx)
> > +{
> > + return (idx == ARMV8_PMU_CYCLE_IDX) ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + idx;
> > +}
> > +
After reading the series, do you think these helpers could be applied to
kvm_pmu_counter_increment() as well?
--
Thanks,
Oliver
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 4/9] KVM: arm64: PMU: Add counter_index_to_*reg() helpers
2022-08-05 13:58 ` [PATCH 4/9] KVM: arm64: PMU: Add counter_index_to_*reg() helpers Marc Zyngier
2022-08-10 7:17 ` Oliver Upton
@ 2022-08-24 4:27 ` Reiji Watanabe
1 sibling, 0 replies; 38+ messages in thread
From: Reiji Watanabe @ 2022-08-24 4:27 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Linux ARM, kvmarm, kvm, kernel-team
Hi Marc,
On Fri, Aug 5, 2022 at 6:58 AM Marc Zyngier <maz@kernel.org> wrote:
>
> In order to reduce the boilerplate code, add two helpers returning
> the counter register index (resp. the event register) in the vcpu
> register file from the counter index.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/kvm/pmu-emul.c | 27 +++++++++++++++------------
> 1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 0ab6f59f433c..9be485d23416 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -75,6 +75,16 @@ static struct kvm_vcpu *kvm_pmc_to_vcpu(struct kvm_pmc *pmc)
> return container_of(vcpu_arch, struct kvm_vcpu, arch);
> }
>
> +static u32 counter_index_to_reg(u64 idx)
> +{
> + return (idx == ARMV8_PMU_CYCLE_IDX) ? PMCCNTR_EL0 : PMEVCNTR0_EL0 + idx;
> +}
> +
> +static u32 counter_index_to_evtreg(u64 idx)
> +{
> + return (idx == ARMV8_PMU_CYCLE_IDX) ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + idx;
> +}
> +
> /**
> * kvm_pmu_get_counter_value - get PMU counter value
> * @vcpu: The vcpu pointer
> @@ -89,8 +99,7 @@ u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx)
> if (!kvm_vcpu_has_pmu(vcpu))
> return 0;
>
> - reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
> - ? PMCCNTR_EL0 : PMEVCNTR0_EL0 + pmc->idx;
> + reg = counter_index_to_reg(select_idx);
> counter = __vcpu_sys_reg(vcpu, reg);
>
> /*
> @@ -120,8 +129,7 @@ void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
> if (!kvm_vcpu_has_pmu(vcpu))
> return;
>
> - reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
> - ? PMCCNTR_EL0 : PMEVCNTR0_EL0 + select_idx;
> + reg = counter_index_to_reg(select_idx);
> __vcpu_sys_reg(vcpu, reg) += (s64)val - kvm_pmu_get_counter_value(vcpu, select_idx);
>
> /* Recreate the perf event to reflect the updated sample_period */
> @@ -156,10 +164,7 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc)
>
> counter = kvm_pmu_get_counter_value(vcpu, pmc->idx);
>
> - if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
> - reg = PMCCNTR_EL0;
> - else
> - reg = PMEVCNTR0_EL0 + pmc->idx;
> + reg = counter_index_to_reg(pmc->idx);
>
> if (!kvm_pmu_idx_is_64bit(vcpu, pmc->idx))
> counter = lower_32_bits(counter);
> @@ -540,8 +545,7 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
> struct perf_event_attr attr;
> u64 eventsel, counter, reg, data;
>
> - reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
> - ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + pmc->idx;
> + reg = counter_index_to_evtreg(select_idx);
> data = __vcpu_sys_reg(vcpu, reg);
>
> kvm_pmu_stop_counter(vcpu, pmc);
> @@ -627,8 +631,7 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
> mask &= ~ARMV8_PMU_EVTYPE_EVENT;
> mask |= kvm_pmu_event_mask(vcpu->kvm);
>
> - reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
> - ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + select_idx;
> + reg = counter_index_to_evtreg(select_idx);
>
> __vcpu_sys_reg(vcpu, reg) = data & mask;
>
> --
counter_index_to_evtreg() could also be used in access_pmu_evtyper()
in sys_regs.c (counter_index_to_evtreg() is currently defined as
static in pmu-emul.c though).
---
static bool access_pmu_evtyper(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
const struct sys_reg_desc *r)
{
<...>
} else if (r->CRn == 14 && (r->CRm & 12) == 12) {
idx = ((r->CRm & 3) << 3) | (r->Op2 & 7);
if (idx == ARMV8_PMU_CYCLE_IDX)
reg = PMCCFILTR_EL0;
else
/* PMEVTYPERn_EL0 */
reg = PMEVTYPER0_EL0 + idx;
<...>
---
Thank you,
Reiji
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 5/9] KVM: arm64: PMU: Simplify setting a counter to a specific value
2022-08-05 13:58 [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support Marc Zyngier
` (3 preceding siblings ...)
2022-08-05 13:58 ` [PATCH 4/9] KVM: arm64: PMU: Add counter_index_to_*reg() helpers Marc Zyngier
@ 2022-08-05 13:58 ` Marc Zyngier
2022-08-10 15:41 ` Oliver Upton
2022-08-05 13:58 ` [PATCH 6/9] KVM: arm64: PMU: Move the ID_AA64DFR0_EL1.PMUver limit to VM creation Marc Zyngier
` (4 subsequent siblings)
9 siblings, 1 reply; 38+ messages in thread
From: Marc Zyngier @ 2022-08-05 13:58 UTC (permalink / raw)
To: linux-arm-kernel, kvmarm, kvm
Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Oliver Upton,
Ricardo Koller, kernel-team
kvm_pmu_set_counter_value() is pretty odd, as it tries to update
the counter value while taking into account the value that is
currently held by the running perf counter.
This is not only complicated, this is quite wrong. Nowhere in
the architecture is it said that the counter would be offset
by something that is pending. The counter should be updated
with the value set by SW, and start counting from there if
required.
Remove the odd computation and just assign the provided value
after having released the perf event (which is then restarted).
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/kvm/pmu-emul.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 9be485d23416..ddd79b64b38a 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -21,6 +21,7 @@ static LIST_HEAD(arm_pmus);
static DEFINE_MUTEX(arm_pmus_lock);
static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx);
+static void kvm_pmu_release_perf_event(struct kvm_pmc *pmc);
static u32 kvm_pmu_event_mask(struct kvm *kvm)
{
@@ -129,8 +130,10 @@ void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
if (!kvm_vcpu_has_pmu(vcpu))
return;
+ kvm_pmu_release_perf_event(&vcpu->arch.pmu.pmc[select_idx]);
+
reg = counter_index_to_reg(select_idx);
- __vcpu_sys_reg(vcpu, reg) += (s64)val - kvm_pmu_get_counter_value(vcpu, select_idx);
+ __vcpu_sys_reg(vcpu, reg) = val;
/* Recreate the perf event to reflect the updated sample_period */
kvm_pmu_create_perf_event(vcpu, select_idx);
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 5/9] KVM: arm64: PMU: Simplify setting a counter to a specific value
2022-08-05 13:58 ` [PATCH 5/9] KVM: arm64: PMU: Simplify setting a counter to a specific value Marc Zyngier
@ 2022-08-10 15:41 ` Oliver Upton
0 siblings, 0 replies; 38+ messages in thread
From: Oliver Upton @ 2022-08-10 15:41 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, kvmarm, kvm, James Morse, Suzuki K Poulose,
Alexandru Elisei, Ricardo Koller, kernel-team
On Fri, Aug 05, 2022 at 02:58:09PM +0100, Marc Zyngier wrote:
> kvm_pmu_set_counter_value() is pretty odd, as it tries to update
> the counter value while taking into account the value that is
> currently held by the running perf counter.
>
> This is not only complicated, this is quite wrong. Nowhere in
> the architecture is it said that the counter would be offset
> by something that is pending. The counter should be updated
> with the value set by SW, and start counting from there if
> required.
>
> Remove the odd computation and just assign the provided value
> after having released the perf event (which is then restarted).
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
Looks much better.
Reviewed-by: Oliver Upton <oliver.upton@linux.dev>
> ---
> arch/arm64/kvm/pmu-emul.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 9be485d23416..ddd79b64b38a 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -21,6 +21,7 @@ static LIST_HEAD(arm_pmus);
> static DEFINE_MUTEX(arm_pmus_lock);
>
> static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx);
> +static void kvm_pmu_release_perf_event(struct kvm_pmc *pmc);
>
> static u32 kvm_pmu_event_mask(struct kvm *kvm)
> {
> @@ -129,8 +130,10 @@ void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val)
> if (!kvm_vcpu_has_pmu(vcpu))
> return;
>
> + kvm_pmu_release_perf_event(&vcpu->arch.pmu.pmc[select_idx]);
> +
<bikeshed>
Not your code, but since we're here: it seems as though there is some
inconsistency in parameterization as some functions take an index and
others take a kvm_pmc pointer. For example,
kvm_pmu_{create,release}_perf_event() are inconsistent.
It might be nice to pick a scheme and apply consistently throughout.
</bikeshed>
--
Thanks,
Oliver
> reg = counter_index_to_reg(select_idx);
> - __vcpu_sys_reg(vcpu, reg) += (s64)val - kvm_pmu_get_counter_value(vcpu, select_idx);
> + __vcpu_sys_reg(vcpu, reg) = val;
>
> /* Recreate the perf event to reflect the updated sample_period */
> kvm_pmu_create_perf_event(vcpu, select_idx);
> --
> 2.34.1
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 6/9] KVM: arm64: PMU: Move the ID_AA64DFR0_EL1.PMUver limit to VM creation
2022-08-05 13:58 [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support Marc Zyngier
` (4 preceding siblings ...)
2022-08-05 13:58 ` [PATCH 5/9] KVM: arm64: PMU: Simplify setting a counter to a specific value Marc Zyngier
@ 2022-08-05 13:58 ` Marc Zyngier
2022-08-26 4:34 ` Reiji Watanabe
2022-08-05 13:58 ` [PATCH 7/9] KVM: arm64: PMU: Allow ID_AA64DFR0_EL1.PMUver to be set from userspace Marc Zyngier
` (3 subsequent siblings)
9 siblings, 1 reply; 38+ messages in thread
From: Marc Zyngier @ 2022-08-05 13:58 UTC (permalink / raw)
To: linux-arm-kernel, kvmarm, kvm
Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Oliver Upton,
Ricardo Koller, kernel-team
As further patches will enable the selection of a PMU revision
from userspace, sample the supported PMU revision at VM creation
time, rather than building each time the ID_AA64DFR0_EL1 register
is accessed.
This shouldn't result in any change in behaviour.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/include/asm/kvm_host.h | 1 +
arch/arm64/kvm/arm.c | 6 ++++++
arch/arm64/kvm/pmu-emul.c | 11 +++++++++++
arch/arm64/kvm/sys_regs.c | 26 +++++++++++++++++++++-----
include/kvm/arm_pmu.h | 6 ++++++
5 files changed, 45 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f38ef299f13b..411114510634 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -163,6 +163,7 @@ struct kvm_arch {
u8 pfr0_csv2;
u8 pfr0_csv3;
+ u8 dfr0_pmuver;
/* Hypercall features firmware registers' descriptor */
struct kvm_smccc_features smccc_feat;
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 8fe73ee5fa84..e4f80f0c1e97 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -164,6 +164,12 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
set_default_spectre(kvm);
kvm_arm_init_hypercalls(kvm);
+ /*
+ * Initialise the default PMUver before there is a chance to
+ * create an actual PMU.
+ */
+ kvm->arch.dfr0_pmuver = kvm_arm_pmu_get_host_pmuver();
+
return ret;
out_free_stage2_pgd:
kvm_free_stage2_pgd(&kvm->arch.mmu);
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index ddd79b64b38a..33a88ca7b7fd 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -1021,3 +1021,14 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
return -ENXIO;
}
+
+u8 kvm_arm_pmu_get_host_pmuver(void)
+{
+ u64 tmp;
+
+ tmp = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
+ tmp = cpuid_feature_cap_perfmon_field(tmp,
+ ID_AA64DFR0_PMUVER_SHIFT,
+ ID_AA64DFR0_PMUVER_8_4);
+ return FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER), tmp);
+}
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 333efddb1e27..55451f49017c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1062,6 +1062,22 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
return true;
}
+static u8 pmuver_to_perfmon(const struct kvm_vcpu *vcpu)
+{
+ if (!kvm_vcpu_has_pmu(vcpu))
+ return 0;
+
+ switch (vcpu->kvm->arch.dfr0_pmuver) {
+ case ID_AA64DFR0_PMUVER_8_0:
+ return ID_DFR0_PERFMON_8_0;
+ case ID_AA64DFR0_PMUVER_IMP_DEF:
+ return 0;
+ default:
+ /* Anything ARMv8.4+ has the same value. For now. */
+ return vcpu->kvm->arch.dfr0_pmuver;
+ }
+}
+
/* Read a sanitised cpufeature ID register by sys_reg_desc */
static u64 read_id_reg(const struct kvm_vcpu *vcpu,
struct sys_reg_desc const *r, bool raz)
@@ -1112,10 +1128,10 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
/* Limit debug to ARMv8.0 */
val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER);
val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER), 6);
- /* 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_4 : 0);
+ /* Set PMUver to the required version */
+ val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER);
+ val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER),
+ kvm_vcpu_has_pmu(vcpu) ? vcpu->kvm->arch.dfr0_pmuver : 0);
/* Hide SPE from guests */
val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_PMSVER);
break;
@@ -1123,7 +1139,7 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
/* 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_4 : 0);
+ pmuver_to_perfmon(vcpu));
break;
}
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 96b192139a23..6bda9b071084 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -89,6 +89,8 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu);
vcpu->arch.pmu.events = *kvm_get_pmu_events(); \
} while (0)
+u8 kvm_arm_pmu_get_host_pmuver(void);
+
#else
struct kvm_pmu {
};
@@ -154,6 +156,10 @@ static inline u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
static inline void kvm_pmu_update_vcpu_events(struct kvm_vcpu *vcpu) {}
static inline void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu) {}
static inline void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu) {}
+static inline u8 kvm_arm_pmu_get_host_pmuver(void)
+{
+ return 0;
+}
#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 6/9] KVM: arm64: PMU: Move the ID_AA64DFR0_EL1.PMUver limit to VM creation
2022-08-05 13:58 ` [PATCH 6/9] KVM: arm64: PMU: Move the ID_AA64DFR0_EL1.PMUver limit to VM creation Marc Zyngier
@ 2022-08-26 4:34 ` Reiji Watanabe
2022-08-26 6:02 ` Reiji Watanabe
0 siblings, 1 reply; 38+ messages in thread
From: Reiji Watanabe @ 2022-08-26 4:34 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Linux ARM, kvmarm, kvm, kernel-team
Hi Marc,
On Fri, Aug 5, 2022 at 6:58 AM Marc Zyngier <maz@kernel.org> wrote:
>
> As further patches will enable the selection of a PMU revision
> from userspace, sample the supported PMU revision at VM creation
> time, rather than building each time the ID_AA64DFR0_EL1 register
> is accessed.
>
> This shouldn't result in any change in behaviour.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/include/asm/kvm_host.h | 1 +
> arch/arm64/kvm/arm.c | 6 ++++++
> arch/arm64/kvm/pmu-emul.c | 11 +++++++++++
> arch/arm64/kvm/sys_regs.c | 26 +++++++++++++++++++++-----
> include/kvm/arm_pmu.h | 6 ++++++
> 5 files changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index f38ef299f13b..411114510634 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -163,6 +163,7 @@ struct kvm_arch {
>
> u8 pfr0_csv2;
> u8 pfr0_csv3;
> + u8 dfr0_pmuver;
>
> /* Hypercall features firmware registers' descriptor */
> struct kvm_smccc_features smccc_feat;
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 8fe73ee5fa84..e4f80f0c1e97 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -164,6 +164,12 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> set_default_spectre(kvm);
> kvm_arm_init_hypercalls(kvm);
>
> + /*
> + * Initialise the default PMUver before there is a chance to
> + * create an actual PMU.
> + */
> + kvm->arch.dfr0_pmuver = kvm_arm_pmu_get_host_pmuver();
> +
> return ret;
> out_free_stage2_pgd:
> kvm_free_stage2_pgd(&kvm->arch.mmu);
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index ddd79b64b38a..33a88ca7b7fd 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -1021,3 +1021,14 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>
> return -ENXIO;
> }
> +
> +u8 kvm_arm_pmu_get_host_pmuver(void)
Nit: Since this function doesn't simply return the host's pmuver, but the
pmuver limit for guests, perhaps "kvm_arm_pmu_get_guest_pmuver_limit"
might be more clear (closer to what it does) ?
> +{
> + u64 tmp;
> +
> + tmp = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> + tmp = cpuid_feature_cap_perfmon_field(tmp,
> + ID_AA64DFR0_PMUVER_SHIFT,
> + ID_AA64DFR0_PMUVER_8_4);
> + return FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER), tmp);
> +}
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 333efddb1e27..55451f49017c 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1062,6 +1062,22 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
> return true;
> }
>
> +static u8 pmuver_to_perfmon(const struct kvm_vcpu *vcpu)
> +{
> + if (!kvm_vcpu_has_pmu(vcpu))
> + return 0;
> +
> + switch (vcpu->kvm->arch.dfr0_pmuver) {
> + case ID_AA64DFR0_PMUVER_8_0:
> + return ID_DFR0_PERFMON_8_0;
> + case ID_AA64DFR0_PMUVER_IMP_DEF:
> + return 0;
> + default:
> + /* Anything ARMv8.4+ has the same value. For now. */
> + return vcpu->kvm->arch.dfr0_pmuver;
> + }
> +}
> +
> /* Read a sanitised cpufeature ID register by sys_reg_desc */
> static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> struct sys_reg_desc const *r, bool raz)
> @@ -1112,10 +1128,10 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> /* Limit debug to ARMv8.0 */
> val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER);
> val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER), 6);
> - /* 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_4 : 0);
> + /* Set PMUver to the required version */
> + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER);
> + val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER),
> + kvm_vcpu_has_pmu(vcpu) ? vcpu->kvm->arch.dfr0_pmuver : 0);
> /* Hide SPE from guests */
> val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_PMSVER);
> break;
> @@ -1123,7 +1139,7 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> /* Limit guests to PMUv3 for ARMv8.4 */
Nit: I think the comment above should be removed like you did for
ID_AA64DFR0_EL1 (or move it to kvm_arm_pmu_get_host_pmuver()?).
Reviewed-by: Reiji Watanabe <reijiw@google.com>
Thank you,
Reiji
> val = cpuid_feature_cap_perfmon_field(val,
> ID_DFR0_PERFMON_SHIFT,
> - kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0);
> + pmuver_to_perfmon(vcpu));
> break;
> }
>
> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> index 96b192139a23..6bda9b071084 100644
> --- a/include/kvm/arm_pmu.h
> +++ b/include/kvm/arm_pmu.h
> @@ -89,6 +89,8 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu);
> vcpu->arch.pmu.events = *kvm_get_pmu_events(); \
> } while (0)
>
> +u8 kvm_arm_pmu_get_host_pmuver(void);
> +
> #else
> struct kvm_pmu {
> };
> @@ -154,6 +156,10 @@ static inline u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
> static inline void kvm_pmu_update_vcpu_events(struct kvm_vcpu *vcpu) {}
> static inline void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu) {}
> static inline void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu) {}
> +static inline u8 kvm_arm_pmu_get_host_pmuver(void)
> +{
> + return 0;
> +}
>
> #endif
>
> --
> 2.34.1
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 6/9] KVM: arm64: PMU: Move the ID_AA64DFR0_EL1.PMUver limit to VM creation
2022-08-26 4:34 ` Reiji Watanabe
@ 2022-08-26 6:02 ` Reiji Watanabe
2022-10-26 14:43 ` Marc Zyngier
0 siblings, 1 reply; 38+ messages in thread
From: Reiji Watanabe @ 2022-08-26 6:02 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Linux ARM, kvmarm, kvm, kernel-team
Hi Marc,
On Thu, Aug 25, 2022 at 9:34 PM Reiji Watanabe <reijiw@google.com> wrote:
>
> Hi Marc,
>
> On Fri, Aug 5, 2022 at 6:58 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > As further patches will enable the selection of a PMU revision
> > from userspace, sample the supported PMU revision at VM creation
> > time, rather than building each time the ID_AA64DFR0_EL1 register
> > is accessed.
> >
> > This shouldn't result in any change in behaviour.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> > arch/arm64/include/asm/kvm_host.h | 1 +
> > arch/arm64/kvm/arm.c | 6 ++++++
> > arch/arm64/kvm/pmu-emul.c | 11 +++++++++++
> > arch/arm64/kvm/sys_regs.c | 26 +++++++++++++++++++++-----
> > include/kvm/arm_pmu.h | 6 ++++++
> > 5 files changed, 45 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > index f38ef299f13b..411114510634 100644
> > --- a/arch/arm64/include/asm/kvm_host.h
> > +++ b/arch/arm64/include/asm/kvm_host.h
> > @@ -163,6 +163,7 @@ struct kvm_arch {
> >
> > u8 pfr0_csv2;
> > u8 pfr0_csv3;
> > + u8 dfr0_pmuver;
> >
> > /* Hypercall features firmware registers' descriptor */
> > struct kvm_smccc_features smccc_feat;
> > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > index 8fe73ee5fa84..e4f80f0c1e97 100644
> > --- a/arch/arm64/kvm/arm.c
> > +++ b/arch/arm64/kvm/arm.c
> > @@ -164,6 +164,12 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> > set_default_spectre(kvm);
> > kvm_arm_init_hypercalls(kvm);
> >
> > + /*
> > + * Initialise the default PMUver before there is a chance to
> > + * create an actual PMU.
> > + */
> > + kvm->arch.dfr0_pmuver = kvm_arm_pmu_get_host_pmuver();
> > +
> > return ret;
> > out_free_stage2_pgd:
> > kvm_free_stage2_pgd(&kvm->arch.mmu);
> > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > index ddd79b64b38a..33a88ca7b7fd 100644
> > --- a/arch/arm64/kvm/pmu-emul.c
> > +++ b/arch/arm64/kvm/pmu-emul.c
> > @@ -1021,3 +1021,14 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
> >
> > return -ENXIO;
> > }
> > +
> > +u8 kvm_arm_pmu_get_host_pmuver(void)
>
> Nit: Since this function doesn't simply return the host's pmuver, but the
> pmuver limit for guests, perhaps "kvm_arm_pmu_get_guest_pmuver_limit"
> might be more clear (closer to what it does) ?
>
> > +{
> > + u64 tmp;
> > +
> > + tmp = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> > + tmp = cpuid_feature_cap_perfmon_field(tmp,
> > + ID_AA64DFR0_PMUVER_SHIFT,
> > + ID_AA64DFR0_PMUVER_8_4);
> > + return FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER), tmp);
> > +}
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 333efddb1e27..55451f49017c 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1062,6 +1062,22 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
> > return true;
> > }
> >
> > +static u8 pmuver_to_perfmon(const struct kvm_vcpu *vcpu)
> > +{
> > + if (!kvm_vcpu_has_pmu(vcpu))
> > + return 0;
> > +
> > + switch (vcpu->kvm->arch.dfr0_pmuver) {
> > + case ID_AA64DFR0_PMUVER_8_0:
> > + return ID_DFR0_PERFMON_8_0;
> > + case ID_AA64DFR0_PMUVER_IMP_DEF:
> > + return 0;
> > + default:
> > + /* Anything ARMv8.4+ has the same value. For now. */
> > + return vcpu->kvm->arch.dfr0_pmuver;
> > + }
> > +}
> > +
> > /* Read a sanitised cpufeature ID register by sys_reg_desc */
> > static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> > struct sys_reg_desc const *r, bool raz)
> > @@ -1112,10 +1128,10 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> > /* Limit debug to ARMv8.0 */
> > val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER);
> > val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER), 6);
> > - /* 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_4 : 0);
> > + /* Set PMUver to the required version */
> > + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER);
> > + val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER),
> > + kvm_vcpu_has_pmu(vcpu) ? vcpu->kvm->arch.dfr0_pmuver : 0);
I've just noticed one issue in this patch while I'm reviewing patch-7.
I would think that this patch makes PMUVER and PERFMON inconsistent
when PMU is not enabled for the vCPU, and the host's sanitised PMUVER
is IMP_DEF.
Previously, when PMU is not enabled for the vCPU and the host's
sanitized value of PMUVER is IMP_DEF(0xf), the vCPU's PMUVER and PERFMON
are set to IMP_DEF due to a bug of cpuid_feature_cap_perfmon_field().
(https://lore.kernel.org/all/20220214065746.1230608-11-reijiw@google.com/)
With this patch, the vCPU's PMUVER will be 0 for the same case,
while the vCPU's PERFMON will stay the same (IMP_DEF).
I guess you unintentionally corrected only the PMUVER value of the VCPU.
Thank you,
Reiji
> > /* Hide SPE from guests */
> > val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_PMSVER);
> > break;
> > @@ -1123,7 +1139,7 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> > /* Limit guests to PMUv3 for ARMv8.4 */
>
> Nit: I think the comment above should be removed like you did for
> ID_AA64DFR0_EL1 (or move it to kvm_arm_pmu_get_host_pmuver()?).
>
> Reviewed-by: Reiji Watanabe <reijiw@google.com>
>
> Thank you,
> Reiji
>
>
>
> > val = cpuid_feature_cap_perfmon_field(val,
> > ID_DFR0_PERFMON_SHIFT,
> > - kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0);
> > + pmuver_to_perfmon(vcpu));
> > break;
> > }
> >
> > diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> > index 96b192139a23..6bda9b071084 100644
> > --- a/include/kvm/arm_pmu.h
> > +++ b/include/kvm/arm_pmu.h
> > @@ -89,6 +89,8 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu);
> > vcpu->arch.pmu.events = *kvm_get_pmu_events(); \
> > } while (0)
> >
> > +u8 kvm_arm_pmu_get_host_pmuver(void);
> > +
> > #else
> > struct kvm_pmu {
> > };
> > @@ -154,6 +156,10 @@ static inline u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
> > static inline void kvm_pmu_update_vcpu_events(struct kvm_vcpu *vcpu) {}
> > static inline void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu) {}
> > static inline void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu) {}
> > +static inline u8 kvm_arm_pmu_get_host_pmuver(void)
> > +{
> > + return 0;
> > +}
> >
> > #endif
> >
> > --
> > 2.34.1
> >
> > _______________________________________________
> > kvmarm mailing list
> > kvmarm@lists.cs.columbia.edu
> > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 6/9] KVM: arm64: PMU: Move the ID_AA64DFR0_EL1.PMUver limit to VM creation
2022-08-26 6:02 ` Reiji Watanabe
@ 2022-10-26 14:43 ` Marc Zyngier
2022-10-27 16:09 ` Reiji Watanabe
0 siblings, 1 reply; 38+ messages in thread
From: Marc Zyngier @ 2022-10-26 14:43 UTC (permalink / raw)
To: Reiji Watanabe; +Cc: Linux ARM, kvmarm, kvm, kernel-team
Hi Reiji,
Sorry it took so long to get back to this.
On Fri, 26 Aug 2022 07:02:21 +0100,
Reiji Watanabe <reijiw@google.com> wrote:
>
> Hi Marc,
>
> On Thu, Aug 25, 2022 at 9:34 PM Reiji Watanabe <reijiw@google.com> wrote:
> >
> > Hi Marc,
> >
> > On Fri, Aug 5, 2022 at 6:58 AM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > As further patches will enable the selection of a PMU revision
> > > from userspace, sample the supported PMU revision at VM creation
> > > time, rather than building each time the ID_AA64DFR0_EL1 register
> > > is accessed.
> > >
> > > This shouldn't result in any change in behaviour.
> > >
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > ---
> > > arch/arm64/include/asm/kvm_host.h | 1 +
> > > arch/arm64/kvm/arm.c | 6 ++++++
> > > arch/arm64/kvm/pmu-emul.c | 11 +++++++++++
> > > arch/arm64/kvm/sys_regs.c | 26 +++++++++++++++++++++-----
> > > include/kvm/arm_pmu.h | 6 ++++++
> > > 5 files changed, 45 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > index f38ef299f13b..411114510634 100644
> > > --- a/arch/arm64/include/asm/kvm_host.h
> > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > @@ -163,6 +163,7 @@ struct kvm_arch {
> > >
> > > u8 pfr0_csv2;
> > > u8 pfr0_csv3;
> > > + u8 dfr0_pmuver;
> > >
> > > /* Hypercall features firmware registers' descriptor */
> > > struct kvm_smccc_features smccc_feat;
> > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > > index 8fe73ee5fa84..e4f80f0c1e97 100644
> > > --- a/arch/arm64/kvm/arm.c
> > > +++ b/arch/arm64/kvm/arm.c
> > > @@ -164,6 +164,12 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> > > set_default_spectre(kvm);
> > > kvm_arm_init_hypercalls(kvm);
> > >
> > > + /*
> > > + * Initialise the default PMUver before there is a chance to
> > > + * create an actual PMU.
> > > + */
> > > + kvm->arch.dfr0_pmuver = kvm_arm_pmu_get_host_pmuver();
> > > +
> > > return ret;
> > > out_free_stage2_pgd:
> > > kvm_free_stage2_pgd(&kvm->arch.mmu);
> > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > > index ddd79b64b38a..33a88ca7b7fd 100644
> > > --- a/arch/arm64/kvm/pmu-emul.c
> > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > @@ -1021,3 +1021,14 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
> > >
> > > return -ENXIO;
> > > }
> > > +
> > > +u8 kvm_arm_pmu_get_host_pmuver(void)
> >
> > Nit: Since this function doesn't simply return the host's pmuver, but the
> > pmuver limit for guests, perhaps "kvm_arm_pmu_get_guest_pmuver_limit"
> > might be more clear (closer to what it does) ?
Maybe a bit verbose, but I'll work something out.
> >
> > > +{
> > > + u64 tmp;
> > > +
> > > + tmp = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> > > + tmp = cpuid_feature_cap_perfmon_field(tmp,
> > > + ID_AA64DFR0_PMUVER_SHIFT,
> > > + ID_AA64DFR0_PMUVER_8_4);
> > > + return FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER), tmp);
> > > +}
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index 333efddb1e27..55451f49017c 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -1062,6 +1062,22 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
> > > return true;
> > > }
> > >
> > > +static u8 pmuver_to_perfmon(const struct kvm_vcpu *vcpu)
> > > +{
> > > + if (!kvm_vcpu_has_pmu(vcpu))
> > > + return 0;
> > > +
> > > + switch (vcpu->kvm->arch.dfr0_pmuver) {
> > > + case ID_AA64DFR0_PMUVER_8_0:
> > > + return ID_DFR0_PERFMON_8_0;
> > > + case ID_AA64DFR0_PMUVER_IMP_DEF:
> > > + return 0;
> > > + default:
> > > + /* Anything ARMv8.4+ has the same value. For now. */
> > > + return vcpu->kvm->arch.dfr0_pmuver;
> > > + }
> > > +}
> > > +
> > > /* Read a sanitised cpufeature ID register by sys_reg_desc */
> > > static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> > > struct sys_reg_desc const *r, bool raz)
> > > @@ -1112,10 +1128,10 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> > > /* Limit debug to ARMv8.0 */
> > > val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER);
> > > val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER), 6);
> > > - /* 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_4 : 0);
> > > + /* Set PMUver to the required version */
> > > + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER);
> > > + val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER),
> > > + kvm_vcpu_has_pmu(vcpu) ? vcpu->kvm->arch.dfr0_pmuver : 0);
>
> I've just noticed one issue in this patch while I'm reviewing patch-7.
>
> I would think that this patch makes PMUVER and PERFMON inconsistent
> when PMU is not enabled for the vCPU, and the host's sanitised PMUVER
> is IMP_DEF.
>
> Previously, when PMU is not enabled for the vCPU and the host's
> sanitized value of PMUVER is IMP_DEF(0xf), the vCPU's PMUVER and PERFMON
> are set to IMP_DEF due to a bug of cpuid_feature_cap_perfmon_field().
> (https://lore.kernel.org/all/20220214065746.1230608-11-reijiw@google.com/)
>
> With this patch, the vCPU's PMUVER will be 0 for the same case,
> while the vCPU's PERFMON will stay the same (IMP_DEF).
> I guess you unintentionally corrected only the PMUVER value of the VCPU.
I think that with this patch both PMUVer and Perfmon values get set to
0 (pmuver_to_perfmon() returns 0 for both ID_AA64DFR0_PMUVER_IMP_DEF
and no PMU at all). Am I missing anything here?
However, you definitely have a point that we should handle a guest
being restored with an IMPDEF PMU. Which means I need to revisit this
patch and the userspace accessors. Oh well...
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 6/9] KVM: arm64: PMU: Move the ID_AA64DFR0_EL1.PMUver limit to VM creation
2022-10-26 14:43 ` Marc Zyngier
@ 2022-10-27 16:09 ` Reiji Watanabe
2022-10-27 17:24 ` Marc Zyngier
0 siblings, 1 reply; 38+ messages in thread
From: Reiji Watanabe @ 2022-10-27 16:09 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Linux ARM, kvmarm, kvm, kernel-team
Hi Marc,
> Sorry it took so long to get back to this.
No problem!
>
> On Fri, 26 Aug 2022 07:02:21 +0100,
> Reiji Watanabe <reijiw@google.com> wrote:
> >
> > Hi Marc,
> >
> > On Thu, Aug 25, 2022 at 9:34 PM Reiji Watanabe <reijiw@google.com> wrote:
> > >
> > > Hi Marc,
> > >
> > > On Fri, Aug 5, 2022 at 6:58 AM Marc Zyngier <maz@kernel.org> wrote:
> > > >
> > > > As further patches will enable the selection of a PMU revision
> > > > from userspace, sample the supported PMU revision at VM creation
> > > > time, rather than building each time the ID_AA64DFR0_EL1 register
> > > > is accessed.
> > > >
> > > > This shouldn't result in any change in behaviour.
> > > >
> > > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > > ---
> > > > arch/arm64/include/asm/kvm_host.h | 1 +
> > > > arch/arm64/kvm/arm.c | 6 ++++++
> > > > arch/arm64/kvm/pmu-emul.c | 11 +++++++++++
> > > > arch/arm64/kvm/sys_regs.c | 26 +++++++++++++++++++++-----
> > > > include/kvm/arm_pmu.h | 6 ++++++
> > > > 5 files changed, 45 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> > > > index f38ef299f13b..411114510634 100644
> > > > --- a/arch/arm64/include/asm/kvm_host.h
> > > > +++ b/arch/arm64/include/asm/kvm_host.h
> > > > @@ -163,6 +163,7 @@ struct kvm_arch {
> > > >
> > > > u8 pfr0_csv2;
> > > > u8 pfr0_csv3;
> > > > + u8 dfr0_pmuver;
> > > >
> > > > /* Hypercall features firmware registers' descriptor */
> > > > struct kvm_smccc_features smccc_feat;
> > > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> > > > index 8fe73ee5fa84..e4f80f0c1e97 100644
> > > > --- a/arch/arm64/kvm/arm.c
> > > > +++ b/arch/arm64/kvm/arm.c
> > > > @@ -164,6 +164,12 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
> > > > set_default_spectre(kvm);
> > > > kvm_arm_init_hypercalls(kvm);
> > > >
> > > > + /*
> > > > + * Initialise the default PMUver before there is a chance to
> > > > + * create an actual PMU.
> > > > + */
> > > > + kvm->arch.dfr0_pmuver = kvm_arm_pmu_get_host_pmuver();
> > > > +
> > > > return ret;
> > > > out_free_stage2_pgd:
> > > > kvm_free_stage2_pgd(&kvm->arch.mmu);
> > > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > > > index ddd79b64b38a..33a88ca7b7fd 100644
> > > > --- a/arch/arm64/kvm/pmu-emul.c
> > > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > > @@ -1021,3 +1021,14 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
> > > >
> > > > return -ENXIO;
> > > > }
> > > > +
> > > > +u8 kvm_arm_pmu_get_host_pmuver(void)
> > >
> > > Nit: Since this function doesn't simply return the host's pmuver, but the
> > > pmuver limit for guests, perhaps "kvm_arm_pmu_get_guest_pmuver_limit"
> > > might be more clear (closer to what it does) ?
>
> Maybe a bit verbose, but I'll work something out.
>
> > >
> > > > +{
> > > > + u64 tmp;
> > > > +
> > > > + tmp = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> > > > + tmp = cpuid_feature_cap_perfmon_field(tmp,
> > > > + ID_AA64DFR0_PMUVER_SHIFT,
> > > > + ID_AA64DFR0_PMUVER_8_4);
> > > > + return FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER), tmp);
> > > > +}
> > > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > > index 333efddb1e27..55451f49017c 100644
> > > > --- a/arch/arm64/kvm/sys_regs.c
> > > > +++ b/arch/arm64/kvm/sys_regs.c
> > > > @@ -1062,6 +1062,22 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
> > > > return true;
> > > > }
> > > >
> > > > +static u8 pmuver_to_perfmon(const struct kvm_vcpu *vcpu)
> > > > +{
> > > > + if (!kvm_vcpu_has_pmu(vcpu))
> > > > + return 0;
> > > > +
> > > > + switch (vcpu->kvm->arch.dfr0_pmuver) {
> > > > + case ID_AA64DFR0_PMUVER_8_0:
> > > > + return ID_DFR0_PERFMON_8_0;
> > > > + case ID_AA64DFR0_PMUVER_IMP_DEF:
> > > > + return 0;
> > > > + default:
> > > > + /* Anything ARMv8.4+ has the same value. For now. */
> > > > + return vcpu->kvm->arch.dfr0_pmuver;
> > > > + }
> > > > +}
> > > > +
> > > > /* Read a sanitised cpufeature ID register by sys_reg_desc */
> > > > static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> > > > struct sys_reg_desc const *r, bool raz)
> > > > @@ -1112,10 +1128,10 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> > > > /* Limit debug to ARMv8.0 */
> > > > val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER);
> > > > val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_DEBUGVER), 6);
> > > > - /* 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_4 : 0);
> > > > + /* Set PMUver to the required version */
> > > > + val &= ~ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER);
> > > > + val |= FIELD_PREP(ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER),
> > > > + kvm_vcpu_has_pmu(vcpu) ? vcpu->kvm->arch.dfr0_pmuver : 0);
> >
> > I've just noticed one issue in this patch while I'm reviewing patch-7.
> >
> > I would think that this patch makes PMUVER and PERFMON inconsistent
> > when PMU is not enabled for the vCPU, and the host's sanitised PMUVER
> > is IMP_DEF.
> >
> > Previously, when PMU is not enabled for the vCPU and the host's
> > sanitized value of PMUVER is IMP_DEF(0xf), the vCPU's PMUVER and PERFMON
> > are set to IMP_DEF due to a bug of cpuid_feature_cap_perfmon_field().
> > (https://lore.kernel.org/all/20220214065746.1230608-11-reijiw@google.com/)
> >
> > With this patch, the vCPU's PMUVER will be 0 for the same case,
> > while the vCPU's PERFMON will stay the same (IMP_DEF).
> > I guess you unintentionally corrected only the PMUVER value of the VCPU.
>
> I think that with this patch both PMUVer and Perfmon values get set to
> 0 (pmuver_to_perfmon() returns 0 for both ID_AA64DFR0_PMUVER_IMP_DEF
> and no PMU at all). Am I missing anything here?
> > With this patch, the vCPU's PMUVER will be 0 for the same case,
> > while the vCPU's PERFMON will stay the same (IMP_DEF).
> > I guess you unintentionally corrected only the PMUVER value of the VCPU.
>
> I think that with this patch both PMUVer and Perfmon values get set to
> 0 (pmuver_to_perfmon() returns 0 for both ID_AA64DFR0_PMUVER_IMP_DEF
> and no PMU at all). Am I missing anything here?
When pmuver_to_perfmon() returns 0 for ID_AA64DFR0_PMUVER_IMP_DEF,
cpuid_feature_cap_perfmon_field() is called with 'cap' == 0. Then,
the code in cpuid_feature_cap_perfmon_field() updates the 'val' with 0
if the given 'features' (sanitized) value is ID_AA64DFR0_PMUVER_IMP_DEF.
So, now the val(== 0) is not larger than the cap (== 0), and
cpuid_feature_cap_perfmon_field() ends up returning the given 'features'
value as it is without updating the PERFMON field.
Or am I missing anything here?
Thank you,
Reiji
>
> However, you definitely have a point that we should handle a guest
> being restored with an IMPDEF PMU. Which means I need to revisit this
> patch and the userspace accessors. Oh well...
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 6/9] KVM: arm64: PMU: Move the ID_AA64DFR0_EL1.PMUver limit to VM creation
2022-10-27 16:09 ` Reiji Watanabe
@ 2022-10-27 17:24 ` Marc Zyngier
0 siblings, 0 replies; 38+ messages in thread
From: Marc Zyngier @ 2022-10-27 17:24 UTC (permalink / raw)
To: Reiji Watanabe; +Cc: Linux ARM, kvmarm, kvm, kernel-team
On 2022-10-27 17:09, Reiji Watanabe wrote:
>> I think that with this patch both PMUVer and Perfmon values get set to
>> 0 (pmuver_to_perfmon() returns 0 for both ID_AA64DFR0_PMUVER_IMP_DEF
>> and no PMU at all). Am I missing anything here?
>
> When pmuver_to_perfmon() returns 0 for ID_AA64DFR0_PMUVER_IMP_DEF,
> cpuid_feature_cap_perfmon_field() is called with 'cap' == 0. Then,
> the code in cpuid_feature_cap_perfmon_field() updates the 'val' with 0
> if the given 'features' (sanitized) value is
> ID_AA64DFR0_PMUVER_IMP_DEF.
> So, now the val(== 0) is not larger than the cap (== 0), and
> cpuid_feature_cap_perfmon_field() ends up returning the given
> 'features'
> value as it is without updating the PERFMON field.
Ah, thanks for spelling it out for me, I was definitely looking
at the wrong side of things. You're absolutely right. The code
I have now makes sure to:
(1) preserve the IMP_DEF view of the PMU if userspace provides
such setting
(2) directly places the emulated PMU revision in the feature
set without calling cpuid_feature_cap_perfmon_field(),
which indeed does the wrong thing.
Hopefully I got it right this time! ;-)
Thanks again,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 7/9] KVM: arm64: PMU: Allow ID_AA64DFR0_EL1.PMUver to be set from userspace
2022-08-05 13:58 [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support Marc Zyngier
` (5 preceding siblings ...)
2022-08-05 13:58 ` [PATCH 6/9] KVM: arm64: PMU: Move the ID_AA64DFR0_EL1.PMUver limit to VM creation Marc Zyngier
@ 2022-08-05 13:58 ` Marc Zyngier
2022-08-10 7:08 ` Oliver Upton
2022-08-26 7:01 ` Reiji Watanabe
2022-08-05 13:58 ` [PATCH 8/9] KVM: arm64: PMU: Implement PMUv3p5 long counter support Marc Zyngier
` (2 subsequent siblings)
9 siblings, 2 replies; 38+ messages in thread
From: Marc Zyngier @ 2022-08-05 13:58 UTC (permalink / raw)
To: linux-arm-kernel, kvmarm, kvm
Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Oliver Upton,
Ricardo Koller, kernel-team
Allow userspace to write ID_AA64DFR0_EL1, on the condition that only
the PMUver field can be altered and be at most the one that was
initially computed for the guest.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/kvm/sys_regs.c | 35 ++++++++++++++++++++++++++++++++++-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 55451f49017c..c0595f31dab8 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1236,6 +1236,38 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
return 0;
}
+static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
+ const struct sys_reg_desc *rd,
+ u64 val)
+{
+ u8 pmuver, host_pmuver;
+
+ host_pmuver = kvm_arm_pmu_get_host_pmuver();
+
+ /*
+ * Allow AA64DFR0_EL1.PMUver to be set from userspace as long
+ * as it doesn't promise more than what the HW gives us. We
+ * don't allow an IMPDEF PMU though.
+ */
+ pmuver = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER), val);
+ if (pmuver == ID_AA64DFR0_PMUVER_IMP_DEF || pmuver > host_pmuver)
+ return -EINVAL;
+
+ /* We already have a PMU, don't try to disable it... */
+ if (kvm_vcpu_has_pmu(vcpu) && pmuver == 0)
+ return -EINVAL;
+
+ /* We can only differ with PMUver, and anything else is an error */
+ val ^= read_id_reg(vcpu, rd, false);
+ val &= ~(0xFUL << ID_AA64DFR0_PMUVER_SHIFT);
+ if (val)
+ return -EINVAL;
+
+ vcpu->kvm->arch.dfr0_pmuver = pmuver;
+
+ return 0;
+}
+
/*
* cpufeature ID register user accessors
*
@@ -1510,7 +1542,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
ID_UNALLOCATED(4,7),
/* CRm=5 */
- ID_SANITISED(ID_AA64DFR0_EL1),
+ { SYS_DESC(SYS_ID_AA64DFR0_EL1), .access = access_id_reg,
+ .get_user = get_id_reg, .set_user = set_id_aa64dfr0_el1, },
ID_SANITISED(ID_AA64DFR1_EL1),
ID_UNALLOCATED(5,2),
ID_UNALLOCATED(5,3),
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 7/9] KVM: arm64: PMU: Allow ID_AA64DFR0_EL1.PMUver to be set from userspace
2022-08-05 13:58 ` [PATCH 7/9] KVM: arm64: PMU: Allow ID_AA64DFR0_EL1.PMUver to be set from userspace Marc Zyngier
@ 2022-08-10 7:08 ` Oliver Upton
2022-08-10 9:27 ` Marc Zyngier
2022-08-26 7:01 ` Reiji Watanabe
1 sibling, 1 reply; 38+ messages in thread
From: Oliver Upton @ 2022-08-10 7:08 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, kvmarm, kvm, James Morse, Suzuki K Poulose,
Alexandru Elisei, Ricardo Koller, kernel-team
Hi Marc,
On Fri, Aug 05, 2022 at 02:58:11PM +0100, Marc Zyngier wrote:
> Allow userspace to write ID_AA64DFR0_EL1, on the condition that only
> the PMUver field can be altered and be at most the one that was
> initially computed for the guest.
As DFR0_EL1 is exposed to userspace, isn't a ->set_user() hook required
for it as well?
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/kvm/sys_regs.c | 35 ++++++++++++++++++++++++++++++++++-
> 1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 55451f49017c..c0595f31dab8 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1236,6 +1236,38 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> return 0;
> }
>
> +static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *rd,
> + u64 val)
> +{
> + u8 pmuver, host_pmuver;
> +
> + host_pmuver = kvm_arm_pmu_get_host_pmuver();
> +
> + /*
> + * Allow AA64DFR0_EL1.PMUver to be set from userspace as long
> + * as it doesn't promise more than what the HW gives us. We
> + * don't allow an IMPDEF PMU though.
> + */
> + pmuver = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER), val);
> + if (pmuver == ID_AA64DFR0_PMUVER_IMP_DEF || pmuver > host_pmuver)
> + return -EINVAL;
> +
> + /* We already have a PMU, don't try to disable it... */
> + if (kvm_vcpu_has_pmu(vcpu) && pmuver == 0)
> + return -EINVAL;
> +
> + /* We can only differ with PMUver, and anything else is an error */
> + val ^= read_id_reg(vcpu, rd, false);
> + val &= ~(0xFUL << ID_AA64DFR0_PMUVER_SHIFT);
nit: ~ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER)
--
Thanks,
Oliver
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 7/9] KVM: arm64: PMU: Allow ID_AA64DFR0_EL1.PMUver to be set from userspace
2022-08-10 7:08 ` Oliver Upton
@ 2022-08-10 9:27 ` Marc Zyngier
0 siblings, 0 replies; 38+ messages in thread
From: Marc Zyngier @ 2022-08-10 9:27 UTC (permalink / raw)
To: Oliver Upton
Cc: linux-arm-kernel, kvmarm, kvm, James Morse, Suzuki K Poulose,
Alexandru Elisei, Ricardo Koller, kernel-team
On Wed, 10 Aug 2022 08:08:06 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
>
> Hi Marc,
>
> On Fri, Aug 05, 2022 at 02:58:11PM +0100, Marc Zyngier wrote:
> > Allow userspace to write ID_AA64DFR0_EL1, on the condition that only
> > the PMUver field can be altered and be at most the one that was
> > initially computed for the guest.
>
> As DFR0_EL1 is exposed to userspace, isn't a ->set_user() hook required
> for it as well?
Hmm. Yes, absolutely. Which is really annoying. It also pushed me to
have a look at what PMUv3p5 means for AArch32, and it is utter
nonsense...
Here's what the spec says about PMEVCNTR<n>:
<quote>
If FEAT_PMUv3p5 is implemented, the event counter is 64 bits and only
the least-significant part of the event counter is accessible in
AArch32 state:
- Reads from PMEVCNTR<n> return bits [31:0] of the counter.
- Writes to PMEVCNTR<n> update bits [31:0] and leave bits [63:32] unchanged.
- There is no means to access bits [63:32] directly from AArch32 state
</quote>
But PMCR.LP does exist! You just can't make any reasonable use of it.
Yet another reason to want AArch32 dead.
>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> > arch/arm64/kvm/sys_regs.c | 35 ++++++++++++++++++++++++++++++++++-
> > 1 file changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 55451f49017c..c0595f31dab8 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1236,6 +1236,38 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> > return 0;
> > }
> >
> > +static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> > + const struct sys_reg_desc *rd,
> > + u64 val)
> > +{
> > + u8 pmuver, host_pmuver;
> > +
> > + host_pmuver = kvm_arm_pmu_get_host_pmuver();
> > +
> > + /*
> > + * Allow AA64DFR0_EL1.PMUver to be set from userspace as long
> > + * as it doesn't promise more than what the HW gives us. We
> > + * don't allow an IMPDEF PMU though.
> > + */
> > + pmuver = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER), val);
> > + if (pmuver == ID_AA64DFR0_PMUVER_IMP_DEF || pmuver > host_pmuver)
> > + return -EINVAL;
> > +
> > + /* We already have a PMU, don't try to disable it... */
> > + if (kvm_vcpu_has_pmu(vcpu) && pmuver == 0)
> > + return -EINVAL;
> > +
> > + /* We can only differ with PMUver, and anything else is an error */
> > + val ^= read_id_reg(vcpu, rd, false);
> > + val &= ~(0xFUL << ID_AA64DFR0_PMUVER_SHIFT);
>
> nit: ~ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER)
Good point.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 7/9] KVM: arm64: PMU: Allow ID_AA64DFR0_EL1.PMUver to be set from userspace
2022-08-05 13:58 ` [PATCH 7/9] KVM: arm64: PMU: Allow ID_AA64DFR0_EL1.PMUver to be set from userspace Marc Zyngier
2022-08-10 7:08 ` Oliver Upton
@ 2022-08-26 7:01 ` Reiji Watanabe
1 sibling, 0 replies; 38+ messages in thread
From: Reiji Watanabe @ 2022-08-26 7:01 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Linux ARM, kvmarm, kvm, kernel-team
Hi Marc,
On Fri, Aug 5, 2022 at 6:58 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Allow userspace to write ID_AA64DFR0_EL1, on the condition that only
> the PMUver field can be altered and be at most the one that was
> initially computed for the guest.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/kvm/sys_regs.c | 35 ++++++++++++++++++++++++++++++++++-
> 1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 55451f49017c..c0595f31dab8 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1236,6 +1236,38 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
> return 0;
> }
>
> +static int set_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> + const struct sys_reg_desc *rd,
> + u64 val)
The function prototype doesn't appear to be right as the
set_user of sys_reg_desc().
---
[From sys_regs.h]
[sys_regs.h]
int (*set_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
const struct kvm_one_reg *reg, void __user *uaddr);
---
> +{
> + u8 pmuver, host_pmuver;
> +
> + host_pmuver = kvm_arm_pmu_get_host_pmuver();
> +
> + /*
> + * Allow AA64DFR0_EL1.PMUver to be set from userspace as long
> + * as it doesn't promise more than what the HW gives us. We
> + * don't allow an IMPDEF PMU though.
> + */
> + pmuver = FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER), val);
> + if (pmuver == ID_AA64DFR0_PMUVER_IMP_DEF || pmuver > host_pmuver)
> + return -EINVAL;
As mentioned in my comments for the patch-6, the vCPU's PMUVER could
currently be IMP_DEF. So, with this IMP_DEF checking, a guest with
IMP_DEF PMU cannot be migrated to a newer KVM host.
Do we need to tolerate writes of IMP_DEF for compatibility ?
Oliver originally point this out for my ID register series, and
my V6 or newer series tried to not return an error for this by
ignoring the user requested IMP_DEF when PMU is not enabled for
the vCPU (Instead, the field is set to 0x0).
https://lore.kernel.org/all/20220419065544.3616948-16-reijiw@google.com/
Thank you,
Reiji
> +
> + /* We already have a PMU, don't try to disable it... */
> + if (kvm_vcpu_has_pmu(vcpu) && pmuver == 0)
> + return -EINVAL;
> +
> + /* We can only differ with PMUver, and anything else is an error */
> + val ^= read_id_reg(vcpu, rd, false);
> + val &= ~(0xFUL << ID_AA64DFR0_PMUVER_SHIFT);
> + if (val)
> + return -EINVAL;
> +
> + vcpu->kvm->arch.dfr0_pmuver = pmuver;
> +
> + return 0;
> +}
> +
> /*
> * cpufeature ID register user accessors
> *
> @@ -1510,7 +1542,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> ID_UNALLOCATED(4,7),
>
> /* CRm=5 */
> - ID_SANITISED(ID_AA64DFR0_EL1),
> + { SYS_DESC(SYS_ID_AA64DFR0_EL1), .access = access_id_reg,
> + .get_user = get_id_reg, .set_user = set_id_aa64dfr0_el1, },
> ID_SANITISED(ID_AA64DFR1_EL1),
> ID_UNALLOCATED(5,2),
> ID_UNALLOCATED(5,3),
> --
> 2.34.1
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 8/9] KVM: arm64: PMU: Implement PMUv3p5 long counter support
2022-08-05 13:58 [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support Marc Zyngier
` (6 preceding siblings ...)
2022-08-05 13:58 ` [PATCH 7/9] KVM: arm64: PMU: Allow ID_AA64DFR0_EL1.PMUver to be set from userspace Marc Zyngier
@ 2022-08-05 13:58 ` Marc Zyngier
2022-08-10 7:16 ` Oliver Upton
2022-08-05 13:58 ` [PATCH 9/9] KVM: arm64: PMU: Allow PMUv3p5 to be exposed to the guest Marc Zyngier
2022-08-10 18:46 ` [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support Ricardo Koller
9 siblings, 1 reply; 38+ messages in thread
From: Marc Zyngier @ 2022-08-05 13:58 UTC (permalink / raw)
To: linux-arm-kernel, kvmarm, kvm
Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Oliver Upton,
Ricardo Koller, kernel-team
PMUv3p5 (which is mandatory with ARMv8.5) comes with some extra
features:
- All counters are 64bit
- The overflow point is controlled by the PMCR_EL0.LP bit
Add the required checks in the helpers that control counter
width and overflow, as well as the sysreg handling for the LP
bit. A new kvm_pmu_is_3p5() helper makes it easy to spot the
PMUv3p5 specific handling.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/kvm/pmu-emul.c | 8 +++++---
arch/arm64/kvm/sys_regs.c | 4 ++++
include/kvm/arm_pmu.h | 8 ++++++++
3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 33a88ca7b7fd..b33a2953cbf6 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -50,13 +50,15 @@ static u32 kvm_pmu_event_mask(struct kvm *kvm)
*/
static bool kvm_pmu_idx_is_64bit(struct kvm_vcpu *vcpu, u64 select_idx)
{
- return (select_idx == ARMV8_PMU_CYCLE_IDX);
+ return (select_idx == ARMV8_PMU_CYCLE_IDX || kvm_pmu_is_3p5(vcpu));
}
static bool kvm_pmu_idx_has_64bit_overflow(struct kvm_vcpu *vcpu, u64 select_idx)
{
- return (select_idx == ARMV8_PMU_CYCLE_IDX &&
- __vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_LC);
+ u64 val = __vcpu_sys_reg(vcpu, PMCR_EL0);
+
+ return (select_idx < ARMV8_PMU_CYCLE_IDX && (val & ARMV8_PMU_PMCR_LP)) ||
+ (select_idx == ARMV8_PMU_CYCLE_IDX && (val & ARMV8_PMU_PMCR_LC));
}
static bool kvm_pmu_counter_can_chain(struct kvm_vcpu *vcpu, u64 idx)
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index c0595f31dab8..2b5e0ec5c100 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -654,6 +654,8 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
| (ARMV8_PMU_PMCR_MASK & 0xdecafbad)) & (~ARMV8_PMU_PMCR_E);
if (!system_supports_32bit_el0())
val |= ARMV8_PMU_PMCR_LC;
+ if (!kvm_pmu_is_3p5(vcpu))
+ val &= ~ARMV8_PMU_PMCR_LP;
__vcpu_sys_reg(vcpu, r->reg) = val;
}
@@ -703,6 +705,8 @@ static bool access_pmcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
val |= p->regval & ARMV8_PMU_PMCR_MASK;
if (!system_supports_32bit_el0())
val |= ARMV8_PMU_PMCR_LC;
+ if (!kvm_pmu_is_3p5(vcpu))
+ val &= ~ARMV8_PMU_PMCR_LP;
__vcpu_sys_reg(vcpu, PMCR_EL0) = val;
kvm_pmu_handle_pmcr(vcpu, val);
kvm_vcpu_pmu_restore_guest(vcpu);
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 6bda9b071084..846502251923 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -89,6 +89,13 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu);
vcpu->arch.pmu.events = *kvm_get_pmu_events(); \
} while (0)
+/*
+ * Evaluates as true when emulating PMUv3p5, and false otherwise.
+ */
+#define kvm_pmu_is_3p5(vcpu) \
+ (vcpu->kvm->arch.dfr0_pmuver >= ID_AA64DFR0_PMUVER_8_5 && \
+ vcpu->kvm->arch.dfr0_pmuver != ID_AA64DFR0_PMUVER_IMP_DEF)
+
u8 kvm_arm_pmu_get_host_pmuver(void);
#else
@@ -153,6 +160,7 @@ static inline u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
}
#define kvm_vcpu_has_pmu(vcpu) ({ false; })
+#define kvm_pmu_is_3p5(vcpu) ({ false; })
static inline void kvm_pmu_update_vcpu_events(struct kvm_vcpu *vcpu) {}
static inline void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu) {}
static inline void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu) {}
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 8/9] KVM: arm64: PMU: Implement PMUv3p5 long counter support
2022-08-05 13:58 ` [PATCH 8/9] KVM: arm64: PMU: Implement PMUv3p5 long counter support Marc Zyngier
@ 2022-08-10 7:16 ` Oliver Upton
2022-08-10 9:28 ` Marc Zyngier
0 siblings, 1 reply; 38+ messages in thread
From: Oliver Upton @ 2022-08-10 7:16 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, kvmarm, kvm, James Morse, Suzuki K Poulose,
Alexandru Elisei, Ricardo Koller, kernel-team
Hi Marc,
On Fri, Aug 05, 2022 at 02:58:12PM +0100, Marc Zyngier wrote:
> PMUv3p5 (which is mandatory with ARMv8.5) comes with some extra
> features:
>
> - All counters are 64bit
>
> - The overflow point is controlled by the PMCR_EL0.LP bit
>
> Add the required checks in the helpers that control counter
> width and overflow, as well as the sysreg handling for the LP
> bit. A new kvm_pmu_is_3p5() helper makes it easy to spot the
> PMUv3p5 specific handling.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/kvm/pmu-emul.c | 8 +++++---
> arch/arm64/kvm/sys_regs.c | 4 ++++
> include/kvm/arm_pmu.h | 8 ++++++++
> 3 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 33a88ca7b7fd..b33a2953cbf6 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -50,13 +50,15 @@ static u32 kvm_pmu_event_mask(struct kvm *kvm)
> */
> static bool kvm_pmu_idx_is_64bit(struct kvm_vcpu *vcpu, u64 select_idx)
> {
> - return (select_idx == ARMV8_PMU_CYCLE_IDX);
> + return (select_idx == ARMV8_PMU_CYCLE_IDX || kvm_pmu_is_3p5(vcpu));
> }
>
> static bool kvm_pmu_idx_has_64bit_overflow(struct kvm_vcpu *vcpu, u64 select_idx)
> {
> - return (select_idx == ARMV8_PMU_CYCLE_IDX &&
> - __vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_LC);
> + u64 val = __vcpu_sys_reg(vcpu, PMCR_EL0);
> +
> + return (select_idx < ARMV8_PMU_CYCLE_IDX && (val & ARMV8_PMU_PMCR_LP)) ||
> + (select_idx == ARMV8_PMU_CYCLE_IDX && (val & ARMV8_PMU_PMCR_LC));
> }
>
> static bool kvm_pmu_counter_can_chain(struct kvm_vcpu *vcpu, u64 idx)
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index c0595f31dab8..2b5e0ec5c100 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -654,6 +654,8 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> | (ARMV8_PMU_PMCR_MASK & 0xdecafbad)) & (~ARMV8_PMU_PMCR_E);
> if (!system_supports_32bit_el0())
> val |= ARMV8_PMU_PMCR_LC;
> + if (!kvm_pmu_is_3p5(vcpu))
> + val &= ~ARMV8_PMU_PMCR_LP;
> __vcpu_sys_reg(vcpu, r->reg) = val;
> }
>
> @@ -703,6 +705,8 @@ static bool access_pmcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> val |= p->regval & ARMV8_PMU_PMCR_MASK;
> if (!system_supports_32bit_el0())
> val |= ARMV8_PMU_PMCR_LC;
> + if (!kvm_pmu_is_3p5(vcpu))
> + val &= ~ARMV8_PMU_PMCR_LP;
> __vcpu_sys_reg(vcpu, PMCR_EL0) = val;
> kvm_pmu_handle_pmcr(vcpu, val);
> kvm_vcpu_pmu_restore_guest(vcpu);
> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> index 6bda9b071084..846502251923 100644
> --- a/include/kvm/arm_pmu.h
> +++ b/include/kvm/arm_pmu.h
> @@ -89,6 +89,13 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu);
> vcpu->arch.pmu.events = *kvm_get_pmu_events(); \
> } while (0)
>
> +/*
> + * Evaluates as true when emulating PMUv3p5, and false otherwise.
> + */
> +#define kvm_pmu_is_3p5(vcpu) \
> + (vcpu->kvm->arch.dfr0_pmuver >= ID_AA64DFR0_PMUVER_8_5 && \
> + vcpu->kvm->arch.dfr0_pmuver != ID_AA64DFR0_PMUVER_IMP_DEF)
I don't believe the IMP_DEF condition will ever evaluate to false as
dfr0_pmuver is sanitized at initialization and writes from userspace.
> u8 kvm_arm_pmu_get_host_pmuver(void);
>
> #else
> @@ -153,6 +160,7 @@ static inline u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
> }
>
> #define kvm_vcpu_has_pmu(vcpu) ({ false; })
> +#define kvm_pmu_is_3p5(vcpu) ({ false; })
> static inline void kvm_pmu_update_vcpu_events(struct kvm_vcpu *vcpu) {}
> static inline void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu) {}
> static inline void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu) {}
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 8/9] KVM: arm64: PMU: Implement PMUv3p5 long counter support
2022-08-10 7:16 ` Oliver Upton
@ 2022-08-10 9:28 ` Marc Zyngier
2022-08-27 7:09 ` Reiji Watanabe
0 siblings, 1 reply; 38+ messages in thread
From: Marc Zyngier @ 2022-08-10 9:28 UTC (permalink / raw)
To: Oliver Upton
Cc: linux-arm-kernel, kvmarm, kvm, James Morse, Suzuki K Poulose,
Alexandru Elisei, Ricardo Koller, kernel-team
On Wed, 10 Aug 2022 08:16:14 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
>
> Hi Marc,
>
> On Fri, Aug 05, 2022 at 02:58:12PM +0100, Marc Zyngier wrote:
> > PMUv3p5 (which is mandatory with ARMv8.5) comes with some extra
> > features:
> >
> > - All counters are 64bit
> >
> > - The overflow point is controlled by the PMCR_EL0.LP bit
> >
> > Add the required checks in the helpers that control counter
> > width and overflow, as well as the sysreg handling for the LP
> > bit. A new kvm_pmu_is_3p5() helper makes it easy to spot the
> > PMUv3p5 specific handling.
> >
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> > arch/arm64/kvm/pmu-emul.c | 8 +++++---
> > arch/arm64/kvm/sys_regs.c | 4 ++++
> > include/kvm/arm_pmu.h | 8 ++++++++
> > 3 files changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > index 33a88ca7b7fd..b33a2953cbf6 100644
> > --- a/arch/arm64/kvm/pmu-emul.c
> > +++ b/arch/arm64/kvm/pmu-emul.c
> > @@ -50,13 +50,15 @@ static u32 kvm_pmu_event_mask(struct kvm *kvm)
> > */
> > static bool kvm_pmu_idx_is_64bit(struct kvm_vcpu *vcpu, u64 select_idx)
> > {
> > - return (select_idx == ARMV8_PMU_CYCLE_IDX);
> > + return (select_idx == ARMV8_PMU_CYCLE_IDX || kvm_pmu_is_3p5(vcpu));
> > }
> >
> > static bool kvm_pmu_idx_has_64bit_overflow(struct kvm_vcpu *vcpu, u64 select_idx)
> > {
> > - return (select_idx == ARMV8_PMU_CYCLE_IDX &&
> > - __vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_LC);
> > + u64 val = __vcpu_sys_reg(vcpu, PMCR_EL0);
> > +
> > + return (select_idx < ARMV8_PMU_CYCLE_IDX && (val & ARMV8_PMU_PMCR_LP)) ||
> > + (select_idx == ARMV8_PMU_CYCLE_IDX && (val & ARMV8_PMU_PMCR_LC));
> > }
> >
> > static bool kvm_pmu_counter_can_chain(struct kvm_vcpu *vcpu, u64 idx)
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index c0595f31dab8..2b5e0ec5c100 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -654,6 +654,8 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > | (ARMV8_PMU_PMCR_MASK & 0xdecafbad)) & (~ARMV8_PMU_PMCR_E);
> > if (!system_supports_32bit_el0())
> > val |= ARMV8_PMU_PMCR_LC;
> > + if (!kvm_pmu_is_3p5(vcpu))
> > + val &= ~ARMV8_PMU_PMCR_LP;
> > __vcpu_sys_reg(vcpu, r->reg) = val;
> > }
> >
> > @@ -703,6 +705,8 @@ static bool access_pmcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> > val |= p->regval & ARMV8_PMU_PMCR_MASK;
> > if (!system_supports_32bit_el0())
> > val |= ARMV8_PMU_PMCR_LC;
> > + if (!kvm_pmu_is_3p5(vcpu))
> > + val &= ~ARMV8_PMU_PMCR_LP;
> > __vcpu_sys_reg(vcpu, PMCR_EL0) = val;
> > kvm_pmu_handle_pmcr(vcpu, val);
> > kvm_vcpu_pmu_restore_guest(vcpu);
> > diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> > index 6bda9b071084..846502251923 100644
> > --- a/include/kvm/arm_pmu.h
> > +++ b/include/kvm/arm_pmu.h
> > @@ -89,6 +89,13 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu);
> > vcpu->arch.pmu.events = *kvm_get_pmu_events(); \
> > } while (0)
> >
> > +/*
> > + * Evaluates as true when emulating PMUv3p5, and false otherwise.
> > + */
> > +#define kvm_pmu_is_3p5(vcpu) \
> > + (vcpu->kvm->arch.dfr0_pmuver >= ID_AA64DFR0_PMUVER_8_5 && \
> > + vcpu->kvm->arch.dfr0_pmuver != ID_AA64DFR0_PMUVER_IMP_DEF)
>
> I don't believe the IMP_DEF condition will ever evaluate to false as
> dfr0_pmuver is sanitized at initialization and writes from userspace.
Good point. That's a leftover from a previous version. I'll fix that.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 8/9] KVM: arm64: PMU: Implement PMUv3p5 long counter support
2022-08-10 9:28 ` Marc Zyngier
@ 2022-08-27 7:09 ` Reiji Watanabe
0 siblings, 0 replies; 38+ messages in thread
From: Reiji Watanabe @ 2022-08-27 7:09 UTC (permalink / raw)
To: Marc Zyngier; +Cc: Oliver Upton, kvm, kernel-team, kvmarm, Linux ARM
On Wed, Aug 10, 2022 at 2:28 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Wed, 10 Aug 2022 08:16:14 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> >
> > Hi Marc,
> >
> > On Fri, Aug 05, 2022 at 02:58:12PM +0100, Marc Zyngier wrote:
> > > PMUv3p5 (which is mandatory with ARMv8.5) comes with some extra
> > > features:
> > >
> > > - All counters are 64bit
> > >
> > > - The overflow point is controlled by the PMCR_EL0.LP bit
> > >
> > > Add the required checks in the helpers that control counter
> > > width and overflow, as well as the sysreg handling for the LP
> > > bit. A new kvm_pmu_is_3p5() helper makes it easy to spot the
> > > PMUv3p5 specific handling.
> > >
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > ---
> > > arch/arm64/kvm/pmu-emul.c | 8 +++++---
> > > arch/arm64/kvm/sys_regs.c | 4 ++++
> > > include/kvm/arm_pmu.h | 8 ++++++++
> > > 3 files changed, 17 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > > index 33a88ca7b7fd..b33a2953cbf6 100644
> > > --- a/arch/arm64/kvm/pmu-emul.c
> > > +++ b/arch/arm64/kvm/pmu-emul.c
> > > @@ -50,13 +50,15 @@ static u32 kvm_pmu_event_mask(struct kvm *kvm)
> > > */
> > > static bool kvm_pmu_idx_is_64bit(struct kvm_vcpu *vcpu, u64 select_idx)
> > > {
> > > - return (select_idx == ARMV8_PMU_CYCLE_IDX);
> > > + return (select_idx == ARMV8_PMU_CYCLE_IDX || kvm_pmu_is_3p5(vcpu));
> > > }
> > >
> > > static bool kvm_pmu_idx_has_64bit_overflow(struct kvm_vcpu *vcpu, u64 select_idx)
> > > {
> > > - return (select_idx == ARMV8_PMU_CYCLE_IDX &&
> > > - __vcpu_sys_reg(vcpu, PMCR_EL0) & ARMV8_PMU_PMCR_LC);
> > > + u64 val = __vcpu_sys_reg(vcpu, PMCR_EL0);
> > > +
> > > + return (select_idx < ARMV8_PMU_CYCLE_IDX && (val & ARMV8_PMU_PMCR_LP)) ||
> > > + (select_idx == ARMV8_PMU_CYCLE_IDX && (val & ARMV8_PMU_PMCR_LC));
> > > }
> > >
> > > static bool kvm_pmu_counter_can_chain(struct kvm_vcpu *vcpu, u64 idx)
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index c0595f31dab8..2b5e0ec5c100 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -654,6 +654,8 @@ static void reset_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
> > > | (ARMV8_PMU_PMCR_MASK & 0xdecafbad)) & (~ARMV8_PMU_PMCR_E);
Not directly related to this series, but using 0xdecafbad above
appears to be odd. I think that would lead the bit 3 and 5 to be
unconditionally set in the register's reset value that the guest will
initially see even on the configuration where those should be RES0.
> > > if (!system_supports_32bit_el0())
> > > val |= ARMV8_PMU_PMCR_LC;
> > > + if (!kvm_pmu_is_3p5(vcpu))
> > > + val &= ~ARMV8_PMU_PMCR_LP;
> > > __vcpu_sys_reg(vcpu, r->reg) = val;
> > > }
> > >
> > > @@ -703,6 +705,8 @@ static bool access_pmcr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> > > val |= p->regval & ARMV8_PMU_PMCR_MASK;
> > > if (!system_supports_32bit_el0())
> > > val |= ARMV8_PMU_PMCR_LC;
> > > + if (!kvm_pmu_is_3p5(vcpu))
> > > + val &= ~ARMV8_PMU_PMCR_LP;
> > > __vcpu_sys_reg(vcpu, PMCR_EL0) = val;
> > > kvm_pmu_handle_pmcr(vcpu, val);
> > > kvm_vcpu_pmu_restore_guest(vcpu);
> > > diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> > > index 6bda9b071084..846502251923 100644
> > > --- a/include/kvm/arm_pmu.h
> > > +++ b/include/kvm/arm_pmu.h
> > > @@ -89,6 +89,13 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu);
> > > vcpu->arch.pmu.events = *kvm_get_pmu_events(); \
> > > } while (0)
> > >
> > > +/*
> > > + * Evaluates as true when emulating PMUv3p5, and false otherwise.
> > > + */
> > > +#define kvm_pmu_is_3p5(vcpu) \
> > > + (vcpu->kvm->arch.dfr0_pmuver >= ID_AA64DFR0_PMUVER_8_5 && \
> > > + vcpu->kvm->arch.dfr0_pmuver != ID_AA64DFR0_PMUVER_IMP_DEF)
> >
> > I don't believe the IMP_DEF condition will ever evaluate to false as
> > dfr0_pmuver is sanitized at initialization and writes from userspace.
>
> Good point. That's a leftover from a previous version. I'll fix that.
With the current series, I think the dfr0_pmuver could be IMP_DEF
due to the same bug that I mentioned for the patch-6.
(https://lore.kernel.org/all/20220214065746.1230608-11-reijiw@google.com/)
Thank you,
Reiji
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 9/9] KVM: arm64: PMU: Allow PMUv3p5 to be exposed to the guest
2022-08-05 13:58 [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support Marc Zyngier
` (7 preceding siblings ...)
2022-08-05 13:58 ` [PATCH 8/9] KVM: arm64: PMU: Implement PMUv3p5 long counter support Marc Zyngier
@ 2022-08-05 13:58 ` Marc Zyngier
2022-08-10 7:16 ` Oliver Upton
2022-08-10 18:46 ` [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support Ricardo Koller
9 siblings, 1 reply; 38+ messages in thread
From: Marc Zyngier @ 2022-08-05 13:58 UTC (permalink / raw)
To: linux-arm-kernel, kvmarm, kvm
Cc: James Morse, Suzuki K Poulose, Alexandru Elisei, Oliver Upton,
Ricardo Koller, kernel-team
Now that the infrastructure is in place, bummp the PMU support up
to PMUv3p5.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/kvm/pmu-emul.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index b33a2953cbf6..fbbe6a7e3837 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -1031,6 +1031,6 @@ u8 kvm_arm_pmu_get_host_pmuver(void)
tmp = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
tmp = cpuid_feature_cap_perfmon_field(tmp,
ID_AA64DFR0_PMUVER_SHIFT,
- ID_AA64DFR0_PMUVER_8_4);
+ ID_AA64DFR0_PMUVER_8_5);
return FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER), tmp);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 9/9] KVM: arm64: PMU: Allow PMUv3p5 to be exposed to the guest
2022-08-05 13:58 ` [PATCH 9/9] KVM: arm64: PMU: Allow PMUv3p5 to be exposed to the guest Marc Zyngier
@ 2022-08-10 7:16 ` Oliver Upton
0 siblings, 0 replies; 38+ messages in thread
From: Oliver Upton @ 2022-08-10 7:16 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, kvmarm, kvm, James Morse, Suzuki K Poulose,
Alexandru Elisei, Ricardo Koller, kernel-team
On Fri, Aug 05, 2022 at 02:58:13PM +0100, Marc Zyngier wrote:
> Now that the infrastructure is in place, bummp the PMU support up
typo: bump
> to PMUv3p5.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/kvm/pmu-emul.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index b33a2953cbf6..fbbe6a7e3837 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -1031,6 +1031,6 @@ u8 kvm_arm_pmu_get_host_pmuver(void)
> tmp = read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1);
> tmp = cpuid_feature_cap_perfmon_field(tmp,
> ID_AA64DFR0_PMUVER_SHIFT,
> - ID_AA64DFR0_PMUVER_8_4);
> + ID_AA64DFR0_PMUVER_8_5);
> return FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_PMUVER), tmp);
> }
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support
2022-08-05 13:58 [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support Marc Zyngier
` (8 preceding siblings ...)
2022-08-05 13:58 ` [PATCH 9/9] KVM: arm64: PMU: Allow PMUv3p5 to be exposed to the guest Marc Zyngier
@ 2022-08-10 18:46 ` Ricardo Koller
2022-08-10 19:33 ` Oliver Upton
9 siblings, 1 reply; 38+ messages in thread
From: Ricardo Koller @ 2022-08-10 18:46 UTC (permalink / raw)
To: Marc Zyngier
Cc: linux-arm-kernel, kvmarm, kvm, James Morse, Suzuki K Poulose,
Alexandru Elisei, Oliver Upton, kernel-team
On Fri, Aug 05, 2022 at 02:58:04PM +0100, Marc Zyngier wrote:
> Ricardo recently reported[1] that our PMU emulation was busted when it
> comes to chained events, as we cannot expose the overflow on a 32bit
> boundary (which the architecture requires).
>
> This series aims at fixing this (by deleting a lot of code), and as a
> bonus adds support for PMUv3p5, as this requires us to fix a few more
> things.
>
> Tested on A53 (PMUv3) and FVP (PMUv3p5).
>
> [1] https://lore.kernel.org/r/20220805004139.990531-1-ricarkol@google.com
>
> Marc Zyngier (9):
> KVM: arm64: PMU: Align chained counter implementation with
> architecture pseudocode
> KVM: arm64: PMU: Distinguish between 64bit counter and 64bit overflow
> KVM: arm64: PMU: Only narrow counters that are not 64bit wide
> KVM: arm64: PMU: Add counter_index_to_*reg() helpers
> KVM: arm64: PMU: Simplify setting a counter to a specific value
> KVM: arm64: PMU: Move the ID_AA64DFR0_EL1.PMUver limit to VM creation
> KVM: arm64: PMU: Aleven ID_AA64DFR0_EL1.PMUver to be set from userspace
> KVM: arm64: PMU: Implement PMUv3p5 long counter support
> KVM: arm64: PMU: Aleven PMUv3p5 to be exposed to the guest
>
> arch/arm64/include/asm/kvm_host.h | 1 +
> arch/arm64/kvm/arm.c | 6 +
> arch/arm64/kvm/pmu-emul.c | 372 ++++++++++--------------------
> arch/arm64/kvm/sys_regs.c | 65 +++++-
> include/kvm/arm_pmu.h | 16 +-
> 5 files changed, 208 insertions(+), 252 deletions(-)
>
> --
> 2.34.1
>
Hi Marc,
There is one extra potential issue with exposing PMUv3p5. I see this
weird behavior when doing passthrough ("bare metal") on the fast-model
configured to emulate PMUv3p5: the [63:32] half of the counters
overflowing at 32-bits is still incremented.
Fast model - ARMv8.5:
Assuming the initial state is even=0xFFFFFFFF and odd=0x0,
incrementing the even counter leads to:
0x00000001_00000000 0x00000000_00000001 0x1
even counter odd counter PMOVSET
Assuming the initial state is even=0xFFFFFFFF and odd=0xFFFFFFFF,
incrementing the even counter leads to:
0x00000001_00000000 0x00000001_00000000 0x3
even counter odd counter PMOVSET
More specifically, the pmu-chained-sw-incr kvm-unit-test fails when
doing passthrough of PMUv3p5 (fast model - ARMv8.5):
INFO: PMU version: 0x5
INFO: PMU implementer/ID code: 0x41("A")/0
INFO: Implements 8 event counters
PASS: pmu: pmu-chained-sw-incr: overflow and chain counter incremented after 100 SW_INCR/CHAIN
INFO: pmu: pmu-chained-sw-incr: overflow=0x1, #0=4294967380 #1=1
^^^^^^^^^^^^^
#0=0x00000001_00000054
#1=0x00000000_00000001
FAIL: pmu: pmu-chained-sw-incr: expected overflows and values after 100 SW_INCR/CHAIN
INFO: pmu: pmu-chained-sw-incr: overflow=0x3, #0=4294967380 #1=4294967296
^^^^^^^^^^^^^^^^^^^^^^^^^^^
#0=0x00000001_00000054
#1=0x00000001_00000000
There's really no good use for this behavior, and not sure if it's worth
emulating it. Can't find any reference in the ARM ARM.
Thanks,
Ricardo
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support
2022-08-10 18:46 ` [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support Ricardo Koller
@ 2022-08-10 19:33 ` Oliver Upton
2022-08-10 21:55 ` Ricardo Koller
0 siblings, 1 reply; 38+ messages in thread
From: Oliver Upton @ 2022-08-10 19:33 UTC (permalink / raw)
To: Ricardo Koller
Cc: Marc Zyngier, linux-arm-kernel, kvmarm, kvm, James Morse,
Suzuki K Poulose, Alexandru Elisei, kernel-team
Hi Ricardo,
On Wed, Aug 10, 2022 at 11:46:22AM -0700, Ricardo Koller wrote:
> On Fri, Aug 05, 2022 at 02:58:04PM +0100, Marc Zyngier wrote:
> > Ricardo recently reported[1] that our PMU emulation was busted when it
> > comes to chained events, as we cannot expose the overflow on a 32bit
> > boundary (which the architecture requires).
> >
> > This series aims at fixing this (by deleting a lot of code), and as a
> > bonus adds support for PMUv3p5, as this requires us to fix a few more
> > things.
> >
> > Tested on A53 (PMUv3) and FVP (PMUv3p5).
> >
> > [1] https://lore.kernel.org/r/20220805004139.990531-1-ricarkol@google.com
> >
> > Marc Zyngier (9):
> > KVM: arm64: PMU: Align chained counter implementation with
> > architecture pseudocode
> > KVM: arm64: PMU: Distinguish between 64bit counter and 64bit overflow
> > KVM: arm64: PMU: Only narrow counters that are not 64bit wide
> > KVM: arm64: PMU: Add counter_index_to_*reg() helpers
> > KVM: arm64: PMU: Simplify setting a counter to a specific value
> > KVM: arm64: PMU: Move the ID_AA64DFR0_EL1.PMUver limit to VM creation
> > KVM: arm64: PMU: Aleven ID_AA64DFR0_EL1.PMUver to be set from userspace
> > KVM: arm64: PMU: Implement PMUv3p5 long counter support
> > KVM: arm64: PMU: Aleven PMUv3p5 to be exposed to the guest
> >
> > arch/arm64/include/asm/kvm_host.h | 1 +
> > arch/arm64/kvm/arm.c | 6 +
> > arch/arm64/kvm/pmu-emul.c | 372 ++++++++++--------------------
> > arch/arm64/kvm/sys_regs.c | 65 +++++-
> > include/kvm/arm_pmu.h | 16 +-
> > 5 files changed, 208 insertions(+), 252 deletions(-)
> >
> > --
> > 2.34.1
> >
>
> Hi Marc,
>
> There is one extra potential issue with exposing PMUv3p5. I see this
> weird behavior when doing passthrough ("bare metal") on the fast-model
> configured to emulate PMUv3p5: the [63:32] half of the counters
> overflowing at 32-bits is still incremented.
>
> Fast model - ARMv8.5:
>
> Assuming the initial state is even=0xFFFFFFFF and odd=0x0,
> incrementing the even counter leads to:
>
> 0x00000001_00000000 0x00000000_00000001 0x1
> even counter odd counter PMOVSET
>
> Assuming the initial state is even=0xFFFFFFFF and odd=0xFFFFFFFF,
> incrementing the even counter leads to:
>
> 0x00000001_00000000 0x00000001_00000000 0x3
> even counter odd counter PMOVSET
This is to be expected, actually. PMUv8p5 counters are always 64 bit,
regardless of the configured overflow.
DDI 0487H D8.3 Behavior on overflow
If FEAT_PMUv3p5 is implemented, 64-bit event counters are implemented,
HDCR.HPMN is not 0, and either n is in the range [0 .. (HDCR.HPMN-1)]
or EL2 is not implemented, then event counter overflow is configured
by PMCR.LP:
— When PMCR.LP is set to 0, if incrementing PMEVCNTR<n> causes an unsigned
overflow of bits [31:0] of the event counter, the PE sets PMOVSCLR[n] to 1.
— When PMCR.LP is set to 1, if incrementing PMEVCNTR<n> causes an unsigned
overflow of bits [63:0] of the event counter, the PE sets PMOVSCLR[n] to 1.
[...]
For all 64-bit counters, incrementing the counter is the same whether an
unsigned overflow occurs at [31:0] or [63:0]. If the counter increments
for an event, bits [63:0] are always incremented.
Do you see this same (expected) failure w/ Marc's series?
--
Thanks,
Oliver
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support
2022-08-10 19:33 ` Oliver Upton
@ 2022-08-10 21:55 ` Ricardo Koller
2022-08-11 12:56 ` Marc Zyngier
0 siblings, 1 reply; 38+ messages in thread
From: Ricardo Koller @ 2022-08-10 21:55 UTC (permalink / raw)
To: Oliver Upton
Cc: Marc Zyngier, linux-arm-kernel, kvmarm, kvm, James Morse,
Suzuki K Poulose, Alexandru Elisei, kernel-team
On Wed, Aug 10, 2022 at 02:33:53PM -0500, Oliver Upton wrote:
> Hi Ricardo,
>
> On Wed, Aug 10, 2022 at 11:46:22AM -0700, Ricardo Koller wrote:
> > On Fri, Aug 05, 2022 at 02:58:04PM +0100, Marc Zyngier wrote:
> > > Ricardo recently reported[1] that our PMU emulation was busted when it
> > > comes to chained events, as we cannot expose the overflow on a 32bit
> > > boundary (which the architecture requires).
> > >
> > > This series aims at fixing this (by deleting a lot of code), and as a
> > > bonus adds support for PMUv3p5, as this requires us to fix a few more
> > > things.
> > >
> > > Tested on A53 (PMUv3) and FVP (PMUv3p5).
> > >
> > > [1] https://lore.kernel.org/r/20220805004139.990531-1-ricarkol@google.com
> > >
> > > Marc Zyngier (9):
> > > KVM: arm64: PMU: Align chained counter implementation with
> > > architecture pseudocode
> > > KVM: arm64: PMU: Distinguish between 64bit counter and 64bit overflow
> > > KVM: arm64: PMU: Only narrow counters that are not 64bit wide
> > > KVM: arm64: PMU: Add counter_index_to_*reg() helpers
> > > KVM: arm64: PMU: Simplify setting a counter to a specific value
> > > KVM: arm64: PMU: Move the ID_AA64DFR0_EL1.PMUver limit to VM creation
> > > KVM: arm64: PMU: Aleven ID_AA64DFR0_EL1.PMUver to be set from userspace
> > > KVM: arm64: PMU: Implement PMUv3p5 long counter support
> > > KVM: arm64: PMU: Aleven PMUv3p5 to be exposed to the guest
> > >
> > > arch/arm64/include/asm/kvm_host.h | 1 +
> > > arch/arm64/kvm/arm.c | 6 +
> > > arch/arm64/kvm/pmu-emul.c | 372 ++++++++++--------------------
> > > arch/arm64/kvm/sys_regs.c | 65 +++++-
> > > include/kvm/arm_pmu.h | 16 +-
> > > 5 files changed, 208 insertions(+), 252 deletions(-)
> > >
> > > --
> > > 2.34.1
> > >
> >
> > Hi Marc,
> >
> > There is one extra potential issue with exposing PMUv3p5. I see this
> > weird behavior when doing passthrough ("bare metal") on the fast-model
> > configured to emulate PMUv3p5: the [63:32] half of the counters
> > overflowing at 32-bits is still incremented.
> >
> > Fast model - ARMv8.5:
> >
> > Assuming the initial state is even=0xFFFFFFFF and odd=0x0,
> > incrementing the even counter leads to:
> >
> > 0x00000001_00000000 0x00000000_00000001 0x1
> > even counter odd counter PMOVSET
> >
> > Assuming the initial state is even=0xFFFFFFFF and odd=0xFFFFFFFF,
> > incrementing the even counter leads to:
> >
> > 0x00000001_00000000 0x00000001_00000000 0x3
> > even counter odd counter PMOVSET
>
> This is to be expected, actually. PMUv8p5 counters are always 64 bit,
> regardless of the configured overflow.
>
> DDI 0487H D8.3 Behavior on overflow
>
> If FEAT_PMUv3p5 is implemented, 64-bit event counters are implemented,
> HDCR.HPMN is not 0, and either n is in the range [0 .. (HDCR.HPMN-1)]
> or EL2 is not implemented, then event counter overflow is configured
> by PMCR.LP:
>
> — When PMCR.LP is set to 0, if incrementing PMEVCNTR<n> causes an unsigned
> overflow of bits [31:0] of the event counter, the PE sets PMOVSCLR[n] to 1.
> — When PMCR.LP is set to 1, if incrementing PMEVCNTR<n> causes an unsigned
> overflow of bits [63:0] of the event counter, the PE sets PMOVSCLR[n] to 1.
>
> [...]
>
> For all 64-bit counters, incrementing the counter is the same whether an
> unsigned overflow occurs at [31:0] or [63:0]. If the counter increments
> for an event, bits [63:0] are always incremented.
>
> Do you see this same (expected) failure w/ Marc's series?
I don't know, I'm hitting another bug it seems.
Just realized that KVM does not offer PMUv3p5 (with this series applied)
when the real hardware is only Armv8.2 (the setup I originally tried).
So, tried these other two setups on the fast model:
has_arm_v8-5=1
# ./lkvm-static run --nodefaults --pmu pmu.flat -p pmu-chained-sw-incr
# lkvm run -k pmu.flat -m 704 -c 8 --name guest-135
INFO: PMU version: 0x6
^^^
PMUv3 for Armv8.5
INFO: PMU implementer/ID code: 0x41("A")/0
INFO: Implements 8 event counters
FAIL: pmu: pmu-chained-sw-incr: overflow and chain counter incremented after 100 SW_INCR/CHAIN
INFO: pmu: pmu-chained-sw-incr: overflow=0x0, #0=4294967380 #1=0
^^^
no overflows
FAIL: pmu: pmu-chained-sw-incr: expected overflows and values after 100 SW_INCR/CHAIN
INFO: pmu: pmu-chained-sw-incr: overflow=0x0, #0=84 #1=-1
INFO: pmu: pmu-chained-sw-incr: overflow=0x0, #0=4294967380 #1=4294967295
SUMMARY: 2 tests, 2 unexpected failures
has_arm_v8-2=1
# ./lkvm-static run --nodefaults --pmu pmu.flat -p pmu-chained-sw-incr
# lkvm run -k pmu.flat -m 704 -c 8 --name guest-134
INFO: PMU version: 0x4
^^^
PMUv3 for Armv8.1
INFO: PMU implementer/ID code: 0x41("A")/0
INFO: Implements 8 event counters
PASS: pmu: pmu-chained-sw-incr: overflow and chain counter incremented after 100 SW_INCR/CHAIN
INFO: pmu: pmu-chained-sw-incr: overflow=0x1, #0=84 #1=1
PASS: pmu: pmu-chained-sw-incr: expected overflows and values after 100 SW_INCR/CHAIN
INFO: pmu: pmu-chained-sw-incr: overflow=0x3, #0=84 #1=0
SUMMARY: 2 tests
The Armv8.2 case is working as expected, but the Armv8.5 one is not.
>
> --
> Thanks,
> Oliver
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support
2022-08-10 21:55 ` Ricardo Koller
@ 2022-08-11 12:56 ` Marc Zyngier
2022-08-12 22:53 ` Ricardo Koller
0 siblings, 1 reply; 38+ messages in thread
From: Marc Zyngier @ 2022-08-11 12:56 UTC (permalink / raw)
To: Ricardo Koller
Cc: Oliver Upton, linux-arm-kernel, kvmarm, kvm, James Morse,
Suzuki K Poulose, Alexandru Elisei, kernel-team
On Wed, 10 Aug 2022 22:55:03 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
>
> On Wed, Aug 10, 2022 at 02:33:53PM -0500, Oliver Upton wrote:
> > Hi Ricardo,
> >
> > On Wed, Aug 10, 2022 at 11:46:22AM -0700, Ricardo Koller wrote:
> > > On Fri, Aug 05, 2022 at 02:58:04PM +0100, Marc Zyngier wrote:
> > > > Ricardo recently reported[1] that our PMU emulation was busted when it
> > > > comes to chained events, as we cannot expose the overflow on a 32bit
> > > > boundary (which the architecture requires).
> > > >
> > > > This series aims at fixing this (by deleting a lot of code), and as a
> > > > bonus adds support for PMUv3p5, as this requires us to fix a few more
> > > > things.
> > > >
> > > > Tested on A53 (PMUv3) and FVP (PMUv3p5).
> > > >
> > > > [1] https://lore.kernel.org/r/20220805004139.990531-1-ricarkol@google.com
> > > >
> > > > Marc Zyngier (9):
> > > > KVM: arm64: PMU: Align chained counter implementation with
> > > > architecture pseudocode
> > > > KVM: arm64: PMU: Distinguish between 64bit counter and 64bit overflow
> > > > KVM: arm64: PMU: Only narrow counters that are not 64bit wide
> > > > KVM: arm64: PMU: Add counter_index_to_*reg() helpers
> > > > KVM: arm64: PMU: Simplify setting a counter to a specific value
> > > > KVM: arm64: PMU: Move the ID_AA64DFR0_EL1.PMUver limit to VM creation
> > > > KVM: arm64: PMU: Aleven ID_AA64DFR0_EL1.PMUver to be set from userspace
> > > > KVM: arm64: PMU: Implement PMUv3p5 long counter support
> > > > KVM: arm64: PMU: Aleven PMUv3p5 to be exposed to the guest
> > > >
> > > > arch/arm64/include/asm/kvm_host.h | 1 +
> > > > arch/arm64/kvm/arm.c | 6 +
> > > > arch/arm64/kvm/pmu-emul.c | 372 ++++++++++--------------------
> > > > arch/arm64/kvm/sys_regs.c | 65 +++++-
> > > > include/kvm/arm_pmu.h | 16 +-
> > > > 5 files changed, 208 insertions(+), 252 deletions(-)
> > > >
> > > > --
> > > > 2.34.1
> > > >
> > >
> > > Hi Marc,
> > >
> > > There is one extra potential issue with exposing PMUv3p5. I see this
> > > weird behavior when doing passthrough ("bare metal") on the fast-model
> > > configured to emulate PMUv3p5: the [63:32] half of the counters
> > > overflowing at 32-bits is still incremented.
> > >
> > > Fast model - ARMv8.5:
> > >
> > > Assuming the initial state is even=0xFFFFFFFF and odd=0x0,
> > > incrementing the even counter leads to:
> > >
> > > 0x00000001_00000000 0x00000000_00000001 0x1
> > > even counter odd counter PMOVSET
> > >
> > > Assuming the initial state is even=0xFFFFFFFF and odd=0xFFFFFFFF,
> > > incrementing the even counter leads to:
> > >
> > > 0x00000001_00000000 0x00000001_00000000 0x3
> > > even counter odd counter PMOVSET
> >
> > This is to be expected, actually. PMUv8p5 counters are always 64 bit,
> > regardless of the configured overflow.
> >
> > DDI 0487H D8.3 Behavior on overflow
> >
> > If FEAT_PMUv3p5 is implemented, 64-bit event counters are implemented,
> > HDCR.HPMN is not 0, and either n is in the range [0 .. (HDCR.HPMN-1)]
> > or EL2 is not implemented, then event counter overflow is configured
> > by PMCR.LP:
> >
> > — When PMCR.LP is set to 0, if incrementing PMEVCNTR<n> causes an unsigned
> > overflow of bits [31:0] of the event counter, the PE sets PMOVSCLR[n] to 1.
> > — When PMCR.LP is set to 1, if incrementing PMEVCNTR<n> causes an unsigned
> > overflow of bits [63:0] of the event counter, the PE sets PMOVSCLR[n] to 1.
> >
> > [...]
> >
> > For all 64-bit counters, incrementing the counter is the same whether an
> > unsigned overflow occurs at [31:0] or [63:0]. If the counter increments
> > for an event, bits [63:0] are always incremented.
> >
> > Do you see this same (expected) failure w/ Marc's series?
>
> I don't know, I'm hitting another bug it seems.
>
> Just realized that KVM does not offer PMUv3p5 (with this series applied)
> when the real hardware is only Armv8.2 (the setup I originally tried).
> So, tried these other two setups on the fast model:
>
> has_arm_v8-5=1
>
> # ./lkvm-static run --nodefaults --pmu pmu.flat -p pmu-chained-sw-incr
> # lkvm run -k pmu.flat -m 704 -c 8 --name guest-135
>
> INFO: PMU version: 0x6
> ^^^
> PMUv3 for Armv8.5
> INFO: PMU implementer/ID code: 0x41("A")/0
> INFO: Implements 8 event counters
> FAIL: pmu: pmu-chained-sw-incr: overflow and chain counter incremented after 100 SW_INCR/CHAIN
> INFO: pmu: pmu-chained-sw-incr: overflow=0x0, #0=4294967380 #1=0
> ^^^
> no overflows
> FAIL: pmu: pmu-chained-sw-incr: expected overflows and values after 100 SW_INCR/CHAIN
> INFO: pmu: pmu-chained-sw-incr: overflow=0x0, #0=84 #1=-1
> INFO: pmu: pmu-chained-sw-incr: overflow=0x0, #0=4294967380 #1=4294967295
> SUMMARY: 2 tests, 2 unexpected failures
Hmm. I think I see what's wrong. In kvm_pmu_create_perf_event(), we
have this:
if (kvm_pmu_idx_is_64bit(vcpu, select_idx))
attr.config1 |= 1;
counter = kvm_pmu_get_counter_value(vcpu, select_idx);
/* The initial sample period (overflow count) of an event. */
if (kvm_pmu_idx_has_64bit_overflow(vcpu, select_idx))
attr.sample_period = (-counter) & GENMASK(63, 0);
else
attr.sample_period = (-counter) & GENMASK(31, 0);
but the initial sampling period shouldn't be based on the *guest*
counter overflow. It really is about the getting to an overflow on the
*host*, so the initial code was correct, and only the width of the
counter matters here.
/me goes back to running the FVP...
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support
2022-08-11 12:56 ` Marc Zyngier
@ 2022-08-12 22:53 ` Ricardo Koller
2022-10-24 18:05 ` Marc Zyngier
0 siblings, 1 reply; 38+ messages in thread
From: Ricardo Koller @ 2022-08-12 22:53 UTC (permalink / raw)
To: Marc Zyngier
Cc: Oliver Upton, linux-arm-kernel, kvmarm, kvm, James Morse,
Suzuki K Poulose, Alexandru Elisei, kernel-team
On Thu, Aug 11, 2022 at 01:56:21PM +0100, Marc Zyngier wrote:
> On Wed, 10 Aug 2022 22:55:03 +0100,
> Ricardo Koller <ricarkol@google.com> wrote:
> >
> > On Wed, Aug 10, 2022 at 02:33:53PM -0500, Oliver Upton wrote:
> > > Hi Ricardo,
> > >
> > > On Wed, Aug 10, 2022 at 11:46:22AM -0700, Ricardo Koller wrote:
> > > > On Fri, Aug 05, 2022 at 02:58:04PM +0100, Marc Zyngier wrote:
> > > > > Ricardo recently reported[1] that our PMU emulation was busted when it
> > > > > comes to chained events, as we cannot expose the overflow on a 32bit
> > > > > boundary (which the architecture requires).
> > > > >
> > > > > This series aims at fixing this (by deleting a lot of code), and as a
> > > > > bonus adds support for PMUv3p5, as this requires us to fix a few more
> > > > > things.
> > > > >
> > > > > Tested on A53 (PMUv3) and FVP (PMUv3p5).
> > > > >
> > > > > [1] https://lore.kernel.org/r/20220805004139.990531-1-ricarkol@google.com
> > > > >
> > > > > Marc Zyngier (9):
> > > > > KVM: arm64: PMU: Align chained counter implementation with
> > > > > architecture pseudocode
> > > > > KVM: arm64: PMU: Distinguish between 64bit counter and 64bit overflow
> > > > > KVM: arm64: PMU: Only narrow counters that are not 64bit wide
> > > > > KVM: arm64: PMU: Add counter_index_to_*reg() helpers
> > > > > KVM: arm64: PMU: Simplify setting a counter to a specific value
> > > > > KVM: arm64: PMU: Move the ID_AA64DFR0_EL1.PMUver limit to VM creation
> > > > > KVM: arm64: PMU: Aleven ID_AA64DFR0_EL1.PMUver to be set from userspace
> > > > > KVM: arm64: PMU: Implement PMUv3p5 long counter support
> > > > > KVM: arm64: PMU: Aleven PMUv3p5 to be exposed to the guest
> > > > >
> > > > > arch/arm64/include/asm/kvm_host.h | 1 +
> > > > > arch/arm64/kvm/arm.c | 6 +
> > > > > arch/arm64/kvm/pmu-emul.c | 372 ++++++++++--------------------
> > > > > arch/arm64/kvm/sys_regs.c | 65 +++++-
> > > > > include/kvm/arm_pmu.h | 16 +-
> > > > > 5 files changed, 208 insertions(+), 252 deletions(-)
> > > > >
> > > > > --
> > > > > 2.34.1
> > > > >
> > > >
> > > > Hi Marc,
> > > >
> > > > There is one extra potential issue with exposing PMUv3p5. I see this
> > > > weird behavior when doing passthrough ("bare metal") on the fast-model
> > > > configured to emulate PMUv3p5: the [63:32] half of the counters
> > > > overflowing at 32-bits is still incremented.
> > > >
> > > > Fast model - ARMv8.5:
> > > >
> > > > Assuming the initial state is even=0xFFFFFFFF and odd=0x0,
> > > > incrementing the even counter leads to:
> > > >
> > > > 0x00000001_00000000 0x00000000_00000001 0x1
> > > > even counter odd counter PMOVSET
> > > >
> > > > Assuming the initial state is even=0xFFFFFFFF and odd=0xFFFFFFFF,
> > > > incrementing the even counter leads to:
> > > >
> > > > 0x00000001_00000000 0x00000001_00000000 0x3
> > > > even counter odd counter PMOVSET
> > >
> > > This is to be expected, actually. PMUv8p5 counters are always 64 bit,
> > > regardless of the configured overflow.
> > >
> > > DDI 0487H D8.3 Behavior on overflow
> > >
> > > If FEAT_PMUv3p5 is implemented, 64-bit event counters are implemented,
> > > HDCR.HPMN is not 0, and either n is in the range [0 .. (HDCR.HPMN-1)]
> > > or EL2 is not implemented, then event counter overflow is configured
> > > by PMCR.LP:
> > >
> > > — When PMCR.LP is set to 0, if incrementing PMEVCNTR<n> causes an unsigned
> > > overflow of bits [31:0] of the event counter, the PE sets PMOVSCLR[n] to 1.
> > > — When PMCR.LP is set to 1, if incrementing PMEVCNTR<n> causes an unsigned
> > > overflow of bits [63:0] of the event counter, the PE sets PMOVSCLR[n] to 1.
> > >
> > > [...]
> > >
> > > For all 64-bit counters, incrementing the counter is the same whether an
> > > unsigned overflow occurs at [31:0] or [63:0]. If the counter increments
> > > for an event, bits [63:0] are always incremented.
> > >
> > > Do you see this same (expected) failure w/ Marc's series?
> >
> > I don't know, I'm hitting another bug it seems.
> >
> > Just realized that KVM does not offer PMUv3p5 (with this series applied)
> > when the real hardware is only Armv8.2 (the setup I originally tried).
> > So, tried these other two setups on the fast model:
> >
> > has_arm_v8-5=1
> >
> > # ./lkvm-static run --nodefaults --pmu pmu.flat -p pmu-chained-sw-incr
> > # lkvm run -k pmu.flat -m 704 -c 8 --name guest-135
> >
> > INFO: PMU version: 0x6
> > ^^^
> > PMUv3 for Armv8.5
> > INFO: PMU implementer/ID code: 0x41("A")/0
> > INFO: Implements 8 event counters
> > FAIL: pmu: pmu-chained-sw-incr: overflow and chain counter incremented after 100 SW_INCR/CHAIN
> > INFO: pmu: pmu-chained-sw-incr: overflow=0x0, #0=4294967380 #1=0
> > ^^^
> > no overflows
> > FAIL: pmu: pmu-chained-sw-incr: expected overflows and values after 100 SW_INCR/CHAIN
> > INFO: pmu: pmu-chained-sw-incr: overflow=0x0, #0=84 #1=-1
> > INFO: pmu: pmu-chained-sw-incr: overflow=0x0, #0=4294967380 #1=4294967295
> > SUMMARY: 2 tests, 2 unexpected failures
>
> Hmm. I think I see what's wrong. In kvm_pmu_create_perf_event(), we
> have this:
>
> if (kvm_pmu_idx_is_64bit(vcpu, select_idx))
> attr.config1 |= 1;
>
> counter = kvm_pmu_get_counter_value(vcpu, select_idx);
>
> /* The initial sample period (overflow count) of an event. */
> if (kvm_pmu_idx_has_64bit_overflow(vcpu, select_idx))
> attr.sample_period = (-counter) & GENMASK(63, 0);
> else
> attr.sample_period = (-counter) & GENMASK(31, 0);
>
> but the initial sampling period shouldn't be based on the *guest*
> counter overflow. It really is about the getting to an overflow on the
> *host*, so the initial code was correct, and only the width of the
> counter matters here.
Right, I think this requires bringing back some of the chained related
code (like update_pmc_chained() and pmc_is_chained()), because
attr.sample_period = (-counter) & GENMASK(31, 0);
should also be used when the counter is chained.
Thanks,
Ricardo
>
> /me goes back to running the FVP...
>
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 0/9] KVM: arm64: PMU: Fixing chained events, and PMUv3p5 support
2022-08-12 22:53 ` Ricardo Koller
@ 2022-10-24 18:05 ` Marc Zyngier
0 siblings, 0 replies; 38+ messages in thread
From: Marc Zyngier @ 2022-10-24 18:05 UTC (permalink / raw)
To: Ricardo Koller
Cc: Oliver Upton, linux-arm-kernel, kvmarm, kvm, James Morse,
Suzuki K Poulose, Alexandru Elisei, kernel-team
Hi Ricardo,
On Fri, 12 Aug 2022 23:53:44 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
>
> On Thu, Aug 11, 2022 at 01:56:21PM +0100, Marc Zyngier wrote:
> > On Wed, 10 Aug 2022 22:55:03 +0100,
> > Ricardo Koller <ricarkol@google.com> wrote:
> > >
> > > Just realized that KVM does not offer PMUv3p5 (with this series applied)
> > > when the real hardware is only Armv8.2 (the setup I originally tried).
> > > So, tried these other two setups on the fast model:
> > >
> > > has_arm_v8-5=1
> > >
> > > # ./lkvm-static run --nodefaults --pmu pmu.flat -p pmu-chained-sw-incr
> > > # lkvm run -k pmu.flat -m 704 -c 8 --name guest-135
> > >
> > > INFO: PMU version: 0x6
> > > ^^^
> > > PMUv3 for Armv8.5
> > > INFO: PMU implementer/ID code: 0x41("A")/0
> > > INFO: Implements 8 event counters
> > > FAIL: pmu: pmu-chained-sw-incr: overflow and chain counter incremented after 100 SW_INCR/CHAIN
> > > INFO: pmu: pmu-chained-sw-incr: overflow=0x0, #0=4294967380 #1=0
> > > ^^^
> > > no overflows
> > > FAIL: pmu: pmu-chained-sw-incr: expected overflows and values after 100 SW_INCR/CHAIN
> > > INFO: pmu: pmu-chained-sw-incr: overflow=0x0, #0=84 #1=-1
> > > INFO: pmu: pmu-chained-sw-incr: overflow=0x0, #0=4294967380 #1=4294967295
> > > SUMMARY: 2 tests, 2 unexpected failures
> >
> > Hmm. I think I see what's wrong. In kvm_pmu_create_perf_event(), we
> > have this:
> >
> > if (kvm_pmu_idx_is_64bit(vcpu, select_idx))
> > attr.config1 |= 1;
> >
> > counter = kvm_pmu_get_counter_value(vcpu, select_idx);
> >
> > /* The initial sample period (overflow count) of an event. */
> > if (kvm_pmu_idx_has_64bit_overflow(vcpu, select_idx))
> > attr.sample_period = (-counter) & GENMASK(63, 0);
> > else
> > attr.sample_period = (-counter) & GENMASK(31, 0);
> >
> > but the initial sampling period shouldn't be based on the *guest*
> > counter overflow. It really is about the getting to an overflow on the
> > *host*, so the initial code was correct, and only the width of the
> > counter matters here.
>
> Right, I think this requires bringing back some of the chained related
> code (like update_pmc_chained() and pmc_is_chained()), because
>
> attr.sample_period = (-counter) & GENMASK(31, 0);
>
> should also be used when the counter is chained.
Almost, but not quite. I came up with the following hack (not
everything is relevant, but you'll get my drift):
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 9f29212e8fcd..6470a42e981d 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -450,6 +450,9 @@ static void kvm_pmu_counter_increment(struct kvm_vcpu *vcpu,
reg = lower_32_bits(reg);
__vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) = reg;
+ if (!kvm_pmu_idx_has_64bit_overflow(vcpu, i))
+ reg = lower_32_bits(reg);
+
if (reg) /* No overflow? move on */
continue;
@@ -483,7 +486,7 @@ static void kvm_pmu_perf_overflow(struct perf_event *perf_event,
*/
period = -(local64_read(&perf_event->count));
- if (!kvm_pmu_idx_has_64bit_overflow(vcpu, pmc->idx))
+ if (!kvm_pmu_idx_is_64bit(vcpu, pmc->idx))
period &= GENMASK(31, 0);
local64_set(&perf_event->hw.period_left, 0);
@@ -605,17 +608,24 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
attr.exclude_host = 1; /* Don't count host events */
attr.config = eventsel;
- /* If counting a 64bit event, advertise it to the perf code */
- if (kvm_pmu_idx_is_64bit(vcpu, select_idx))
- attr.config1 |= 1;
-
counter = kvm_pmu_get_counter_value(vcpu, select_idx);
- /* The initial sample period (overflow count) of an event. */
- if (kvm_pmu_idx_has_64bit_overflow(vcpu, select_idx))
- attr.sample_period = (-counter) & GENMASK(63, 0);
- else
+ /*
+ * If counting with a 64bit counter, advertise it to the perf
+ * code, carefully dealing with the initial sample period
+ * which also depends on the overflow.
+ */
+ if (kvm_pmu_idx_is_64bit(vcpu, select_idx)) {
+ attr.config1 |= 1;
+
+ if (!kvm_pmu_idx_has_64bit_overflow(vcpu, select_idx)) {
+ attr.sample_period = -(counter & GENMASK(31, 0));
+ } else {
+ attr.sample_period = (-counter) & GENMASK(63, 0);
+ }
+ } else {
attr.sample_period = (-counter) & GENMASK(31, 0);
+ }
event = perf_event_create_kernel_counter(&attr, -1, current,
kvm_pmu_perf_overflow, pmc);
With this, I'm back in business (in QEMU, as I *still* cannot get ARM
to give me a model that runs natively on arm64...):
root@debian:~/kvm-unit-tests# ../kvmtool/lkvm run --nodefaults --pmu --firmware arm/pmu.flat -p pmu-chained-sw-incr
# lkvm run --firmware arm/pmu.flat -m 448 -c 4 --name guest-400
Info: Removed ghost socket file "/root/.lkvm//guest-400.sock".
WARNING: early print support may not work. Found uart at 0x1000000, but early base is 0x9000000.
INFO: PMU version: 0x6
INFO: PMU implementer/ID code: 0x41("A")/0x1
INFO: Implements 6 event counters
FAIL: pmu: pmu-chained-sw-incr: no overflow and chain counter incremented after 100 SW_INCR/CHAIN
INFO: pmu: pmu-chained-sw-incr: overflow=0x1, #0=4294967380 #1=1
FAIL: pmu: pmu-chained-sw-incr: overflow on chain counter and expected values after 100 SW_INCR/CHAIN
INFO: pmu: pmu-chained-sw-incr: overflow=0x3, #0=4294967380 #1=4294967296
SUMMARY: 2 tests, 2 unexpected failures
The tests themselves need some extra love to account for the fact that
the counters are always 64bit irrespective of the overflow, but at
least I'm now correctly seeing the odd counter incrementing.
I'll try to continue addressing the comments tomorrow.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply related [flat|nested] 38+ messages in thread