Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 08 Aug 2023 09:27:20 +0100
Message-ID: <87350uq6qf.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     James Clark <james.clark@arm.com>
Cc:     coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org,
        kvmarm@lists.linux.dev, Oliver Upton <oliver.upton@linux.dev>,
        James Morse <james.morse@arm.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Zenghui Yu <yuzenghui@huawei.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Mike Leach <mike.leach@linaro.org>,
        Leo Yan <leo.yan@linaro.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Anshuman Khandual <anshuman.khandual@arm.com>,
        Rob Herring <robh@kernel.org>, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 1/3] arm64: KVM: Add support for exclude_guest and exclude_host for ETM
In-Reply-To: <20230804101317.460697-2-james.clark@arm.com>
References: <20230804101317.460697-1-james.clark@arm.com>
        <20230804101317.460697-2-james.clark@arm.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Fri, 04 Aug 2023 11:13:11 +0100,
James Clark <james.clark@arm.com> wrote:
> 
> Add an interface for the Coresight driver to use to set the current
> exclude settings for the current CPU. This will be used to configure
> TRFCR_EL1.

Can you start by stating the problem? There is *some* rationale in the
cover letter, but not enough to get the full picture. Specially if you
haven't looked at the trace subsystem in the past... 7 years or so.

> 
> The settings must be copied to the vCPU before each run in the same
> way that PMU events are because the per-cpu struct isn't accessible in
> protected mode.

I'm pretty sure that for protected guests, we'd like to disable
tracing altogether (debug mode excepted).

>
> This is only needed for nVHE, otherwise it works automatically with

How about hVHE, which uses VHE at EL2 only? Doesn't it require the
same treatment?

> TRFCR_EL{1,2}. Unfortunately it can't be gated on CONFIG_CORESIGHT
> because Coresight can be built as a module. It can however be gated on
> CONFIG_PERF_EVENTS because that is required by Coresight.

Why does it need to be gated *at all*? We need this for the PMU
because of the way we call into the perf subsystem, but I don't see
anything like that here. In general, conditional compilation sucks,
and I'd like to avoid it as much as possible.

> 
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
>  arch/arm64/include/asm/kvm_host.h | 10 ++++++-
>  arch/arm64/kvm/Makefile           |  1 +
>  arch/arm64/kvm/arm.c              |  1 +
>  arch/arm64/kvm/etm.c              | 48 +++++++++++++++++++++++++++++++
>  include/kvm/etm.h                 | 43 +++++++++++++++++++++++++++
>  5 files changed, 102 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm64/kvm/etm.c
>  create mode 100644 include/kvm/etm.h
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index d7b1403a3fb2..f33262217c84 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -35,6 +35,7 @@
>  #include <kvm/arm_vgic.h>
>  #include <kvm/arm_arch_timer.h>
>  #include <kvm/arm_pmu.h>
> +#include <kvm/etm.h>
>  
>  #define KVM_MAX_VCPUS VGIC_V3_MAX_CPUS
>  
> @@ -500,7 +501,7 @@ struct kvm_vcpu_arch {
>  	u8 cflags;
>  
>  	/* Input flags to the hypervisor code, potentially cleared after use */
> -	u8 iflags;
> +	u16 iflags;

If you make the iflags bigger, what ripple effect does it have on the
alignment of the other data structures? Consider reordering things if
it helps filling holes.

>  
>  	/* State flags for kernel bookkeeping, unused by the hypervisor code */
>  	u8 sflags;
> @@ -541,6 +542,9 @@ struct kvm_vcpu_arch {
>  		u64 pmscr_el1;
>  		/* Self-hosted trace */
>  		u64 trfcr_el1;
> +		/* exclude_guest settings for nVHE */
> +		struct kvm_etm_event etm_event;
> +

Spurious blank line. More importantly, how is that related to the
trfcr_el1 field just above?

>  	} host_debug_state;
>  
>  	/* VGIC state */
> @@ -713,6 +717,8 @@ struct kvm_vcpu_arch {
>  #define DEBUG_STATE_SAVE_TRBE	__vcpu_single_flag(iflags, BIT(6))
>  /* vcpu running in HYP context */
>  #define VCPU_HYP_CONTEXT	__vcpu_single_flag(iflags, BIT(7))
> +/* Save TRFCR and apply exclude_guest rules */
> +#define DEBUG_STATE_SAVE_TRFCR	__vcpu_single_flag(iflags, BIT(8))
>  
>  /* SVE enabled for host EL0 */
>  #define HOST_SVE_ENABLED	__vcpu_single_flag(sflags, BIT(0))
> @@ -1096,6 +1102,8 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu);
>  void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr);
>  void kvm_clr_pmu_events(u32 clr);
>  bool kvm_set_pmuserenr(u64 val);
> +void kvm_set_etm_events(struct perf_event_attr *attr);
> +void kvm_clr_etm_events(void);
>  #else
>  static inline void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr) {}
>  static inline void kvm_clr_pmu_events(u32 clr) {}
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index c0c050e53157..0faff57423c4 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -23,6 +23,7 @@ kvm-y += arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \
>  	 vgic/vgic-its.o vgic/vgic-debug.o
>  
>  kvm-$(CONFIG_HW_PERF_EVENTS)  += pmu-emul.o pmu.o
> +kvm-$(CONFIG_PERF_EVENTS) += etm.o
>
>  always-y := hyp_constants.h hyp-constants.s
>  
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index b1a9d47fb2f3..7bd5975328a3 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -952,6 +952,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>  		kvm_vgic_flush_hwstate(vcpu);
>  
>  		kvm_pmu_update_vcpu_events(vcpu);
> +		kvm_etm_update_vcpu_events(vcpu);
>  
>  		/*
>  		 * Ensure we set mode to IN_GUEST_MODE after we disable
> diff --git a/arch/arm64/kvm/etm.c b/arch/arm64/kvm/etm.c
> new file mode 100644
> index 000000000000..359c37745de2
> --- /dev/null
> +++ b/arch/arm64/kvm/etm.c
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/kvm_host.h>
> +
> +#include <kvm/etm.h>
> +
> +static DEFINE_PER_CPU(struct kvm_etm_event, kvm_etm_events);
> +
> +struct kvm_etm_event *kvm_get_etm_event(void)
> +{
> +	return this_cpu_ptr(&kvm_etm_events);
> +}
> +
> +void kvm_etm_set_events(struct perf_event_attr *attr)
> +{
> +	struct kvm_etm_event *etm_event;
> +
> +	/*
> +	 * Exclude guest option only requires extra work with nVHE.
> +	 * Otherwise it works automatically with TRFCR_EL{1,2}
> +	 */
> +	if (has_vhe())
> +		return;
> +
> +	etm_event = kvm_get_etm_event();
> +
> +	etm_event->exclude_guest = attr->exclude_guest;
> +	etm_event->exclude_host = attr->exclude_host;
> +	etm_event->exclude_kernel = attr->exclude_kernel;
> +	etm_event->exclude_user = attr->exclude_user;
> +}
> +EXPORT_SYMBOL_GPL(kvm_etm_set_events);
> +
> +void kvm_etm_clr_events(void)
> +{
> +	struct kvm_etm_event *etm_event;
> +
> +	if (has_vhe())
> +		return;
> +
> +	etm_event = kvm_get_etm_event();
> +
> +	etm_event->exclude_guest = false;
> +	etm_event->exclude_host = false;
> +	etm_event->exclude_kernel = false;
> +	etm_event->exclude_user = false;
> +}
> +EXPORT_SYMBOL_GPL(kvm_etm_clr_events);

Does it really need its own compilation unit if we were to build it at
all times?

> diff --git a/include/kvm/etm.h b/include/kvm/etm.h
> new file mode 100644
> index 000000000000..95c4809fa2b0
> --- /dev/null
> +++ b/include/kvm/etm.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __KVM_DEBUG_H
> +#define __KVM_DEBUG_H
> +
> +struct perf_event_attr;
> +struct kvm_vcpu;
> +
> +#if IS_ENABLED(CONFIG_KVM) && IS_ENABLED(CONFIG_PERF_EVENTS)
> +
> +struct kvm_etm_event {
> +	bool exclude_host;
> +	bool exclude_guest;
> +	bool exclude_kernel;
> +	bool exclude_user;
> +};
> +
> +struct kvm_etm_event *kvm_get_etm_event(void);
> +void kvm_etm_clr_events(void);
> +void kvm_etm_set_events(struct perf_event_attr *attr);
> +
> +/*
> + * Updates the vcpu's view of the etm events for this cpu. Must be
> + * called before every vcpu run after disabling interrupts, to ensure
> + * that an interrupt cannot fire and update the structure.
> + */
> +#define kvm_etm_update_vcpu_events(vcpu)						\
> +	do {										\
> +		if (!has_vhe() && vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRFCR))		\
> +			vcpu->arch.host_debug_state.etm_event = *kvm_get_etm_event();	\
> +	} while (0)
> +

Why is it a macro and not a function, which would avoid exposing
kvm_get_etm_event?

Thanks,

	M.

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