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

[James Clark <james.clark@linaro.org>]

On 21/12/2024 12:34 pm, Marc Zyngier wrote:
> On Wed, 27 Nov 2024 10:01:24 +0000,
> James Clark <james.clark@linaro.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@linaro.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?

>>   	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