Re: [PATCH v6 5/8] arm64: KVM: Add iflag for FEAT_TRF
flat view
From: Marc Zyngier <maz@kernel.org>
Date: 2024-02-26 13:35:32
Also in:
kvmarm, lkml
Subsystem:
arm64 port (aarch64 architecture), kernel virtual machine for arm64 (kvm/arm64), the rest · Maintainers:
Catalin Marinas, Will Deacon, Marc Zyngier, Oliver Upton, Linus Torvalds
On Mon, 26 Feb 2024 11:30:33 +0000, James Clark [off-list ref] wrote:
Add an extra iflag to signify if the TRFCR register is accessible.
That's not what this flag means: it indicates whether TRFCR needs to be saved. At lease that's what the name suggests.
Because TRBE requires FEAT_TRF, DEBUG_STATE_SAVE_TRBE still has the same behavior even though it's only set when FEAT_TRF is present.
This sentence seems completely out of context, because you didn't explain that you were making TRBE *conditional* on TRF being implemented, as per the architecture requirements.
The following holes are left in struct kvm_vcpu_arch, but there aren't enough other 8 bit fields to rearrange it to leave any hole smaller than 7 bytes: u8 cflags; /* 2292 1 */ /* XXX 1 byte hole, try to pack */ u16 iflags; /* 2294 2 */ u8 sflags; /* 2296 1 */ bool pause; /* 2297 1 */ /* XXX 6 bytes hole, try to pack */
I don't think that's particularly useful in a commit message, but more relevant to the cover letter. However, see below.
quoted hunk ↗ jump to hunk
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> Signed-off-by: James Clark <redacted> --- arch/arm64/include/asm/kvm_host.h | 4 +++- arch/arm64/kvm/debug.c | 24 ++++++++++++++++++++---- 2 files changed, 23 insertions(+), 5 deletions(-)diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 21c57b812569..85b5477bd1b4 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h@@ -569,7 +569,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;@@ -779,6 +779,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 trace filter controls */ +#define DEBUG_STATE_SAVE_TRFCR __vcpu_single_flag(iflags, BIT(8))
I'd rather you cherry-pick [1] and avoid expanding the iflags. [1] https://lore.kernel.org/r/20240226100601.2379693-4-maz@kernel.org (local) Now, I think the whole SPE/TRBE/TRCR flag management should be improved, see below.
quoted hunk ↗ jump to hunk
/* SVE enabled for host EL0 */ #define HOST_SVE_ENABLED __vcpu_single_flag(sflags, BIT(0))diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c index ce8886122ed3..49a13e72ddd2 100644 --- a/arch/arm64/kvm/debug.c +++ b/arch/arm64/kvm/debug.c@@ -332,14 +332,30 @@ void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu) !(read_sysreg_s(SYS_PMBIDR_EL1) & BIT(PMBIDR_EL1_P_SHIFT))) vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_SPE); - /* Check if we have TRBE implemented and available at the host */ - 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); + /* + * Set SAVE_TRFCR flag if FEAT_TRF (TraceFilt) exists. This flag + * signifies that the exclude_host/exclude_guest settings of any active + * host Perf session on a core running a VCPU can be written into + * TRFCR_EL1 on guest switch. + */ + if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_TraceFilt_SHIFT)) { + vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRFCR);
Can we avoid doing this unconditionally? It only makes sense to save the trace crud if it is going to be changed, right?
quoted hunk ↗ jump to hunk
+ /* + * Check if we have TRBE implemented and available at the host. + * If it's in use at the time of guest switch then trace will + * need to be completely disabled. The architecture mandates + * FEAT_TRF with TRBE, so we only need to check for TRBE after + * TRF. + */ + 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); + } } 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); }
Dealing with flags that are strongly coupled in a disjoined way a
pretty bad idea. Look at the generated code, and realise we flip the
preempt flag on each access.
Can we do better? You bet. The vcpu_{set,clear}_flags infrastructure
is capable of dealing with multiple flags at once, as demonstrated by
the way we deal with exception encoding.
Something like:
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index addf79ba8fa0..3e50e535fdd4 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h@@ -885,6 +885,10 @@ struct kvm_vcpu_arch { #define DEBUG_STATE_SAVE_SPE __vcpu_single_flag(iflags, BIT(5)) /* Save TRBE context if active */ #define DEBUG_STATE_SAVE_TRBE __vcpu_single_flag(iflags, BIT(6)) +/* Save Trace Filter Controls */ +#define DEBUG_STATE_SAVE_TRFCR __vcpu_single_flag(iflags, BIT(7)) +/* Global debug mask */ +#define DEBUG_STATE_SAVE_MASK __vcpu_single_flag(iflags, GENMASK(7, 5)) /* SVE enabled for host EL0 */ #define HOST_SVE_ENABLED __vcpu_single_flag(sflags, BIT(0))
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 8725291cb00a..f9b197a00582 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c@@ -339,6 +339,6 @@ void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu) 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); + if (!has_vhe()) + vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_MASK); }
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