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

[PATCH V15 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>
---
 arch/arm64/kvm/sys_regs.c | 130 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 130 insertions(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 4735e1b37fb3..42701065b3cd 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1583,6 +1583,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;
 }
 
@@ -2042,6 +2045,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),
@@ -2072,6 +2077,131 @@ 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.
+	 */
+	{ SYS_DESC(SYS_BRBINF0_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC0_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT0_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBINF16_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC16_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT16_EL1), undef_access },
+
+	{ SYS_DESC(SYS_BRBINF1_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC1_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT1_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBINF17_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC17_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT17_EL1), undef_access },
+
+	{ SYS_DESC(SYS_BRBINF2_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC2_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT2_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBINF18_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC18_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT18_EL1), undef_access },
+
+	{ SYS_DESC(SYS_BRBINF3_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC3_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT3_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBINF19_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC19_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT19_EL1), undef_access },
+
+	{ SYS_DESC(SYS_BRBINF4_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC4_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT4_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBINF20_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC20_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT20_EL1), undef_access },
+
+	{ SYS_DESC(SYS_BRBINF5_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC5_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT5_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBINF21_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC21_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT21_EL1), undef_access },
+
+	{ SYS_DESC(SYS_BRBINF6_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC6_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT6_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBINF22_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC22_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT22_EL1), undef_access },
+
+	{ SYS_DESC(SYS_BRBINF7_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC7_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT7_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBINF23_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC23_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT23_EL1), undef_access },
+
+	{ SYS_DESC(SYS_BRBINF8_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC8_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT8_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBINF24_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC24_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT24_EL1), undef_access },
+
+	{ SYS_DESC(SYS_BRBINF9_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC9_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT9_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBINF25_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC25_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT25_EL1), undef_access },
+
+	{ SYS_DESC(SYS_BRBINF10_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC10_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT10_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBINF26_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC26_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT26_EL1), undef_access },
+
+	{ SYS_DESC(SYS_BRBINF11_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC11_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT11_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBINF27_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC27_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT27_EL1), undef_access },
+
+	{ SYS_DESC(SYS_BRBINF12_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC12_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT12_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBINF28_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC28_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT28_EL1), undef_access },
+
+	{ SYS_DESC(SYS_BRBINF13_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC13_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT13_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBINF29_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC29_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT29_EL1), undef_access },
+
+	{ SYS_DESC(SYS_BRBINF14_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC14_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT14_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBINF30_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC30_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT30_EL1), undef_access },
+
+	{ SYS_DESC(SYS_BRBINF15_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC15_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT15_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBINF31_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBSRC31_EL1), undef_access },
+	{ SYS_DESC(SYS_BRBTGT31_EL1), undef_access },
+
+	/* 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 V15 2/8] KVM: arm64: Prevent guest accesses into BRBE system registers/instructions

[Marc Zyngier]

On Fri, 01 Dec 2023 05:39:00 +0000,
Anshuman Khandual <anshuman.khandual@arm.com> 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>
> ---
>  arch/arm64/kvm/sys_regs.c | 130 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 130 insertions(+)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 4735e1b37fb3..42701065b3cd 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1583,6 +1583,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;
>  }
>  
> @@ -2042,6 +2045,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),
> @@ -2072,6 +2077,131 @@ 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.
> +	 */
> +	{ SYS_DESC(SYS_BRBINF0_EL1), undef_access },
> +	{ SYS_DESC(SYS_BRBSRC0_EL1), undef_access },
> +	{ SYS_DESC(SYS_BRBTGT0_EL1), undef_access },
> +	{ SYS_DESC(SYS_BRBINF16_EL1), undef_access },
> +	{ SYS_DESC(SYS_BRBSRC16_EL1), undef_access },
> +	{ SYS_DESC(SYS_BRBTGT16_EL1), undef_access },

Surely we can do better than this wall of text. Please look at what we
do for the debug registers, and adopt a similar pattern. This should
result in one line per group of 3 registers.

What is the plan for KVM support beyond this?

	M.

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

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

[Anshuman Khandual]

On 12/4/23 13:52, Marc Zyngier wrote:
> On Fri, 01 Dec 2023 05:39:00 +0000,
> Anshuman Khandual <anshuman.khandual@arm.com> 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>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 130 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 130 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 4735e1b37fb3..42701065b3cd 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1583,6 +1583,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;
>>  }
>>  
>> @@ -2042,6 +2045,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),
>> @@ -2072,6 +2077,131 @@ 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.
>> +	 */
>> +	{ SYS_DESC(SYS_BRBINF0_EL1), undef_access },
>> +	{ SYS_DESC(SYS_BRBSRC0_EL1), undef_access },
>> +	{ SYS_DESC(SYS_BRBTGT0_EL1), undef_access },
>> +	{ SYS_DESC(SYS_BRBINF16_EL1), undef_access },
>> +	{ SYS_DESC(SYS_BRBSRC16_EL1), undef_access },
>> +	{ SYS_DESC(SYS_BRBTGT16_EL1), undef_access },
> Surely we can do better than this wall of text. Please look at what we
> do for the debug registers, and adopt a similar pattern. This should
> result in one line per group of 3 registers.

Sure, will these replace via the following macro.

+#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 }         \
....
+       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),
....

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

[Anshuman Khandual]

On 12/4/23 13:52, Marc Zyngier wrote:
> On Fri, 01 Dec 2023 05:39:00 +0000,
> Anshuman Khandual <anshuman.khandual@arm.com> 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>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 130 ++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 130 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 4735e1b37fb3..42701065b3cd 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -1583,6 +1583,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;
>>  }
>>  
>> @@ -2042,6 +2045,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),
>> @@ -2072,6 +2077,131 @@ 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.
>> +	 */
>> +	{ SYS_DESC(SYS_BRBINF0_EL1), undef_access },
>> +	{ SYS_DESC(SYS_BRBSRC0_EL1), undef_access },
>> +	{ SYS_DESC(SYS_BRBTGT0_EL1), undef_access },
>> +	{ SYS_DESC(SYS_BRBINF16_EL1), undef_access },
>> +	{ SYS_DESC(SYS_BRBSRC16_EL1), undef_access },
>> +	{ SYS_DESC(SYS_BRBTGT16_EL1), undef_access },
> Surely we can do better than this wall of text. Please look at what we
> do for the debug registers, and adopt a similar pattern. This should
> result in one line per group of 3 registers.
> 
> What is the plan for KVM support beyond this?
We are planning to add support for simultaneous and independent branch record
tracing via perf branch stack sampling inside the guest and the host. But will
prevent tracing of the guest execution from the host.

[PATCH V15 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 is just to avoid capturing records
in the guest kernel/userspace, which would be confusing the 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 V15:

- Dropped runtime BRBE enable for setting DEBUG_STATE_SAVE_BRBE
- Dropped BRBFCR_EL1 from __debug_save_brbe()/__debug_restore_brbe()
- Always save the live SYS_BRBCR_EL1 in host context and then check if
  BRBE was enabled before resetting SYS_BRBCR_EL1 for the host

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

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 68421c74283a..1faa0430d8dd 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -449,6 +449,8 @@ enum vcpu_sysreg {
 	CNTHV_CVAL_EL2,
 	PMSCR_EL1,	/* Statistical profiling extension */
 	TRFCR_EL1,	/* Self-hosted trace filters */
+	BRBCR_EL1,	/* Branch Record Buffer Control Register */
+	BRBFCR_EL1,	/* Branch Record Buffer Function Control Register */
 
 	NR_SYS_REGS	/* Nothing after this line! */
 };
@@ -753,6 +755,8 @@ struct kvm_vcpu_arch {
 #define VCPU_HYP_CONTEXT	__vcpu_single_flag(iflags, BIT(7))
 /* Save trace filter controls */
 #define DEBUG_STATE_SAVE_TRFCR	__vcpu_single_flag(iflags, BIT(8))
+/* Save BRBE context if active  */
+#define DEBUG_STATE_SAVE_BRBE	__vcpu_single_flag(iflags, BIT(9))
 
 /* 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 2ab41b954512..fa46a70a9503 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -354,6 +354,10 @@ void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu)
 		    !(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)
@@ -361,6 +365,7 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu)
 	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE);
 	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
 	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRFCR);
+	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_BRBE);
 }
 
 void kvm_etm_set_guest_trfcr(u64 trfcr_guest)
diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
index 6174f710948e..1994fc48b57c 100644
--- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
+++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
@@ -93,6 +93,33 @@ static void __debug_restore_trace(struct kvm_cpu_context *host_ctxt,
 		write_sysreg_s(ctxt_sys_reg(host_ctxt, TRFCR_EL1), SYS_TRFCR_EL1);
 }
 
+static void __debug_save_brbe(struct kvm_cpu_context *host_ctxt)
+{
+	ctxt_sys_reg(host_ctxt, BRBCR_EL1) = read_sysreg_s(SYS_BRBCR_EL1);
+
+	/* Check if the BRBE is enabled */
+	if (!(ctxt_sys_reg(host_ctxt, 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.
+	 */
+	write_sysreg_s(0, SYS_BRBCR_EL1);
+	isb();
+}
+
+static void __debug_restore_brbe(struct kvm_cpu_context *host_ctxt)
+{
+	if (!ctxt_sys_reg(host_ctxt, BRBCR_EL1))
+		return;
+
+	/* Restore BRBE controls */
+	write_sysreg_s(ctxt_sys_reg(host_ctxt, BRBCR_EL1), SYS_BRBCR_EL1);
+	isb();
+}
+
 void __debug_save_host_buffers_nvhe(struct kvm_cpu_context *host_ctxt,
 				    struct kvm_cpu_context *guest_ctxt)
 {
@@ -102,6 +129,10 @@ void __debug_save_host_buffers_nvhe(struct kvm_cpu_context *host_ctxt,
 
 	if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_TRFCR))
 		__debug_save_trace(host_ctxt, guest_ctxt);
+
+	/* Disable BRBE branch records */
+	if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_BRBE))
+		__debug_save_brbe(host_ctxt);
 }
 
 void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
@@ -116,6 +147,8 @@ void __debug_restore_host_buffers_nvhe(struct kvm_cpu_context *host_ctxt,
 		__debug_restore_spe(host_ctxt);
 	if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_TRFCR))
 		__debug_restore_trace(host_ctxt, guest_ctxt);
+	if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_BRBE))
+		__debug_restore_brbe(host_ctxt);
 }
 
 void __debug_switch_to_host(struct kvm_vcpu *vcpu)
-- 
2.25.1

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

[Marc Zyngier]

On Fri, 01 Dec 2023 05:39:03 +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 is just to avoid capturing records
> in the guest kernel/userspace, which would be confusing the samples.

Why does it have to be limited to non-VHE? What protects host EL0
records from guest's EL0 execution when the host is VHE?

> 
> 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 V15:
> 
> - Dropped runtime BRBE enable for setting DEBUG_STATE_SAVE_BRBE
> - Dropped BRBFCR_EL1 from __debug_save_brbe()/__debug_restore_brbe()
> - Always save the live SYS_BRBCR_EL1 in host context and then check if
>   BRBE was enabled before resetting SYS_BRBCR_EL1 for the host
> 
>  arch/arm64/include/asm/kvm_host.h  |  4 ++++
>  arch/arm64/kvm/debug.c             |  5 +++++
>  arch/arm64/kvm/hyp/nvhe/debug-sr.c | 33 ++++++++++++++++++++++++++++++
>  3 files changed, 42 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 68421c74283a..1faa0430d8dd 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -449,6 +449,8 @@ enum vcpu_sysreg {
>  	CNTHV_CVAL_EL2,
>  	PMSCR_EL1,	/* Statistical profiling extension */
>  	TRFCR_EL1,	/* Self-hosted trace filters */
> +	BRBCR_EL1,	/* Branch Record Buffer Control Register */
> +	BRBFCR_EL1,	/* Branch Record Buffer Function Control Register */

Whose state is this? If this is limited to the host, it has no purpose
in this enum. Once you add guest support, then it will make sense.

>
>  	NR_SYS_REGS	/* Nothing after this line! */
>  };
> @@ -753,6 +755,8 @@ struct kvm_vcpu_arch {
>  #define VCPU_HYP_CONTEXT	__vcpu_single_flag(iflags, BIT(7))
>  /* Save trace filter controls */
>  #define DEBUG_STATE_SAVE_TRFCR	__vcpu_single_flag(iflags, BIT(8))
> +/* Save BRBE context if active  */
> +#define DEBUG_STATE_SAVE_BRBE	__vcpu_single_flag(iflags, BIT(9))
>  
>  /* 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 2ab41b954512..fa46a70a9503 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -354,6 +354,10 @@ void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu)
>  		    !(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)
> @@ -361,6 +365,7 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu)
>  	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE);
>  	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
>  	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRFCR);
> +	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_BRBE);
>  }
>  
>  void kvm_etm_set_guest_trfcr(u64 trfcr_guest)
> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> index 6174f710948e..1994fc48b57c 100644
> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
> @@ -93,6 +93,33 @@ static void __debug_restore_trace(struct kvm_cpu_context *host_ctxt,
>  		write_sysreg_s(ctxt_sys_reg(host_ctxt, TRFCR_EL1), SYS_TRFCR_EL1);
>  }
>  
> +static void __debug_save_brbe(struct kvm_cpu_context *host_ctxt)
> +{
> +	ctxt_sys_reg(host_ctxt, BRBCR_EL1) = read_sysreg_s(SYS_BRBCR_EL1);
> +
> +	/* Check if the BRBE is enabled */
> +	if (!(ctxt_sys_reg(host_ctxt, BRBCR_EL1) & (BRBCR_ELx_E0BRE | BRBCR_ELx_ExBRE)))
> +		return;

Why save BRBCR_EL1 if there is nothing enabled? It isn't like it can
change behind your back, can it?

> +
> +	/*
> +	 * 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.
> +	 */
> +	write_sysreg_s(0, SYS_BRBCR_EL1);
> +	isb();

What is the point of this ISB? We're at EL2, and this only affects
EL1.

> +}
> +
> +static void __debug_restore_brbe(struct kvm_cpu_context *host_ctxt)
> +{
> +	if (!ctxt_sys_reg(host_ctxt, BRBCR_EL1))
> +		return;

So on one side you're using a flag, and on the other you're using the
*value*. You need some consistency.

> +
> +	/* Restore BRBE controls */
> +	write_sysreg_s(ctxt_sys_reg(host_ctxt, BRBCR_EL1), SYS_BRBCR_EL1);
> +	isb();

Same question.

> +}
> +
>  void __debug_save_host_buffers_nvhe(struct kvm_cpu_context *host_ctxt,
>  				    struct kvm_cpu_context *guest_ctxt)
>  {
> @@ -102,6 +129,10 @@ void __debug_save_host_buffers_nvhe(struct kvm_cpu_context *host_ctxt,
>  
>  	if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_TRFCR))
>  		__debug_save_trace(host_ctxt, guest_ctxt);
> +
> +	/* Disable BRBE branch records */
> +	if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_BRBE))
> +		__debug_save_brbe(host_ctxt);
>  }
>  
>  void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
> @@ -116,6 +147,8 @@ void __debug_restore_host_buffers_nvhe(struct kvm_cpu_context *host_ctxt,
>  		__debug_restore_spe(host_ctxt);
>  	if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_TRFCR))
>  		__debug_restore_trace(host_ctxt, guest_ctxt);
> +	if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_BRBE))
> +		__debug_restore_brbe(host_ctxt);
>  }
>  
>  void __debug_switch_to_host(struct kvm_vcpu *vcpu)

The lifetime of this flag seems bogus, specially when there is nothing
to do, which will always be the arch-majority of the executions.

	M.

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

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

[Anshuman Khandual]

On 12/4/23 14:12, Marc Zyngier wrote:
> On Fri, 01 Dec 2023 05:39:03 +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 is just to avoid capturing records
>> in the guest kernel/userspace, which would be confusing the samples.
> 
> Why does it have to be limited to non-VHE? What protects host EL0
> records from guest's EL0 execution when the host is VHE?

In a scenario when running the host in VHE mode
 
- The host might have enabled branch records for host EL0 through BRBCR_EL2.E0HBRE
  indirectly via accessing BRBCR_EL1.E0BRE

- But after the guest switches in on the cpu - BRBCR_EL2.E0HBRE will still remain
  set and enable branch records in guest EL0 as well because BRBCR_EL1.E0BRE will
  not have any effect when EL2 is implemented and HCR_EL2.TGE == 1. The guest EL0
  execution branch records will find their way into branch records being captured
  for host EL0

You are right. The host EL0 branch records too need to be protected from guest EL0
execution. A similar BRBCR_EL1 save/restore mechanism is needed for VHE as well ?

> 
>>
>> 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 V15:
>>
>> - Dropped runtime BRBE enable for setting DEBUG_STATE_SAVE_BRBE
>> - Dropped BRBFCR_EL1 from __debug_save_brbe()/__debug_restore_brbe()
>> - Always save the live SYS_BRBCR_EL1 in host context and then check if
>>   BRBE was enabled before resetting SYS_BRBCR_EL1 for the host
>>
>>  arch/arm64/include/asm/kvm_host.h  |  4 ++++
>>  arch/arm64/kvm/debug.c             |  5 +++++
>>  arch/arm64/kvm/hyp/nvhe/debug-sr.c | 33 ++++++++++++++++++++++++++++++
>>  3 files changed, 42 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index 68421c74283a..1faa0430d8dd 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -449,6 +449,8 @@ enum vcpu_sysreg {
>>  	CNTHV_CVAL_EL2,
>>  	PMSCR_EL1,	/* Statistical profiling extension */
>>  	TRFCR_EL1,	/* Self-hosted trace filters */
>> +	BRBCR_EL1,	/* Branch Record Buffer Control Register */
>> +	BRBFCR_EL1,	/* Branch Record Buffer Function Control Register */
> 
> Whose state is this? If this is limited to the host, it has no purpose
> in this enum. Once you add guest support, then it will make sense.

yes, this is limited to host but if not here, where this register (BRBCR_EL1)
should be stored during guest context switch ? This place holder is used for
that purpose via ctxt_sys_reg(host_ctxt, BRBCR_EL1).

> 
>>
>>  	NR_SYS_REGS	/* Nothing after this line! */
>>  };
>> @@ -753,6 +755,8 @@ struct kvm_vcpu_arch {
>>  #define VCPU_HYP_CONTEXT	__vcpu_single_flag(iflags, BIT(7))
>>  /* Save trace filter controls */
>>  #define DEBUG_STATE_SAVE_TRFCR	__vcpu_single_flag(iflags, BIT(8))
>> +/* Save BRBE context if active  */
>> +#define DEBUG_STATE_SAVE_BRBE	__vcpu_single_flag(iflags, BIT(9))
>>  
>>  /* 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 2ab41b954512..fa46a70a9503 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -354,6 +354,10 @@ void kvm_arch_vcpu_load_debug_state_flags(struct kvm_vcpu *vcpu)
>>  		    !(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)
>> @@ -361,6 +365,7 @@ void kvm_arch_vcpu_put_debug_state_flags(struct kvm_vcpu *vcpu)
>>  	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_SPE);
>>  	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRBE);
>>  	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_TRFCR);
>> +	vcpu_clear_flag(vcpu, DEBUG_STATE_SAVE_BRBE);
>>  }
>>  
>>  void kvm_etm_set_guest_trfcr(u64 trfcr_guest)
>> diff --git a/arch/arm64/kvm/hyp/nvhe/debug-sr.c b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> index 6174f710948e..1994fc48b57c 100644
>> --- a/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> +++ b/arch/arm64/kvm/hyp/nvhe/debug-sr.c
>> @@ -93,6 +93,33 @@ static void __debug_restore_trace(struct kvm_cpu_context *host_ctxt,
>>  		write_sysreg_s(ctxt_sys_reg(host_ctxt, TRFCR_EL1), SYS_TRFCR_EL1);
>>  }
>>  
>> +static void __debug_save_brbe(struct kvm_cpu_context *host_ctxt)
>> +{
>> +	ctxt_sys_reg(host_ctxt, BRBCR_EL1) = read_sysreg_s(SYS_BRBCR_EL1);
>> +
>> +	/* Check if the BRBE is enabled */
>> +	if (!(ctxt_sys_reg(host_ctxt, BRBCR_EL1) & (BRBCR_ELx_E0BRE | BRBCR_ELx_ExBRE)))
>> +		return;
> 
> Why save BRBCR_EL1 if there is nothing enabled? It isn't like it can
> change behind your back, can it?

James mentioned that always setting the host value with real BRBCR_EL1 is preferred.

https://lore.kernel.org/linux-arm-kernel/1a94cccd-d871-1824-9fad-a8f7b99bb02a@arm.com/
 
> 
>> +
>> +	/*
>> +	 * 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.
>> +	 */
>> +	write_sysreg_s(0, SYS_BRBCR_EL1);
>> +	isb();
> 
> What is the point of this ISB? We're at EL2, and this only affects
> EL1.

Makes sense, will drop this isb().

> 
>> +}
>> +
>> +static void __debug_restore_brbe(struct kvm_cpu_context *host_ctxt)
>> +{
>> +	if (!ctxt_sys_reg(host_ctxt, BRBCR_EL1))
>> +		return;
> 
> So on one side you're using a flag, and on the other you're using the
> *value*. You need some consistency.

Both DEBUG_STATE_SAVE_BRBE flag and BRBCR_EL1 value is checked on both save
and restore side.

__debug_save_host_buffers_nvhe()
	vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_BRBE)
		__debug_save_brbe(host_ctxt)
			Save BRBE but reset if BRBE is running

__debug_restore_host_buffers_nvhe()
	vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_BRBE)
		__debug_restore_brbe(host_ctxt)
			Restore when saved BRBCR_EL1 is positive

> 
>> +
>> +	/* Restore BRBE controls */
>> +	write_sysreg_s(ctxt_sys_reg(host_ctxt, BRBCR_EL1), SYS_BRBCR_EL1);
>> +	isb();
> 
> Same question.
> 
>> +}
>> +
>>  void __debug_save_host_buffers_nvhe(struct kvm_cpu_context *host_ctxt,
>>  				    struct kvm_cpu_context *guest_ctxt)
>>  {
>> @@ -102,6 +129,10 @@ void __debug_save_host_buffers_nvhe(struct kvm_cpu_context *host_ctxt,
>>  
>>  	if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_TRFCR))
>>  		__debug_save_trace(host_ctxt, guest_ctxt);
>> +
>> +	/* Disable BRBE branch records */
>> +	if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_BRBE))
>> +		__debug_save_brbe(host_ctxt);
>>  }
>>  
>>  void __debug_switch_to_guest(struct kvm_vcpu *vcpu)
>> @@ -116,6 +147,8 @@ void __debug_restore_host_buffers_nvhe(struct kvm_cpu_context *host_ctxt,
>>  		__debug_restore_spe(host_ctxt);
>>  	if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_TRFCR))
>>  		__debug_restore_trace(host_ctxt, guest_ctxt);
>> +	if (vcpu_get_flag(host_ctxt->__hyp_running_vcpu, DEBUG_STATE_SAVE_BRBE))
>> +		__debug_restore_brbe(host_ctxt);
>>  }
>>  
>>  void __debug_switch_to_host(struct kvm_vcpu *vcpu)
> 
> The lifetime of this flag seems bogus, specially when there is nothing
> to do, which will always be the arch-majority of the executions.

Not sure if I follow your point, could you please be more specific here ?

DEBUG_STATE_SAVE_BRBE follows all existing register state context save/restore
mechanism such as DEBUG_STATE_SAVE_SPE and DEBUG_STATE_SAVE_TRFCR. How is this
any different or problematic ?

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

[Anshuman Khandual]

On 12/11/23 11:30, Anshuman Khandual wrote:
> On 12/4/23 14:12, Marc Zyngier wrote:
>> On Fri, 01 Dec 2023 05:39:03 +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 is just to avoid capturing records
>>> in the guest kernel/userspace, which would be confusing the samples.
>> Why does it have to be limited to non-VHE? What protects host EL0
>> records from guest's EL0 execution when the host is VHE?
> In a scenario when running the host in VHE mode
>  
> - The host might have enabled branch records for host EL0 through BRBCR_EL2.E0HBRE
>   indirectly via accessing BRBCR_EL1.E0BRE
> 
> - But after the guest switches in on the cpu - BRBCR_EL2.E0HBRE will still remain
>   set and enable branch records in guest EL0 as well because BRBCR_EL1.E0BRE will
>   not have any effect when EL2 is implemented and HCR_EL2.TGE == 1. The guest EL0
>   execution branch records will find their way into branch records being captured
>   for host EL0
> 
> You are right. The host EL0 branch records too need to be protected from guest EL0
> execution. A similar BRBCR_EL1 save/restore mechanism is needed for VHE as well ?

Looking at this again, seems like host EL0 records will be protected from
guest EL0 execution as HCR_EL2.TGE toggles when th guest switches in thus
enforcing BRBCR_EL1.E0BRE (which is clear) requirement for capturing guest
EL0 branch records.

arch/arm64/kvm/hyp/vhe/tlb.c

__tlb_switch_to_guest() {
	....
	val = read_sysreg(hcr_el2);
        val &= ~HCR_TGE;
        write_sysreg(val, hcr_el2);
	isb();
}

HCR_TGE comes back via HCR_HOST_VHE_FLAGS when the host switches back in.

__tlb_switch_to_host() {
	write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2);
        isb();
	....
}