[PATCH V18 2/9] KVM: arm64: Explicitly handle BRBE traps as UNDEFINED

[PATCH V18 2/9] KVM: arm64: Explicitly handle BRBE traps as UNDEFINED

[Anshuman Khandual]

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 this 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 neither the registers
nor the instructions are 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 use 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 explicitly handling all of the BRBE system
registers and instructions as UNDEFINED.

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 V18:

- Updated the commit message

 arch/arm64/kvm/sys_regs.c | 56 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
---
 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 22b45a15d068..3d4686abe5ee 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_EL1(n)), undef_access },	\
+	{ SYS_DESC(SYS_BRBSRC_EL1(n)), undef_access },	\
+	{ SYS_DESC(SYS_BRBTGT_EL1(n)), 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)),					\
@@ -1722,6 +1727,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;
 }
 
@@ -2240,6 +2248,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
@@ -2751,6 +2805,8 @@ static struct sys_reg_desc sys_insn_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 },
 };
 
 static const struct sys_reg_desc *first_idreg;
-- 
2.25.1

Re: [PATCH V18 2/9] KVM: arm64: Explicitly handle BRBE traps as UNDEFINED

[Mark Rutland]

On Thu, Jun 13, 2024 at 11:47:24AM +0530, Anshuman Khandual wrote:
> 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 this 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 neither the registers
> nor the instructions are 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 use 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 explicitly handling all of the BRBE system
> registers and instructions as UNDEFINED.
> 
> 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>

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

> ----
> Changes in V18:
> 
> - Updated the commit message
> 
>  arch/arm64/kvm/sys_regs.c | 56 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 56 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)

Something has gone wrong here, but that doesn't affect the patch itself.

Mark.

> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 22b45a15d068..3d4686abe5ee 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_EL1(n)), undef_access },	\
> +	{ SYS_DESC(SYS_BRBSRC_EL1(n)), undef_access },	\
> +	{ SYS_DESC(SYS_BRBTGT_EL1(n)), 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)),					\
> @@ -1722,6 +1727,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;
>  }
>  
> @@ -2240,6 +2248,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
> @@ -2751,6 +2805,8 @@ static struct sys_reg_desc sys_insn_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 },
>  };
>  
>  static const struct sys_reg_desc *first_idreg;
> -- 
> 2.25.1
> 

Re: [PATCH V18 2/9] KVM: arm64: Explicitly handle BRBE traps as UNDEFINED

[Marc Zyngier]

On Thu, 13 Jun 2024 07:17:24 +0100,
Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> 
> 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 this 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 neither the registers
> nor the instructions are 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 use 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 explicitly handling all of the BRBE system
> registers and instructions as UNDEFINED.
> 
> 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 V18:
> 
> - Updated the commit message
> 
>  arch/arm64/kvm/sys_regs.c | 56 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> ---
>  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 22b45a15d068..3d4686abe5ee 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_EL1(n)), undef_access },	\
> +	{ SYS_DESC(SYS_BRBSRC_EL1(n)), undef_access },	\
> +	{ SYS_DESC(SYS_BRBTGT_EL1(n)), 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)),					\
> @@ -1722,6 +1727,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;
>  }
>  
> @@ -2240,6 +2248,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
> @@ -2751,6 +2805,8 @@ static struct sys_reg_desc sys_insn_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 },
>  };
>  
>  static const struct sys_reg_desc *first_idreg;

I don't think we need any update to the sys_reg table to handle
this. Instead, we should make use of the FGU infrastructure that has
been in since 6.9 to make this stuff UNDEF unconditionally.

It should be as simple as:

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index ee33f5467ce5..7cafe3f72c01 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -4964,6 +4964,11 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu)
 		kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 |
 						  HAFGRTR_EL2_RES1);
 
+	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, BRBE, IMP))
+		kvm->arch.fgu[HDFGRTR_GROUP] |= (HDFGRTR_nBRBDATA |
+						 HDFGRTR_nBRBCTL  |
+						 HDFGRTR_nBRBIDR);
+
 	set_bit(KVM_ARCH_FLAG_FGU_INITIALIZED, &kvm->arch.flags);
 out:
 	mutex_unlock(&kvm->arch.config_lock);

which is of course untested, but that I expect to be correct.

Thanks,

	M.

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

Re: [PATCH V18 2/9] KVM: arm64: Explicitly handle BRBE traps as UNDEFINED

[Marc Zyngier]

On Fri, 14 Jun 2024 13:33:37 +0100,
Marc Zyngier <maz@kernel.org> wrote:
> 
> On Thu, 13 Jun 2024 07:17:24 +0100,
> Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> > 
> > 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 this 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 neither the registers
> > nor the instructions are 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 use 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 explicitly handling all of the BRBE system
> > registers and instructions as UNDEFINED.
> > 
> > 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 V18:
> > 
> > - Updated the commit message
> > 
> >  arch/arm64/kvm/sys_regs.c | 56 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> > Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> > ---
> >  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 22b45a15d068..3d4686abe5ee 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_EL1(n)), undef_access },	\
> > +	{ SYS_DESC(SYS_BRBSRC_EL1(n)), undef_access },	\
> > +	{ SYS_DESC(SYS_BRBTGT_EL1(n)), 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)),					\
> > @@ -1722,6 +1727,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;
> >  }
> >  
> > @@ -2240,6 +2248,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
> > @@ -2751,6 +2805,8 @@ static struct sys_reg_desc sys_insn_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 },
> >  };
> >  
> >  static const struct sys_reg_desc *first_idreg;
> 
> I don't think we need any update to the sys_reg table to handle
> this. Instead, we should make use of the FGU infrastructure that has
> been in since 6.9 to make this stuff UNDEF unconditionally.
> 
> It should be as simple as:
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index ee33f5467ce5..7cafe3f72c01 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -4964,6 +4964,11 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu)
>  		kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 |
>  						  HAFGRTR_EL2_RES1);
>  
> +	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, BRBE, IMP))
> +		kvm->arch.fgu[HDFGRTR_GROUP] |= (HDFGRTR_nBRBDATA |
> +						 HDFGRTR_nBRBCTL  |
> +						 HDFGRTR_nBRBIDR);
> +
>  	set_bit(KVM_ARCH_FLAG_FGU_INITIALIZED, &kvm->arch.flags);
>  out:
>  	mutex_unlock(&kvm->arch.config_lock);
> 
> which is of course untested, but that I expect to be correct.

Actually, to disable the *instructions*, a similar hack must be
applied to HFGITR_EL2. The resulting patch should be something like:

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index ee33f5467ce5..49d86dae8d80 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -4964,6 +4964,15 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu)
 		kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 |
 						  HAFGRTR_EL2_RES1);
 
+	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, BRBE, IMP)) {
+		kvm->arch.fgu[HDFGRTR_GROUP] |= (HDFGRTR_nBRBDATA  |
+						 HDFGRTR_nBRBCTL   |
+						 HDFGRTR_nBRBIDR);
+		kvm->arch.fgu[HFGITR_GROUP] |= (HFGITR_EL2_nBRBINJ |
+						HFGITR_EL2_nBRBIALL);
+	}
+
+
 	set_bit(KVM_ARCH_FLAG_FGU_INITIALIZED, &kvm->arch.flags);
 out:
 	mutex_unlock(&kvm->arch.config_lock);

The implicit dependency here is that FGT is always present on a system
that implements BRBE. The architecture supports this assertion:

- BRBE is not available before ARMv9.1
- FGT is mandatory from ARMv8.6

Given that v9.1 is congruent to v8.6, we have the required overlap.

Thanks,

	M.

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

Re: [PATCH V18 2/9] KVM: arm64: Explicitly handle BRBE traps as UNDEFINED

[Anshuman Khandual]

On 6/14/24 18:39, Marc Zyngier wrote:
> On Fri, 14 Jun 2024 13:33:37 +0100,
> Marc Zyngier <maz@kernel.org> wrote:
>>
>> On Thu, 13 Jun 2024 07:17:24 +0100,
>> Anshuman Khandual <anshuman.khandual@arm.com> wrote:
>>>
>>> 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 this 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 neither the registers
>>> nor the instructions are 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 use 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 explicitly handling all of the BRBE system
>>> registers and instructions as UNDEFINED.
>>>
>>> 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 V18:
>>>
>>> - Updated the commit message
>>>
>>>  arch/arm64/kvm/sys_regs.c | 56 +++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 56 insertions(+)
>>> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
>>> ---
>>>  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 22b45a15d068..3d4686abe5ee 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_EL1(n)), undef_access },	\
>>> +	{ SYS_DESC(SYS_BRBSRC_EL1(n)), undef_access },	\
>>> +	{ SYS_DESC(SYS_BRBTGT_EL1(n)), 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)),					\
>>> @@ -1722,6 +1727,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;
>>>  }
>>>  
>>> @@ -2240,6 +2248,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
>>> @@ -2751,6 +2805,8 @@ static struct sys_reg_desc sys_insn_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 },
>>>  };
>>>  
>>>  static const struct sys_reg_desc *first_idreg;
>>
>> I don't think we need any update to the sys_reg table to handle
>> this. Instead, we should make use of the FGU infrastructure that has
>> been in since 6.9 to make this stuff UNDEF unconditionally.
>>
>> It should be as simple as:
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index ee33f5467ce5..7cafe3f72c01 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -4964,6 +4964,11 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu)
>>  		kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 |
>>  						  HAFGRTR_EL2_RES1);
>>  
>> +	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, BRBE, IMP))
>> +		kvm->arch.fgu[HDFGRTR_GROUP] |= (HDFGRTR_nBRBDATA |
>> +						 HDFGRTR_nBRBCTL  |
>> +						 HDFGRTR_nBRBIDR);
>> +
>>  	set_bit(KVM_ARCH_FLAG_FGU_INITIALIZED, &kvm->arch.flags);
>>  out:
>>  	mutex_unlock(&kvm->arch.config_lock);
>>
>> which is of course untested, but that I expect to be correct.
> 
> Actually, to disable the *instructions*, a similar hack must be
> applied to HFGITR_EL2. The resulting patch should be something like:
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index ee33f5467ce5..49d86dae8d80 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -4964,6 +4964,15 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu)
>  		kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 |
>  						  HAFGRTR_EL2_RES1);
>  
> +	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, BRBE, IMP)) {
> +		kvm->arch.fgu[HDFGRTR_GROUP] |= (HDFGRTR_nBRBDATA  |
> +						 HDFGRTR_nBRBCTL   |
> +						 HDFGRTR_nBRBIDR);
> +		kvm->arch.fgu[HFGITR_GROUP] |= (HFGITR_EL2_nBRBINJ |
> +						HFGITR_EL2_nBRBIALL);
> +	}
> +
> +
>  	set_bit(KVM_ARCH_FLAG_FGU_INITIALIZED, &kvm->arch.flags);
>  out:
>  	mutex_unlock(&kvm->arch.config_lock);

This makes sense, will remove all the changes to sys_reg table and
instead fold the above suggestion into the patch.

> 
> The implicit dependency here is that FGT is always present on a system
> that implements BRBE. The architecture supports this assertion:
> 
> - BRBE is not available before ARMv9.1
> - FGT is mandatory from ARMv8.6
> 
> Given that v9.1 is congruent to v8.6, we have the required overlap.

So this overlap need not be asserted in software again ?

Re: [PATCH V18 2/9] KVM: arm64: Explicitly handle BRBE traps as UNDEFINED

[Marc Zyngier]

On Mon, 17 Jun 2024 07:27:13 +0100,
Anshuman Khandual <anshuman.khandual@arm.com> wrote:
> On 6/14/24 18:39, Marc Zyngier wrote:
> > On Fri, 14 Jun 2024 13:33:37 +0100,
> >
> > Actually, to disable the *instructions*, a similar hack must be
> > applied to HFGITR_EL2. The resulting patch should be something like:
> > 
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index ee33f5467ce5..49d86dae8d80 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -4964,6 +4964,15 @@ void kvm_init_sysreg(struct kvm_vcpu *vcpu)
> >  		kvm->arch.fgu[HAFGRTR_GROUP] |= ~(HAFGRTR_EL2_RES0 |
> >  						  HAFGRTR_EL2_RES1);
> >  
> > +	if (!kvm_has_feat(kvm, ID_AA64DFR0_EL1, BRBE, IMP)) {
> > +		kvm->arch.fgu[HDFGRTR_GROUP] |= (HDFGRTR_nBRBDATA  |
> > +						 HDFGRTR_nBRBCTL   |
> > +						 HDFGRTR_nBRBIDR);

Obviously, this needs to be spelled HDFGRTR_EL2_nBRB* so that it
actually compiles.

> > +		kvm->arch.fgu[HFGITR_GROUP] |= (HFGITR_EL2_nBRBINJ |
> > +						HFGITR_EL2_nBRBIALL);
> > +	}
> > +
> > +
> >  	set_bit(KVM_ARCH_FLAG_FGU_INITIALIZED, &kvm->arch.flags);
> >  out:
> >  	mutex_unlock(&kvm->arch.config_lock);
> 
> This makes sense, will remove all the changes to sys_reg table and
> instead fold the above suggestion into the patch.
> 
> > 
> > The implicit dependency here is that FGT is always present on a system
> > that implements BRBE. The architecture supports this assertion:
> > 
> > - BRBE is not available before ARMv9.1
> > - FGT is mandatory from ARMv8.6
> > 
> > Given that v9.1 is congruent to v8.6, we have the required overlap.
> 
> So this overlap need not be asserted in software again ?

I don't think there's a need for that.

We went through the same thing with SME (which has the exact same
dependency), and concluded that there was no need to paper over broken
implementations at the moment (only QEMU was affected, and that was
quickly fixed). If we find an implementation in the wild that didn't
get the memo, we can add a workaround at that time.

	M.

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

[PATCH V18 6/9] 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 V18:

- Used host_data_ptr() to access host_debug_state.brbcr_el1 register
- Changed DEBUG_STATE_SAVE_BRBE to use BIT(7)
- Reverted back iflags as u8

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

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 36b8e97bf49e..db922c10bd2a 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -579,6 +579,7 @@ struct kvm_host_data {
 		u64 trfcr_el1;
 		/* Values of trap registers for the host before guest entry. */
 		u64 mdcr_el2;
+		u64 brbcr_el1;
 	} host_debug_state;
 };
 
@@ -842,6 +843,8 @@ struct kvm_vcpu_arch {
 #define DEBUG_STATE_SAVE_SPE	__vcpu_single_flag(iflags, BIT(5))
 /* Save TRBE context if active  */
 #define DEBUG_STATE_SAVE_TRBE	__vcpu_single_flag(iflags, BIT(6))
+/* Save BRBE context if active  */
+#define DEBUG_STATE_SAVE_BRBE	__vcpu_single_flag(iflags, BIT(7))
 
 /* SVE enabled for host EL0 */
 #define HOST_SVE_ENABLED	__vcpu_single_flag(sflags, BIT(0))
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index ce8886122ed3..8fa648943f0f 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -336,10 +336,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 53efda0235cf..97e861df1b45 100644
--- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
+++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
@@ -79,6 +79,32 @@ static void __debug_restore_trace(u64 trfcr_el1)
 	write_sysreg_el1(trfcr_el1, SYS_TRFCR);
 }
 
+static void __debug_save_brbe(u64 *brbcr_el1)
+{
+	*brbcr_el1 = 0;
+
+	/* Check if the BRBE is enabled */
+	if (!(read_sysreg_el1(SYS_BRBCR) & (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_el1(SYS_BRBCR);
+	write_sysreg_el1(0, SYS_BRBCR);
+}
+
+static void __debug_restore_brbe(u64 brbcr_el1)
+{
+	if (!brbcr_el1)
+		return;
+
+	/* Restore BRBE controls */
+	write_sysreg_el1(brbcr_el1, SYS_BRBCR);
+}
+
 void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu)
 {
 	/* Disable and flush SPE data generation */
@@ -87,6 +113,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(host_data_ptr(host_debug_state.trfcr_el1));
+	/* Disable BRBE branch records */
+	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE))
+		__debug_save_brbe(host_data_ptr(host_debug_state.brbcr_el1));
 }
 
 void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
@@ -100,6 +129,8 @@ void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu)
 		__debug_restore_spe(*host_data_ptr(host_debug_state.pmscr_el1));
 	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
 		__debug_restore_trace(*host_data_ptr(host_debug_state.trfcr_el1));
+	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE))
+		__debug_restore_brbe(*host_data_ptr(host_debug_state.brbcr_el1));
 }
 
 void __debug_switch_to_host(struct kvm_vcpu *vcpu)
-- 
2.25.1

Re: [PATCH V18 6/9] KVM: arm64: nvhe: Disable branch generation in nVHE guests

[Mark Rutland]

On Thu, Jun 13, 2024 at 11:47:28AM +0530, 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.

It'd be good to explain why we need to do this for nVHE, but not for
VHE. I *think* that you're relying on BRBCR_EL2.EL0HBRE being ignored
when HCR_EL2.TGE == 0, and BRBCR_EL1.E{1,0}BRE being initialized to 0
out-of-reset.

What should a user do if they *want* samples from a guest? Is that
possible to do on other architectures, or do is that always prevented?

Mark.

> 
> 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 V18:
> 
> - Used host_data_ptr() to access host_debug_state.brbcr_el1 register
> - Changed DEBUG_STATE_SAVE_BRBE to use BIT(7)
> - Reverted back iflags as u8
> 
>  arch/arm64/include/asm/kvm_host.h  |  3 +++
>  arch/arm64/kvm/debug.c             |  5 +++++
>  arch/arm64/kvm/hyp/nvhe/debug-sr.c | 31 ++++++++++++++++++++++++++++++
>  3 files changed, 39 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 36b8e97bf49e..db922c10bd2a 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -579,6 +579,7 @@ struct kvm_host_data {
>  		u64 trfcr_el1;
>  		/* Values of trap registers for the host before guest entry. */
>  		u64 mdcr_el2;
> +		u64 brbcr_el1;
>  	} host_debug_state;
>  };
>  
> @@ -842,6 +843,8 @@ struct kvm_vcpu_arch {
>  #define DEBUG_STATE_SAVE_SPE	__vcpu_single_flag(iflags, BIT(5))
>  /* Save TRBE context if active  */
>  #define DEBUG_STATE_SAVE_TRBE	__vcpu_single_flag(iflags, BIT(6))
> +/* Save BRBE context if active  */
> +#define DEBUG_STATE_SAVE_BRBE	__vcpu_single_flag(iflags, BIT(7))
>  
>  /* SVE enabled for host EL0 */
>  #define HOST_SVE_ENABLED	__vcpu_single_flag(sflags, BIT(0))
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index ce8886122ed3..8fa648943f0f 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -336,10 +336,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 53efda0235cf..97e861df1b45 100644
> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> @@ -79,6 +79,32 @@ static void __debug_restore_trace(u64 trfcr_el1)
>  	write_sysreg_el1(trfcr_el1, SYS_TRFCR);
>  }
>  
> +static void __debug_save_brbe(u64 *brbcr_el1)
> +{
> +	*brbcr_el1 = 0;
> +
> +	/* Check if the BRBE is enabled */
> +	if (!(read_sysreg_el1(SYS_BRBCR) & (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_el1(SYS_BRBCR);
> +	write_sysreg_el1(0, SYS_BRBCR);
> +}
> +
> +static void __debug_restore_brbe(u64 brbcr_el1)
> +{
> +	if (!brbcr_el1)
> +		return;
> +
> +	/* Restore BRBE controls */
> +	write_sysreg_el1(brbcr_el1, SYS_BRBCR);
> +}
> +
>  void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu)
>  {
>  	/* Disable and flush SPE data generation */
> @@ -87,6 +113,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(host_data_ptr(host_debug_state.trfcr_el1));
> +	/* Disable BRBE branch records */
> +	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE))
> +		__debug_save_brbe(host_data_ptr(host_debug_state.brbcr_el1));
>  }
>  
>  void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
> @@ -100,6 +129,8 @@ void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu)
>  		__debug_restore_spe(*host_data_ptr(host_debug_state.pmscr_el1));
>  	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
>  		__debug_restore_trace(*host_data_ptr(host_debug_state.trfcr_el1));
> +	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE))
> +		__debug_restore_brbe(*host_data_ptr(host_debug_state.brbcr_el1));
>  }
>  
>  void __debug_switch_to_host(struct kvm_vcpu *vcpu)
> -- 
> 2.25.1
> 

Re: [PATCH V18 6/9] KVM: arm64: nvhe: Disable branch generation in nVHE guests

[Anshuman Khandual]

On 6/14/24 20:53, Mark Rutland wrote:
> On Thu, Jun 13, 2024 at 11:47:28AM +0530, 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.
> 
> It'd be good to explain why we need to do this for nVHE, but not for
> VHE. I *think* that you're relying on BRBCR_EL2.EL0HBRE being ignored
> when HCR_EL2.TGE == 0, and BRBCR_EL1.E{1,0}BRE being initialized to 0
> out-of-reset.

That's right, there is no possibility for the host and guest BRBE config
to overlap.

> 
> What should a user do if they *want* samples from a guest? Is that

That is not supported currently. But in order to enable capturing guest
branch samples from inside the host - BRBCR_EL2 configs need to migrate
into BRBCR_EL1 when the guest runs on the cpu.

> possible to do on other architectures, or do is that always prevented?

I am not sure about other architectures, but for now this falls within
guest support which might be looked into later. But is not the proposed
patch complete in itself without any further guest support ?

> 
> Mark.
> 
>>
>> 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 V18:
>>
>> - Used host_data_ptr() to access host_debug_state.brbcr_el1 register
>> - Changed DEBUG_STATE_SAVE_BRBE to use BIT(7)
>> - Reverted back iflags as u8
>>
>>  arch/arm64/include/asm/kvm_host.h  |  3 +++
>>  arch/arm64/kvm/debug.c             |  5 +++++
>>  arch/arm64/kvm/hyp/nvhe/debug-sr.c | 31 ++++++++++++++++++++++++++++++
>>  3 files changed, 39 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 36b8e97bf49e..db922c10bd2a 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -579,6 +579,7 @@ struct kvm_host_data {
>>  		u64 trfcr_el1;
>>  		/* Values of trap registers for the host before guest entry. */
>>  		u64 mdcr_el2;
>> +		u64 brbcr_el1;
>>  	} host_debug_state;
>>  };
>>  
>> @@ -842,6 +843,8 @@ struct kvm_vcpu_arch {
>>  #define DEBUG_STATE_SAVE_SPE	__vcpu_single_flag(iflags, BIT(5))
>>  /* Save TRBE context if active  */
>>  #define DEBUG_STATE_SAVE_TRBE	__vcpu_single_flag(iflags, BIT(6))
>> +/* Save BRBE context if active  */
>> +#define DEBUG_STATE_SAVE_BRBE	__vcpu_single_flag(iflags, BIT(7))
>>  
>>  /* SVE enabled for host EL0 */
>>  #define HOST_SVE_ENABLED	__vcpu_single_flag(sflags, BIT(0))
>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
>> index ce8886122ed3..8fa648943f0f 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -336,10 +336,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 53efda0235cf..97e861df1b45 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> @@ -79,6 +79,32 @@ static void __debug_restore_trace(u64 trfcr_el1)
>>  	write_sysreg_el1(trfcr_el1, SYS_TRFCR);
>>  }
>>  
>> +static void __debug_save_brbe(u64 *brbcr_el1)
>> +{
>> +	*brbcr_el1 = 0;
>> +
>> +	/* Check if the BRBE is enabled */
>> +	if (!(read_sysreg_el1(SYS_BRBCR) & (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_el1(SYS_BRBCR);
>> +	write_sysreg_el1(0, SYS_BRBCR);
>> +}
>> +
>> +static void __debug_restore_brbe(u64 brbcr_el1)
>> +{
>> +	if (!brbcr_el1)
>> +		return;
>> +
>> +	/* Restore BRBE controls */
>> +	write_sysreg_el1(brbcr_el1, SYS_BRBCR);
>> +}
>> +
>>  void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu)
>>  {
>>  	/* Disable and flush SPE data generation */
>> @@ -87,6 +113,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(host_data_ptr(host_debug_state.trfcr_el1));
>> +	/* Disable BRBE branch records */
>> +	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE))
>> +		__debug_save_brbe(host_data_ptr(host_debug_state.brbcr_el1));
>>  }
>>  
>>  void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
>> @@ -100,6 +129,8 @@ void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu)
>>  		__debug_restore_spe(*host_data_ptr(host_debug_state.pmscr_el1));
>>  	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
>>  		__debug_restore_trace(*host_data_ptr(host_debug_state.trfcr_el1));
>> +	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE))
>> +		__debug_restore_brbe(*host_data_ptr(host_debug_state.brbcr_el1));
>>  }
>>  
>>  void __debug_switch_to_host(struct kvm_vcpu *vcpu)
>> -- 
>> 2.25.1
>>

Re: [PATCH V18 6/9] KVM: arm64: nvhe: Disable branch generation in nVHE guests

[Mark Rutland]

On Mon, Jun 17, 2024 at 12:15:15PM +0530, Anshuman Khandual wrote:
> 
> 
> On 6/14/24 20:53, Mark Rutland wrote:
> > On Thu, Jun 13, 2024 at 11:47:28AM +0530, 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.
> > 
> > It'd be good to explain why we need to do this for nVHE, but not for
> > VHE. I *think* that you're relying on BRBCR_EL2.EL0HBRE being ignored
> > when HCR_EL2.TGE == 0, and BRBCR_EL1.E{1,0}BRE being initialized to 0
> > out-of-reset.
> 
> That's right, there is no possibility for the host and guest BRBE config
> to overlap.
> 
> > What should a user do if they *want* samples from a guest? Is that
> 
> That is not supported currently. But in order to enable capturing guest
> branch samples from inside the host - BRBCR_EL2 configs need to migrate
> into BRBCR_EL1 when the guest runs on the cpu.
> 
> > possible to do on other architectures, or do is that always prevented?
> 
> I am not sure about other architectures, but for now this falls within
> guest support which might be looked into later. But is not the proposed
> patch complete in itself without any further guest support ?

My concern here is ABI rather than actual support.

It's not clear to me how this works across architectures, and we should
have some idea of how this would work (e.g. if we're going to require
new ABI or not), so that we don't have to break ABI later on.

Mark.

> 
> > 
> > Mark.
> > 
> >>
> >> 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 V18:
> >>
> >> - Used host_data_ptr() to access host_debug_state.brbcr_el1 register
> >> - Changed DEBUG_STATE_SAVE_BRBE to use BIT(7)
> >> - Reverted back iflags as u8
> >>
> >>  arch/arm64/include/asm/kvm_host.h  |  3 +++
> >>  arch/arm64/kvm/debug.c             |  5 +++++
> >>  arch/arm64/kvm/hyp/nvhe/debug-sr.c | 31 ++++++++++++++++++++++++++++++
> >>  3 files changed, 39 insertions(+)
> >>
> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> >> index 36b8e97bf49e..db922c10bd2a 100644
> >> --- a/arch/arm64/include/asm/kvm_host.h
> >> +++ b/arch/arm64/include/asm/kvm_host.h
> >> @@ -579,6 +579,7 @@ struct kvm_host_data {
> >>  		u64 trfcr_el1;
> >>  		/* Values of trap registers for the host before guest entry. */
> >>  		u64 mdcr_el2;
> >> +		u64 brbcr_el1;
> >>  	} host_debug_state;
> >>  };
> >>  
> >> @@ -842,6 +843,8 @@ struct kvm_vcpu_arch {
> >>  #define DEBUG_STATE_SAVE_SPE	__vcpu_single_flag(iflags, BIT(5))
> >>  /* Save TRBE context if active  */
> >>  #define DEBUG_STATE_SAVE_TRBE	__vcpu_single_flag(iflags, BIT(6))
> >> +/* Save BRBE context if active  */
> >> +#define DEBUG_STATE_SAVE_BRBE	__vcpu_single_flag(iflags, BIT(7))
> >>  
> >>  /* SVE enabled for host EL0 */
> >>  #define HOST_SVE_ENABLED	__vcpu_single_flag(sflags, BIT(0))
> >> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> >> index ce8886122ed3..8fa648943f0f 100644
> >> --- a/arch/arm64/kvm/debug.c
> >> +++ b/arch/arm64/kvm/debug.c
> >> @@ -336,10 +336,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 53efda0235cf..97e861df1b45 100644
> >> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> >> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> >> @@ -79,6 +79,32 @@ static void __debug_restore_trace(u64 trfcr_el1)
> >>  	write_sysreg_el1(trfcr_el1, SYS_TRFCR);
> >>  }
> >>  
> >> +static void __debug_save_brbe(u64 *brbcr_el1)
> >> +{
> >> +	*brbcr_el1 = 0;
> >> +
> >> +	/* Check if the BRBE is enabled */
> >> +	if (!(read_sysreg_el1(SYS_BRBCR) & (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_el1(SYS_BRBCR);
> >> +	write_sysreg_el1(0, SYS_BRBCR);
> >> +}
> >> +
> >> +static void __debug_restore_brbe(u64 brbcr_el1)
> >> +{
> >> +	if (!brbcr_el1)
> >> +		return;
> >> +
> >> +	/* Restore BRBE controls */
> >> +	write_sysreg_el1(brbcr_el1, SYS_BRBCR);
> >> +}
> >> +
> >>  void __debug_save_host_buffers_nvhe(struct kvm_vcpu *vcpu)
> >>  {
> >>  	/* Disable and flush SPE data generation */
> >> @@ -87,6 +113,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(host_data_ptr(host_debug_state.trfcr_el1));
> >> +	/* Disable BRBE branch records */
> >> +	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE))
> >> +		__debug_save_brbe(host_data_ptr(host_debug_state.brbcr_el1));
> >>  }
> >>  
> >>  void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
> >> @@ -100,6 +129,8 @@ void __debug_restore_host_buffers_nvhe(struct kvm_vcpu *vcpu)
> >>  		__debug_restore_spe(*host_data_ptr(host_debug_state.pmscr_el1));
> >>  	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_TRBE))
> >>  		__debug_restore_trace(*host_data_ptr(host_debug_state.trfcr_el1));
> >> +	if (vcpu_get_flag(vcpu, DEBUG_STATE_SAVE_BRBE))
> >> +		__debug_restore_brbe(*host_data_ptr(host_debug_state.brbcr_el1));
> >>  }
> >>  
> >>  void __debug_switch_to_host(struct kvm_vcpu *vcpu)
> >> -- 
> >> 2.25.1
> >>

Re: [PATCH V18 6/9] KVM: arm64: nvhe: Disable branch generation in nVHE guests

[Anshuman Khandual]

On 6/17/24 15:09, Mark Rutland wrote:
> On Mon, Jun 17, 2024 at 12:15:15PM +0530, Anshuman Khandual wrote:
>>
>>
>> On 6/14/24 20:53, Mark Rutland wrote:
>>> On Thu, Jun 13, 2024 at 11:47:28AM +0530, 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.
>>>
>>> It'd be good to explain why we need to do this for nVHE, but not for
>>> VHE. I *think* that you're relying on BRBCR_EL2.EL0HBRE being ignored
>>> when HCR_EL2.TGE == 0, and BRBCR_EL1.E{1,0}BRE being initialized to 0
>>> out-of-reset.
>>
>> That's right, there is no possibility for the host and guest BRBE config
>> to overlap.
>>
>>> What should a user do if they *want* samples from a guest? Is that
>>
>> That is not supported currently. But in order to enable capturing guest
>> branch samples from inside the host - BRBCR_EL2 configs need to migrate
>> into BRBCR_EL1 when the guest runs on the cpu.
>>
>>> possible to do on other architectures, or do is that always prevented?
>>
>> I am not sure about other architectures, but for now this falls within
>> guest support which might be looked into later. But is not the proposed
>> patch complete in itself without any further guest support ?
> 
> My concern here is ABI rather than actual support.
I am trying to understand how this is an ABI problem. Because perf debug
tools could be described as - a best effort based sample collection. All
samples that could be collected for a given perf_event_attr request might
change if the underlying assumptions change later on. AFAICT semantics of
expectations for a given attribute request is not a hard ABI requirement.

> 
> It's not clear to me how this works across architectures, and we should
> have some idea of how this would work (e.g. if we're going to require
> new ABI or not), so that we don't have to break ABI later on.

BRBE HW does not have any guest filter in itself, unless BRBCR_EL2 gets
migrated across BRBCR_EL1 during guest transition, guest branch records
would not be captured.

event->attr.exclude_guest = 0 could have been denied during armpmu_add()
for preventing events with guest branch sample requests being scheduled
on the PMU. But it turns out to be not a very reliable parameter in that
sense as well.

event->attr.exclude_guest = 0 remains clear even for a standard session.

./perf record -e instructions:k -j any_call,save_type ls

perf tools will need some changes in order to avoid the above scenarios
as a default behaviour which would not be desirable as well.

These semantics could be worked out later on when BRBE guest support gets
included, and the current proposal would not prevent any potential future
changes in this regard.

> 
> Mark.

Re: [PATCH V18 6/9] KVM: arm64: nvhe: Disable branch generation in nVHE guests

[Mark Rutland]

On Thu, Jun 20, 2024 at 09:52:05AM +0530, Anshuman Khandual wrote:
> On 6/17/24 15:09, Mark Rutland wrote:
> > On Mon, Jun 17, 2024 at 12:15:15PM +0530, Anshuman Khandual wrote:
> >> On 6/14/24 20:53, Mark Rutland wrote:
> >>> On Thu, Jun 13, 2024 at 11:47:28AM +0530, 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.
> >>>
> >>> It'd be good to explain why we need to do this for nVHE, but not for
> >>> VHE. I *think* that you're relying on BRBCR_EL2.EL0HBRE being ignored
> >>> when HCR_EL2.TGE == 0, and BRBCR_EL1.E{1,0}BRE being initialized to 0
> >>> out-of-reset.
> >>
> >> That's right, there is no possibility for the host and guest BRBE config
> >> to overlap.
> >>
> >>> What should a user do if they *want* samples from a guest? Is that
> >>
> >> That is not supported currently. But in order to enable capturing guest
> >> branch samples from inside the host - BRBCR_EL2 configs need to migrate
> >> into BRBCR_EL1 when the guest runs on the cpu.
> >>
> >>> possible to do on other architectures, or do is that always prevented?
> >>
> >> I am not sure about other architectures, but for now this falls within
> >> guest support which might be looked into later. But is not the proposed
> >> patch complete in itself without any further guest support ?
> > 
> > My concern here is ABI rather than actual support.
> I am trying to understand how this is an ABI problem. Because perf debug
> tools could be described as - a best effort based sample collection. All
> samples that could be collected for a given perf_event_attr request might
> change if the underlying assumptions change later on. AFAICT semantics of
> expectations for a given attribute request is not a hard ABI requirement.

The ABI requirements are certainly unclear, but people get *very* upset
when behaviour changes, so I think we need to have some certainty that
we're not backing ourselves into a corner where we have to make
substantial behavioural changes later.

Surely we can figure out how this works on other architectures today?

There's a substantial argument for aligning with x86, so can we figure
out under which conditions x86 would provide guest samples? e.g. is that
always, never, or when certain attr options are configured?

> > It's not clear to me how this works across architectures, and we should
> > have some idea of how this would work (e.g. if we're going to require
> > new ABI or not), so that we don't have to break ABI later on.
> 
> BRBE HW does not have any guest filter in itself, unless BRBCR_EL2 gets
> migrated across BRBCR_EL1 during guest transition, guest branch records
> would not be captured.
> 
> event->attr.exclude_guest = 0 could have been denied during armpmu_add()
> for preventing events with guest branch sample requests being scheduled
> on the PMU. But it turns out to be not a very reliable parameter in that
> sense as well.
> 
> event->attr.exclude_guest = 0 remains clear even for a standard session.
> 
> ./perf record -e instructions:k -j any_call,save_type ls
> 
> perf tools will need some changes in order to avoid the above scenarios
> as a default behaviour which would not be desirable as well.

If we're liable to need perf tool changes, then we *definitely* need to
understand this better.

> These semantics could be worked out later on when BRBE guest support gets
> included, and the current proposal would not prevent any potential future
> changes in this regard.

That depends entirely on what changes we'd expect would be necessary in
the perf tools. We need to be certain that we don't enable some use case
that subseuqent changes break.

Mark.