On 04/12/2023 09:48, Marc Zyngier wrote:

On Thu, 19 Oct 2023 17:55:01 +0100, James Clark james.clark@arm.com wrote:

...

Add an extra iflag to signify if the TRFCR register is accessible. 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.

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

Signed-off-by: James Clark james.clark@arm.com

arch/arm64/include/asm/kvm_host.h | 4 +++- arch/arm64/kvm/debug.c | 22 ++++++++++++++++++---- 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 7c82927ddaf2..0f0bf8e641bd 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -535,7 +535,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; @@ -741,6 +741,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)) /* 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..20cdd40b3c42 100644 --- a/arch/arm64/kvm/debug.c +++ b/arch/arm64/kvm/debug.c @@ -331,14 +331,28 @@ 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);
  

• /*

• * Save TRFCR on nVHE if FEAT_TRF (TraceFilt) exists. This will be
  

• * done in cases where use of TRBE doesn't completely disable trace and
  

• * handles the exclude_host/exclude_guest rules of the trace session.
  

This comment provides zero information. What will be done? Under which conditions? What are the rules?

...

• */
  

• if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_TraceFilt_SHIFT)) {

• vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_TRFCR);
  

• /*
  

•  * Check if we have TRBE implemented and available at the host. If it's
  

•  * in use at the time of guest switch it will need to be disabled and
  

•  * then restored. 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);
  

• }

Multiple questions:

• Why is it safe to trust the local CPU's capability rather than the consolidated view from the cpufeature infrastructure?

The coresight driver is capable of handling heterogeneous systems. i.e., some CPUs may not have FEAT_TRF or FEAT_TRBE. This could be for various reasons (e.g., CPU Erratum disabling TRBE, though not used now). It already needs to treat each CPU differently (due to the capabilities of the ETM).

That said, we could reject events with exclude_guest/exclude_host flags on CPUs that do not support FEAT_TRF. But that doesn't solve checking the local capability.

Suzuki

• Why defer the saving of the registers if there are no changes made to them in the interval?

Thanks,

M.