linux-kernel - Re: [PATCH v8 7/8] KVM: arm64: Support trace filtering for guests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <86r061p63b.wl-maz@kernel.org>
Date: Sat, 21 Dec 2024 12:34:48 +0000
From: Marc Zyngier <maz@...nel.org>
To: James Clark <james.clark@...aro.org>
Cc: kvmarm@...ts.linux.dev,
	oliver.upton@...ux.dev,
	suzuki.poulose@....com,
	coresight@...ts.linaro.org,
	Joey Gouly <joey.gouly@....com>,
	Zenghui Yu <yuzenghui@...wei.com>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>,
	Mike Leach <mike.leach@...aro.org>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Mark Brown <broonie@...nel.org>,
	Anshuman Khandual <anshuman.khandual@....com>,
	James Morse <james.morse@....com>,
	"Rob Herring (Arm)" <robh@...nel.org>,
	Shiqi Liu <shiqiliu@...t.edu.cn>,
	Fuad Tabba <tabba@...gle.com>,
	Raghavendra Rao Ananta <rananta@...gle.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 7/8] KVM: arm64: Support trace filtering for guests

On Wed, 27 Nov 2024 10:01:24 +0000,
James Clark <james.clark@...aro.org> wrote:
> 
> For nVHE, switch the filter value in and out if the Coresight driver
> asks for it. This will support filters for guests when sinks other than
> TRBE are used.
> 
> For VHE, just write the filter directly to TRFCR_EL1 where trace can be
> used even with TRBE sinks.
> 
> Signed-off-by: James Clark <james.clark@...aro.org>
> ---
>  arch/arm64/include/asm/kvm_host.h  |  5 +++++
>  arch/arm64/kvm/debug.c             | 28 ++++++++++++++++++++++++++++
>  arch/arm64/kvm/hyp/nvhe/debug-sr.c |  1 +
>  3 files changed, 34 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index ba251caa593b..cce07887551b 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -613,6 +613,7 @@ struct kvm_host_data {
>  #define KVM_HOST_DATA_FLAG_HAS_SPE	0
>  #define KVM_HOST_DATA_FLAG_HAS_TRF	1
>  #define KVM_HOST_DATA_FLAG_TRBE_ENABLED	2
> +#define KVM_HOST_DATA_FLAG_GUEST_FILTER	3

Guest filter what? This is meaningless.

>  	unsigned long flags;
>  
>  	struct kvm_cpu_context host_ctxt;
> @@ -1387,6 +1388,8 @@ void kvm_clr_pmu_events(u64 clr);
>  bool kvm_set_pmuserenr(u64 val);
>  void kvm_enable_trbe(void);
>  void kvm_disable_trbe(void);
> +void kvm_set_trfcr(u64 guest_trfcr);
> +void kvm_clear_trfcr(void);
>  #else
>  static inline void kvm_set_pmu_events(u64 set, struct perf_event_attr *attr) {}
>  static inline void kvm_clr_pmu_events(u64 clr) {}
> @@ -1396,6 +1399,8 @@ static inline bool kvm_set_pmuserenr(u64 val)
>  }
>  static inline void kvm_enable_trbe(void) {}
>  static inline void kvm_disable_trbe(void) {}
> +static inline void kvm_set_trfcr(u64 guest_trfcr) {}
> +static inline void kvm_clear_trfcr(void) {}
>  #endif
>  
>  void kvm_vcpu_load_vhe(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 0c340ae7b5d1..9266f2776991 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -337,3 +337,31 @@ void kvm_disable_trbe(void)
>  	host_data_clear_flag(TRBE_ENABLED);
>  }
>  EXPORT_SYMBOL_GPL(kvm_disable_trbe);
> +
> +void kvm_set_trfcr(u64 guest_trfcr)

Again. Is this the guest's view? or the host view while running the
guest? I asked the question on the previous patch, and you didn't
reply.

> +{
> +	if (is_protected_kvm_enabled() || WARN_ON_ONCE(preemptible()))
> +		return;
> +
> +	if (has_vhe())
> +		write_sysreg_s(guest_trfcr, SYS_TRFCR_EL12);
> +	else {
> +		*host_data_ptr(guest_trfcr_el1) = guest_trfcr;
> +		host_data_set_flag(GUEST_FILTER);
> +	}

Oh come on. This is basic coding style, see section 3 in
Documentation/process/coding-style.rst.

> +}
> +EXPORT_SYMBOL_GPL(kvm_set_trfcr);
> +
> +void kvm_clear_trfcr(void)
> +{
> +	if (is_protected_kvm_enabled() || WARN_ON_ONCE(preemptible()))
> +		return;
> +
> +	if (has_vhe())
> +		write_sysreg_s(0, SYS_TRFCR_EL12);
> +	else {
> +		*host_data_ptr(guest_trfcr_el1) = 0;
> +		host_data_clear_flag(GUEST_FILTER);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(kvm_clear_trfcr);

Why do we have two helpers? Clearly, calling kvm_set_trfcr() with
E{1,0}TRE=={0,0} should result in *disabling* things.  Except it
doesn't, and you should fix it. Once that is fixed, it becomes obvious
that kvm_clear_trfcr() serves no purpose.

To sum it up, KVM's API should reflect the architecture instead of
making things up.

> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> index 9479bee41801..7edee7ace433 100644
> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> @@ -67,6 +67,7 @@ static void __trace_do_switch(u64 *saved_trfcr, u64 new_trfcr)
>  static bool __trace_needs_switch(void)
>  {
>  	return host_data_test_flag(TRBE_ENABLED) ||
> +	       host_data_test_flag(GUEST_FILTER) ||
>  	       (is_protected_kvm_enabled() && host_data_test_flag(HAS_TRF));

Wouldn't it make more sense to just force the "GUEST_FILTER" flag in
the pKVM case, and drop the 3rd term altogether?

	M.

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