| 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
| ||
|
Message-ID: <0cfda877-7c3d-4e59-a1b1-fe245f7298f0@linaro.org>
Date: Mon, 23 Dec 2024 11:28:06 +0000
From: James Clark <james.clark@...aro.org>
To: Marc Zyngier <maz@...nel.org>, Oliver Upton <oliver.upton@...ux.dev>
Cc: 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 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?
>> 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
Powered by blists - more mailing lists