* [RFC PATCH 1/3] arm64: KVM: Add support for exclude_guest and exclude_host for ETM
2023-08-04 10:13 [RFC PATCH 0/3] coresight: Support exclude_guest with Feat_TRF and nVHE James Clark
@ 2023-08-04 10:13 ` James Clark
2023-08-08 8:27 ` Marc Zyngier
2023-08-04 10:13 ` [RFC PATCH 2/3] arm64: KVM: Support exclude_guest for Coresight trace in nVHE James Clark
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: James Clark @ 2023-08-04 10:13 UTC (permalink / raw)
To: coresight, linux-arm-kernel, kvmarm
Cc: James Clark, Marc Zyngier, Oliver Upton, James Morse,
Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Will Deacon,
Mike Leach, Leo Yan, Alexander Shishkin, Anshuman Khandual,
Rob Herring, linux-kernel
Add an interface for the Coresight driver to use to set the current
exclude settings for the current CPU. This will be used to configure
TRFCR_EL1.
The settings must be copied to the vCPU before each run in the same
way that PMU events are because the per-cpu struct isn't accessible in
protected mode.
This is only needed for nVHE, otherwise it works automatically with
TRFCR_EL{1,2}. Unfortunately it can't be gated on CONFIG_CORESIGHT
because Coresight can be built as a module. It can however be gated on
CONFIG_PERF_EVENTS because that is required by Coresight.
Signed-off-by: James Clark <james.clark@arm.com>
---
arch/arm64/include/asm/kvm_host.h | 10 ++++++-
arch/arm64/kvm/Makefile | 1 +
arch/arm64/kvm/arm.c | 1 +
arch/arm64/kvm/etm.c | 48 +++++++++++++++++++++++++++++++
include/kvm/etm.h | 43 +++++++++++++++++++++++++++
5 files changed, 102 insertions(+), 1 deletion(-)
create mode 100644 arch/arm64/kvm/etm.c
create mode 100644 include/kvm/etm.h
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index d7b1403a3fb2..f33262217c84 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -35,6 +35,7 @@
#include <kvm/arm_vgic.h>
#include <kvm/arm_arch_timer.h>
#include <kvm/arm_pmu.h>
+#include <kvm/etm.h>
#define KVM_MAX_VCPUS VGIC_V3_MAX_CPUS
@@ -500,7 +501,7 @@ struct kvm_vcpu_arch {
u8 cflags;
/* Input flags to the hypervisor code, potentially cleared after use */
- u8 iflags;
+ u16 iflags;
/* State flags for kernel bookkeeping, unused by the hypervisor code */
u8 sflags;
@@ -541,6 +542,9 @@ struct kvm_vcpu_arch {
u64 pmscr_el1;
/* Self-hosted trace */
u64 trfcr_el1;
+ /* exclude_guest settings for nVHE */
+ struct kvm_etm_event etm_event;
+
} host_debug_state;
/* VGIC state */
@@ -713,6 +717,8 @@ struct kvm_vcpu_arch {
#define DEBUG_STATE_SAVE_TRBE __vcpu_single_flag(iflags, BIT(6))
/* vcpu running in HYP context */
#define VCPU_HYP_CONTEXT __vcpu_single_flag(iflags, BIT(7))
+/* Save TRFCR and apply exclude_guest rules */
+#define DEBUG_STATE_SAVE_TRFCR __vcpu_single_flag(iflags, BIT(8))
/* SVE enabled for host EL0 */
#define HOST_SVE_ENABLED __vcpu_single_flag(sflags, BIT(0))
@@ -1096,6 +1102,8 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu);
void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr);
void kvm_clr_pmu_events(u32 clr);
bool kvm_set_pmuserenr(u64 val);
+void kvm_set_etm_events(struct perf_event_attr *attr);
+void kvm_clr_etm_events(void);
#else
static inline void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr) {}
static inline void kvm_clr_pmu_events(u32 clr) {}
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index c0c050e53157..0faff57423c4 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -23,6 +23,7 @@ kvm-y += arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \
vgic/vgic-its.o vgic/vgic-debug.o
kvm-$(CONFIG_HW_PERF_EVENTS) += pmu-emul.o pmu.o
+kvm-$(CONFIG_PERF_EVENTS) += etm.o
always-y := hyp_constants.h hyp-constants.s
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index b1a9d47fb2f3..7bd5975328a3 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -952,6 +952,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
kvm_vgic_flush_hwstate(vcpu);
kvm_pmu_update_vcpu_events(vcpu);
+ kvm_etm_update_vcpu_events(vcpu);
/*
* Ensure we set mode to IN_GUEST_MODE after we disable
diff --git a/arch/arm64/kvm/etm.c b/arch/arm64/kvm/etm.c
new file mode 100644
index 000000000000..359c37745de2
--- /dev/null
+++ b/arch/arm64/kvm/etm.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/kvm_host.h>
+
+#include <kvm/etm.h>
+
+static DEFINE_PER_CPU(struct kvm_etm_event, kvm_etm_events);
+
+struct kvm_etm_event *kvm_get_etm_event(void)
+{
+ return this_cpu_ptr(&kvm_etm_events);
+}
+
+void kvm_etm_set_events(struct perf_event_attr *attr)
+{
+ struct kvm_etm_event *etm_event;
+
+ /*
+ * Exclude guest option only requires extra work with nVHE.
+ * Otherwise it works automatically with TRFCR_EL{1,2}
+ */
+ if (has_vhe())
+ return;
+
+ etm_event = kvm_get_etm_event();
+
+ etm_event->exclude_guest = attr->exclude_guest;
+ etm_event->exclude_host = attr->exclude_host;
+ etm_event->exclude_kernel = attr->exclude_kernel;
+ etm_event->exclude_user = attr->exclude_user;
+}
+EXPORT_SYMBOL_GPL(kvm_etm_set_events);
+
+void kvm_etm_clr_events(void)
+{
+ struct kvm_etm_event *etm_event;
+
+ if (has_vhe())
+ return;
+
+ etm_event = kvm_get_etm_event();
+
+ etm_event->exclude_guest = false;
+ etm_event->exclude_host = false;
+ etm_event->exclude_kernel = false;
+ etm_event->exclude_user = false;
+}
+EXPORT_SYMBOL_GPL(kvm_etm_clr_events);
diff --git a/include/kvm/etm.h b/include/kvm/etm.h
new file mode 100644
index 000000000000..95c4809fa2b0
--- /dev/null
+++ b/include/kvm/etm.h
@@ -0,0 +1,43 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __KVM_DEBUG_H
+#define __KVM_DEBUG_H
+
+struct perf_event_attr;
+struct kvm_vcpu;
+
+#if IS_ENABLED(CONFIG_KVM) && IS_ENABLED(CONFIG_PERF_EVENTS)
+
+struct kvm_etm_event {
+ bool exclude_host;
+ bool exclude_guest;
+ bool exclude_kernel;
+ bool exclude_user;
+};
+
+struct kvm_etm_event *kvm_get_etm_event(void);
+void kvm_etm_clr_events(void);
+void kvm_etm_set_events(struct perf_event_attr *attr);
+
+/*
+ * Updates the vcpu's view of the etm events for this cpu. Must be
+ * called before every vcpu run after disabling interrupts, to ensure
+ * that an interrupt cannot fire and update the structure.
+ */
+#define kvm_etm_update_vcpu_events(vcpu) \
+ do { \
+ if (!has_vhe() && vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRFCR)) \
+ vcpu->arch.host_debug_state.etm_event = *kvm_get_etm_event(); \
+ } while (0)
+
+#else
+
+struct kvm_etm_event {};
+
+static inline void kvm_etm_update_vcpu_events(struct kvm_vcpu *vcpu) {}
+static inline void kvm_etm_set_events(struct perf_event_attr *attr) {}
+static inline void kvm_etm_clr_events(void) {}
+
+#endif
+
+#endif
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [RFC PATCH 1/3] arm64: KVM: Add support for exclude_guest and exclude_host for ETM
2023-08-04 10:13 ` [RFC PATCH 1/3] arm64: KVM: Add support for exclude_guest and exclude_host for ETM James Clark
@ 2023-08-08 8:27 ` Marc Zyngier
2023-08-09 14:17 ` James Clark
0 siblings, 1 reply; 12+ messages in thread
From: Marc Zyngier @ 2023-08-08 8:27 UTC (permalink / raw)
To: James Clark
Cc: coresight, linux-arm-kernel, kvmarm, Oliver Upton, James Morse,
Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Will Deacon,
Mike Leach, Leo Yan, Alexander Shishkin, Anshuman Khandual,
Rob Herring, linux-kernel
On Fri, 04 Aug 2023 11:13:11 +0100,
James Clark <james.clark@arm.com> wrote:
>
> Add an interface for the Coresight driver to use to set the current
> exclude settings for the current CPU. This will be used to configure
> TRFCR_EL1.
Can you start by stating the problem? There is *some* rationale in the
cover letter, but not enough to get the full picture. Specially if you
haven't looked at the trace subsystem in the past... 7 years or so.
>
> The settings must be copied to the vCPU before each run in the same
> way that PMU events are because the per-cpu struct isn't accessible in
> protected mode.
I'm pretty sure that for protected guests, we'd like to disable
tracing altogether (debug mode excepted).
>
> This is only needed for nVHE, otherwise it works automatically with
How about hVHE, which uses VHE at EL2 only? Doesn't it require the
same treatment?
> TRFCR_EL{1,2}. Unfortunately it can't be gated on CONFIG_CORESIGHT
> because Coresight can be built as a module. It can however be gated on
> CONFIG_PERF_EVENTS because that is required by Coresight.
Why does it need to be gated *at all*? We need this for the PMU
because of the way we call into the perf subsystem, but I don't see
anything like that here. In general, conditional compilation sucks,
and I'd like to avoid it as much as possible.
>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
> arch/arm64/include/asm/kvm_host.h | 10 ++++++-
> arch/arm64/kvm/Makefile | 1 +
> arch/arm64/kvm/arm.c | 1 +
> arch/arm64/kvm/etm.c | 48 +++++++++++++++++++++++++++++++
> include/kvm/etm.h | 43 +++++++++++++++++++++++++++
> 5 files changed, 102 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm64/kvm/etm.c
> create mode 100644 include/kvm/etm.h
>
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index d7b1403a3fb2..f33262217c84 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -35,6 +35,7 @@
> #include <kvm/arm_vgic.h>
> #include <kvm/arm_arch_timer.h>
> #include <kvm/arm_pmu.h>
> +#include <kvm/etm.h>
>
> #define KVM_MAX_VCPUS VGIC_V3_MAX_CPUS
>
> @@ -500,7 +501,7 @@ struct kvm_vcpu_arch {
> u8 cflags;
>
> /* Input flags to the hypervisor code, potentially cleared after use */
> - u8 iflags;
> + u16 iflags;
If you make the iflags bigger, what ripple effect does it have on the
alignment of the other data structures? Consider reordering things if
it helps filling holes.
>
> /* State flags for kernel bookkeeping, unused by the hypervisor code */
> u8 sflags;
> @@ -541,6 +542,9 @@ struct kvm_vcpu_arch {
> u64 pmscr_el1;
> /* Self-hosted trace */
> u64 trfcr_el1;
> + /* exclude_guest settings for nVHE */
> + struct kvm_etm_event etm_event;
> +
Spurious blank line. More importantly, how is that related to the
trfcr_el1 field just above?
> } host_debug_state;
>
> /* VGIC state */
> @@ -713,6 +717,8 @@ struct kvm_vcpu_arch {
> #define DEBUG_STATE_SAVE_TRBE __vcpu_single_flag(iflags, BIT(6))
> /* vcpu running in HYP context */
> #define VCPU_HYP_CONTEXT __vcpu_single_flag(iflags, BIT(7))
> +/* Save TRFCR and apply exclude_guest rules */
> +#define DEBUG_STATE_SAVE_TRFCR __vcpu_single_flag(iflags, BIT(8))
>
> /* SVE enabled for host EL0 */
> #define HOST_SVE_ENABLED __vcpu_single_flag(sflags, BIT(0))
> @@ -1096,6 +1102,8 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu);
> void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr);
> void kvm_clr_pmu_events(u32 clr);
> bool kvm_set_pmuserenr(u64 val);
> +void kvm_set_etm_events(struct perf_event_attr *attr);
> +void kvm_clr_etm_events(void);
> #else
> static inline void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr) {}
> static inline void kvm_clr_pmu_events(u32 clr) {}
> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> index c0c050e53157..0faff57423c4 100644
> --- a/arch/arm64/kvm/Makefile
> +++ b/arch/arm64/kvm/Makefile
> @@ -23,6 +23,7 @@ kvm-y += arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \
> vgic/vgic-its.o vgic/vgic-debug.o
>
> kvm-$(CONFIG_HW_PERF_EVENTS) += pmu-emul.o pmu.o
> +kvm-$(CONFIG_PERF_EVENTS) += etm.o
>
> always-y := hyp_constants.h hyp-constants.s
>
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index b1a9d47fb2f3..7bd5975328a3 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -952,6 +952,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
> kvm_vgic_flush_hwstate(vcpu);
>
> kvm_pmu_update_vcpu_events(vcpu);
> + kvm_etm_update_vcpu_events(vcpu);
>
> /*
> * Ensure we set mode to IN_GUEST_MODE after we disable
> diff --git a/arch/arm64/kvm/etm.c b/arch/arm64/kvm/etm.c
> new file mode 100644
> index 000000000000..359c37745de2
> --- /dev/null
> +++ b/arch/arm64/kvm/etm.c
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/kvm_host.h>
> +
> +#include <kvm/etm.h>
> +
> +static DEFINE_PER_CPU(struct kvm_etm_event, kvm_etm_events);
> +
> +struct kvm_etm_event *kvm_get_etm_event(void)
> +{
> + return this_cpu_ptr(&kvm_etm_events);
> +}
> +
> +void kvm_etm_set_events(struct perf_event_attr *attr)
> +{
> + struct kvm_etm_event *etm_event;
> +
> + /*
> + * Exclude guest option only requires extra work with nVHE.
> + * Otherwise it works automatically with TRFCR_EL{1,2}
> + */
> + if (has_vhe())
> + return;
> +
> + etm_event = kvm_get_etm_event();
> +
> + etm_event->exclude_guest = attr->exclude_guest;
> + etm_event->exclude_host = attr->exclude_host;
> + etm_event->exclude_kernel = attr->exclude_kernel;
> + etm_event->exclude_user = attr->exclude_user;
> +}
> +EXPORT_SYMBOL_GPL(kvm_etm_set_events);
> +
> +void kvm_etm_clr_events(void)
> +{
> + struct kvm_etm_event *etm_event;
> +
> + if (has_vhe())
> + return;
> +
> + etm_event = kvm_get_etm_event();
> +
> + etm_event->exclude_guest = false;
> + etm_event->exclude_host = false;
> + etm_event->exclude_kernel = false;
> + etm_event->exclude_user = false;
> +}
> +EXPORT_SYMBOL_GPL(kvm_etm_clr_events);
Does it really need its own compilation unit if we were to build it at
all times?
> diff --git a/include/kvm/etm.h b/include/kvm/etm.h
> new file mode 100644
> index 000000000000..95c4809fa2b0
> --- /dev/null
> +++ b/include/kvm/etm.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __KVM_DEBUG_H
> +#define __KVM_DEBUG_H
> +
> +struct perf_event_attr;
> +struct kvm_vcpu;
> +
> +#if IS_ENABLED(CONFIG_KVM) && IS_ENABLED(CONFIG_PERF_EVENTS)
> +
> +struct kvm_etm_event {
> + bool exclude_host;
> + bool exclude_guest;
> + bool exclude_kernel;
> + bool exclude_user;
> +};
> +
> +struct kvm_etm_event *kvm_get_etm_event(void);
> +void kvm_etm_clr_events(void);
> +void kvm_etm_set_events(struct perf_event_attr *attr);
> +
> +/*
> + * Updates the vcpu's view of the etm events for this cpu. Must be
> + * called before every vcpu run after disabling interrupts, to ensure
> + * that an interrupt cannot fire and update the structure.
> + */
> +#define kvm_etm_update_vcpu_events(vcpu) \
> + do { \
> + if (!has_vhe() && vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRFCR)) \
> + vcpu->arch.host_debug_state.etm_event = *kvm_get_etm_event(); \
> + } while (0)
> +
Why is it a macro and not a function, which would avoid exposing
kvm_get_etm_event?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFC PATCH 1/3] arm64: KVM: Add support for exclude_guest and exclude_host for ETM
2023-08-08 8:27 ` Marc Zyngier
@ 2023-08-09 14:17 ` James Clark
0 siblings, 0 replies; 12+ messages in thread
From: James Clark @ 2023-08-09 14:17 UTC (permalink / raw)
To: Marc Zyngier
Cc: coresight, linux-arm-kernel, kvmarm, Oliver Upton, James Morse,
Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Will Deacon,
Mike Leach, Leo Yan, Alexander Shishkin, Anshuman Khandual,
Rob Herring, linux-kernel
On 08/08/2023 09:27, Marc Zyngier wrote:
> On Fri, 04 Aug 2023 11:13:11 +0100,
> James Clark <james.clark@arm.com> wrote:
>>
>> Add an interface for the Coresight driver to use to set the current
>> exclude settings for the current CPU. This will be used to configure
>> TRFCR_EL1.
>
> Can you start by stating the problem? There is *some* rationale in the
> cover letter, but not enough to get the full picture. Specially if you
> haven't looked at the trace subsystem in the past... 7 years or so.
>
Yeah I will expand on it a bit.
>>
>> The settings must be copied to the vCPU before each run in the same
>> way that PMU events are because the per-cpu struct isn't accessible in
>> protected mode.
>
> I'm pretty sure that for protected guests, we'd like to disable
> tracing altogether (debug mode excepted).
>
Do you mean disable the ability to trace guests from the host? Or to
disable generation of any trace altogether when in the guest, even if
the guest started the session?
Currently the ETM driver isn't exposed to guests, so it's not possible
for the guest to start a session. That leaves only the host being able
to trace the guest. If you think that shouldn't happen outside of debug
mode then maybe we need another change to prevent that.
Another issue is that this only works with Feat_TRF, without that we'd
have to do something else again to prevent protected guests from being
traced, as currently there is nothing to stop it.
I'm not sure about debug mode though? Is that a kernel config or some
kvm option? I suppose this could be disabled for users by not loading
the Coresight driver, but there's currently nothing a guest can do to
stop its self from being traced.
>>
>> This is only needed for nVHE, otherwise it works automatically with
>
> How about hVHE, which uses VHE at EL2 only? Doesn't it require the
> same treatment?
>
Yes it sounds like it would. Does it use the same code as nVHE in
arch/arm64/kvm/hyp/nvhe? I think it might already work in that case, but
I need to read a bit about hVHE.
>> TRFCR_EL{1,2}. Unfortunately it can't be gated on CONFIG_CORESIGHT
>> because Coresight can be built as a module. It can however be gated on
>> CONFIG_PERF_EVENTS because that is required by Coresight.
>
> Why does it need to be gated *at all*? We need this for the PMU
> because of the way we call into the perf subsystem, but I don't see
> anything like that here. In general, conditional compilation sucks,
> and I'd like to avoid it as much as possible.
>
No real reason really, I was probably trying to minimise the impact
because it added a few bytes here and there, but it's tiny. If you're ok
with it I can quite easily remove the conditional compilation and it
should simplify it a bit.
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>> arch/arm64/include/asm/kvm_host.h | 10 ++++++-
>> arch/arm64/kvm/Makefile | 1 +
>> arch/arm64/kvm/arm.c | 1 +
>> arch/arm64/kvm/etm.c | 48 +++++++++++++++++++++++++++++++
>> include/kvm/etm.h | 43 +++++++++++++++++++++++++++
>> 5 files changed, 102 insertions(+), 1 deletion(-)
>> create mode 100644 arch/arm64/kvm/etm.c
>> create mode 100644 include/kvm/etm.h
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index d7b1403a3fb2..f33262217c84 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -35,6 +35,7 @@
>> #include <kvm/arm_vgic.h>
>> #include <kvm/arm_arch_timer.h>
>> #include <kvm/arm_pmu.h>
>> +#include <kvm/etm.h>
>>
>> #define KVM_MAX_VCPUS VGIC_V3_MAX_CPUS
>>
>> @@ -500,7 +501,7 @@ struct kvm_vcpu_arch {
>> u8 cflags;
>>
>> /* Input flags to the hypervisor code, potentially cleared after use */
>> - u8 iflags;
>> + u16 iflags;
>
> If you make the iflags bigger, what ripple effect does it have on the
> alignment of the other data structures? Consider reordering things if
> it helps filling holes.
>
I think you might be right. Possibly flipping iflags and sflags will be
enough but will have to check the exact alignment up to that point.
>>
>> /* State flags for kernel bookkeeping, unused by the hypervisor code */
>> u8 sflags;
>> @@ -541,6 +542,9 @@ struct kvm_vcpu_arch {
>> u64 pmscr_el1;
>> /* Self-hosted trace */
>> u64 trfcr_el1;
>> + /* exclude_guest settings for nVHE */
>> + struct kvm_etm_event etm_event;
>> +
>
> Spurious blank line. More importantly, how is that related to the
> trfcr_el1 field just above?
>
It's not really related, it's just the same place that the code that
runs at a similar time and does a similar thing has its data stored.
Something that you mentioned in another comment might be relevant here.
If I do it only in terms of registers that are saved and restored then I
don't need to save the etm event settings and the low level code doesn't
need to know anything about it.
>> } host_debug_state;
>>
>> /* VGIC state */
>> @@ -713,6 +717,8 @@ struct kvm_vcpu_arch {
>> #define DEBUG_STATE_SAVE_TRBE __vcpu_single_flag(iflags, BIT(6))
>> /* vcpu running in HYP context */
>> #define VCPU_HYP_CONTEXT __vcpu_single_flag(iflags, BIT(7))
>> +/* Save TRFCR and apply exclude_guest rules */
>> +#define DEBUG_STATE_SAVE_TRFCR __vcpu_single_flag(iflags, BIT(8))
>>
>> /* SVE enabled for host EL0 */
>> #define HOST_SVE_ENABLED __vcpu_single_flag(sflags, BIT(0))
>> @@ -1096,6 +1102,8 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu);
>> void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr);
>> void kvm_clr_pmu_events(u32 clr);
>> bool kvm_set_pmuserenr(u64 val);
>> +void kvm_set_etm_events(struct perf_event_attr *attr);
>> +void kvm_clr_etm_events(void);
>> #else
>> static inline void kvm_set_pmu_events(u32 set, struct perf_event_attr *attr) {}
>> static inline void kvm_clr_pmu_events(u32 clr) {}
>> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> index c0c050e53157..0faff57423c4 100644
>> --- a/arch/arm64/kvm/Makefile
>> +++ b/arch/arm64/kvm/Makefile
>> @@ -23,6 +23,7 @@ kvm-y += arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \
>> vgic/vgic-its.o vgic/vgic-debug.o
>>
>> kvm-$(CONFIG_HW_PERF_EVENTS) += pmu-emul.o pmu.o
>> +kvm-$(CONFIG_PERF_EVENTS) += etm.o
>>
>> always-y := hyp_constants.h hyp-constants.s
>>
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index b1a9d47fb2f3..7bd5975328a3 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -952,6 +952,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu)
>> kvm_vgic_flush_hwstate(vcpu);
>>
>> kvm_pmu_update_vcpu_events(vcpu);
>> + kvm_etm_update_vcpu_events(vcpu);
>>
>> /*
>> * Ensure we set mode to IN_GUEST_MODE after we disable
>> diff --git a/arch/arm64/kvm/etm.c b/arch/arm64/kvm/etm.c
>> new file mode 100644
>> index 000000000000..359c37745de2
>> --- /dev/null
>> +++ b/arch/arm64/kvm/etm.c
>> @@ -0,0 +1,48 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +#include <linux/kvm_host.h>
>> +
>> +#include <kvm/etm.h>
>> +
>> +static DEFINE_PER_CPU(struct kvm_etm_event, kvm_etm_events);
>> +
>> +struct kvm_etm_event *kvm_get_etm_event(void)
>> +{
>> + return this_cpu_ptr(&kvm_etm_events);
>> +}
>> +
>> +void kvm_etm_set_events(struct perf_event_attr *attr)
>> +{
>> + struct kvm_etm_event *etm_event;
>> +
>> + /*
>> + * Exclude guest option only requires extra work with nVHE.
>> + * Otherwise it works automatically with TRFCR_EL{1,2}
>> + */
>> + if (has_vhe())
>> + return;
>> +
>> + etm_event = kvm_get_etm_event();
>> +
>> + etm_event->exclude_guest = attr->exclude_guest;
>> + etm_event->exclude_host = attr->exclude_host;
>> + etm_event->exclude_kernel = attr->exclude_kernel;
>> + etm_event->exclude_user = attr->exclude_user;
>> +}
>> +EXPORT_SYMBOL_GPL(kvm_etm_set_events);
>> +
>> +void kvm_etm_clr_events(void)
>> +{
>> + struct kvm_etm_event *etm_event;
>> +
>> + if (has_vhe())
>> + return;
>> +
>> + etm_event = kvm_get_etm_event();
>> +
>> + etm_event->exclude_guest = false;
>> + etm_event->exclude_host = false;
>> + etm_event->exclude_kernel = false;
>> + etm_event->exclude_user = false;
>> +}
>> +EXPORT_SYMBOL_GPL(kvm_etm_clr_events);
>
> Does it really need its own compilation unit if we were to build it at
> all times?
>
I can probably throw it in arch/arm64/kvm/debug.c. I think I did that
originally but it had the conditional stuff around it so I made the new
file instead.
But yeah if I remove the conditional stuff it's probably cleaner to put
somewhere else.
>> diff --git a/include/kvm/etm.h b/include/kvm/etm.h
>> new file mode 100644
>> index 000000000000..95c4809fa2b0
>> --- /dev/null
>> +++ b/include/kvm/etm.h
>> @@ -0,0 +1,43 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef __KVM_DEBUG_H
>> +#define __KVM_DEBUG_H
>> +
>> +struct perf_event_attr;
>> +struct kvm_vcpu;
>> +
>> +#if IS_ENABLED(CONFIG_KVM) && IS_ENABLED(CONFIG_PERF_EVENTS)
>> +
>> +struct kvm_etm_event {
>> + bool exclude_host;
>> + bool exclude_guest;
>> + bool exclude_kernel;
>> + bool exclude_user;
>> +};
>> +
>> +struct kvm_etm_event *kvm_get_etm_event(void);
>> +void kvm_etm_clr_events(void);
>> +void kvm_etm_set_events(struct perf_event_attr *attr);
>> +
>> +/*
>> + * Updates the vcpu's view of the etm events for this cpu. Must be
>> + * called before every vcpu run after disabling interrupts, to ensure
>> + * that an interrupt cannot fire and update the structure.
>> + */
>> +#define kvm_etm_update_vcpu_events(vcpu) \
>> + do { \
>> + if (!has_vhe() && vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRFCR)) \
>> + vcpu->arch.host_debug_state.etm_event = *kvm_get_etm_event(); \
>> + } while (0)
>> +
>
> Why is it a macro and not a function, which would avoid exposing
> kvm_get_etm_event?
>
I think I saw that kvm_pmu_update_vcpu_events() was done that way, and I
couldn't inline it without some circular header issue so I made it a macro.
But yes a non inlined function works fine and then kvm_get_etm_event can
be hidden so I'll do that.
> Thanks,
>
> M.
>
Thanks for the review.
James
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH 2/3] arm64: KVM: Support exclude_guest for Coresight trace in nVHE
2023-08-04 10:13 [RFC PATCH 0/3] coresight: Support exclude_guest with Feat_TRF and nVHE James Clark
2023-08-04 10:13 ` [RFC PATCH 1/3] arm64: KVM: Add support for exclude_guest and exclude_host for ETM James Clark
@ 2023-08-04 10:13 ` James Clark
2023-08-08 11:04 ` Marc Zyngier
2023-08-04 10:13 ` [RFC PATCH 3/3] coresight: Support exclude_guest with Feat_TRF and nVHE James Clark
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: James Clark @ 2023-08-04 10:13 UTC (permalink / raw)
To: coresight, linux-arm-kernel, kvmarm
Cc: James Clark, Marc Zyngier, Oliver Upton, James Morse,
Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Will Deacon,
Mike Leach, Leo Yan, Alexander Shishkin, Anshuman Khandual,
Rob Herring, linux-kernel
Currently trace will always be generated in nVHE as long as TRBE isn't
being used. To allow filtering out guest trace, re-apply the filter
rules before switching to the guest.
The TRFCR restore function remains the same.
Signed-off-by: James Clark <james.clark@arm.com>
---
arch/arm64/kvm/debug.c | 7 ++++
arch/arm64/kvm/hyp/nvhe/debug-sr.c | 56 +++++++++++++++++++++++++++---
2 files changed, 59 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 8725291cb00a..ebb4db20a859 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -335,10 +335,17 @@ void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu)
if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_TraceBuffer_SHIFT) &&
!(read_sysreg_s(SYS_TRBIDR_EL1) & TRBIDR_EL1_P))
vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
+ /*
+ * Save TRFCR on nVHE if FEAT_TRF exists. This will be done in cases
+ * where DEBUG_STATE_SAVE_TRBE doesn't completely disable trace.
+ */
+ if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_TraceFilt_SHIFT))
+ vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRFCR);
}
void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu)
{
vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE);
vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
+ vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRFCR);
}
diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
index 4558c02eb352..0e8c85b29b92 100644
--- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
+++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
@@ -51,13 +51,17 @@ static void __debug_restore_spe(u64 pmscr_el1)
write_sysreg_s(pmscr_el1, SYS_PMSCR_EL1);
}
-static void __debug_save_trace(u64 *trfcr_el1)
+/*
+ * Save TRFCR and disable trace completely if TRBE is being used. Return true
+ * if trace was disabled.
+ */
+static bool __debug_save_trace(u64 *trfcr_el1)
{
*trfcr_el1 = 0;
/* Check if the TRBE is enabled */
if (!(read_sysreg_s(SYS_TRBLIMITR_EL1) & TRBLIMITR_EL1_E))
- return;
+ return false;
/*
* Prohibit trace generation while we are in guest.
* Since access to TRFCR_EL1 is trapped, the guest can't
@@ -68,6 +72,8 @@ static void __debug_save_trace(u64 *trfcr_el1)
isb();
/* Drain the trace buffer to memory */
tsb_csync();
+
+ return true;
}
static void __debug_restore_trace(u64 trfcr_el1)
@@ -79,14 +85,55 @@ static void __debug_restore_trace(u64 trfcr_el1)
write_sysreg_s(trfcr_el1, SYS_TRFCR_EL1);
}
+#if IS_ENABLED(CONFIG_PERF_EVENTS)
+static inline void __debug_save_trfcr(struct kvm_vcpu *vcpu)
+{
+ u64 trfcr;
+ struct kvm_etm_event etm_event = vcpu->arch.host_debug_state.etm_event;
+
+ /* No change if neither are excluded */
+ if (!etm_event.exclude_guest && !etm_event.exclude_host) {
+ /* Zeroing prevents restoring a stale value */
+ vcpu->arch.host_debug_state.trfcr_el1 = 0;
+ return;
+ }
+
+ trfcr = read_sysreg_s(SYS_TRFCR_EL1);
+ vcpu->arch.host_debug_state.trfcr_el1 = trfcr;
+
+ if (etm_event.exclude_guest) {
+ trfcr &= ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE);
+ } else {
+ /*
+ * If host was excluded then EL0 and ELx tracing bits will
+ * already be cleared so they need to be set now for the guest.
+ */
+ trfcr |= etm_event.exclude_kernel ? 0 : TRFCR_ELx_ExTRE;
+ trfcr |= etm_event.exclude_user ? 0 : TRFCR_ELx_E0TRE;
+ }
+ write_sysreg_s(trfcr, SYS_TRFCR_EL1);
+}
+#else
+static inline void __debug_save_trfcr(struct kvm_vcpu *vcpu) {}
+#endif
+
void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu)
{
+ bool trc_disabled = false;
+
/* Disable and flush SPE data generation */
if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_SPE))
__debug_save_spe(&vcpu->arch.host_debug_state.pmscr_el1);
/* Disable and flush Self-Hosted Trace generation */
if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
- __debug_save_trace(&vcpu->arch.host_debug_state.trfcr_el1);
+ trc_disabled = __debug_save_trace(&vcpu->arch.host_debug_state.trfcr_el1);
+
+ /*
+ * As long as trace wasn't completely disabled due to use of TRBE,
+ * TRFCR can be saved and the exclude_guest rules applied.
+ */
+ if (!trc_disabled && vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRFCR))
+ __debug_save_trfcr(vcpu);
}
void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
@@ -98,7 +145,8 @@ void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu)
{
if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_SPE))
__debug_restore_spe(vcpu->arch.host_debug_state.pmscr_el1);
- if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
+ if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE) ||
+ vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRFCR))
__debug_restore_trace(vcpu->arch.host_debug_state.trfcr_el1);
}
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [RFC PATCH 2/3] arm64: KVM: Support exclude_guest for Coresight trace in nVHE
2023-08-04 10:13 ` [RFC PATCH 2/3] arm64: KVM: Support exclude_guest for Coresight trace in nVHE James Clark
@ 2023-08-08 11:04 ` Marc Zyngier
2023-08-09 14:20 ` James Clark
0 siblings, 1 reply; 12+ messages in thread
From: Marc Zyngier @ 2023-08-08 11:04 UTC (permalink / raw)
To: James Clark
Cc: coresight, linux-arm-kernel, kvmarm, Oliver Upton, James Morse,
Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Will Deacon,
Mike Leach, Leo Yan, Alexander Shishkin, Anshuman Khandual,
Rob Herring, linux-kernel
On Fri, 04 Aug 2023 11:13:12 +0100,
James Clark <james.clark@arm.com> wrote:
>
> Currently trace will always be generated in nVHE as long as TRBE isn't
> being used. To allow filtering out guest trace, re-apply the filter
> rules before switching to the guest.
>
> The TRFCR restore function remains the same.
>
> Signed-off-by: James Clark <james.clark@arm.com>
> ---
> arch/arm64/kvm/debug.c | 7 ++++
> arch/arm64/kvm/hyp/nvhe/debug-sr.c | 56 +++++++++++++++++++++++++++---
> 2 files changed, 59 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 8725291cb00a..ebb4db20a859 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -335,10 +335,17 @@ void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu)
> if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_TraceBuffer_SHIFT) &&
> !(read_sysreg_s(SYS_TRBIDR_EL1) & TRBIDR_EL1_P))
> vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
> + /*
> + * Save TRFCR on nVHE if FEAT_TRF exists. This will be done in cases
> + * where DEBUG_STATE_SAVE_TRBE doesn't completely disable trace.
> + */
> + if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_TraceFilt_SHIFT))
> + vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRFCR);
> }
>
> void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu)
> {
> vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE);
> vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
> + vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRFCR);
> }
> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> index 4558c02eb352..0e8c85b29b92 100644
> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> @@ -51,13 +51,17 @@ static void __debug_restore_spe(u64 pmscr_el1)
> write_sysreg_s(pmscr_el1, SYS_PMSCR_EL1);
> }
>
> -static void __debug_save_trace(u64 *trfcr_el1)
> +/*
> + * Save TRFCR and disable trace completely if TRBE is being used. Return true
> + * if trace was disabled.
> + */
> +static bool __debug_save_trace(u64 *trfcr_el1)
> {
> *trfcr_el1 = 0;
>
> /* Check if the TRBE is enabled */
> if (!(read_sysreg_s(SYS_TRBLIMITR_EL1) & TRBLIMITR_EL1_E))
> - return;
> + return false;
While you're refactoring this code, please move the zeroing of
*trfcr_el1 under the if statement.
> /*
> * Prohibit trace generation while we are in guest.
> * Since access to TRFCR_EL1 is trapped, the guest can't
> @@ -68,6 +72,8 @@ static void __debug_save_trace(u64 *trfcr_el1)
> isb();
> /* Drain the trace buffer to memory */
> tsb_csync();
> +
> + return true;
> }
>
> static void __debug_restore_trace(u64 trfcr_el1)
> @@ -79,14 +85,55 @@ static void __debug_restore_trace(u64 trfcr_el1)
> write_sysreg_s(trfcr_el1, SYS_TRFCR_EL1);
> }
>
> +#if IS_ENABLED(CONFIG_PERF_EVENTS)
As previously stated, just always compile this. There shouldn't be
anything here that's so large that it becomes a candidate for
exclusion. Hell, even the whole of NV+pKVM are permanent features,
even of most people won't use *any* of that.
> +static inline void __debug_save_trfcr(struct kvm_vcpu *vcpu)
> +{
> + u64 trfcr;
> + struct kvm_etm_event etm_event = vcpu->arch.host_debug_state.etm_event;
> +
> + /* No change if neither are excluded */
> + if (!etm_event.exclude_guest && !etm_event.exclude_host) {
> + /* Zeroing prevents restoring a stale value */
> + vcpu->arch.host_debug_state.trfcr_el1 = 0;
I find this "zero means do nothing" part very odd. I can see it is
already done, but I really dislike this sort of assumption to avoid
writing to a register.
I'd really prefer we track another version of TRFCR_EL1, compare host
and guest, and decide to avoid writing if they are equal. At least, it
would be readable.
And in the end, expressing *everything* in terms of the register would
really help, instead of the exclude_* stuff that has no place in the
low-level arch code.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFC PATCH 2/3] arm64: KVM: Support exclude_guest for Coresight trace in nVHE
2023-08-08 11:04 ` Marc Zyngier
@ 2023-08-09 14:20 ` James Clark
0 siblings, 0 replies; 12+ messages in thread
From: James Clark @ 2023-08-09 14:20 UTC (permalink / raw)
To: Marc Zyngier
Cc: coresight, linux-arm-kernel, kvmarm, Oliver Upton, James Morse,
Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Will Deacon,
Mike Leach, Leo Yan, Alexander Shishkin, Anshuman Khandual,
Rob Herring, linux-kernel
On 08/08/2023 12:04, Marc Zyngier wrote:
> On Fri, 04 Aug 2023 11:13:12 +0100,
> James Clark <james.clark@arm.com> wrote:
>>
>> Currently trace will always be generated in nVHE as long as TRBE isn't
>> being used. To allow filtering out guest trace, re-apply the filter
>> rules before switching to the guest.
>>
>> The TRFCR restore function remains the same.
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>> arch/arm64/kvm/debug.c | 7 ++++
>> arch/arm64/kvm/hyp/nvhe/debug-sr.c | 56 +++++++++++++++++++++++++++---
>> 2 files changed, 59 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
>> index 8725291cb00a..ebb4db20a859 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -335,10 +335,17 @@ void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu)
>> if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_TraceBuffer_SHIFT) &&
>> !(read_sysreg_s(SYS_TRBIDR_EL1) & TRBIDR_EL1_P))
>> vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
>> + /*
>> + * Save TRFCR on nVHE if FEAT_TRF exists. This will be done in cases
>> + * where DEBUG_STATE_SAVE_TRBE doesn't completely disable trace.
>> + */
>> + if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_TraceFilt_SHIFT))
>> + vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRFCR);
>> }
>>
>> void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu)
>> {
>> vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE);
>> vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
>> + vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRFCR);
>> }
>> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> index 4558c02eb352..0e8c85b29b92 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> @@ -51,13 +51,17 @@ static void __debug_restore_spe(u64 pmscr_el1)
>> write_sysreg_s(pmscr_el1, SYS_PMSCR_EL1);
>> }
>>
>> -static void __debug_save_trace(u64 *trfcr_el1)
>> +/*
>> + * Save TRFCR and disable trace completely if TRBE is being used. Return true
>> + * if trace was disabled.
>> + */
>> +static bool __debug_save_trace(u64 *trfcr_el1)
>> {
>> *trfcr_el1 = 0;
>>
>> /* Check if the TRBE is enabled */
>> if (!(read_sysreg_s(SYS_TRBLIMITR_EL1) & TRBLIMITR_EL1_E))
>> - return;
>> + return false;
>
> While you're refactoring this code, please move the zeroing of
> *trfcr_el1 under the if statement.
>
>> /*
>> * Prohibit trace generation while we are in guest.
>> * Since access to TRFCR_EL1 is trapped, the guest can't
>> @@ -68,6 +72,8 @@ static void __debug_save_trace(u64 *trfcr_el1)
>> isb();
>> /* Drain the trace buffer to memory */
>> tsb_csync();
>> +
>> + return true;
>> }
>>
>> static void __debug_restore_trace(u64 trfcr_el1)
>> @@ -79,14 +85,55 @@ static void __debug_restore_trace(u64 trfcr_el1)
>> write_sysreg_s(trfcr_el1, SYS_TRFCR_EL1);
>> }
>>
>> +#if IS_ENABLED(CONFIG_PERF_EVENTS)
>
> As previously stated, just always compile this. There shouldn't be
> anything here that's so large that it becomes a candidate for
> exclusion. Hell, even the whole of NV+pKVM are permanent features,
> even of most people won't use *any* of that.
>
>> +static inline void __debug_save_trfcr(struct kvm_vcpu *vcpu)
>> +{
>> + u64 trfcr;
>> + struct kvm_etm_event etm_event = vcpu->arch.host_debug_state.etm_event;
>> +
>> + /* No change if neither are excluded */
>> + if (!etm_event.exclude_guest && !etm_event.exclude_host) {
>> + /* Zeroing prevents restoring a stale value */
>> + vcpu->arch.host_debug_state.trfcr_el1 = 0;
>
> I find this "zero means do nothing" part very odd. I can see it is
> already done, but I really dislike this sort of assumption to avoid
> writing to a register.
>
> I'd really prefer we track another version of TRFCR_EL1, compare host
> and guest, and decide to avoid writing if they are equal. At least, it
> would be readable.
>
> And in the end, expressing *everything* in terms of the register would
> really help, instead of the exclude_* stuff that has no place in the
> low-level arch code.
>
Yep, I agree with all of the above, I can make these changes for the
next version. I just want to clarify your point about disabling trace
for protected guests when not in debug mode that I asked about in the
review on patch 1.
> Thanks,
>
> M.
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH 3/3] coresight: Support exclude_guest with Feat_TRF and nVHE
2023-08-04 10:13 [RFC PATCH 0/3] coresight: Support exclude_guest with Feat_TRF and nVHE James Clark
2023-08-04 10:13 ` [RFC PATCH 1/3] arm64: KVM: Add support for exclude_guest and exclude_host for ETM James Clark
2023-08-04 10:13 ` [RFC PATCH 2/3] arm64: KVM: Support exclude_guest for Coresight trace in nVHE James Clark
@ 2023-08-04 10:13 ` James Clark
2023-08-04 19:09 ` [RFC PATCH 0/3] " Marc Zyngier
2023-08-05 10:14 ` Suzuki K Poulose
4 siblings, 0 replies; 12+ messages in thread
From: James Clark @ 2023-08-04 10:13 UTC (permalink / raw)
To: coresight, linux-arm-kernel, kvmarm
Cc: James Clark, Marc Zyngier, Oliver Upton, James Morse,
Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Will Deacon,
Mike Leach, Leo Yan, Alexander Shishkin, Anshuman Khandual,
Rob Herring, linux-kernel
With nVHE the filters need to be applied before switching to the guest,
so supply the per-cpu filter status to KVM.
Signed-off-by: James Clark <james.clark@arm.com>
---
drivers/hwtracing/coresight/coresight-etm-perf.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 5ca6278baff4..f78f05e656f5 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -9,6 +9,7 @@
#include <linux/coresight-pmu.h>
#include <linux/cpumask.h>
#include <linux/device.h>
+#include <linux/kvm_host.h>
#include <linux/list.h>
#include <linux/mm.h>
#include <linux/init.h>
@@ -510,6 +511,7 @@ static void etm_event_start(struct perf_event *event, int flags)
}
out:
+ kvm_etm_set_events(&event->attr);
/* Tell the perf core the event is alive */
event->hw.state = 0;
/* Save the event_data for this ETM */
@@ -627,6 +629,8 @@ static void etm_event_stop(struct perf_event *event, int mode)
/* Disabling the path make its elements available to other sessions */
coresight_disable_path(path);
+
+ kvm_etm_clr_events();
}
static int etm_event_add(struct perf_event *event, int mode)
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/3] coresight: Support exclude_guest with Feat_TRF and nVHE
2023-08-04 10:13 [RFC PATCH 0/3] coresight: Support exclude_guest with Feat_TRF and nVHE James Clark
` (2 preceding siblings ...)
2023-08-04 10:13 ` [RFC PATCH 3/3] coresight: Support exclude_guest with Feat_TRF and nVHE James Clark
@ 2023-08-04 19:09 ` Marc Zyngier
2023-08-05 10:28 ` Suzuki K Poulose
2023-08-05 10:14 ` Suzuki K Poulose
4 siblings, 1 reply; 12+ messages in thread
From: Marc Zyngier @ 2023-08-04 19:09 UTC (permalink / raw)
To: James Clark
Cc: coresight, linux-arm-kernel, kvmarm, Oliver Upton, James Morse,
Suzuki K Poulose, Zenghui Yu, Catalin Marinas, Will Deacon,
Mike Leach, Leo Yan, Alexander Shishkin, Anshuman Khandual,
Rob Herring, linux-kernel
On Fri, 04 Aug 2023 11:13:10 +0100,
James Clark <james.clark@arm.com> wrote:
>
> Hi,
>
> I'm looking for help in testing this and for feedback on whether it's
> useful to anyone. Testing it requires hardware that has Feat_TRF (v8.4)
> but no TRBE. This is because TRBE usage is disabled in nVHE guests.
>
> I don't currently have any access to any hardware, and the FVP model
> can only do self hosted trace using TRBE.
>
> Currently with nVHE you would always get trace from guests, and
> filtering out isn't possible without this patchset. In comparison, with
> VHE guests, they never generate guest trace without [1]. I think the
> existence of trace rather than lack of could suggest that this change is
> less useful than [1]. Also the restricted set of hardware that it works
> on supports that too.
It'd be nice to have some sort of feature parity, but it seems like a
vanishingly small target of users having access to an ETM sink.
>
> Apart from compilation and checking that the exclude guest settings
> are correctly programmed on guest switch, this is untested by me.
I'll have a look at the series, but none of my HW fits in this
description (my ARMv8.4+ boxes don't have any form of tracing).
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [RFC PATCH 0/3] coresight: Support exclude_guest with Feat_TRF and nVHE
2023-08-04 19:09 ` [RFC PATCH 0/3] " Marc Zyngier
@ 2023-08-05 10:28 ` Suzuki K Poulose
2023-08-08 11:06 ` Marc Zyngier
0 siblings, 1 reply; 12+ messages in thread
From: Suzuki K Poulose @ 2023-08-05 10:28 UTC (permalink / raw)
To: Marc Zyngier, James Clark
Cc: coresight, linux-arm-kernel, kvmarm, Oliver Upton, James Morse,
Zenghui Yu, Catalin Marinas, Will Deacon, Mike Leach, Leo Yan,
Alexander Shishkin, Anshuman Khandual, Rob Herring, linux-kernel
Hi Marc
On 04/08/2023 20:09, Marc Zyngier wrote:
> On Fri, 04 Aug 2023 11:13:10 +0100,
> James Clark <james.clark@arm.com> wrote:
>>
>> Hi,
>>
>> I'm looking for help in testing this and for feedback on whether it's
>> useful to anyone. Testing it requires hardware that has Feat_TRF (v8.4)
>> but no TRBE. This is because TRBE usage is disabled in nVHE guests.
>>
>> I don't currently have any access to any hardware, and the FVP model
>> can only do self hosted trace using TRBE.
>>
>> Currently with nVHE you would always get trace from guests, and
>> filtering out isn't possible without this patchset. In comparison, with
>> VHE guests, they never generate guest trace without [1]. I think the
>> existence of trace rather than lack of could suggest that this change is
>> less useful than [1]. Also the restricted set of hardware that it works
>> on supports that too.
>
> It'd be nice to have some sort of feature parity, but it seems like a
> vanishingly small target of users having access to an ETM sink.
>
>>
>> Apart from compilation and checking that the exclude guest settings
>> are correctly programmed on guest switch, this is untested by me.
>
> I'll have a look at the series, but none of my HW fits in this
> description (my ARMv8.4+ boxes don't have any form of tracing).
While I have your attention, we have another series that manages the
trace filtering for Guests on VHE, completely within the Coresight etm4x
driver here [0]. I personally think, it is good to have the guest
filtering for both nVHE and VHE under the KVM control, like we do
in this series. I would like your opinion on this.
[0] https://lkml.kernel.org/r/20230804085219.260790-1-james.clark@arm.com
Suzuki
>
> Thanks,
>
> M.
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/3] coresight: Support exclude_guest with Feat_TRF and nVHE
2023-08-05 10:28 ` Suzuki K Poulose
@ 2023-08-08 11:06 ` Marc Zyngier
0 siblings, 0 replies; 12+ messages in thread
From: Marc Zyngier @ 2023-08-08 11:06 UTC (permalink / raw)
To: Suzuki K Poulose
Cc: James Clark, coresight, linux-arm-kernel, kvmarm, Oliver Upton,
James Morse, Zenghui Yu, Catalin Marinas, Will Deacon, Mike Leach,
Leo Yan, Alexander Shishkin, Anshuman Khandual, Rob Herring,
linux-kernel
On Sat, 05 Aug 2023 11:28:39 +0100,
Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>
> Hi Marc
>
> On 04/08/2023 20:09, Marc Zyngier wrote:
> > On Fri, 04 Aug 2023 11:13:10 +0100,
> > James Clark <james.clark@arm.com> wrote:
> >>
> >> Hi,
> >>
> >> I'm looking for help in testing this and for feedback on whether it's
> >> useful to anyone. Testing it requires hardware that has Feat_TRF (v8.4)
> >> but no TRBE. This is because TRBE usage is disabled in nVHE guests.
> >>
> >> I don't currently have any access to any hardware, and the FVP model
> >> can only do self hosted trace using TRBE.
> >>
> >> Currently with nVHE you would always get trace from guests, and
> >> filtering out isn't possible without this patchset. In comparison, with
> >> VHE guests, they never generate guest trace without [1]. I think the
> >> existence of trace rather than lack of could suggest that this change is
> >> less useful than [1]. Also the restricted set of hardware that it works
> >> on supports that too.
> >
> > It'd be nice to have some sort of feature parity, but it seems like a
> > vanishingly small target of users having access to an ETM sink.
> >
> >>
> >> Apart from compilation and checking that the exclude guest settings
> >> are correctly programmed on guest switch, this is untested by me.
> >
> > I'll have a look at the series, but none of my HW fits in this
> > description (my ARMv8.4+ boxes don't have any form of tracing).
>
> While I have your attention, we have another series that manages the
> trace filtering for Guests on VHE, completely within the Coresight etm4x
> driver here [0]. I personally think, it is good to have the guest
> filtering for both nVHE and VHE under the KVM control, like we do
> in this series. I would like your opinion on this.
I just quickly reviewed the first two patches of the nVHE series, and
I think the shape is a bit wrong. I can absolutely see the interest of
preventing extra tracing when a guest is running, but the way this is
currently plugged makes it hard to reason about it.
> [0] https://lkml.kernel.org/r/20230804085219.260790-1-james.clark@arm.com
I'll try to have a look at that one later.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/3] coresight: Support exclude_guest with Feat_TRF and nVHE
2023-08-04 10:13 [RFC PATCH 0/3] coresight: Support exclude_guest with Feat_TRF and nVHE James Clark
` (3 preceding siblings ...)
2023-08-04 19:09 ` [RFC PATCH 0/3] " Marc Zyngier
@ 2023-08-05 10:14 ` Suzuki K Poulose
4 siblings, 0 replies; 12+ messages in thread
From: Suzuki K Poulose @ 2023-08-05 10:14 UTC (permalink / raw)
To: James Clark, coresight, linux-arm-kernel, kvmarm
Cc: Marc Zyngier, Oliver Upton, James Morse, Zenghui Yu,
Catalin Marinas, Will Deacon, Mike Leach, Leo Yan,
Alexander Shishkin, Anshuman Khandual, Rob Herring, linux-kernel,
Steve Clevenger, Tanmay Jagdale, Ganapatrao Kulkarni
Cc: Ganpatrao, Steve, Tanmay
On 04/08/2023 11:13, James Clark wrote:
> Hi,
>
> I'm looking for help in testing this and for feedback on whether it's
> useful to anyone. Testing it requires hardware that has Feat_TRF (v8.4)
> but no TRBE. This is because TRBE usage is disabled in nVHE guests.
>
> I don't currently have any access to any hardware, and the FVP model
> can only do self hosted trace using TRBE.
If you have a v8.4+ (and not v9) HW, please could you give this a spin ?
Suzuki
>
> Currently with nVHE you would always get trace from guests, and
> filtering out isn't possible without this patchset. In comparison, with
> VHE guests, they never generate guest trace without [1]. I think the
> existence of trace rather than lack of could suggest that this change is
> less useful than [1]. Also the restricted set of hardware that it works
> on supports that too.
>
> Apart from compilation and checking that the exclude guest settings
> are correctly programmed on guest switch, this is untested by me.
>
> Applies to kvmarm/next (3b4e3afb2032)
>
> [1]: https://lore.kernel.org/linux-arm-kernel/20230804085219.260790-3-james.clark@arm.com/
>
> James Clark (3):
> arm64: KVM: Add support for exclude_guest and exclude_host for ETM
> arm64: KVM: Support exclude_guest for Coresight trace in nVHE
> coresight: Support exclude_guest with Feat_TRF and nVHE
>
> arch/arm64/include/asm/kvm_host.h | 10 +++-
> arch/arm64/kvm/Makefile | 1 +
> arch/arm64/kvm/arm.c | 1 +
> arch/arm64/kvm/debug.c | 7 +++
> arch/arm64/kvm/etm.c | 48 ++++++++++++++++
> arch/arm64/kvm/hyp/nvhe/debug-sr.c | 56 +++++++++++++++++--
> .../hwtracing/coresight/coresight-etm-perf.c | 4 ++
> include/kvm/etm.h | 43 ++++++++++++++
> 8 files changed, 165 insertions(+), 5 deletions(-)
> create mode 100644 arch/arm64/kvm/etm.c
> create mode 100644 include/kvm/etm.h
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread