2023-12-04 04:04:39

by Anshuman Khandual

[permalink] [raw]

Subject: Re: [PATCH V15 1/8] arm64/sysreg: Add BRBE registers and fields

On 12/1/23 22:28, Mark Brown wrote:
> On Fri, Dec 01, 2023 at 11:08:59AM +0530, Anshuman Khandual wrote:
>
> This looks good apart from a few small omissions:
>
>> +SysregFields BRBINFx_EL1
>
>> +Enum 13:8 TYPE
>> + 0b000000 UNCOND_DIRECT
>> + 0b000001 INDIRECT
>> + 0b000010 DIRECT_LINK
>> + 0b000011 INDIRECT_LINK
>> + 0b000101 RET
>> + 0b000111 ERET
>> + 0b001000 COND_DIRECT
>> + 0b100001 DEBUG_HALT
>> + 0b100010 CALL
>> + 0b100011 TRAP
>> + 0b100100 SERROR
>> + 0b100110 INSN_DEBUG
>> + 0b100111 DATA_DEBUG
>> + 0b101010 ALIGN_FAULT
>> + 0b101011 INSN_FAULT
>> + 0b101100 DATA_FAULT
>> + 0b101110 IRQ
>> + 0b101111 FIQ
>> + 0b111001 DEBUG_EXIT
>> +EndEnum
>
> DDI0601 2023-09 also defines 0b110000 as an IMPLEMENTATION DEFINED
> exception to EL3.

Should this field be just IMPDEF or IMPDEF_TRAP_EL3 sounds better ?

>
>> +SysregFields BRBCR_ELx
>> +Res0 63:24
>> +Field 23 EXCEPTION
>> +Field 22 ERTN
>> +Res0 21:9
>
> DDI0601 2023-09 defines bit 9 as FZPSS.

Sure, will update.

>
>> +Sysreg BRBINFINJ_EL1 2 1 9 1 0
>
>> +Enum 13:8 TYPE
>> + 0b000000 UNCOND_DIRECT
>> + 0b000001 INDIRECT
>> + 0b000010 DIRECT_LINK
>> + 0b000011 INDIRECT_LINK
>> + 0b000101 RET
>> + 0b000111 ERET
>> + 0b001000 COND_DIRECT
>> + 0b100001 DEBUG_HALT
>> + 0b100010 CALL
>> + 0b100011 TRAP
>> + 0b100100 SERROR
>> + 0b100110 INSN_DEBUG
>> + 0b100111 DATA_DEBUG
>> + 0b101010 ALIGN_FAULT
>> + 0b101011 INSN_FAULT
>> + 0b101100 DATA_FAULT
>> + 0b101110 IRQ
>> + 0b101111 FIQ
>> + 0b111001 DEBUG_EXIT
>> +EndEnum
>
> DDI0601 2023-09 also defines 0b11000 as IMPLEMENTATION DEFINED exception
> to EL3.

Should this field be just IMPDEF or IMPDEF_TRAP_EL3 sounds better ?

Seems like BRBINFx_EL1 could be used for defining BRBINFINJ_EL1 as well.

2023-12-04 08:15:46

by Marc Zyngier

[permalink] [raw]

Subject: Re: [PATCH V15 0/8] arm64/perf: Enable branch stack sampling

On Fri, 01 Dec 2023 05:38:58 +0000,
Anshuman Khandual <[email protected]> wrote:
>
> This series enables perf branch stack sampling support on arm64 platform
> via a new arch feature called Branch Record Buffer Extension (BRBE). All
> the relevant register definitions could be accessed here.
>
> https://developer.arm.com/documentation/ddi0601/2021-12/AArch64-Registers
>
> This series applies on 6.7-rc3 after the following series from James Clark.
>
> https://lore.kernel.org/all/[email protected]/

Please don't base a series on another one. I haven't looked at James
series yet, and dealing with conflict is my business.

You should base it on -rc3, full stop. If there is a dependency, make
it explicit and include a prefix of the other series you depend on.

M.

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

2023-12-04 08:22:40

by Marc Zyngier

[permalink] [raw]

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

On Fri, 01 Dec 2023 05:39:00 +0000,
Anshuman Khandual <[email protected]> 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 <[email protected]>
> Cc: Oliver Upton <[email protected]>
> Cc: James Morse <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> 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.

2023-12-04 08:43:41

by Marc Zyngier

[permalink] [raw]

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

On Fri, 01 Dec 2023 05:39:03 +0000,
Anshuman Khandual <[email protected]> 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 <[email protected]>
> Cc: Oliver Upton <[email protected]>
> Cc: James Morse <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> CC: [email protected]
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> 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.

2023-12-04 12:18:46

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 <[email protected]> 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();
....
}