Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 30 Mar 2021 11:12:22 +0100
Message-ID: <87r1jxq7ax.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Suzuki K Poulose <suzuki.poulose@arm.com>
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        coresight@lists.linaro.org, mathieu.poirier@linaro.org,
        mike.leach@linaro.org, leo.yan@linaro.org,
        anshuman.khandual@arm.com, catalin.marinas@arm.com,
        Will Deacon <will@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexandru Elisei <Alexandru.Elisei@arm.com>
Subject: Re: [PATCH v5 07/19] arm64: kvm: Enable access to TRBE support for host
In-Reply-To: <20210323120647.454211-8-suzuki.poulose@arm.com>
References: <20210323120647.454211-1-suzuki.poulose@arm.com>
        <20210323120647.454211-8-suzuki.poulose@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/27.1
 (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

Hi Suzuki,

[+ Alex]

On Tue, 23 Mar 2021 12:06:35 +0000,
Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
> 
> For a nvhe host, the EL2 must allow the EL1&0 translation
> regime for TraceBuffer (MDCR_EL2.E2TB == 0b11). This must
> be saved/restored over a trip to the guest. Also, before
> entering the guest, we must flush any trace data if the
> TRBE was enabled. And we must prohibit the generation
> of trace while we are in EL1 by clearing the TRFCR_EL1.
> 
> For vhe, the EL2 must prevent the EL1 access to the Trace
> Buffer.
> 
> Cc: Will Deacon <will@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/include/asm/el2_setup.h | 13 +++++++++
>  arch/arm64/include/asm/kvm_arm.h   |  2 ++
>  arch/arm64/include/asm/kvm_host.h  |  2 ++
>  arch/arm64/kernel/hyp-stub.S       |  3 ++-
>  arch/arm64/kvm/debug.c             |  6 ++---
>  arch/arm64/kvm/hyp/nvhe/debug-sr.c | 42 ++++++++++++++++++++++++++++++
>  arch/arm64/kvm/hyp/nvhe/switch.c   |  1 +
>  7 files changed, 65 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
> index d77d358f9395..bda918948471 100644
> --- a/arch/arm64/include/asm/el2_setup.h
> +++ b/arch/arm64/include/asm/el2_setup.h
> @@ -65,6 +65,19 @@
>  						// use EL1&0 translation.
>  
>  .Lskip_spe_\@:
> +	/* Trace buffer */
> +	ubfx	x0, x1, #ID_AA64DFR0_TRBE_SHIFT, #4
> +	cbz	x0, .Lskip_trace_\@		// Skip if TraceBuffer is not present
> +
> +	mrs_s	x0, SYS_TRBIDR_EL1
> +	and	x0, x0, TRBIDR_PROG
> +	cbnz	x0, .Lskip_trace_\@		// If TRBE is available at EL2
> +
> +	mov	x0, #(MDCR_EL2_E2TB_MASK << MDCR_EL2_E2TB_SHIFT)
> +	orr	x2, x2, x0			// allow the EL1&0 translation
> +						// to own it.
> +
> +.Lskip_trace_\@:
>  	msr	mdcr_el2, x2			// Configure debug traps
>  .endm
>  
> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index 94d4025acc0b..692c9049befa 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -278,6 +278,8 @@
>  #define CPTR_EL2_DEFAULT	CPTR_EL2_RES1
>  
>  /* Hyp Debug Configuration Register bits */
> +#define MDCR_EL2_E2TB_MASK	(UL(0x3))
> +#define MDCR_EL2_E2TB_SHIFT	(UL(24))

Where are these bits defined? DDI0487G_a has them as RES0.

>  #define MDCR_EL2_TTRF		(1 << 19)
>  #define MDCR_EL2_TPMS		(1 << 14)
>  #define MDCR_EL2_E2PB_MASK	(UL(0x3))
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 3d10e6527f7d..80d0a1a82a4c 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -315,6 +315,8 @@ struct kvm_vcpu_arch {
>  		struct kvm_guest_debug_arch regs;
>  		/* Statistical profiling extension */
>  		u64 pmscr_el1;
> +		/* Self-hosted trace */
> +		u64 trfcr_el1;
>  	} host_debug_state;
>  
>  	/* VGIC state */
> diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S
> index 5eccbd62fec8..05d25e645b46 100644
> --- a/arch/arm64/kernel/hyp-stub.S
> +++ b/arch/arm64/kernel/hyp-stub.S
> @@ -115,9 +115,10 @@ SYM_CODE_START_LOCAL(mutate_to_vhe)
>  	mrs_s	x0, SYS_VBAR_EL12
>  	msr	vbar_el1, x0
>  
> -	// Use EL2 translations for SPE and disable access from EL1
> +	// Use EL2 translations for SPE & TRBE and disable access from EL1
>  	mrs	x0, mdcr_el2
>  	bic	x0, x0, #(MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT)
> +	bic	x0, x0, #(MDCR_EL2_E2TB_MASK << MDCR_EL2_E2TB_SHIFT)
>  	msr	mdcr_el2, x0
>  
>  	// Transfer the MM state from EL1 to EL2
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index dbc890511631..7b16f42d39f4 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -89,7 +89,7 @@ void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu)
>   *  - Debug ROM Address (MDCR_EL2_TDRA)
>   *  - OS related registers (MDCR_EL2_TDOSA)
>   *  - Statistical profiler (MDCR_EL2_TPMS/MDCR_EL2_E2PB)
> - *  - Self-hosted Trace Filter controls (MDCR_EL2_TTRF)
> + *  - Self-hosted Trace (MDCR_EL2_TTRF/MDCR_EL2_E2TB)

For the record, this is likely to conflict with [1], although that
patch still has some issues.

>   *
>   * Additionally, KVM only traps guest accesses to the debug registers if
>   * the guest is not actively using them (see the KVM_ARM64_DEBUG_DIRTY
> @@ -107,8 +107,8 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  	trace_kvm_arm_setup_debug(vcpu, vcpu->guest_debug);
>  
>  	/*
> -	 * This also clears MDCR_EL2_E2PB_MASK to disable guest access
> -	 * to the profiling buffer.
> +	 * This also clears MDCR_EL2_E2PB_MASK and MDCR_EL2_E2TB_MASK
> +	 * to disable guest access to the profiling and trace buffers
>  	 */
>  	vcpu->arch.mdcr_el2 = __this_cpu_read(mdcr_el2) & MDCR_EL2_HPMN_MASK;
>  	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM |
> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> index f401724f12ef..9499e18dd28f 100644
> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> @@ -58,10 +58,51 @@ static void __debug_restore_spe(u64 pmscr_el1)
>  	write_sysreg_s(pmscr_el1, SYS_PMSCR_EL1);
>  }
>  
> +static void __debug_save_trace(u64 *trfcr_el1)
> +{
> +

Spurious blank line?

> +	*trfcr_el1 = 0;
> +
> +	/* Check if we have TRBE */
> +	if (!cpuid_feature_extract_unsigned_field(read_sysreg(id_aa64dfr0_el1),
> +						  ID_AA64DFR0_TRBE_SHIFT))
> +		return;

Do we have a way to track this that doesn't involve reading an ID
register? This is on the hot path, and is going to really suck badly
with NV (which traps all ID regs for obvious reasons). I would have
hoped that one way or another, we'd have a static key for this.

> +
> +	/* Check we can access the TRBE */
> +	if ((read_sysreg_s(SYS_TRBIDR_EL1) & TRBIDR_PROG))
> +		return;
> +
> +	/* Check if the TRBE is enabled */
> +	if (!(read_sysreg_s(SYS_TRBLIMITR_EL1) & TRBLIMITR_ENABLE))
> +		return;
> +	/*
> +	 * Prohibit trace generation while we are in guest.
> +	 * Since access to TRFCR_EL1 is trapped, the guest can't
> +	 * modify the filtering set by the host.

If TRFCR_EL1 is trapped, where is the trap handling? This series
doesn't touch sys_regs.c, so I assume you rely on the "inject an UNDEF
for anything unknown" default behaviour?

If that's the case, I'd rather you add an explicit handler.

> +	 */
> +	*trfcr_el1 = read_sysreg_s(SYS_TRFCR_EL1);
> +	write_sysreg_s(0, SYS_TRFCR_EL1);
> +	isb();
> +	/* Drain the trace buffer to memory */
> +	tsb_csync();
> +	dsb(nsh);
> +}
> +
> +static void __debug_restore_trace(u64 trfcr_el1)
> +{
> +	if (!trfcr_el1)
> +		return;
> +
> +	/* Restore trace filter controls */
> +	write_sysreg_s(trfcr_el1, SYS_TRFCR_EL1);
> +}
> +
>  void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu)
>  {
>  	/* Disable and flush SPE data generation */
>  	__debug_save_spe(&vcpu->arch.host_debug_state.pmscr_el1);
> +	/* Disable and flush Self-Hosted Trace generation */
> +	__debug_save_trace(&vcpu->arch.host_debug_state.trfcr_el1);
>  }
>  
>  void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
> @@ -72,6 +113,7 @@ void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
>  void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu)
>  {
>  	__debug_restore_spe(vcpu->arch.host_debug_state.pmscr_el1);
> +	__debug_restore_trace(vcpu->arch.host_debug_state.trfcr_el1);
>  }
>  
>  void __debug_switch_to_host(struct kvm_vcpu *vcpu)
> diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
> index 68ab6b4d5141..736805232521 100644
> --- a/arch/arm64/kvm/hyp/nvhe/switch.c
> +++ b/arch/arm64/kvm/hyp/nvhe/switch.c
> @@ -95,6 +95,7 @@ static void __deactivate_traps(struct kvm_vcpu *vcpu)
>  
>  	mdcr_el2 &= MDCR_EL2_HPMN_MASK;
>  	mdcr_el2 |= MDCR_EL2_E2PB_MASK << MDCR_EL2_E2PB_SHIFT;
> +	mdcr_el2 |= MDCR_EL2_E2TB_MASK << MDCR_EL2_E2TB_SHIFT;
>  
>  	write_sysreg(mdcr_el2, mdcr_el2);
>  	if (is_protected_kvm_enabled())
> -- 
> 2.24.1
> 
> 

Thanks,

	M.

[1] https://lore.kernel.org/r/20210323180057.263356-1-alexandru.elisei@arm.com

-- 
Without deviation from the norm, progress is not possible.