Re: [PATCH v8 7/8] KVM: arm64: Support trace filtering for guests
From: James Clark
Date: Mon Dec 23 2024 - 06:28:22 EST
On 21/12/2024 12:34 pm, Marc Zyngier wrote:
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.
KVM_HOST_DATA_FLAG_SWAP_TRFCR maybe?
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.
+}
+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 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.
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().
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.
Yep we can set GUEST_FILTER once at startup and it gets dropped along
with HAS_TRF. That's a lot simpler.
Thanks
James