Subject: Re: [PATCH v3 3/4] KVM: arm64: Add KVM_ARM_VCPU_PMU_V3_SET_PMU attribute

From: Marc Zyngier <[email-masked]>
Date: Tue, 14 Dec 2021 12:28:15 +0000

On Mon, 13 Dec 2021 15:23:08 +0000,
Alexandru Elisei <[email-masked]> wrote:
> 
> When KVM creates an event and there are more than one PMUs present on the
> system, perf_init_event() will go through the list of available PMUs and
> will choose the first one that can create the event. The order of the PMUs
> in the PMU list depends on the probe order, which can change under various
> circumstances, for example if the order of the PMU nodes change in the DTB
> or if asynchronous driver probing is enabled on the kernel command line
> (with the driver_async_probe=armv8-pmu option).
> 
> Another consequence of this approach is that, on heteregeneous systems,
> all virtual machines that KVM creates will use the same PMU. This might
> cause unexpected behaviour for userspace: when a VCPU is executing on
> the physical CPU that uses this PMU, PMU events in the guest work
> correctly; but when the same VCPU executes on another CPU, PMU events in
> the guest will suddenly stop counting.
> 
> Fortunately, perf core allows user to specify on which PMU to create an
> event by using the perf_event_attr->type field, which is used by
> perf_init_event() as an index in the radix tree of available PMUs.
> 
> Add the KVM_ARM_VCPU_PMU_V3_CTRL(KVM_ARM_VCPU_PMU_V3_SET_PMU) VCPU
> attribute to allow userspace to specify the arm_pmu that KVM will use when
> creating events for that VCPU. KVM will make no attempt to run the VCPU on
> the physical CPUs that share this PMU, leaving it up to userspace to
> manage the VCPU threads' affinity accordingly.
> 
> Setting the PMU for a VCPU is an all of nothing affair to avoid exposing an
> asymmetric system to the guest: either all VCPUs have the same PMU, either
> none of the VCPUs have a PMU set. Attempting to do something in between
> will result in an error being returned when doing KVM_ARM_VCPU_PMU_V3_INIT.
> 
> Signed-off-by: Alexandru Elisei <[email-masked]>
> ---
> 
> Checking that all VCPUs have the same PMU is done when the PMU is
> initialized because setting the VCPU PMU is optional, and KVM cannot know
> what the user intends until the KVM_ARM_VCPU_PMU_V3_INIT ioctl, which
> prevents further changes to the VCPU PMU. vcpu->arch.pmu.created has been
> changed to an atomic variable because changes to the VCPU PMU state now
> need to be observable by all physical CPUs.
> 
>  Documentation/virt/kvm/devices/vcpu.rst | 30 ++++++++-
>  arch/arm64/include/uapi/asm/kvm.h       |  1 +
>  arch/arm64/kvm/pmu-emul.c               | 88 ++++++++++++++++++++-----
>  include/kvm/arm_pmu.h                   |  4 +-
>  tools/arch/arm64/include/uapi/asm/kvm.h |  1 +
>  5 files changed, 104 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
> index 60a29972d3f1..b918669bf925 100644
> --- a/Documentation/virt/kvm/devices/vcpu.rst
> +++ b/Documentation/virt/kvm/devices/vcpu.rst
> @@ -49,8 +49,8 @@ Returns:
>  	 =======  ======================================================
>  	 -EEXIST  Interrupt number already used
>  	 -ENODEV  PMUv3 not supported or GIC not initialized
> -	 -ENXIO   PMUv3 not supported, missing VCPU feature or interrupt
> -		  number not set
> +	 -ENXIO   PMUv3 not supported, missing VCPU feature, interrupt
> +		  number not set or mismatched PMUs set
>  	 -EBUSY   PMUv3 already initialized
>  	 =======  ======================================================
>  
> @@ -104,6 +104,32 @@ hardware event. Filtering event 0x1E (CHAIN) has no effect either, as it
>  isn't strictly speaking an event. Filtering the cycle counter is possible
>  using event 0x11 (CPU_CYCLES).
>  
> +1.4 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_SET_PMU
> +------------------------------------------
> +
> +:Parameters: in kvm_device_attr.addr the address to an int representing the PMU
> +             identifier.
> +
> +:Returns:
> +
> +	 =======  ===============================================
> +	 -EBUSY   PMUv3 already initialized
> +	 -EFAULT  Error accessing the PMU identifier
> +	 -ENXIO   PMU not found
> +	 -ENODEV  PMUv3 not supported or GIC not initialized
> +	 -ENOMEM  Could not allocate memory
> +	 =======  ===============================================
> +
> +Request that the VCPU uses the specified hardware PMU when creating guest events
> +for the purpose of PMU emulation. The PMU identifier can be read from the "type"
> +file for the desired PMU instance under /sys/devices (or, equivalent,
> +/sys/bus/even_source). This attribute is particularly useful on heterogeneous
> +systems where there are at least two CPU PMUs on the system. All VCPUs must have
> +the same PMU, otherwise KVM_ARM_VCPU_PMU_V3_INIT will fail.
> +
> +Note that KVM will not make any attempts to run the VCPU on the physical CPUs
> +associated with the PMU specified by this attribute. This is entirely left to
> +userspace.
>  
>  2. GROUP: KVM_ARM_VCPU_TIMER_CTRL
>  =================================
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index b3edde68bc3e..1d0a0a2a9711 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -362,6 +362,7 @@ struct kvm_arm_copy_mte_tags {
>  #define   KVM_ARM_VCPU_PMU_V3_IRQ	0
>  #define   KVM_ARM_VCPU_PMU_V3_INIT	1
>  #define   KVM_ARM_VCPU_PMU_V3_FILTER	2
> +#define   KVM_ARM_VCPU_PMU_V3_SET_PMU	3
>  #define KVM_ARM_VCPU_TIMER_CTRL		1
>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index eb4be96f144d..8de38d7fa493 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -24,9 +24,16 @@ 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)
> +static u32 kvm_pmu_event_mask(struct kvm_vcpu *vcpu)
>  {
> -	switch (kvm->arch.pmuver) {
> +	unsigned int pmuver;
> +
> +	if (vcpu->arch.pmu.arm_pmu)
> +		pmuver = vcpu->arch.pmu.arm_pmu->pmuver;
> +	else
> +		pmuver = vcpu->kvm->arch.pmuver;

This puzzles me throughout the whole patch. Why is the arm_pmu pointer
a per-CPU thing? I would absolutely expect it to be stored in the kvm
structure, making the whole thing much simpler.

> +
> +	switch (pmuver) {
>  	case ID_AA64DFR0_PMUVER_8_0:
>  		return GENMASK(9, 0);
>  	case ID_AA64DFR0_PMUVER_8_1:
> @@ -34,7 +41,7 @@ static u32 kvm_pmu_event_mask(struct kvm *kvm)
>  	case ID_AA64DFR0_PMUVER_8_5:
>  		return GENMASK(15, 0);
>  	default:		/* Shouldn't be here, just for sanity */
> -		WARN_ONCE(1, "Unknown PMU version %d\n", kvm->arch.pmuver);
> +		WARN_ONCE(1, "Unknown PMU version %d\n", pmuver);
>  		return 0;
>  	}
>  }
> @@ -119,7 +126,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) & kvm_pmu_event_mask(vcpu->kvm);
> +	eventsel = __vcpu_sys_reg(vcpu, reg) & kvm_pmu_event_mask(vcpu);
>  
>  	return eventsel == ARMV8_PMUV3_PERFCTR_CHAIN;
>  }
> @@ -534,7 +541,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 &= kvm_pmu_event_mask(vcpu->kvm);
> +		type &= kvm_pmu_event_mask(vcpu);
>  		if (type != ARMV8_PMUV3_PERFCTR_SW_INCR)
>  			continue;
>  
> @@ -602,6 +609,7 @@ static bool kvm_pmu_counter_is_enabled(struct kvm_vcpu *vcpu, u64 select_idx)
>  static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
>  {
>  	struct kvm_pmu *pmu = &vcpu->arch.pmu;
> +	struct arm_pmu *arm_pmu = pmu->arm_pmu;
>  	struct kvm_pmc *pmc;
>  	struct perf_event *event;
>  	struct perf_event_attr attr;
> @@ -622,7 +630,7 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
>  	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>  		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>  	else
> -		eventsel = data & kvm_pmu_event_mask(vcpu->kvm);
> +		eventsel = data & kvm_pmu_event_mask(vcpu);
>  
>  	/* Software increment event doesn't need to be backed by a perf event */
>  	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
> @@ -637,8 +645,7 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
>  		return;
>  
>  	memset(&attr, 0, sizeof(struct perf_event_attr));
> -	attr.type = PERF_TYPE_RAW;
> -	attr.size = sizeof(attr);

Why is this line removed?

> +	attr.type = arm_pmu ? arm_pmu->pmu.type : PERF_TYPE_RAW;
>  	attr.pinned = 1;
>  	attr.disabled = !kvm_pmu_counter_is_enabled(vcpu, pmc->idx);
>  	attr.exclude_user = data & ARMV8_PMU_EXCLUDE_EL0 ? 1 : 0;
> @@ -733,7 +740,7 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
>  
>  	mask  =  ARMV8_PMU_EVTYPE_MASK;
>  	mask &= ~ARMV8_PMU_EVTYPE_EVENT;
> -	mask |= kvm_pmu_event_mask(vcpu->kvm);
> +	mask |= kvm_pmu_event_mask(vcpu);
>  
>  	reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
>  	      ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + select_idx;
> @@ -836,7 +843,7 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
>  	if (!bmap)
>  		return val;
>  
> -	nr_events = kvm_pmu_event_mask(vcpu->kvm) + 1;
> +	nr_events = kvm_pmu_event_mask(vcpu) + 1;
>  
>  	for (i = 0; i < 32; i += 8) {
>  		u64 byte;
> @@ -857,7 +864,7 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>  	if (!kvm_vcpu_has_pmu(vcpu))
>  		return 0;
>  
> -	if (!vcpu->arch.pmu.created)
> +	if (!atomic_read(&vcpu->arch.pmu.created))
>  		return -EINVAL;
>  
>  	/*
> @@ -887,15 +894,20 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>  
>  static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>  {
> -	if (irqchip_in_kernel(vcpu->kvm)) {
> -		int ret;
> +	struct arm_pmu *arm_pmu = vcpu->arch.pmu.arm_pmu;
> +	struct kvm *kvm = vcpu->kvm;
> +	struct kvm_vcpu *v;
> +	int ret = 0;
> +	int i;
> +
> +	if (irqchip_in_kernel(kvm)) {
>  
>  		/*
>  		 * If using the PMU with an in-kernel virtual GIC
>  		 * implementation, we require the GIC to be already
>  		 * initialized when initializing the PMU.
>  		 */
> -		if (!vgic_initialized(vcpu->kvm))
> +		if (!vgic_initialized(kvm))
>  			return -ENODEV;
>  
>  		if (!kvm_arm_pmu_irq_initialized(vcpu))
> @@ -910,7 +922,16 @@ static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>  	init_irq_work(&vcpu->arch.pmu.overflow_work,
>  		      kvm_pmu_perf_overflow_notify_vcpu);
>  
> -	vcpu->arch.pmu.created = true;
> +	atomic_set(&vcpu->arch.pmu.created, 1);
> +
> +	kvm_for_each_vcpu(i, v, kvm) {
> +		if (!atomic_read(&v->arch.pmu.created))
> +			continue;
> +
> +		if (v->arch.pmu.arm_pmu != arm_pmu)
> +			return -ENXIO;
> +	}

If you did store the arm_pmu at the VM level, you wouldn't need this.
You could detect the discrepancy in the set_pmu ioctl.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
[email-masked]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel