Hi Marc, On 9/8/20 9:58 AM, Marc Zyngier wrote: > The PMU code suffers from a small defect where we assume that the event > number provided by the guest is always 16 bit wide, even if the CPU only > implements the ARMv8.0 architecture. This isn't really problematic in > the sense that the event number ends up in a system register, cropping > it to the right width, but still this needs fixing. > > In order to make it work, let's probe the version of the PMU that the > guest is going to use. This is done by temporarily creating a kernel > event and looking at the PMUVer field that has been saved at probe time > in the associated arm_pmu structure. This in turn gets saved in the kvm > structure, and subsequently used to compute the event mask that gets > used throughout the PMU code. > > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx> > --- > arch/arm64/include/asm/kvm_host.h | 2 + > arch/arm64/kvm/pmu-emul.c | 81 +++++++++++++++++++++++++++++-- > 2 files changed, 78 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 65568b23868a..6cd60be69c28 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -110,6 +110,8 @@ struct kvm_arch { > * supported. > */ > bool return_nisv_io_abort_to_user; > + > + unsigned int pmuver; > }; > > struct kvm_vcpu_fault_info { > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c > index 93d797df42c6..8a5f65763814 100644 > --- a/arch/arm64/kvm/pmu-emul.c > +++ b/arch/arm64/kvm/pmu-emul.c > @@ -20,6 +20,20 @@ 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) > +{ > + switch (kvm->arch.pmuver) { > + case 1: /* ARMv8.0 */ > + return GENMASK(9, 0); > + case 4: /* ARMv8.1 */ > + case 5: /* ARMv8.4 */ > + case 6: /* ARMv8.5 */ > + return GENMASK(15, 0); > + default: /* Shouldn't be there, just for sanity */ > + return 0; > + } > +} > + > /** > * kvm_pmu_idx_is_64bit - determine if select_idx is a 64bit counter > * @vcpu: The vcpu pointer > @@ -100,7 +114,7 @@ static bool kvm_pmu_idx_has_chain_evtype(struct kvm_vcpu *vcpu, u64 select_idx) > return false; > > reg = PMEVTYPER0_EL0 + select_idx; > - eventsel = __vcpu_sys_reg(vcpu, reg) & ARMV8_PMU_EVTYPE_EVENT; > + eventsel = __vcpu_sys_reg(vcpu, reg) & kvm_pmu_event_mask(vcpu->kvm); > > return eventsel == ARMV8_PMUV3_PERFCTR_CHAIN; > } > @@ -495,7 +509,7 @@ void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val) > > /* PMSWINC only applies to ... SW_INC! */ > type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i); > - type &= ARMV8_PMU_EVTYPE_EVENT; > + type &= kvm_pmu_event_mask(vcpu->kvm); > if (type != ARMV8_PMUV3_PERFCTR_SW_INCR) > continue; > > @@ -578,7 +592,7 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx) > data = __vcpu_sys_reg(vcpu, reg); > > kvm_pmu_stop_counter(vcpu, pmc); > - eventsel = data & ARMV8_PMU_EVTYPE_EVENT; > + eventsel = data & kvm_pmu_event_mask(vcpu->kvm);; > > /* Software increment event does't need to be backed by a perf event */ > if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR && > @@ -679,17 +693,68 @@ 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) > { > - u64 reg, event_type = data & ARMV8_PMU_EVTYPE_MASK; > + u64 reg, mask; > + > + mask = ARMV8_PMU_EVTYPE_MASK; > + 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; > > - __vcpu_sys_reg(vcpu, reg) = event_type; > + __vcpu_sys_reg(vcpu, reg) = data & mask; > > kvm_pmu_update_pmc_chained(vcpu, select_idx); > kvm_pmu_create_perf_event(vcpu, select_idx); > } > > +static int kvm_pmu_probe_pmuver(void) > +{ > + struct perf_event_attr attr = { }; > + struct perf_event *event; > + struct arm_pmu *pmu; > + int pmuver = 0xf; > + > + /* > + * Create a dummy event that only counts user cycles. As we'll never > + * leave thing function with the event being live, it will never > + * count anything. But it allows us to probe some of the PMU > + * details. Yes, this is terrible. I fail to understand why we can't directly read ID_DFR0_EL1.PerfMon? Thanks Eric > + */ > + attr.type = PERF_TYPE_RAW; > + attr.size = sizeof(attr); > + attr.pinned = 1; > + attr.disabled = 0; > + attr.exclude_user = 0; > + attr.exclude_kernel = 1; > + attr.exclude_hv = 1; > + attr.exclude_host = 1; > + attr.config = ARMV8_PMUV3_PERFCTR_CPU_CYCLES; > + attr.sample_period = GENMASK(63, 0); > + > + event = perf_event_create_kernel_counter(&attr, -1, current, > + kvm_pmu_perf_overflow, &attr); > + > + if (IS_ERR(event)) { > + pr_err_once("kvm: pmu event creation failed %ld\n", > + PTR_ERR(event)); > + return 0xf; > + } > + > + if (event->pmu) { > + pmu = to_arm_pmu(event->pmu); > + if (pmu->pmuver) > + pmuver = pmu->pmuver; > + pr_debug("PMU on CPUs %*pbl version %x\n", > + cpumask_pr_args(&pmu->supported_cpus), pmuver); > + } > + > + perf_event_disable(event); > + perf_event_release_kernel(event); > + > + return pmuver; > +} > + > bool kvm_arm_support_pmu_v3(void) > { > /* > @@ -796,6 +861,12 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr) > if (vcpu->arch.pmu.created) > return -EBUSY; > > + if (!vcpu->kvm->arch.pmuver) > + vcpu->kvm->arch.pmuver = kvm_pmu_probe_pmuver(); > + > + if (vcpu->kvm->arch.pmuver == 0xf) > + return -ENODEV; > + > switch (attr->attr) { > case KVM_ARM_VCPU_PMU_V3_IRQ: { > int __user *uaddr = (int __user *)(long)attr->addr; > _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm