Message-ID: <87ttaqr0bb.wl-maz@kernel.org>
Date: Thu, 26 Dec 2024 14:22:32 +0000
From: Marc Zyngier <maz@...nel.org>
To: James Clark <james.clark@...aro.org>
Cc: Oliver Upton <oliver.upton@...ux.dev>,
	kvmarm@...ts.linux.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 Mon, 23 Dec 2024 11:28:06 +0000,
James Clark <james.clark@...aro.org> wrote:
> 
> 
> 
> On 21/12/2024 12:34 pm, Marc Zyngier wrote:
> > 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.
> > 
> 
> KVM_HOST_DATA_FLAG_SWAP_TRFCR maybe?

A flag indicates a state, just like a level interrupt. So in cannot be
a verb that indicates an action, after which the flag should be
dropped (just like an edge interrupt). Maybe if you explained what
this is supposed to indicate, we could come up with a better name.

I would have thought that something like EL1_TRACING_ENABLED would be
adequate, but it is unusually hard to understand what this is supposed
to be doing.

> 
> >>   	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.
> > 
> 
> Ah sorry missed that one:
> 
> > Guest value? Or host state while running the guest? If the former,
> > then this has nothing to do here. If the latter, this should be
> > spelled out (trfcr_in_guest?), and the comment amended.
> 
> Yes, the latter, guest TRFCR reads are undef anyway. I can rename this
> and the host_data variable to be trfcr_in_guest.
> 
> >> +{
> >> +	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.
> > 
> 
> Oops, I'd have thought checkpatch could catch something like that. Will fix.

Checkpatch serves little to no purpose, really, and relying on it is a
very bad idea.

>
> >> +}
> >> +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.
> > 
> 
> With only one kvm_set_trfcr() there's no way to distinguish swapping
> in a 0 value or stopping swapping altogether. I thought we wanted a
> single flag that gated the register accesses so the hyp mostly does
> nothing? With only kvm_set_trfcr() you first need to check FEAT_TRF
> then you need to compare the real register with trfcr_in_guest to know
> whether to swap or not every time.
> 
> Actually I think some of the previous versions had something like this
> but it was a bit more complicated.
> 
> Maybe set/clear_trfcr() aren't great names. Perhaps
> kvm_set_trfcr_in_guest() and kvm_disable_trfcr_in_guest()? With the
> second one hinting that it stops the swapping regardless of what the
> values are.

I really think the way you name thing is getting in the way of simply
*understanding* what you are doing.

You don't disable or enable TRFCR. You enable or disable EL1 tracing
while in guest context. TRFCR is the tool by which you are doing that,
and the value you pass to these helpers is the tracing configuration.

> 
> I don't think calling kvm_set_trfcr() with E{1,0}TRE=={0,0} is
> actually broken in this version, it means that the Coresight driver
> wants that value to be installed for guests. So it should actually
> _enable_ swapping in the value of 0, not disable anything.

But you can decide whether there is need for "swapping" if the
configuration is different than what's on the host, can't you?

> 
> > To sum it up, KVM's API should reflect the architecture instead of
> > making things up.
> > 
> 
> We had kvm_set_trfcr(u64 host_trfcr, u64 guest_trfcr) on the last
> version, which also serves the same purpose I mentioned above because
> you can check if they're the same or not and disable swapping. I don't
> know if that counts as reflecting the architecture better. But Oliver
> mentioned he preferred it more "intent" based which is why I added the
> clear_trfcr().

My personal take on this would be something like:

void kvm_tracing_set_el1_configuration(u64 trfcr_while_in_guest)
{
	[VHE stuff omitted]

	*host_data_ptr(guest_trfcr_el1) = trfcr_while_in_guest;
	if (read_sysreg_s(SYS_TRFCR_EL1) != trfcr_while_in_guest);
		host_data_set_flag(EL1_TRACING_CONFIGURED);
	else
		host_data_clear_flag(EL1_TRACING_CONFIGURED);
}

and that'd about about it.

Thanks,

	M.

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

Powered by blists - more mailing lists