Re: [PATCH v3 2/2] KVM: arm64: PMU: Don't overwrite PMUSERENR with vcpu loaded

[Oliver Upton <oliver.upton@linux.dev>]

Hi Reiji,

Apologies, this fell off my list of reviews.

On Sat, Apr 15, 2023 at 09:40:29AM -0700, Reiji Watanabe wrote:

[...]

>  static void armv8pmu_enable_event(struct perf_event *event)
> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
> index 6718731729fd..7e73be12cfaf 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
> @@ -82,12 +82,24 @@ static inline void __activate_traps_common(struct kvm_vcpu *vcpu)
>  	 */
>  	if (kvm_arm_support_pmu_v3()) {
>  		struct kvm_cpu_context *hctxt;
> +		unsigned long flags;
>  
>  		write_sysreg(0, pmselr_el0);
>  
>  		hctxt = &this_cpu_ptr(&kvm_host_data)->host_ctxt;
> +
> +		/*
> +		 * Disable IRQs to prevent a race condition between the
> +		 * following code and IPIs that attempts to update
> +		 * PMUSERENR_EL0. See also kvm_set_pmuserenr().
> +		 */
> +		local_irq_save(flags);
> +
>  		ctxt_sys_reg(hctxt, PMUSERENR_EL0) = read_sysreg(pmuserenr_el0);
>  		write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0);
> +		vcpu_set_flag(vcpu, PMUSERENR_ON_CPU);
> +
> +		local_irq_restore(flags);

Can the IRQ save/restore be moved to {activate,deactivate}_traps_vhe_{load,put}()?

That'd eliminate the dance to avoid using kernel-only symbols in nVHE
and would be consistent with the existing usage of
__{activate,deactivate}_traps_common() from nVHE (IRQs already
disabled).

IMO, the less nVHE knows about the kernel the better.

>  	}
>  
>  	vcpu->arch.mdcr_el2_host = read_sysreg(mdcr_el2);
> @@ -112,9 +124,21 @@ static inline void __deactivate_traps_common(struct kvm_vcpu *vcpu)
>  	write_sysreg(0, hstr_el2);
>  	if (kvm_arm_support_pmu_v3()) {
>  		struct kvm_cpu_context *hctxt;
> +		unsigned long flags;
>  
>  		hctxt = &this_cpu_ptr(&kvm_host_data)->host_ctxt;
> +
> +		/*
> +		 * Disable IRQs to prevent a race condition between the
> +		 * following code and IPIs that attempts to update
> +		 * PMUSERENR_EL0. See also kvm_set_pmuserenr().
> +		 */
> +		local_irq_save(flags);
> +
>  		write_sysreg(ctxt_sys_reg(hctxt, PMUSERENR_EL0), pmuserenr_el0);
> +		vcpu_clear_flag(vcpu, PMUSERENR_ON_CPU);
> +
> +		local_irq_restore(flags);
>  	}
>  
>  	if (cpus_have_final_cap(ARM64_SME)) {
> diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
> index 530347cdebe3..2c08a54ca7d9 100644
> --- a/arch/arm64/kvm/hyp/nvhe/Makefile
> +++ b/arch/arm64/kvm/hyp/nvhe/Makefile
> @@ -10,7 +10,7 @@ asflags-y := -D__KVM_NVHE_HYPERVISOR__ -D__DISABLE_EXPORTS
>  # will explode instantly (Words of Marc Zyngier). So introduce a generic flag
>  # __DISABLE_TRACE_MMIO__ to disable MMIO tracing for nVHE KVM.
>  ccflags-y := -D__KVM_NVHE_HYPERVISOR__ -D__DISABLE_EXPORTS -D__DISABLE_TRACE_MMIO__
> -ccflags-y += -fno-stack-protector	\
> +ccflags-y += -fno-stack-protector -DNO_TRACE_IRQFLAGS \
>  	     -DDISABLE_BRANCH_PROFILING	\
>  	     $(DISABLE_STACKLEAK_PLUGIN)
>  
> diff --git a/arch/arm64/kvm/pmu.c b/arch/arm64/kvm/pmu.c
> index 7887133d15f0..d6a863853bfe 100644
> --- a/arch/arm64/kvm/pmu.c
> +++ b/arch/arm64/kvm/pmu.c
> @@ -209,3 +209,28 @@ void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu)
>  	kvm_vcpu_pmu_enable_el0(events_host);
>  	kvm_vcpu_pmu_disable_el0(events_guest);
>  }
> +
> +/*
> + * With VHE, keep track of the PMUSERENR_EL0 value for the host EL0 on the pCPU
> + * where PMUSERENR_EL0 for the guest is loaded, since PMUSERENR_EL0 is switched
> + * to the value for the guest on vcpu_load().  The value for the host EL0
> + * will be restored on vcpu_put(), before returning to the EL0.

wording: s/the EL0/EL0. Or, alternatively, to avoid repeating yourself
you can just say "returning to userspace".

You may also want to mention in passing why this isn't necessary for
nVHE, as the register is context switched for every guest enter/exit.

> + *
> + * Return true if KVM takes care of the register. Otherwise return false.
> + */
> +bool kvm_set_pmuserenr(u64 val)
> +{
> +	struct kvm_cpu_context *hctxt;
> +	struct kvm_vcpu *vcpu;
> +
> +	if (!kvm_arm_support_pmu_v3() || !has_vhe())
> +		return false;
> +
> +	vcpu = kvm_get_running_vcpu();
> +	if (!vcpu || !vcpu_get_flag(vcpu, PMUSERENR_ON_CPU))
> +		return false;
> +
> +	hctxt = &this_cpu_ptr(&kvm_host_data)->host_ctxt;
> +	ctxt_sys_reg(hctxt, PMUSERENR_EL0) = val;
> +	return true;
> +}

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel