[PATCH V16 2/8] KVM: arm64: Prevent guest accesses into BRBE system registers/instructions

[PATCH V16 2/8] KVM: arm64: Prevent guest accesses into BRBE system registers/instructions

[Anshuman Khandual]

Currently BRBE feature is not supported in a guest environment. This hides
BRBE feature availability via masking ID_AA64DFR0_EL1.BRBE field. This also
blocks guest accesses into BRBE system registers and instructions as if the
underlying hardware never implemented FEAT_BRBE feature.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Oliver Upton <oliver.upton@linux.dev>
Cc: James Morse <james.morse@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: kvmarm@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
Changes in V16:

- Added BRB_INF_SRC_TGT_EL1 macro for corresponding BRB_[INF|SRC|TGT] expansion

 arch/arm64/kvm/sys_regs.c | 56 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 30253bd19917..6a06dc2f0c06 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1304,6 +1304,11 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
 	return 0;
 }
 
+#define BRB_INF_SRC_TGT_EL1(n)					\
+	{ SYS_DESC(SYS_BRBINF##n##_EL1), undef_access },	\
+	{ SYS_DESC(SYS_BRBSRC##n##_EL1), undef_access },	\
+	{ SYS_DESC(SYS_BRBTGT##n##_EL1), undef_access }		\
+
 /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
 #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
 	{ SYS_DESC(SYS_DBGBVRn_EL1(n)),					\
@@ -1707,6 +1712,9 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
 	/* Hide SPE from guests */
 	val &= ~ID_AA64DFR0_EL1_PMSVer_MASK;
 
+	/* Hide BRBE from guests */
+	val &= ~ID_AA64DFR0_EL1_BRBE_MASK;
+
 	return val;
 }
 
@@ -2195,6 +2203,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_DC_CISW), access_dcsw },
 	{ SYS_DESC(SYS_DC_CIGSW), access_dcgsw },
 	{ SYS_DESC(SYS_DC_CIGDSW), access_dcgsw },
+	{ SYS_DESC(OP_BRB_IALL), undef_access },
+	{ SYS_DESC(OP_BRB_INJ), undef_access },
 
 	DBG_BCR_BVR_WCR_WVR_EL1(0),
 	DBG_BCR_BVR_WCR_WVR_EL1(1),
@@ -2225,6 +2235,52 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_DBGCLAIMCLR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_DBGAUTHSTATUS_EL1), trap_dbgauthstatus_el1 },
 
+	/*
+	 * BRBE branch record sysreg address space is interleaved between
+	 * corresponding BRBINF<N>_EL1, BRBSRC<N>_EL1, and BRBTGT<N>_EL1.
+	 */
+	BRB_INF_SRC_TGT_EL1(0),
+	BRB_INF_SRC_TGT_EL1(16),
+	BRB_INF_SRC_TGT_EL1(1),
+	BRB_INF_SRC_TGT_EL1(17),
+	BRB_INF_SRC_TGT_EL1(2),
+	BRB_INF_SRC_TGT_EL1(18),
+	BRB_INF_SRC_TGT_EL1(3),
+	BRB_INF_SRC_TGT_EL1(19),
+	BRB_INF_SRC_TGT_EL1(4),
+	BRB_INF_SRC_TGT_EL1(20),
+	BRB_INF_SRC_TGT_EL1(5),
+	BRB_INF_SRC_TGT_EL1(21),
+	BRB_INF_SRC_TGT_EL1(6),
+	BRB_INF_SRC_TGT_EL1(22),
+	BRB_INF_SRC_TGT_EL1(7),
+	BRB_INF_SRC_TGT_EL1(23),
+	BRB_INF_SRC_TGT_EL1(8),
+	BRB_INF_SRC_TGT_EL1(24),
+	BRB_INF_SRC_TGT_EL1(9),
+	BRB_INF_SRC_TGT_EL1(25),
+	BRB_INF_SRC_TGT_EL1(10),
+	BRB_INF_SRC_TGT_EL1(26),
+	BRB_INF_SRC_TGT_EL1(11),
+	BRB_INF_SRC_TGT_EL1(27),
+	BRB_INF_SRC_TGT_EL1(12),
+	BRB_INF_SRC_TGT_EL1(28),
+	BRB_INF_SRC_TGT_EL1(13),
+	BRB_INF_SRC_TGT_EL1(29),
+	BRB_INF_SRC_TGT_EL1(14),
+	BRB_INF_SRC_TGT_EL1(30),
+	BRB_INF_SRC_TGT_EL1(15),
+	BRB_INF_SRC_TGT_EL1(31),
+
+	/* Remaining BRBE sysreg addresses space */
+	{ SYS_DESC(SYS_BRBCR_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBFCR_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTS_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBINFINJ_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRCINJ_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGTINJ_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBIDR0_EL1), undef_access },
+
 	{ SYS_DESC(SYS_MDCCSR_EL0), trap_raz_wi },
 	{ SYS_DESC(SYS_DBGDTR_EL0), trap_raz_wi },
 	// DBGDTR[TR]X_EL0 share the same encoding
-- 
2.25.1

[PATCH V16 5/8] KVM: arm64: nvhe: Disable branch generation in nVHE guests

[Anshuman Khandual]

Disable the BRBE before we enter the guest, saving the status and enable it
back once we get out of the guest. This avoids capturing branch records in
the guest kernel or userspace, which would be confusing the host samples.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Oliver Upton <oliver.upton@linux.dev>
Cc: James Morse <james.morse@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: kvmarm@lists.linux.dev
Cc: linux-arm-kernel@lists.infradead.org
CC: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
Changes in V16:

- Dropped BRBCR_EL1 and BRBFCR_EL1 from enum vcpu_sysreg
- Reverted back the KVM NVHE patch - used host_debug_state based 'brbcr_el1'
  element, and dropped the previous dependency on Jame's coresight series

 arch/arm64/include/asm/kvm_host.h  |  5 ++++-
 arch/arm64/kvm/debug.c             |  5 +++++
 arch/arm64/kvm/hyp/nvhe/debug-sr.c | 33 ++++++++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 21c57b812569..bce8792092af 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;
@@ -610,6 +610,7 @@ struct kvm_vcpu_arch {
 		u64 pmscr_el1;
 		/* Self-hosted trace */
 		u64 trfcr_el1;
+		u64 brbcr_el1;
 	} host_debug_state;
 
 	/* VGIC state */
@@ -779,6 +780,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 BRBE context if active  */
+#define DEBUG_STATE_SAVE_BRBE	__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..99f85d8acbf3 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -335,10 +335,15 @@ 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);
+
+	/* Check if we have BRBE implemented and available at the host */
+	if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT))
+		vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_BRBE);
 }
 
 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_BRBE);
 }
diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
index 4558c02eb352..79bcf0fb1326 100644
--- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
+++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
@@ -79,6 +79,34 @@ static void __debug_restore_trace(u64 trfcr_el1)
 	write_sysreg_s(trfcr_el1, SYS_TRFCR_EL1);
 }
 
+static void __debug_save_brbe(u64 *brbcr_el1)
+{
+	*brbcr_el1 = 0;
+
+	/* Check if the BRBE is enabled */
+	if (!(read_sysreg_s(SYS_BRBCR_EL1) & (BRBCR_ELx_E0BRE | BRBCR_ELx_ExBRE)))
+		return;
+
+	/*
+	 * Prohibit branch record generation while we are in guest.
+	 * Since access to BRBCR_EL1 is trapped, the guest can't
+	 * modify the filtering set by the host.
+	 */
+	*brbcr_el1 = read_sysreg_s(SYS_BRBCR_EL1);
+	write_sysreg_s(0, SYS_BRBCR_EL1);
+	isb();
+}
+
+static void __debug_restore_brbe(u64 brbcr_el1)
+{
+	if (!brbcr_el1)
+		return;
+
+	/* Restore BRBE controls */
+	write_sysreg_s(brbcr_el1, SYS_BRBCR_EL1);
+	isb();
+}
+
 void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu)
 {
 	/* Disable and flush SPE data generation */
@@ -87,6 +115,9 @@ void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu)
 	/* 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);
+	/* Disable BRBE branch records */
+	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE))
+		__debug_save_brbe(&vcpu->arch.host_debug_state.brbcr_el1);
 }
 
 void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
@@ -100,6 +131,8 @@ void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu)
 		__debug_restore_spe(vcpu->arch.host_debug_state.pmscr_el1);
 	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
 		__debug_restore_trace(vcpu->arch.host_debug_state.trfcr_el1);
+	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE))
+		__debug_restore_brbe(vcpu->arch.host_debug_state.brbcr_el1);
 }
 
 void __debug_switch_to_host(struct kvm_vcpu *vcpu)
-- 
2.25.1

Re: [PATCH V16 2/8] KVM: arm64: Prevent guest accesses into BRBE system registers/instructions

[Suzuki K Poulose]

On 25/01/2024 09:41, Anshuman Khandual wrote:
> Currently BRBE feature is not supported in a guest environment. This hides
> BRBE feature availability via masking ID_AA64DFR0_EL1.BRBE field. This also
> blocks guest accesses into BRBE system registers and instructions as if the
> underlying hardware never implemented FEAT_BRBE feature.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Oliver Upton <oliver.upton@linux.dev>
> Cc: James Morse <james.morse@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: kvmarm@lists.linux.dev
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> Changes in V16:
> 
> - Added BRB_INF_SRC_TGT_EL1 macro for corresponding BRB_[INF|SRC|TGT] expansion
> 
>   arch/arm64/kvm/sys_regs.c | 56 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 56 insertions(+)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 30253bd19917..6a06dc2f0c06 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1304,6 +1304,11 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
>   	return 0;
>   }
>   
> +#define BRB_INF_SRC_TGT_EL1(n)					\
> +	{ SYS_DESC(SYS_BRBINF##n##_EL1), undef_access },	\
> +	{ SYS_DESC(SYS_BRBSRC##n##_EL1), undef_access },	\
> +	{ SYS_DESC(SYS_BRBTGT##n##_EL1), undef_access }		\
> +
>   /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
>   #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
>   	{ SYS_DESC(SYS_DBGBVRn_EL1(n)),					\
> @@ -1707,6 +1712,9 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>   	/* Hide SPE from guests */
>   	val &= ~ID_AA64DFR0_EL1_PMSVer_MASK;
>   
> +	/* Hide BRBE from guests */
> +	val &= ~ID_AA64DFR0_EL1_BRBE_MASK;
> +
>   	return val;
>   }
>   
> @@ -2195,6 +2203,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>   	{ SYS_DESC(SYS_DC_CISW), access_dcsw },
>   	{ SYS_DESC(SYS_DC_CIGSW), access_dcgsw },
>   	{ SYS_DESC(SYS_DC_CIGDSW), access_dcgsw },
> +	{ SYS_DESC(OP_BRB_IALL), undef_access },
> +	{ SYS_DESC(OP_BRB_INJ), undef_access },
>   

heads up: This may conflict with Marc's patches to move the sys 
instructions to a separate table. But otherwise, looks good to me.


Suzuki

Re: [PATCH V16 5/8] KVM: arm64: nvhe: Disable branch generation in nVHE guests

[Suzuki K Poulose]

On 25/01/2024 09:41, Anshuman Khandual wrote:
> Disable the BRBE before we enter the guest, saving the status and enable it
> back once we get out of the guest. This avoids capturing branch records in
> the guest kernel or userspace, which would be confusing the host samples.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Oliver Upton <oliver.upton@linux.dev>
> Cc: James Morse <james.morse@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: kvmarm@lists.linux.dev
> Cc: linux-arm-kernel@lists.infradead.org
> CC: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> Changes in V16:
> 
> - Dropped BRBCR_EL1 and BRBFCR_EL1 from enum vcpu_sysreg
> - Reverted back the KVM NVHE patch - used host_debug_state based 'brbcr_el1'
>    element, and dropped the previous dependency on Jame's coresight series
> 
>   arch/arm64/include/asm/kvm_host.h  |  5 ++++-
>   arch/arm64/kvm/debug.c             |  5 +++++
>   arch/arm64/kvm/hyp/nvhe/debug-sr.c | 33 ++++++++++++++++++++++++++++++
>   3 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 21c57b812569..bce8792092af 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;
> @@ -610,6 +610,7 @@ struct kvm_vcpu_arch {
>   		u64 pmscr_el1;
>   		/* Self-hosted trace */
>   		u64 trfcr_el1;
> +		u64 brbcr_el1;
>   	} host_debug_state;
>   
>   	/* VGIC state */
> @@ -779,6 +780,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 BRBE context if active  */
> +#define DEBUG_STATE_SAVE_BRBE	__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..99f85d8acbf3 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -335,10 +335,15 @@ 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);
> +
> +	/* Check if we have BRBE implemented and available at the host */
> +	if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT))
> +		vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_BRBE);
>   }
>   
>   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_BRBE);
>   }
> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> index 4558c02eb352..79bcf0fb1326 100644
> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> @@ -79,6 +79,34 @@ static void __debug_restore_trace(u64 trfcr_el1)
>   	write_sysreg_s(trfcr_el1, SYS_TRFCR_EL1);
>   }
>   
> +static void __debug_save_brbe(u64 *brbcr_el1)
> +{
> +	*brbcr_el1 = 0;
> +
> +	/* Check if the BRBE is enabled */
> +	if (!(read_sysreg_s(SYS_BRBCR_EL1) & (BRBCR_ELx_E0BRE | BRBCR_ELx_ExBRE)))
> +		return;
> +
> +	/*
> +	 * Prohibit branch record generation while we are in guest.
> +	 * Since access to BRBCR_EL1 is trapped, the guest can't
> +	 * modify the filtering set by the host.
> +	 */
> +	*brbcr_el1 = read_sysreg_s(SYS_BRBCR_EL1);
> +	write_sysreg_s(0, SYS_BRBCR_EL1);
> +	isb();

Is this isb() required here ? This can be synchronised with the Guest 
entry ?

> +}
> +
> +static void __debug_restore_brbe(u64 brbcr_el1)
> +{
> +	if (!brbcr_el1)
> +		return;
> +
> +	/* Restore BRBE controls */
> +	write_sysreg_s(brbcr_el1, SYS_BRBCR_EL1);
> +	isb();

Similarly here, exit back to EL1 host can synchronise the setting ?

Suzuki

> +}
> +
>   void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu)
>   {
>   	/* Disable and flush SPE data generation */
> @@ -87,6 +115,9 @@ void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu)
>   	/* 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);
> +	/* Disable BRBE branch records */
> +	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE))
> +		__debug_save_brbe(&vcpu->arch.host_debug_state.brbcr_el1);
>   }
>   
>   void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
> @@ -100,6 +131,8 @@ void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu)
>   		__debug_restore_spe(vcpu->arch.host_debug_state.pmscr_el1);
>   	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
>   		__debug_restore_trace(vcpu->arch.host_debug_state.trfcr_el1);
> +	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE))
> +		__debug_restore_brbe(vcpu->arch.host_debug_state.brbcr_el1);
>   }
>   
>   void __debug_switch_to_host(struct kvm_vcpu *vcpu)

Re: [PATCH V16 2/8] KVM: arm64: Prevent guest accesses into BRBE system registers/instructions

[Anshuman Khandual]

On 1/29/24 17:45, Suzuki K Poulose wrote:
> On 25/01/2024 09:41, Anshuman Khandual wrote:
>> Currently BRBE feature is not supported in a guest environment. This hides
>> BRBE feature availability via masking ID_AA64DFR0_EL1.BRBE field. This also
>> blocks guest accesses into BRBE system registers and instructions as if the
>> underlying hardware never implemented FEAT_BRBE feature.
>>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: Oliver Upton <oliver.upton@linux.dev>
>> Cc: James Morse <james.morse@arm.com>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: kvmarm@lists.linux.dev
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>> Changes in V16:
>>
>> - Added BRB_INF_SRC_TGT_EL1 macro for corresponding BRB_[INF|SRC|TGT] expansion
>>
>>   arch/arm64/kvm/sys_regs.c | 56 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 56 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 30253bd19917..6a06dc2f0c06 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1304,6 +1304,11 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
>>       return 0;
>>   }
>>   +#define BRB_INF_SRC_TGT_EL1(n)                    \
>> +    { SYS_DESC(SYS_BRBINF##n##_EL1), undef_access },    \
>> +    { SYS_DESC(SYS_BRBSRC##n##_EL1), undef_access },    \
>> +    { SYS_DESC(SYS_BRBTGT##n##_EL1), undef_access }        \
>> +
>>   /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
>>   #define DBG_BCR_BVR_WCR_WVR_EL1(n)                    \
>>       { SYS_DESC(SYS_DBGBVRn_EL1(n)),                    \
>> @@ -1707,6 +1712,9 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>>       /* Hide SPE from guests */
>>       val &= ~ID_AA64DFR0_EL1_PMSVer_MASK;
>>   +    /* Hide BRBE from guests */
>> +    val &= ~ID_AA64DFR0_EL1_BRBE_MASK;
>> +
>>       return val;
>>   }
>>   @@ -2195,6 +2203,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>       { SYS_DESC(SYS_DC_CISW), access_dcsw },
>>       { SYS_DESC(SYS_DC_CIGSW), access_dcgsw },
>>       { SYS_DESC(SYS_DC_CIGDSW), access_dcgsw },
>> +    { SYS_DESC(OP_BRB_IALL), undef_access },
>> +    { SYS_DESC(OP_BRB_INJ), undef_access },
>>   
> 
> heads up: This may conflict with Marc's patches to move the sys instructions to a separate table. But otherwise, looks good to me.

Sure, will rebase this on recent changes.

Re: [PATCH V16 5/8] KVM: arm64: nvhe: Disable branch generation in nVHE guests

[Anshuman Khandual]

On 1/29/24 17:50, Suzuki K Poulose wrote:
> On 25/01/2024 09:41, Anshuman Khandual wrote:
>> Disable the BRBE before we enter the guest, saving the status and enable it
>> back once we get out of the guest. This avoids capturing branch records in
>> the guest kernel or userspace, which would be confusing the host samples.
>>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: Oliver Upton <oliver.upton@linux.dev>
>> Cc: James Morse <james.morse@arm.com>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: kvmarm@lists.linux.dev
>> Cc: linux-arm-kernel@lists.infradead.org
>> CC: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>> Changes in V16:
>>
>> - Dropped BRBCR_EL1 and BRBFCR_EL1 from enum vcpu_sysreg
>> - Reverted back the KVM NVHE patch - used host_debug_state based 'brbcr_el1'
>>    element, and dropped the previous dependency on Jame's coresight series
>>
>>   arch/arm64/include/asm/kvm_host.h  |  5 ++++-
>>   arch/arm64/kvm/debug.c             |  5 +++++
>>   arch/arm64/kvm/hyp/nvhe/debug-sr.c | 33 ++++++++++++++++++++++++++++++
>>   3 files changed, 42 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 21c57b812569..bce8792092af 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;
>> @@ -610,6 +610,7 @@ struct kvm_vcpu_arch {
>>           u64 pmscr_el1;
>>           /* Self-hosted trace */
>>           u64 trfcr_el1;
>> +        u64 brbcr_el1;
>>       } host_debug_state;
>>         /* VGIC state */
>> @@ -779,6 +780,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 BRBE context if active  */
>> +#define DEBUG_STATE_SAVE_BRBE    __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..99f85d8acbf3 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -335,10 +335,15 @@ 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);
>> +
>> +    /* Check if we have BRBE implemented and available at the host */
>> +    if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT))
>> +        vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_BRBE);
>>   }
>>     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_BRBE);
>>   }
>> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> index 4558c02eb352..79bcf0fb1326 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> @@ -79,6 +79,34 @@ static void __debug_restore_trace(u64 trfcr_el1)
>>       write_sysreg_s(trfcr_el1, SYS_TRFCR_EL1);
>>   }
>>   +static void __debug_save_brbe(u64 *brbcr_el1)
>> +{
>> +    *brbcr_el1 = 0;
>> +
>> +    /* Check if the BRBE is enabled */
>> +    if (!(read_sysreg_s(SYS_BRBCR_EL1) & (BRBCR_ELx_E0BRE | BRBCR_ELx_ExBRE)))
>> +        return;
>> +
>> +    /*
>> +     * Prohibit branch record generation while we are in guest.
>> +     * Since access to BRBCR_EL1 is trapped, the guest can't
>> +     * modify the filtering set by the host.
>> +     */
>> +    *brbcr_el1 = read_sysreg_s(SYS_BRBCR_EL1);
>> +    write_sysreg_s(0, SYS_BRBCR_EL1);
>> +    isb();
> 
> Is this isb() required here ? This can be synchronised with the Guest entry ?
> 
>> +}
>> +
>> +static void __debug_restore_brbe(u64 brbcr_el1)
>> +{
>> +    if (!brbcr_el1)
>> +        return;
>> +
>> +    /* Restore BRBE controls */
>> +    write_sysreg_s(brbcr_el1, SYS_BRBCR_EL1);
>> +    isb();
> 
> Similarly here, exit back to EL1 host can synchronise the setting ?

Sure, will drop both the isb() here.

Re: [PATCH V16 2/8] KVM: arm64: Prevent guest accesses into BRBE system registers/instructions

[Mark Rutland]

On Thu, Jan 25, 2024 at 03:11:13PM +0530, Anshuman Khandual wrote:
> Currently BRBE feature is not supported in a guest environment. This hides
> BRBE feature availability via masking ID_AA64DFR0_EL1.BRBE field.

Does that means that a guest can currently see BRBE advertised in the
ID_AA64DFR0_EL1.BRB field, or is that hidden by the regular cpufeature code
today?

> This also blocks guest accesses into BRBE system registers and instructions
> as if the underlying hardware never implemented FEAT_BRBE feature.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Oliver Upton <oliver.upton@linux.dev>
> Cc: James Morse <james.morse@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: kvmarm@lists.linux.dev
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> Changes in V16:
> 
> - Added BRB_INF_SRC_TGT_EL1 macro for corresponding BRB_[INF|SRC|TGT] expansion
> 
>  arch/arm64/kvm/sys_regs.c | 56 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 30253bd19917..6a06dc2f0c06 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1304,6 +1304,11 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
>  	return 0;
>  }
>  
> +#define BRB_INF_SRC_TGT_EL1(n)					\
> +	{ SYS_DESC(SYS_BRBINF##n##_EL1), undef_access },	\
> +	{ SYS_DESC(SYS_BRBSRC##n##_EL1), undef_access },	\
> +	{ SYS_DESC(SYS_BRBTGT##n##_EL1), undef_access }		\

With the changes suggested on the previous patch, this would need to change to be:

	#define BRB_INF_SRC_TGT_EL1(n)					\
		{ SYS_DESC(SYS_BRBINF_EL1(n)), undef_access },	\
		{ SYS_DESC(SYS_BRBSRC_EL1(n)), undef_access },	\
		{ SYS_DESC(SYS_BRBTGT_EL1(n)), undef_access }	\


... which would also be easier for backporting (if necessary), since those
definitions have existed for a while.

Otherwise (modulo Suzuki's comment about rebasing), this looks good to me.

Mark.

>  /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
>  #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
>  	{ SYS_DESC(SYS_DBGBVRn_EL1(n)),					\
> @@ -1707,6 +1712,9 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>  	/* Hide SPE from guests */
>  	val &= ~ID_AA64DFR0_EL1_PMSVer_MASK;
>  
> +	/* Hide BRBE from guests */
> +	val &= ~ID_AA64DFR0_EL1_BRBE_MASK;
> +
>  	return val;
>  }
>  
> @@ -2195,6 +2203,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_DC_CISW), access_dcsw },
>  	{ SYS_DESC(SYS_DC_CIGSW), access_dcgsw },
>  	{ SYS_DESC(SYS_DC_CIGDSW), access_dcgsw },
> +	{ SYS_DESC(OP_BRB_IALL), undef_access },
> +	{ SYS_DESC(OP_BRB_INJ), undef_access },
>  
>  	DBG_BCR_BVR_WCR_WVR_EL1(0),
>  	DBG_BCR_BVR_WCR_WVR_EL1(1),
> @@ -2225,6 +2235,52 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_DBGCLAIMCLR_EL1), trap_raz_wi },
>  	{ SYS_DESC(SYS_DBGAUTHSTATUS_EL1), trap_dbgauthstatus_el1 },
>  
> +	/*
> +	 * BRBE branch record sysreg address space is interleaved between
> +	 * corresponding BRBINF<N>_EL1, BRBSRC<N>_EL1, and BRBTGT<N>_EL1.
> +	 */
> +	BRB_INF_SRC_TGT_EL1(0),
> +	BRB_INF_SRC_TGT_EL1(16),
> +	BRB_INF_SRC_TGT_EL1(1),
> +	BRB_INF_SRC_TGT_EL1(17),
> +	BRB_INF_SRC_TGT_EL1(2),
> +	BRB_INF_SRC_TGT_EL1(18),
> +	BRB_INF_SRC_TGT_EL1(3),
> +	BRB_INF_SRC_TGT_EL1(19),
> +	BRB_INF_SRC_TGT_EL1(4),
> +	BRB_INF_SRC_TGT_EL1(20),
> +	BRB_INF_SRC_TGT_EL1(5),
> +	BRB_INF_SRC_TGT_EL1(21),
> +	BRB_INF_SRC_TGT_EL1(6),
> +	BRB_INF_SRC_TGT_EL1(22),
> +	BRB_INF_SRC_TGT_EL1(7),
> +	BRB_INF_SRC_TGT_EL1(23),
> +	BRB_INF_SRC_TGT_EL1(8),
> +	BRB_INF_SRC_TGT_EL1(24),
> +	BRB_INF_SRC_TGT_EL1(9),
> +	BRB_INF_SRC_TGT_EL1(25),
> +	BRB_INF_SRC_TGT_EL1(10),
> +	BRB_INF_SRC_TGT_EL1(26),
> +	BRB_INF_SRC_TGT_EL1(11),
> +	BRB_INF_SRC_TGT_EL1(27),
> +	BRB_INF_SRC_TGT_EL1(12),
> +	BRB_INF_SRC_TGT_EL1(28),
> +	BRB_INF_SRC_TGT_EL1(13),
> +	BRB_INF_SRC_TGT_EL1(29),
> +	BRB_INF_SRC_TGT_EL1(14),
> +	BRB_INF_SRC_TGT_EL1(30),
> +	BRB_INF_SRC_TGT_EL1(15),
> +	BRB_INF_SRC_TGT_EL1(31),
> +
> +	/* Remaining BRBE sysreg addresses space */
> +	{ SYS_DESC(SYS_BRBCR_EL1), undef_access },
> +	{ SYS_DESC(SYS_BRBFCR_EL1), undef_access },
> +	{ SYS_DESC(SYS_BRBTS_EL1), undef_access },
> +	{ SYS_DESC(SYS_BRBINFINJ_EL1), undef_access },
> +	{ SYS_DESC(SYS_BRBSRCINJ_EL1), undef_access },
> +	{ SYS_DESC(SYS_BRBTGTINJ_EL1), undef_access },
> +	{ SYS_DESC(SYS_BRBIDR0_EL1), undef_access },
> +
>  	{ SYS_DESC(SYS_MDCCSR_EL0), trap_raz_wi },
>  	{ SYS_DESC(SYS_DBGDTR_EL0), trap_raz_wi },
>  	// DBGDTR[TR]X_EL0 share the same encoding
> -- 
> 2.25.1
> 

Re: [PATCH V16 2/8] KVM: arm64: Prevent guest accesses into BRBE system registers/instructions

[Anshuman Khandual]

On 2/21/24 19:31, Mark Rutland wrote:
> On Thu, Jan 25, 2024 at 03:11:13PM +0530, Anshuman Khandual wrote:
>> Currently BRBE feature is not supported in a guest environment. This hides
>> BRBE feature availability via masking ID_AA64DFR0_EL1.BRBE field.
> 
> Does that means that a guest can currently see BRBE advertised in the
> ID_AA64DFR0_EL1.BRB field, or is that hidden by the regular cpufeature code
> today?

IIRC it is hidden, but will have to double check. When experimenting for BRBE
guest support enablement earlier, following changes were need for the feature
to be visible in ID_AA64DFR0_EL1.

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 646591c67e7a..f258568535a8 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -445,6 +445,7 @@ static const struct arm64_ftr_bits ftr_id_mmfr0[] = {
 };
 
 static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
+       S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_IMP),
        S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_DoubleLock_SHIFT, 4, 0),
        ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_PMSVer_SHIFT, 4, 0),
        ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0),

Should we add the following entry - explicitly hiding BRBE from the guest
as a prerequisite patch ?

S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_NI)

> 
>> This also blocks guest accesses into BRBE system registers and instructions
>> as if the underlying hardware never implemented FEAT_BRBE feature.
>>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: Oliver Upton <oliver.upton@linux.dev>
>> Cc: James Morse <james.morse@arm.com>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: kvmarm@lists.linux.dev
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>> Changes in V16:
>>
>> - Added BRB_INF_SRC_TGT_EL1 macro for corresponding BRB_[INF|SRC|TGT] expansion
>>
>>  arch/arm64/kvm/sys_regs.c | 56 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 30253bd19917..6a06dc2f0c06 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1304,6 +1304,11 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
>>  	return 0;
>>  }
>>  
>> +#define BRB_INF_SRC_TGT_EL1(n)					\
>> +	{ SYS_DESC(SYS_BRBINF##n##_EL1), undef_access },	\
>> +	{ SYS_DESC(SYS_BRBSRC##n##_EL1), undef_access },	\
>> +	{ SYS_DESC(SYS_BRBTGT##n##_EL1), undef_access }		\
> 
> With the changes suggested on the previous patch, this would need to change to be:
> 
> 	#define BRB_INF_SRC_TGT_EL1(n)					\
> 		{ SYS_DESC(SYS_BRBINF_EL1(n)), undef_access },	\
> 		{ SYS_DESC(SYS_BRBSRC_EL1(n)), undef_access },	\
> 		{ SYS_DESC(SYS_BRBTGT_EL1(n)), undef_access }	\

Sure, already folded back in these above changes.

> 
> 
> ... which would also be easier for backporting (if necessary), since those
> definitions have existed for a while.
> 
> Otherwise (modulo Suzuki's comment about rebasing), this looks good to me.

Okay.

> 
> Mark.
> 
>>  /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
>>  #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
>>  	{ SYS_DESC(SYS_DBGBVRn_EL1(n)),					\
>> @@ -1707,6 +1712,9 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>>  	/* Hide SPE from guests */
>>  	val &= ~ID_AA64DFR0_EL1_PMSVer_MASK;
>>  
>> +	/* Hide BRBE from guests */
>> +	val &= ~ID_AA64DFR0_EL1_BRBE_MASK;
>> +
>>  	return val;
>>  }
>>  
>> @@ -2195,6 +2203,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>  	{ SYS_DESC(SYS_DC_CISW), access_dcsw },
>>  	{ SYS_DESC(SYS_DC_CIGSW), access_dcgsw },
>>  	{ SYS_DESC(SYS_DC_CIGDSW), access_dcgsw },
>> +	{ SYS_DESC(OP_BRB_IALL), undef_access },
>> +	{ SYS_DESC(OP_BRB_INJ), undef_access },
>>  
>>  	DBG_BCR_BVR_WCR_WVR_EL1(0),
>>  	DBG_BCR_BVR_WCR_WVR_EL1(1),
>> @@ -2225,6 +2235,52 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>  	{ SYS_DESC(SYS_DBGCLAIMCLR_EL1), trap_raz_wi },
>>  	{ SYS_DESC(SYS_DBGAUTHSTATUS_EL1), trap_dbgauthstatus_el1 },
>>  
>> +	/*
>> +	 * BRBE branch record sysreg address space is interleaved between
>> +	 * corresponding BRBINF<N>_EL1, BRBSRC<N>_EL1, and BRBTGT<N>_EL1.
>> +	 */
>> +	BRB_INF_SRC_TGT_EL1(0),
>> +	BRB_INF_SRC_TGT_EL1(16),
>> +	BRB_INF_SRC_TGT_EL1(1),
>> +	BRB_INF_SRC_TGT_EL1(17),
>> +	BRB_INF_SRC_TGT_EL1(2),
>> +	BRB_INF_SRC_TGT_EL1(18),
>> +	BRB_INF_SRC_TGT_EL1(3),
>> +	BRB_INF_SRC_TGT_EL1(19),
>> +	BRB_INF_SRC_TGT_EL1(4),
>> +	BRB_INF_SRC_TGT_EL1(20),
>> +	BRB_INF_SRC_TGT_EL1(5),
>> +	BRB_INF_SRC_TGT_EL1(21),
>> +	BRB_INF_SRC_TGT_EL1(6),
>> +	BRB_INF_SRC_TGT_EL1(22),
>> +	BRB_INF_SRC_TGT_EL1(7),
>> +	BRB_INF_SRC_TGT_EL1(23),
>> +	BRB_INF_SRC_TGT_EL1(8),
>> +	BRB_INF_SRC_TGT_EL1(24),
>> +	BRB_INF_SRC_TGT_EL1(9),
>> +	BRB_INF_SRC_TGT_EL1(25),
>> +	BRB_INF_SRC_TGT_EL1(10),
>> +	BRB_INF_SRC_TGT_EL1(26),
>> +	BRB_INF_SRC_TGT_EL1(11),
>> +	BRB_INF_SRC_TGT_EL1(27),
>> +	BRB_INF_SRC_TGT_EL1(12),
>> +	BRB_INF_SRC_TGT_EL1(28),
>> +	BRB_INF_SRC_TGT_EL1(13),
>> +	BRB_INF_SRC_TGT_EL1(29),
>> +	BRB_INF_SRC_TGT_EL1(14),
>> +	BRB_INF_SRC_TGT_EL1(30),
>> +	BRB_INF_SRC_TGT_EL1(15),
>> +	BRB_INF_SRC_TGT_EL1(31),
>> +
>> +	/* Remaining BRBE sysreg addresses space */
>> +	{ SYS_DESC(SYS_BRBCR_EL1), undef_access },
>> +	{ SYS_DESC(SYS_BRBFCR_EL1), undef_access },
>> +	{ SYS_DESC(SYS_BRBTS_EL1), undef_access },
>> +	{ SYS_DESC(SYS_BRBINFINJ_EL1), undef_access },
>> +	{ SYS_DESC(SYS_BRBSRCINJ_EL1), undef_access },
>> +	{ SYS_DESC(SYS_BRBTGTINJ_EL1), undef_access },
>> +	{ SYS_DESC(SYS_BRBIDR0_EL1), undef_access },
>> +
>>  	{ SYS_DESC(SYS_MDCCSR_EL0), trap_raz_wi },
>>  	{ SYS_DESC(SYS_DBGDTR_EL0), trap_raz_wi },
>>  	// DBGDTR[TR]X_EL0 share the same encoding
>> -- 
>> 2.25.1
>>

Re: [PATCH V16 2/8] KVM: arm64: Prevent guest accesses into BRBE system registers/instructions

[Mark Rutland]

On Fri, Feb 23, 2024 at 12:58:48PM +0530, Anshuman Khandual wrote:
> 
> 
> On 2/21/24 19:31, Mark Rutland wrote:
> > On Thu, Jan 25, 2024 at 03:11:13PM +0530, Anshuman Khandual wrote:
> >> Currently BRBE feature is not supported in a guest environment. This hides
> >> BRBE feature availability via masking ID_AA64DFR0_EL1.BRBE field.
> > 
> > Does that means that a guest can currently see BRBE advertised in the
> > ID_AA64DFR0_EL1.BRB field, or is that hidden by the regular cpufeature code
> > today?
> 
> IIRC it is hidden, but will have to double check. When experimenting for BRBE
> guest support enablement earlier, following changes were need for the feature
> to be visible in ID_AA64DFR0_EL1.
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 646591c67e7a..f258568535a8 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -445,6 +445,7 @@ static const struct arm64_ftr_bits ftr_id_mmfr0[] = {
>  };
>  
>  static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
> +       S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_IMP),
>         S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_DoubleLock_SHIFT, 4, 0),
>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_PMSVer_SHIFT, 4, 0),
>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0),
> 
> Should we add the following entry - explicitly hiding BRBE from the guest
> as a prerequisite patch ?
> 
> S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_NI)

Is it visbile currently, or is it hidden currently?

* If it is visible before this patch, that's a latent bug that we need to go
  fix first, and that'll require more coordination.

* If it is not visible before this patch, there's no problem in the code, but
  the commit message needs to explicitly mention that's the case as the commit
  message currently implies it is visible by only mentioning hiding it.

... so can you please double check as you suggested above? We should be able to
explain why it is or is not visible today.

Mark.

> >> This also blocks guest accesses into BRBE system registers and instructions
> >> as if the underlying hardware never implemented FEAT_BRBE feature.
> >>
> >> Cc: Marc Zyngier <maz@kernel.org>
> >> Cc: Oliver Upton <oliver.upton@linux.dev>
> >> Cc: James Morse <james.morse@arm.com>
> >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> >> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >> Cc: Will Deacon <will@kernel.org>
> >> Cc: kvmarm@lists.linux.dev
> >> Cc: linux-arm-kernel@lists.infradead.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> >> ---
> >> Changes in V16:
> >>
> >> - Added BRB_INF_SRC_TGT_EL1 macro for corresponding BRB_[INF|SRC|TGT] expansion
> >>
> >>  arch/arm64/kvm/sys_regs.c | 56 +++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 56 insertions(+)
> >>
> >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> >> index 30253bd19917..6a06dc2f0c06 100644
> >> --- a/arch/arm64/kvm/sys_regs.c
> >> +++ b/arch/arm64/kvm/sys_regs.c
> >> @@ -1304,6 +1304,11 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
> >>  	return 0;
> >>  }
> >>  
> >> +#define BRB_INF_SRC_TGT_EL1(n)					\
> >> +	{ SYS_DESC(SYS_BRBINF##n##_EL1), undef_access },	\
> >> +	{ SYS_DESC(SYS_BRBSRC##n##_EL1), undef_access },	\
> >> +	{ SYS_DESC(SYS_BRBTGT##n##_EL1), undef_access }		\
> > 
> > With the changes suggested on the previous patch, this would need to change to be:
> > 
> > 	#define BRB_INF_SRC_TGT_EL1(n)					\
> > 		{ SYS_DESC(SYS_BRBINF_EL1(n)), undef_access },	\
> > 		{ SYS_DESC(SYS_BRBSRC_EL1(n)), undef_access },	\
> > 		{ SYS_DESC(SYS_BRBTGT_EL1(n)), undef_access }	\
> 
> Sure, already folded back in these above changes.
> 
> > 
> > 
> > ... which would also be easier for backporting (if necessary), since those
> > definitions have existed for a while.
> > 
> > Otherwise (modulo Suzuki's comment about rebasing), this looks good to me.
> 
> Okay.
> 
> > 
> > Mark.
> > 
> >>  /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
> >>  #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
> >>  	{ SYS_DESC(SYS_DBGBVRn_EL1(n)),					\
> >> @@ -1707,6 +1712,9 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
> >>  	/* Hide SPE from guests */
> >>  	val &= ~ID_AA64DFR0_EL1_PMSVer_MASK;
> >>  
> >> +	/* Hide BRBE from guests */
> >> +	val &= ~ID_AA64DFR0_EL1_BRBE_MASK;
> >> +
> >>  	return val;
> >>  }
> >>  
> >> @@ -2195,6 +2203,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >>  	{ SYS_DESC(SYS_DC_CISW), access_dcsw },
> >>  	{ SYS_DESC(SYS_DC_CIGSW), access_dcgsw },
> >>  	{ SYS_DESC(SYS_DC_CIGDSW), access_dcgsw },
> >> +	{ SYS_DESC(OP_BRB_IALL), undef_access },
> >> +	{ SYS_DESC(OP_BRB_INJ), undef_access },
> >>  
> >>  	DBG_BCR_BVR_WCR_WVR_EL1(0),
> >>  	DBG_BCR_BVR_WCR_WVR_EL1(1),
> >> @@ -2225,6 +2235,52 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >>  	{ SYS_DESC(SYS_DBGCLAIMCLR_EL1), trap_raz_wi },
> >>  	{ SYS_DESC(SYS_DBGAUTHSTATUS_EL1), trap_dbgauthstatus_el1 },
> >>  
> >> +	/*
> >> +	 * BRBE branch record sysreg address space is interleaved between
> >> +	 * corresponding BRBINF<N>_EL1, BRBSRC<N>_EL1, and BRBTGT<N>_EL1.
> >> +	 */
> >> +	BRB_INF_SRC_TGT_EL1(0),
> >> +	BRB_INF_SRC_TGT_EL1(16),
> >> +	BRB_INF_SRC_TGT_EL1(1),
> >> +	BRB_INF_SRC_TGT_EL1(17),
> >> +	BRB_INF_SRC_TGT_EL1(2),
> >> +	BRB_INF_SRC_TGT_EL1(18),
> >> +	BRB_INF_SRC_TGT_EL1(3),
> >> +	BRB_INF_SRC_TGT_EL1(19),
> >> +	BRB_INF_SRC_TGT_EL1(4),
> >> +	BRB_INF_SRC_TGT_EL1(20),
> >> +	BRB_INF_SRC_TGT_EL1(5),
> >> +	BRB_INF_SRC_TGT_EL1(21),
> >> +	BRB_INF_SRC_TGT_EL1(6),
> >> +	BRB_INF_SRC_TGT_EL1(22),
> >> +	BRB_INF_SRC_TGT_EL1(7),
> >> +	BRB_INF_SRC_TGT_EL1(23),
> >> +	BRB_INF_SRC_TGT_EL1(8),
> >> +	BRB_INF_SRC_TGT_EL1(24),
> >> +	BRB_INF_SRC_TGT_EL1(9),
> >> +	BRB_INF_SRC_TGT_EL1(25),
> >> +	BRB_INF_SRC_TGT_EL1(10),
> >> +	BRB_INF_SRC_TGT_EL1(26),
> >> +	BRB_INF_SRC_TGT_EL1(11),
> >> +	BRB_INF_SRC_TGT_EL1(27),
> >> +	BRB_INF_SRC_TGT_EL1(12),
> >> +	BRB_INF_SRC_TGT_EL1(28),
> >> +	BRB_INF_SRC_TGT_EL1(13),
> >> +	BRB_INF_SRC_TGT_EL1(29),
> >> +	BRB_INF_SRC_TGT_EL1(14),
> >> +	BRB_INF_SRC_TGT_EL1(30),
> >> +	BRB_INF_SRC_TGT_EL1(15),
> >> +	BRB_INF_SRC_TGT_EL1(31),
> >> +
> >> +	/* Remaining BRBE sysreg addresses space */
> >> +	{ SYS_DESC(SYS_BRBCR_EL1), undef_access },
> >> +	{ SYS_DESC(SYS_BRBFCR_EL1), undef_access },
> >> +	{ SYS_DESC(SYS_BRBTS_EL1), undef_access },
> >> +	{ SYS_DESC(SYS_BRBINFINJ_EL1), undef_access },
> >> +	{ SYS_DESC(SYS_BRBSRCINJ_EL1), undef_access },
> >> +	{ SYS_DESC(SYS_BRBTGTINJ_EL1), undef_access },
> >> +	{ SYS_DESC(SYS_BRBIDR0_EL1), undef_access },
> >> +
> >>  	{ SYS_DESC(SYS_MDCCSR_EL0), trap_raz_wi },
> >>  	{ SYS_DESC(SYS_DBGDTR_EL0), trap_raz_wi },
> >>  	// DBGDTR[TR]X_EL0 share the same encoding
> >> -- 
> >> 2.25.1
> >>

Re: [PATCH V16 2/8] KVM: arm64: Prevent guest accesses into BRBE system registers/instructions

[Anshuman Khandual]

On 2/27/24 15:34, Mark Rutland wrote:
> On Fri, Feb 23, 2024 at 12:58:48PM +0530, Anshuman Khandual wrote:
>>
>>
>> On 2/21/24 19:31, Mark Rutland wrote:
>>> On Thu, Jan 25, 2024 at 03:11:13PM +0530, Anshuman Khandual wrote:
>>>> Currently BRBE feature is not supported in a guest environment. This hides
>>>> BRBE feature availability via masking ID_AA64DFR0_EL1.BRBE field.
>>>
>>> Does that means that a guest can currently see BRBE advertised in the
>>> ID_AA64DFR0_EL1.BRB field, or is that hidden by the regular cpufeature code
>>> today?
>>
>> IIRC it is hidden, but will have to double check. When experimenting for BRBE
>> guest support enablement earlier, following changes were need for the feature
>> to be visible in ID_AA64DFR0_EL1.
>>
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index 646591c67e7a..f258568535a8 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -445,6 +445,7 @@ static const struct arm64_ftr_bits ftr_id_mmfr0[] = {
>>  };
>>  
>>  static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
>> +       S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_IMP),
>>         S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_DoubleLock_SHIFT, 4, 0),
>>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_PMSVer_SHIFT, 4, 0),
>>         ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0),
>>
>> Should we add the following entry - explicitly hiding BRBE from the guest
>> as a prerequisite patch ?
>>
>> S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_NI)
> 
> Is it visbile currently, or is it hidden currently?
> 
> * If it is visible before this patch, that's a latent bug that we need to go
>   fix first, and that'll require more coordination.
> 
> * If it is not visible before this patch, there's no problem in the code, but
>   the commit message needs to explicitly mention that's the case as the commit
>   message currently implies it is visible by only mentioning hiding it.
> 
> ... so can you please double check as you suggested above? We should be able to
> explain why it is or is not visible today.

It is currently hidden i.e following code returns 1 in the host
but returns 0 inside the guest.

aa64dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
brbe = cpuid_feature_extract_unsigned_field(aa64dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT);

Hence - will update the commit message here as suggested.

> 
> Mark.
> 
>>>> This also blocks guest accesses into BRBE system registers and instructions
>>>> as if the underlying hardware never implemented FEAT_BRBE feature.
>>>>
>>>> Cc: Marc Zyngier <maz@kernel.org>
>>>> Cc: Oliver Upton <oliver.upton@linux.dev>
>>>> Cc: James Morse <james.morse@arm.com>
>>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Will Deacon <will@kernel.org>
>>>> Cc: kvmarm@lists.linux.dev
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>> ---
>>>> Changes in V16:
>>>>
>>>> - Added BRB_INF_SRC_TGT_EL1 macro for corresponding BRB_[INF|SRC|TGT] expansion
>>>>
>>>>  arch/arm64/kvm/sys_regs.c | 56 +++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 56 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>>> index 30253bd19917..6a06dc2f0c06 100644
>>>> --- a/arch/arm64/kvm/sys_regs.c
>>>> +++ b/arch/arm64/kvm/sys_regs.c
>>>> @@ -1304,6 +1304,11 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +#define BRB_INF_SRC_TGT_EL1(n)					\
>>>> +	{ SYS_DESC(SYS_BRBINF##n##_EL1), undef_access },	\
>>>> +	{ SYS_DESC(SYS_BRBSRC##n##_EL1), undef_access },	\
>>>> +	{ SYS_DESC(SYS_BRBTGT##n##_EL1), undef_access }		\
>>>
>>> With the changes suggested on the previous patch, this would need to change to be:
>>>
>>> 	#define BRB_INF_SRC_TGT_EL1(n)					\
>>> 		{ SYS_DESC(SYS_BRBINF_EL1(n)), undef_access },	\
>>> 		{ SYS_DESC(SYS_BRBSRC_EL1(n)), undef_access },	\
>>> 		{ SYS_DESC(SYS_BRBTGT_EL1(n)), undef_access }	\
>>
>> Sure, already folded back in these above changes.
>>
>>>
>>>
>>> ... which would also be easier for backporting (if necessary), since those
>>> definitions have existed for a while.
>>>
>>> Otherwise (modulo Suzuki's comment about rebasing), this looks good to me.
>>
>> Okay.
>>
>>>
>>> Mark.
>>>
>>>>  /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
>>>>  #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
>>>>  	{ SYS_DESC(SYS_DBGBVRn_EL1(n)),					\
>>>> @@ -1707,6 +1712,9 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>>>>  	/* Hide SPE from guests */
>>>>  	val &= ~ID_AA64DFR0_EL1_PMSVer_MASK;
>>>>  
>>>> +	/* Hide BRBE from guests */
>>>> +	val &= ~ID_AA64DFR0_EL1_BRBE_MASK;
>>>> +
>>>>  	return val;
>>>>  }
>>>>  
>>>> @@ -2195,6 +2203,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>>>  	{ SYS_DESC(SYS_DC_CISW), access_dcsw },
>>>>  	{ SYS_DESC(SYS_DC_CIGSW), access_dcgsw },
>>>>  	{ SYS_DESC(SYS_DC_CIGDSW), access_dcgsw },
>>>> +	{ SYS_DESC(OP_BRB_IALL), undef_access },
>>>> +	{ SYS_DESC(OP_BRB_INJ), undef_access },
>>>>  
>>>>  	DBG_BCR_BVR_WCR_WVR_EL1(0),
>>>>  	DBG_BCR_BVR_WCR_WVR_EL1(1),
>>>> @@ -2225,6 +2235,52 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>>>  	{ SYS_DESC(SYS_DBGCLAIMCLR_EL1), trap_raz_wi },
>>>>  	{ SYS_DESC(SYS_DBGAUTHSTATUS_EL1), trap_dbgauthstatus_el1 },
>>>>  
>>>> +	/*
>>>> +	 * BRBE branch record sysreg address space is interleaved between
>>>> +	 * corresponding BRBINF<N>_EL1, BRBSRC<N>_EL1, and BRBTGT<N>_EL1.
>>>> +	 */
>>>> +	BRB_INF_SRC_TGT_EL1(0),
>>>> +	BRB_INF_SRC_TGT_EL1(16),
>>>> +	BRB_INF_SRC_TGT_EL1(1),
>>>> +	BRB_INF_SRC_TGT_EL1(17),
>>>> +	BRB_INF_SRC_TGT_EL1(2),
>>>> +	BRB_INF_SRC_TGT_EL1(18),
>>>> +	BRB_INF_SRC_TGT_EL1(3),
>>>> +	BRB_INF_SRC_TGT_EL1(19),
>>>> +	BRB_INF_SRC_TGT_EL1(4),
>>>> +	BRB_INF_SRC_TGT_EL1(20),
>>>> +	BRB_INF_SRC_TGT_EL1(5),
>>>> +	BRB_INF_SRC_TGT_EL1(21),
>>>> +	BRB_INF_SRC_TGT_EL1(6),
>>>> +	BRB_INF_SRC_TGT_EL1(22),
>>>> +	BRB_INF_SRC_TGT_EL1(7),
>>>> +	BRB_INF_SRC_TGT_EL1(23),
>>>> +	BRB_INF_SRC_TGT_EL1(8),
>>>> +	BRB_INF_SRC_TGT_EL1(24),
>>>> +	BRB_INF_SRC_TGT_EL1(9),
>>>> +	BRB_INF_SRC_TGT_EL1(25),
>>>> +	BRB_INF_SRC_TGT_EL1(10),
>>>> +	BRB_INF_SRC_TGT_EL1(26),
>>>> +	BRB_INF_SRC_TGT_EL1(11),
>>>> +	BRB_INF_SRC_TGT_EL1(27),
>>>> +	BRB_INF_SRC_TGT_EL1(12),
>>>> +	BRB_INF_SRC_TGT_EL1(28),
>>>> +	BRB_INF_SRC_TGT_EL1(13),
>>>> +	BRB_INF_SRC_TGT_EL1(29),
>>>> +	BRB_INF_SRC_TGT_EL1(14),
>>>> +	BRB_INF_SRC_TGT_EL1(30),
>>>> +	BRB_INF_SRC_TGT_EL1(15),
>>>> +	BRB_INF_SRC_TGT_EL1(31),
>>>> +
>>>> +	/* Remaining BRBE sysreg addresses space */
>>>> +	{ SYS_DESC(SYS_BRBCR_EL1), undef_access },
>>>> +	{ SYS_DESC(SYS_BRBFCR_EL1), undef_access },
>>>> +	{ SYS_DESC(SYS_BRBTS_EL1), undef_access },
>>>> +	{ SYS_DESC(SYS_BRBINFINJ_EL1), undef_access },
>>>> +	{ SYS_DESC(SYS_BRBSRCINJ_EL1), undef_access },
>>>> +	{ SYS_DESC(SYS_BRBTGTINJ_EL1), undef_access },
>>>> +	{ SYS_DESC(SYS_BRBIDR0_EL1), undef_access },
>>>> +
>>>>  	{ SYS_DESC(SYS_MDCCSR_EL0), trap_raz_wi },
>>>>  	{ SYS_DESC(SYS_DBGDTR_EL0), trap_raz_wi },
>>>>  	// DBGDTR[TR]X_EL0 share the same encoding
>>>> -- 
>>>> 2.25.1
>>>>

Re: [PATCH V16 2/8] KVM: arm64: Prevent guest accesses into BRBE system registers/instructions

[Suzuki K Poulose]

On 27/02/2024 11:13, Anshuman Khandual wrote:
> 
> 
> On 2/27/24 15:34, Mark Rutland wrote:
>> On Fri, Feb 23, 2024 at 12:58:48PM +0530, Anshuman Khandual wrote:
>>>
>>>
>>> On 2/21/24 19:31, Mark Rutland wrote:
>>>> On Thu, Jan 25, 2024 at 03:11:13PM +0530, Anshuman Khandual wrote:
>>>>> Currently BRBE feature is not supported in a guest environment. This hides
>>>>> BRBE feature availability via masking ID_AA64DFR0_EL1.BRBE field.
>>>>
>>>> Does that means that a guest can currently see BRBE advertised in the
>>>> ID_AA64DFR0_EL1.BRB field, or is that hidden by the regular cpufeature code
>>>> today?
>>>
>>> IIRC it is hidden, but will have to double check. When experimenting for BRBE
>>> guest support enablement earlier, following changes were need for the feature
>>> to be visible in ID_AA64DFR0_EL1.
>>>
>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>> index 646591c67e7a..f258568535a8 100644
>>> --- a/arch/arm64/kernel/cpufeature.c
>>> +++ b/arch/arm64/kernel/cpufeature.c
>>> @@ -445,6 +445,7 @@ static const struct arm64_ftr_bits ftr_id_mmfr0[] = {
>>>   };
>>>   
>>>   static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
>>> +       S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_IMP),
>>>          S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_DoubleLock_SHIFT, 4, 0),
>>>          ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_PMSVer_SHIFT, 4, 0),
>>>          ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0),
>>>
>>> Should we add the following entry - explicitly hiding BRBE from the guest
>>> as a prerequisite patch ?

This has nothing to do with the Guest visibility of the BRBE. This is
specifically for host "userspace" (via MRS emulation).

>>>
>>> S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_NI)
>>
>> Is it visbile currently, or is it hidden currently?
>>
>> * If it is visible before this patch, that's a latent bug that we need to go
>>    fix first, and that'll require more coordination.
>>
>> * If it is not visible before this patch, there's no problem in the code, but
>>    the commit message needs to explicitly mention that's the case as the commit
>>    message currently implies it is visible by only mentioning hiding it.
>>
>> ... so can you please double check as you suggested above? We should be able to
>> explain why it is or is not visible today.
> 
> It is currently hidden i.e following code returns 1 in the host
> but returns 0 inside the guest.
> 
> aa64dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
> brbe = cpuid_feature_extract_unsigned_field(aa64dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT);
> 
> Hence - will update the commit message here as suggested.

This is by virtue of the masking we do in the kvm/sysreg.c below.

> 
>>
>> Mark.
>>
>>>>> This also blocks guest accesses into BRBE system registers and instructions
>>>>> as if the underlying hardware never implemented FEAT_BRBE feature.
>>>>>
>>>>> Cc: Marc Zyngier <maz@kernel.org>
>>>>> Cc: Oliver Upton <oliver.upton@linux.dev>
>>>>> Cc: James Morse <james.morse@arm.com>
>>>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>> Cc: Will Deacon <will@kernel.org>
>>>>> Cc: kvmarm@lists.linux.dev
>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>> Cc: linux-kernel@vger.kernel.org
>>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>>> ---
>>>>> Changes in V16:
>>>>>
>>>>> - Added BRB_INF_SRC_TGT_EL1 macro for corresponding BRB_[INF|SRC|TGT] expansion
>>>>>
>>>>>   arch/arm64/kvm/sys_regs.c | 56 +++++++++++++++++++++++++++++++++++++++
>>>>>   1 file changed, 56 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>>>> index 30253bd19917..6a06dc2f0c06 100644
>>>>> --- a/arch/arm64/kvm/sys_regs.c
>>>>> +++ b/arch/arm64/kvm/sys_regs.c
>>>>> @@ -1304,6 +1304,11 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
>>>>>   	return 0;
>>>>>   }
>>>>>   
>>>>> +#define BRB_INF_SRC_TGT_EL1(n)					\
>>>>> +	{ SYS_DESC(SYS_BRBINF##n##_EL1), undef_access },	\
>>>>> +	{ SYS_DESC(SYS_BRBSRC##n##_EL1), undef_access },	\
>>>>> +	{ SYS_DESC(SYS_BRBTGT##n##_EL1), undef_access }		\
>>>>
>>>> With the changes suggested on the previous patch, this would need to change to be:
>>>>
>>>> 	#define BRB_INF_SRC_TGT_EL1(n)					\
>>>> 		{ SYS_DESC(SYS_BRBINF_EL1(n)), undef_access },	\
>>>> 		{ SYS_DESC(SYS_BRBSRC_EL1(n)), undef_access },	\
>>>> 		{ SYS_DESC(SYS_BRBTGT_EL1(n)), undef_access }	\
>>>
>>> Sure, already folded back in these above changes.
>>>
>>>>
>>>>
>>>> ... which would also be easier for backporting (if necessary), since those
>>>> definitions have existed for a while.
>>>>
>>>> Otherwise (modulo Suzuki's comment about rebasing), this looks good to me.
>>>
>>> Okay.
>>>
>>>>
>>>> Mark.
>>>>
>>>>>   /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
>>>>>   #define DBG_BCR_BVR_WCR_WVR_EL1(n)					\
>>>>>   	{ SYS_DESC(SYS_DBGBVRn_EL1(n)),					\
>>>>> @@ -1707,6 +1712,9 @@ static u64 read_sanitised_id_aa64dfr0_el1(struct kvm_vcpu *vcpu,
>>>>>   	/* Hide SPE from guests */
>>>>>   	val &= ~ID_AA64DFR0_EL1_PMSVer_MASK;
>>>>>   
>>>>> +	/* Hide BRBE from guests */
>>>>> +	val &= ~ID_AA64DFR0_EL1_BRBE_MASK;
>>>>> +

This controls what the guest sees.

Suzuki


>>>>>   	return val;
>>>>>   }
>>>>>   
>>>>> @@ -2195,6 +2203,8 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>>>>   	{ SYS_DESC(SYS_DC_CISW), access_dcsw },
>>>>>   	{ SYS_DESC(SYS_DC_CIGSW), access_dcgsw },
>>>>>   	{ SYS_DESC(SYS_DC_CIGDSW), access_dcgsw },
>>>>> +	{ SYS_DESC(OP_BRB_IALL), undef_access },
>>>>> +	{ SYS_DESC(OP_BRB_INJ), undef_access },
>>>>>   
>>>>>   	DBG_BCR_BVR_WCR_WVR_EL1(0),
>>>>>   	DBG_BCR_BVR_WCR_WVR_EL1(1),
>>>>> @@ -2225,6 +2235,52 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>>>>>   	{ SYS_DESC(SYS_DBGCLAIMCLR_EL1), trap_raz_wi },
>>>>>   	{ SYS_DESC(SYS_DBGAUTHSTATUS_EL1), trap_dbgauthstatus_el1 },
>>>>>   
>>>>> +	/*
>>>>> +	 * BRBE branch record sysreg address space is interleaved between
>>>>> +	 * corresponding BRBINF<N>_EL1, BRBSRC<N>_EL1, and BRBTGT<N>_EL1.
>>>>> +	 */
>>>>> +	BRB_INF_SRC_TGT_EL1(0),
>>>>> +	BRB_INF_SRC_TGT_EL1(16),
>>>>> +	BRB_INF_SRC_TGT_EL1(1),
>>>>> +	BRB_INF_SRC_TGT_EL1(17),
>>>>> +	BRB_INF_SRC_TGT_EL1(2),
>>>>> +	BRB_INF_SRC_TGT_EL1(18),
>>>>> +	BRB_INF_SRC_TGT_EL1(3),
>>>>> +	BRB_INF_SRC_TGT_EL1(19),
>>>>> +	BRB_INF_SRC_TGT_EL1(4),
>>>>> +	BRB_INF_SRC_TGT_EL1(20),
>>>>> +	BRB_INF_SRC_TGT_EL1(5),
>>>>> +	BRB_INF_SRC_TGT_EL1(21),
>>>>> +	BRB_INF_SRC_TGT_EL1(6),
>>>>> +	BRB_INF_SRC_TGT_EL1(22),
>>>>> +	BRB_INF_SRC_TGT_EL1(7),
>>>>> +	BRB_INF_SRC_TGT_EL1(23),
>>>>> +	BRB_INF_SRC_TGT_EL1(8),
>>>>> +	BRB_INF_SRC_TGT_EL1(24),
>>>>> +	BRB_INF_SRC_TGT_EL1(9),
>>>>> +	BRB_INF_SRC_TGT_EL1(25),
>>>>> +	BRB_INF_SRC_TGT_EL1(10),
>>>>> +	BRB_INF_SRC_TGT_EL1(26),
>>>>> +	BRB_INF_SRC_TGT_EL1(11),
>>>>> +	BRB_INF_SRC_TGT_EL1(27),
>>>>> +	BRB_INF_SRC_TGT_EL1(12),
>>>>> +	BRB_INF_SRC_TGT_EL1(28),
>>>>> +	BRB_INF_SRC_TGT_EL1(13),
>>>>> +	BRB_INF_SRC_TGT_EL1(29),
>>>>> +	BRB_INF_SRC_TGT_EL1(14),
>>>>> +	BRB_INF_SRC_TGT_EL1(30),
>>>>> +	BRB_INF_SRC_TGT_EL1(15),
>>>>> +	BRB_INF_SRC_TGT_EL1(31),
>>>>> +
>>>>> +	/* Remaining BRBE sysreg addresses space */
>>>>> +	{ SYS_DESC(SYS_BRBCR_EL1), undef_access },
>>>>> +	{ SYS_DESC(SYS_BRBFCR_EL1), undef_access },
>>>>> +	{ SYS_DESC(SYS_BRBTS_EL1), undef_access },
>>>>> +	{ SYS_DESC(SYS_BRBINFINJ_EL1), undef_access },
>>>>> +	{ SYS_DESC(SYS_BRBSRCINJ_EL1), undef_access },
>>>>> +	{ SYS_DESC(SYS_BRBTGTINJ_EL1), undef_access },
>>>>> +	{ SYS_DESC(SYS_BRBIDR0_EL1), undef_access },
>>>>> +
>>>>>   	{ SYS_DESC(SYS_MDCCSR_EL0), trap_raz_wi },
>>>>>   	{ SYS_DESC(SYS_DBGDTR_EL0), trap_raz_wi },
>>>>>   	// DBGDTR[TR]X_EL0 share the same encoding
>>>>> -- 
>>>>> 2.25.1
>>>>>
> 

Re: [PATCH V16 2/8] KVM: arm64: Prevent guest accesses into BRBE system registers/instructions

[Mark Rutland]

Hi Suzuki,

On Thu, Feb 29, 2024 at 11:45:08AM +0000, Suzuki K Poulose wrote:
> On 27/02/2024 11:13, Anshuman Khandual wrote:
> > On 2/27/24 15:34, Mark Rutland wrote:
> > > On Fri, Feb 23, 2024 at 12:58:48PM +0530, Anshuman Khandual wrote:
> > > > On 2/21/24 19:31, Mark Rutland wrote:
> > > > > On Thu, Jan 25, 2024 at 03:11:13PM +0530, Anshuman Khandual wrote:
> > > > > > Currently BRBE feature is not supported in a guest environment. This hides
> > > > > > BRBE feature availability via masking ID_AA64DFR0_EL1.BRBE field.
> > > > > 
> > > > > Does that means that a guest can currently see BRBE advertised in the
> > > > > ID_AA64DFR0_EL1.BRB field, or is that hidden by the regular cpufeature code
> > > > > today?
> > > > 
> > > > IIRC it is hidden, but will have to double check. When experimenting for BRBE
> > > > guest support enablement earlier, following changes were need for the feature
> > > > to be visible in ID_AA64DFR0_EL1.
> > > > 
> > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > > index 646591c67e7a..f258568535a8 100644
> > > > --- a/arch/arm64/kernel/cpufeature.c
> > > > +++ b/arch/arm64/kernel/cpufeature.c
> > > > @@ -445,6 +445,7 @@ static const struct arm64_ftr_bits ftr_id_mmfr0[] = {
> > > >   };
> > > >   static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
> > > > +       S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_IMP),
> > > >          S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_DoubleLock_SHIFT, 4, 0),
> > > >          ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_PMSVer_SHIFT, 4, 0),
> > > >          ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0),
> > > > 
> > > > Should we add the following entry - explicitly hiding BRBE from the guest
> > > > as a prerequisite patch ?
> 
> This has nothing to do with the Guest visibility of the BRBE. This is
> specifically for host "userspace" (via MRS emulation).
> 
> > > > 
> > > > S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_NI)
> > > 
> > > Is it visbile currently, or is it hidden currently?
> > > 
> > > * If it is visible before this patch, that's a latent bug that we need to go
> > >    fix first, and that'll require more coordination.
> > > 
> > > * If it is not visible before this patch, there's no problem in the code, but
> > >    the commit message needs to explicitly mention that's the case as the commit
> > >    message currently implies it is visible by only mentioning hiding it.
> > > 
> > > ... so can you please double check as you suggested above? We should be able to
> > > explain why it is or is not visible today.
> > 
> > It is currently hidden i.e following code returns 1 in the host
> > but returns 0 inside the guest.
> > 
> > aa64dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
> > brbe = cpuid_feature_extract_unsigned_field(aa64dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT);
> > 
> > Hence - will update the commit message here as suggested.
> 
> This is by virtue of the masking we do in the kvm/sysreg.c below.

Yep, once this patch is applied.

I think we might have some crossed wires here; I'm only really asking for the
commit message (and title) to be updated and clarified.

Ignoring the patchlet above, and just considering the original patch:

IIUC before the patch is applied, the ID_AA64DFR0_EL1.BRBE field is zero for
the guest because we don't have an arm64_ftr_bits entry for the
ID_AA64DFR0_EL1.BRBE field, and so init_cpu_ftr_reg() will leave that as zero
in arm64_ftr_reg::sys_val, and hence when read_sanitised_id_aa64dfr0_el1()
calls read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1), the BRBE field will be zero.

This series as-is doesn't add an arm64_ftr_bits entry for ID_AA64DFR0_EL1.BRBE,
so it'd still be hidden from a guest regardless of whether we add explicit
masking in read_sanitised_id_aa64dfr0_el1(). The reason to add that masking is
to be explicit, so that if/when we add an arm64_ftr_bits entry for
ID_AA64DFR0_EL1.BRBE, it isn't exposed to a guest unexpectedly.

Similarly, IIUC the BRBE register accesses are *already* trapped, and
emulate_sys_reg() will log a warning an inject an UNDEFINED exception into the
guest if the guest tries to access the BRBE registers. Any well-behaved guest
*shouldn't* do that, but a poorly-behaved guest could do that and (slowly) spam
dmesg with messages about the unhandled sysreg traps. The reasons to handle
thos regs is largely to suppress that warning, and to make it clear that we
intend for those to be handled as undef.

So the commit title should be something like:

  KVM: arm64: explicitly handle BRBE register accesses as UNDEFINED

... and the message should mention the key points from the above.

Suzuki, does that sound right to you?

Anshuman, can you go re-write the commit message with that in mind?

Mark.

Re: [PATCH V16 2/8] KVM: arm64: Prevent guest accesses into BRBE system registers/instructions

[Suzuki K Poulose]

On 29/02/2024 12:50, Mark Rutland wrote:
> Hi Suzuki,
> 
> On Thu, Feb 29, 2024 at 11:45:08AM +0000, Suzuki K Poulose wrote:
>> On 27/02/2024 11:13, Anshuman Khandual wrote:
>>> On 2/27/24 15:34, Mark Rutland wrote:
>>>> On Fri, Feb 23, 2024 at 12:58:48PM +0530, Anshuman Khandual wrote:
>>>>> On 2/21/24 19:31, Mark Rutland wrote:
>>>>>> On Thu, Jan 25, 2024 at 03:11:13PM +0530, Anshuman Khandual wrote:
>>>>>>> Currently BRBE feature is not supported in a guest environment. This hides
>>>>>>> BRBE feature availability via masking ID_AA64DFR0_EL1.BRBE field.
>>>>>>
>>>>>> Does that means that a guest can currently see BRBE advertised in the
>>>>>> ID_AA64DFR0_EL1.BRB field, or is that hidden by the regular cpufeature code
>>>>>> today?
>>>>>
>>>>> IIRC it is hidden, but will have to double check. When experimenting for BRBE
>>>>> guest support enablement earlier, following changes were need for the feature
>>>>> to be visible in ID_AA64DFR0_EL1.
>>>>>
>>>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>>>> index 646591c67e7a..f258568535a8 100644
>>>>> --- a/arch/arm64/kernel/cpufeature.c
>>>>> +++ b/arch/arm64/kernel/cpufeature.c
>>>>> @@ -445,6 +445,7 @@ static const struct arm64_ftr_bits ftr_id_mmfr0[] = {
>>>>>    };
>>>>>    static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
>>>>> +       S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_IMP),
>>>>>           S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_DoubleLock_SHIFT, 4, 0),
>>>>>           ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_PMSVer_SHIFT, 4, 0),
>>>>>           ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0),
>>>>>
>>>>> Should we add the following entry - explicitly hiding BRBE from the guest
>>>>> as a prerequisite patch ?
>>
>> This has nothing to do with the Guest visibility of the BRBE. This is
>> specifically for host "userspace" (via MRS emulation).
>>
>>>>>
>>>>> S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_NI)
>>>>
>>>> Is it visbile currently, or is it hidden currently?
>>>>
>>>> * If it is visible before this patch, that's a latent bug that we need to go
>>>>     fix first, and that'll require more coordination.
>>>>
>>>> * If it is not visible before this patch, there's no problem in the code, but
>>>>     the commit message needs to explicitly mention that's the case as the commit
>>>>     message currently implies it is visible by only mentioning hiding it.
>>>>
>>>> ... so can you please double check as you suggested above? We should be able to
>>>> explain why it is or is not visible today.
>>>
>>> It is currently hidden i.e following code returns 1 in the host
>>> but returns 0 inside the guest.
>>>
>>> aa64dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
>>> brbe = cpuid_feature_extract_unsigned_field(aa64dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT);
>>>
>>> Hence - will update the commit message here as suggested.
>>
>> This is by virtue of the masking we do in the kvm/sysreg.c below.
> 
> Yep, once this patch is applied.
> 
> I think we might have some crossed wires here; I'm only really asking for the
> commit message (and title) to be updated and clarified.
> 
> Ignoring the patchlet above, and just considering the original patch:
> 
> IIUC before the patch is applied, the ID_AA64DFR0_EL1.BRBE field is zero for
> the guest because we don't have an arm64_ftr_bits entry for the
> ID_AA64DFR0_EL1.BRBE field, and so init_cpu_ftr_reg() will leave that as zero
> in arm64_ftr_reg::sys_val, and hence when read_sanitised_id_aa64dfr0_el1()
> calls read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1), the BRBE field will be zero.
> 
> This series as-is doesn't add an arm64_ftr_bits entry for ID_AA64DFR0_EL1.BRBE,
> so it'd still be hidden from a guest regardless of whether we add explicit
> masking in read_sanitised_id_aa64dfr0_el1(). The reason to add that masking is
> to be explicit, so that if/when we add an arm64_ftr_bits entry for
> ID_AA64DFR0_EL1.BRBE, it isn't exposed to a guest unexpectedly.
> 
> Similarly, IIUC the BRBE register accesses are *already* trapped, and
> emulate_sys_reg() will log a warning an inject an UNDEFINED exception into the
> guest if the guest tries to access the BRBE registers. Any well-behaved guest
> *shouldn't* do that, but a poorly-behaved guest could do that and (slowly) spam
> dmesg with messages about the unhandled sysreg traps. The reasons to handle
> thos regs is largely to suppress that warning, and to make it clear that we
> intend for those to be handled as undef.
> 
> So the commit title should be something like:
> 
>    KVM: arm64: explicitly handle BRBE register accesses as UNDEFINED
> 
> ... and the message should mention the key points from the above.
> 
> Suzuki, does that sound right to you?

Yes, that makes perfect sense to me. Thanks for clarifying

Suzuki

> 
> Anshuman, can you go re-write the commit message with that in mind?
> 
> Mark.

Re: [PATCH V16 5/8] KVM: arm64: nvhe: Disable branch generation in nVHE guests

[Marc Zyngier]

On Thu, 25 Jan 2024 09:41:16 +0000,
Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> 
> Disable the BRBE before we enter the guest, saving the status and enable it
> back once we get out of the guest. This avoids capturing branch records in
> the guest kernel or userspace, which would be confusing the host samples.
> 
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Oliver Upton <oliver.upton@linux.dev>
> Cc: James Morse <james.morse@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: kvmarm@lists.linux.dev
> Cc: linux-arm-kernel@lists.infradead.org
> CC: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> Changes in V16:
> 
> - Dropped BRBCR_EL1 and BRBFCR_EL1 from enum vcpu_sysreg
> - Reverted back the KVM NVHE patch - used host_debug_state based 'brbcr_el1'
>   element, and dropped the previous dependency on Jame's coresight series
> 
>  arch/arm64/include/asm/kvm_host.h  |  5 ++++-
>  arch/arm64/kvm/debug.c             |  5 +++++
>  arch/arm64/kvm/hyp/nvhe/debug-sr.c | 33 ++++++++++++++++++++++++++++++
>  3 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 21c57b812569..bce8792092af 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;
> @@ -610,6 +610,7 @@ struct kvm_vcpu_arch {
>  		u64 pmscr_el1;
>  		/* Self-hosted trace */
>  		u64 trfcr_el1;
> +		u64 brbcr_el1;
>  	} host_debug_state;
>  
>  	/* VGIC state */
> @@ -779,6 +780,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 BRBE context if active  */
> +#define DEBUG_STATE_SAVE_BRBE	__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..99f85d8acbf3 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -335,10 +335,15 @@ 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);
> +
> +	/* Check if we have BRBE implemented and available at the host */
> +	if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT))
> +		vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_BRBE);
>  }
>  
>  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_BRBE);
>  }
> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> index 4558c02eb352..79bcf0fb1326 100644
> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> @@ -79,6 +79,34 @@ static void __debug_restore_trace(u64 trfcr_el1)
>  	write_sysreg_s(trfcr_el1, SYS_TRFCR_EL1);
>  }
>  
> +static void __debug_save_brbe(u64 *brbcr_el1)
> +{
> +	*brbcr_el1 = 0;
> +
> +	/* Check if the BRBE is enabled */
> +	if (!(read_sysreg_s(SYS_BRBCR_EL1) & (BRBCR_ELx_E0BRE | BRBCR_ELx_ExBRE)))
> +		return;
> +
> +	/*
> +	 * Prohibit branch record generation while we are in guest.
> +	 * Since access to BRBCR_EL1 is trapped, the guest can't
> +	 * modify the filtering set by the host.
> +	 */
> +	*brbcr_el1 = read_sysreg_s(SYS_BRBCR_EL1);
> +	write_sysreg_s(0, SYS_BRBCR_EL1);

As for TRFCR and PMSCR, this is broken on hVHE.

Please see [1]

	M.

[1] https://lore.kernel.org/r/20240229145417.3606279-1-maz@kernel.org

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

Re: [PATCH V16 5/8] KVM: arm64: nvhe: Disable branch generation in nVHE guests

[Anshuman Khandual]

On 3/1/24 00:10, Marc Zyngier wrote:
> On Thu, 25 Jan 2024 09:41:16 +0000,
> Anshuman Khandual <anshuman.khandual@arm.com> wrote:
>>
>> Disable the BRBE before we enter the guest, saving the status and enable it
>> back once we get out of the guest. This avoids capturing branch records in
>> the guest kernel or userspace, which would be confusing the host samples.
>>
>> Cc: Marc Zyngier <maz@kernel.org>
>> Cc: Oliver Upton <oliver.upton@linux.dev>
>> Cc: James Morse <james.morse@arm.com>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: kvmarm@lists.linux.dev
>> Cc: linux-arm-kernel@lists.infradead.org
>> CC: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>> Changes in V16:
>>
>> - Dropped BRBCR_EL1 and BRBFCR_EL1 from enum vcpu_sysreg
>> - Reverted back the KVM NVHE patch - used host_debug_state based 'brbcr_el1'
>>   element, and dropped the previous dependency on Jame's coresight series
>>
>>  arch/arm64/include/asm/kvm_host.h  |  5 ++++-
>>  arch/arm64/kvm/debug.c             |  5 +++++
>>  arch/arm64/kvm/hyp/nvhe/debug-sr.c | 33 ++++++++++++++++++++++++++++++
>>  3 files changed, 42 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 21c57b812569..bce8792092af 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;
>> @@ -610,6 +610,7 @@ struct kvm_vcpu_arch {
>>  		u64 pmscr_el1;
>>  		/* Self-hosted trace */
>>  		u64 trfcr_el1;
>> +		u64 brbcr_el1;
>>  	} host_debug_state;
>>  
>>  	/* VGIC state */
>> @@ -779,6 +780,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 BRBE context if active  */
>> +#define DEBUG_STATE_SAVE_BRBE	__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..99f85d8acbf3 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -335,10 +335,15 @@ 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);
>> +
>> +	/* Check if we have BRBE implemented and available at the host */
>> +	if (cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT))
>> +		vcpu_set_flag(vcpu, DEBUG_STATE_SAVE_BRBE);
>>  }
>>  
>>  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_BRBE);
>>  }
>> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> index 4558c02eb352..79bcf0fb1326 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> @@ -79,6 +79,34 @@ static void __debug_restore_trace(u64 trfcr_el1)
>>  	write_sysreg_s(trfcr_el1, SYS_TRFCR_EL1);
>>  }
>>  
>> +static void __debug_save_brbe(u64 *brbcr_el1)
>> +{
>> +	*brbcr_el1 = 0;
>> +
>> +	/* Check if the BRBE is enabled */
>> +	if (!(read_sysreg_s(SYS_BRBCR_EL1) & (BRBCR_ELx_E0BRE | BRBCR_ELx_ExBRE)))
>> +		return;
>> +
>> +	/*
>> +	 * Prohibit branch record generation while we are in guest.
>> +	 * Since access to BRBCR_EL1 is trapped, the guest can't
>> +	 * modify the filtering set by the host.
>> +	 */
>> +	*brbcr_el1 = read_sysreg_s(SYS_BRBCR_EL1);
>> +	write_sysreg_s(0, SYS_BRBCR_EL1);
> 
> As for TRFCR and PMSCR, this is broken on hVHE.
> 
> Please see [1]
> 
> 	M.
> 
> [1] https://lore.kernel.org/r/20240229145417.3606279-1-maz@kernel.org
> 

Ahh I see, so the unified accessors read_sysreg_el1()/write_sysreg_el1()
need to be used here - which will choose between BRBCR_EL1 & BRBCR_EL12
as required. Will do the changes, thanks for pointing out.

Re: [PATCH V16 2/8] KVM: arm64: Prevent guest accesses into BRBE system registers/instructions

[Anshuman Khandual]

On 2/29/24 18:20, Mark Rutland wrote:
> Hi Suzuki,
> 
> On Thu, Feb 29, 2024 at 11:45:08AM +0000, Suzuki K Poulose wrote:
>> On 27/02/2024 11:13, Anshuman Khandual wrote:
>>> On 2/27/24 15:34, Mark Rutland wrote:
>>>> On Fri, Feb 23, 2024 at 12:58:48PM +0530, Anshuman Khandual wrote:
>>>>> On 2/21/24 19:31, Mark Rutland wrote:
>>>>>> On Thu, Jan 25, 2024 at 03:11:13PM +0530, Anshuman Khandual wrote:
>>>>>>> Currently BRBE feature is not supported in a guest environment. This hides
>>>>>>> BRBE feature availability via masking ID_AA64DFR0_EL1.BRBE field.
>>>>>>
>>>>>> Does that means that a guest can currently see BRBE advertised in the
>>>>>> ID_AA64DFR0_EL1.BRB field, or is that hidden by the regular cpufeature code
>>>>>> today?
>>>>>
>>>>> IIRC it is hidden, but will have to double check. When experimenting for BRBE
>>>>> guest support enablement earlier, following changes were need for the feature
>>>>> to be visible in ID_AA64DFR0_EL1.
>>>>>
>>>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>>>> index 646591c67e7a..f258568535a8 100644
>>>>> --- a/arch/arm64/kernel/cpufeature.c
>>>>> +++ b/arch/arm64/kernel/cpufeature.c
>>>>> @@ -445,6 +445,7 @@ static const struct arm64_ftr_bits ftr_id_mmfr0[] = {
>>>>>   };
>>>>>   static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
>>>>> +       S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_IMP),
>>>>>          S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_DoubleLock_SHIFT, 4, 0),
>>>>>          ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_PMSVer_SHIFT, 4, 0),
>>>>>          ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0),
>>>>>
>>>>> Should we add the following entry - explicitly hiding BRBE from the guest
>>>>> as a prerequisite patch ?
>>
>> This has nothing to do with the Guest visibility of the BRBE. This is
>> specifically for host "userspace" (via MRS emulation).
>>
>>>>>
>>>>> S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_NI)
>>>>
>>>> Is it visbile currently, or is it hidden currently?
>>>>
>>>> * If it is visible before this patch, that's a latent bug that we need to go
>>>>    fix first, and that'll require more coordination.
>>>>
>>>> * If it is not visible before this patch, there's no problem in the code, but
>>>>    the commit message needs to explicitly mention that's the case as the commit
>>>>    message currently implies it is visible by only mentioning hiding it.
>>>>
>>>> ... so can you please double check as you suggested above? We should be able to
>>>> explain why it is or is not visible today.
>>>
>>> It is currently hidden i.e following code returns 1 in the host
>>> but returns 0 inside the guest.
>>>
>>> aa64dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
>>> brbe = cpuid_feature_extract_unsigned_field(aa64dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT);
>>>
>>> Hence - will update the commit message here as suggested.
>>
>> This is by virtue of the masking we do in the kvm/sysreg.c below.
> 
> Yep, once this patch is applied.
> 
> I think we might have some crossed wires here; I'm only really asking for the
> commit message (and title) to be updated and clarified.

Understood.

> 
> Ignoring the patchlet above, and just considering the original patch:
> 
> IIUC before the patch is applied, the ID_AA64DFR0_EL1.BRBE field is zero for
> the guest because we don't have an arm64_ftr_bits entry for the
> ID_AA64DFR0_EL1.BRBE field, and so init_cpu_ftr_reg() will leave that as zero
> in arm64_ftr_reg::sys_val, and hence when read_sanitised_id_aa64dfr0_el1()
> calls read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1), the BRBE field will be zero.

Makes sense, but should not arm64_ftr_reg::sys_val be explicitly set to '0' via
ID_AA64DFR0_EL1_BRBE_NI via adding a S_ARM64_FTR_BITS() into ftr_id_aa64dfr0[] ?
OR because it's going to be made visible via S_ARM64_FTR_BITS(FTR_VISIBLE
, ...., ID_AA64DFR0_EL1_BRBE_IMP) for enabling it in the guest, this might not be
necessary for now. Besides it is also being blocked explicitly now via this patch
in read_sanitised_id_aa64dfr0_el1().

> 
> This series as-is doesn't add an arm64_ftr_bits entry for ID_AA64DFR0_EL1.BRBE,
> so it'd still be hidden from a guest regardless of whether we add explicit
> masking in read_sanitised_id_aa64dfr0_el1(). The reason to add that masking is
> to be explicit, so that if/when we add an arm64_ftr_bits entry for
> ID_AA64DFR0_EL1.BRBE, it isn't exposed to a guest unexpectedly.

> 
> Similarly, IIUC the BRBE register accesses are *already* trapped, and
> emulate_sys_reg() will log a warning an inject an UNDEFINED exception into the
> guest if the guest tries to access the BRBE registers. Any well-behaved guest
> *shouldn't* do that, but a poorly-behaved guest could do that and (slowly) spam
> dmesg with messages about the unhandled sysreg traps. The reasons to handle
> thos regs is largely to suppress that warning, and to make it clear that we
> intend for those to be handled as undef.

Understood.

> 
> So the commit title should be something like:
> 
>   KVM: arm64: explicitly handle BRBE register accesses as UNDEFINED
> 
> ... and the message should mention the key points from the above.
> 
> Suzuki, does that sound right to you?
> 
> Anshuman, can you go re-write the commit message with that in mind?

Sure, will something like the following be okay ?

KVM: arm64: Explicitly handle BRBE register accesses as UNDEFINED

Although ID_AA64DFR0_EL1.BRBE field is zero for the guest because there is
no arm64_ftr_bits[] entry for the ID_AA64DFR0_EL1.BRBE field while getting
processed for read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1), this masks BRBE
feature here to be rather explicit. This will prevent unexpected exposure
of BRBE feature to guest when arm64_ftr_bits[] changes for ID_AA64DFR0_EL1.
This also makes all guest accesses into BRBE registers, and instructions
as undefined access explicitly.

Re: [PATCH V16 2/8] KVM: arm64: Prevent guest accesses into BRBE system registers/instructions

[Mark Rutland]

On Fri, Mar 01, 2024 at 01:16:10PM +0530, Anshuman Khandual wrote:
> 
> On 2/29/24 18:20, Mark Rutland wrote:
> > Hi Suzuki,
> > 
> > On Thu, Feb 29, 2024 at 11:45:08AM +0000, Suzuki K Poulose wrote:
> >> On 27/02/2024 11:13, Anshuman Khandual wrote:
> >>> On 2/27/24 15:34, Mark Rutland wrote:
> >>>> On Fri, Feb 23, 2024 at 12:58:48PM +0530, Anshuman Khandual wrote:
> >>>>> On 2/21/24 19:31, Mark Rutland wrote:
> >>>>>> On Thu, Jan 25, 2024 at 03:11:13PM +0530, Anshuman Khandual wrote:
> >>>>>>> Currently BRBE feature is not supported in a guest environment. This hides
> >>>>>>> BRBE feature availability via masking ID_AA64DFR0_EL1.BRBE field.
> >>>>>>
> >>>>>> Does that means that a guest can currently see BRBE advertised in the
> >>>>>> ID_AA64DFR0_EL1.BRB field, or is that hidden by the regular cpufeature code
> >>>>>> today?
> >>>>>
> >>>>> IIRC it is hidden, but will have to double check. When experimenting for BRBE
> >>>>> guest support enablement earlier, following changes were need for the feature
> >>>>> to be visible in ID_AA64DFR0_EL1.
> >>>>>
> >>>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> >>>>> index 646591c67e7a..f258568535a8 100644
> >>>>> --- a/arch/arm64/kernel/cpufeature.c
> >>>>> +++ b/arch/arm64/kernel/cpufeature.c
> >>>>> @@ -445,6 +445,7 @@ static const struct arm64_ftr_bits ftr_id_mmfr0[] = {
> >>>>>   };
> >>>>>   static const struct arm64_ftr_bits ftr_id_aa64dfr0[] = {
> >>>>> +       S_ARM64_FTR_BITS(FTR_VISIBLE, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_IMP),
> >>>>>          S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_DoubleLock_SHIFT, 4, 0),
> >>>>>          ARM64_FTR_BITS(FTR_HIDDEN, FTR_NONSTRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_PMSVer_SHIFT, 4, 0),
> >>>>>          ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_CTX_CMPs_SHIFT, 4, 0),
> >>>>>
> >>>>> Should we add the following entry - explicitly hiding BRBE from the guest
> >>>>> as a prerequisite patch ?
> >>
> >> This has nothing to do with the Guest visibility of the BRBE. This is
> >> specifically for host "userspace" (via MRS emulation).
> >>
> >>>>>
> >>>>> S_ARM64_FTR_BITS(FTR_HIDDEN, FTR_STRICT, FTR_LOWER_SAFE, ID_AA64DFR0_EL1_BRBE_SHIFT, 4, ID_AA64DFR0_EL1_BRBE_NI)
> >>>>
> >>>> Is it visbile currently, or is it hidden currently?
> >>>>
> >>>> * If it is visible before this patch, that's a latent bug that we need to go
> >>>>    fix first, and that'll require more coordination.
> >>>>
> >>>> * If it is not visible before this patch, there's no problem in the code, but
> >>>>    the commit message needs to explicitly mention that's the case as the commit
> >>>>    message currently implies it is visible by only mentioning hiding it.
> >>>>
> >>>> ... so can you please double check as you suggested above? We should be able to
> >>>> explain why it is or is not visible today.
> >>>
> >>> It is currently hidden i.e following code returns 1 in the host
> >>> but returns 0 inside the guest.
> >>>
> >>> aa64dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
> >>> brbe = cpuid_feature_extract_unsigned_field(aa64dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT);
> >>>
> >>> Hence - will update the commit message here as suggested.
> >>
> >> This is by virtue of the masking we do in the kvm/sysreg.c below.
> > 
> > Yep, once this patch is applied.
> > 
> > I think we might have some crossed wires here; I'm only really asking for the
> > commit message (and title) to be updated and clarified.
> 
> Understood.
> 
> > Ignoring the patchlet above, and just considering the original patch:
> > 
> > IIUC before the patch is applied, the ID_AA64DFR0_EL1.BRBE field is zero for
> > the guest because we don't have an arm64_ftr_bits entry for the
> > ID_AA64DFR0_EL1.BRBE field, and so init_cpu_ftr_reg() will leave that as zero
> > in arm64_ftr_reg::sys_val, and hence when read_sanitised_id_aa64dfr0_el1()
> > calls read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1), the BRBE field will be zero.
> 
> Makes sense, but should not arm64_ftr_reg::sys_val be explicitly set to '0' via
> ID_AA64DFR0_EL1_BRBE_NI via adding a S_ARM64_FTR_BITS() into ftr_id_aa64dfr0[] ?

I don't understand what you're asking here -- there's no way that a
arm64_ftr_bits entry can explicitly zero a field.

> OR because it's going to be made visible via S_ARM64_FTR_BITS(FTR_VISIBLE
> , ...., ID_AA64DFR0_EL1_BRBE_IMP) for enabling it in the guest, this might not be
> necessary for now. Besides it is also being blocked explicitly now via this patch
> in read_sanitised_id_aa64dfr0_el1().

We are not going to add a FTR_VISIBLE entry -- as Suzuki already pointed out,
that means *visible to userspace*.

We currently have no need for an arm64_ftr_bits entry for BRBE. We can add one
for the sake of documenting our policy for that field, like we do for PMUVer,
but that's the only reason to do so, and doing that requires that we also mask
the field within read_sanitised_id_aa64dfr0_el1().

> > So the commit title should be something like:
> > 
> >   KVM: arm64: explicitly handle BRBE register accesses as UNDEFINED
> > 
> > ... and the message should mention the key points from the above.
> > 
> > Suzuki, does that sound right to you?
> > 
> > Anshuman, can you go re-write the commit message with that in mind?
> 
> Sure, will something like the following be okay ?
> 
> KVM: arm64: Explicitly handle BRBE register accesses as UNDEFINED
> 
> Although ID_AA64DFR0_EL1.BRBE field is zero for the guest because there is
> no arm64_ftr_bits[] entry for the ID_AA64DFR0_EL1.BRBE field while getting
> processed for read_sanitised_ftr_reg(SYS_ID_AA64DFR0_EL1), this masks BRBE
> feature here to be rather explicit. This will prevent unexpected exposure
> of BRBE feature to guest when arm64_ftr_bits[] changes for ID_AA64DFR0_EL1.
> This also makes all guest accesses into BRBE registers, and instructions
> as undefined access explicitly.

How about:

| KVM: arm64: Explicitly handle BRBE traps as UNDEFINED
|
| The Branch Record Buffer Extension (BRBE) adds a number of system registers
| and instructions which we don't currently intend to expose to guests. Our
| existing logic handles this safely, but could be improved with some explicit
| handling of BRBE.
|
| The presence of BRBE is currently hidden from guests as the cpufeature code's
| ftr_id_aa64dfr0[] table doesn't have an entry for the BRBE field, and so this
| will be zero in the sanitised value of ID_AA64DFR0 exposed to guests via
| read_sanitised_id_aa64dfr0_el1(). As the ftr_id_aa64dfr0[] table may gain an
| entry for the BRBE field in future, for robustness we should explicitly mask
| out the BRBE field in read_sanitised_id_aa64dfr0_el1().
|
| The BRBE system registers and instructions are currently trapped by the
| existing configuration of the fine-grained traps. As the registers and
| instructions are not described in the sys_reg_descs[] table,
| emulate_sys_reg() will warn that these are unknown before injecting an
| UNDEFINED exception into the guest. Well-behaved guests shouldn't try to use
| the registers or instructions, but badly-behaved guests could, these,
| resulting in unnecessary warnings. To avoid those warnings, we should
| explicitly handle the BRBE registers and instructions as UNDEFINED.
|
| Address the above by having read_sanitised_id_aa64dfr0_el1() mask out the
| ID_AA64DFR0.BRBE field, and by adding sys_reg_desc entries for all of the
| BRBE system registers and instructions, treating these all as UNDEFINED.

Mark.