Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 08 Aug 2023 12:04:19 +0100
Message-ID: <871qgdre18.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     James Clark <james.clark@arm.com>
Cc:     coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org,
        kvmarm@lists.linux.dev, Oliver Upton <oliver.upton@linux.dev>,
        James Morse <james.morse@arm.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Zenghui Yu <yuzenghui@huawei.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Mike Leach <mike.leach@linaro.org>,
        Leo Yan <leo.yan@linaro.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Anshuman Khandual <anshuman.khandual@arm.com>,
        Rob Herring <robh@kernel.org>, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 2/3] arm64: KVM: Support exclude_guest for Coresight trace in nVHE
In-Reply-To: <20230804101317.460697-3-james.clark@arm.com>
References: <20230804101317.460697-1-james.clark@arm.com>
        <20230804101317.460697-3-james.clark@arm.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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.