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

[Marc Zyngier]

On Wed, 27 Nov 2024 10:01:24 +0000,
James Clark <james.clark@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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.