Re: [PATCH v7 1/3] arm64: kvm: Save/restore MTE registers
On 2021-02-04 14:33, Steven Price wrote: On 02/02/2021 15:36, Marc Zyngier wrote: On 2021-01-15 15:28, Steven Price wrote: Define the new system registers that MTE introduces and context switch them. The MTE feature is still hidden from the ID register as it isn't supported in a VM yet. Signed-off-by: Steven Price --- arch/arm64/include/asm/kvm_host.h | 4 ++ arch/arm64/include/asm/kvm_mte.h | 74 ++ arch/arm64/include/asm/sysreg.h | 3 +- arch/arm64/kernel/asm-offsets.c | 3 + arch/arm64/kvm/hyp/entry.S | 7 ++ arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 4 ++ arch/arm64/kvm/sys_regs.c | 14 ++-- 7 files changed, 104 insertions(+), 5 deletions(-) create mode 100644 arch/arm64/include/asm/kvm_mte.h diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 11beda85ee7e..51590a397e4b 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -148,6 +148,8 @@ enum vcpu_sysreg { SCTLR_EL1, /* System Control Register */ ACTLR_EL1, /* Auxiliary Control Register */ CPACR_EL1, /* Coprocessor Access Control */ + RGSR_EL1, /* Random Allocation Tag Seed Register */ + GCR_EL1, /* Tag Control Register */ ZCR_EL1, /* SVE Control */ TTBR0_EL1, /* Translation Table Base Register 0 */ TTBR1_EL1, /* Translation Table Base Register 1 */ @@ -164,6 +166,8 @@ enum vcpu_sysreg { TPIDR_EL1, /* Thread ID, Privileged */ AMAIR_EL1, /* Aux Memory Attribute Indirection Register */ CNTKCTL_EL1, /* Timer Control Register (EL1) */ + TFSRE0_EL1, /* Tag Fault Status Register (EL0) */ + TFSR_EL1, /* Tag Fault Stauts Register (EL1) */ s/Stauts/Status/ Is there any reason why the MTE registers aren't grouped together? I has been under the impression this list is sorted by the encoding of the system registers, although double checking I've screwed up the order of TFSRE0_EL1/TFSR_EL1, and not all the other fields are sorted that way. It grew organically, and was initially matching the original order of the save/restore sequence. This order has long disappeared with VHE, and this is essentially nothing more than a bag of indices (although NV does bring some order back to deal with VNCR-backed registers). [...] diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h index cce43bfe158f..94d9736f0133 100644 --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h @@ -45,6 +45,8 @@ static inline void __sysreg_save_el1_state(struct kvm_cpu_context *ctxt) ctxt_sys_reg(ctxt, CNTKCTL_EL1) = read_sysreg_el1(SYS_CNTKCTL); ctxt_sys_reg(ctxt, PAR_EL1) = read_sysreg_par(); ctxt_sys_reg(ctxt, TPIDR_EL1) = read_sysreg(tpidr_el1); + if (system_supports_mte()) + ctxt_sys_reg(ctxt, TFSR_EL1) = read_sysreg_el1(SYS_TFSR); I already asked for it, and I'm going to ask for it again: Most of the sysreg save/restore is guarded by a per-vcpu check (HCR_EL2.ATA), while this one is unconditionally saved/restore if the host is MTE capable. Why is that so? Sorry, I thought your concern was for registers that affect the host (as they are obviously more performance critical as they are hit on every guest exit). Although I guess that's incorrect for nVHE which is what all the cool kids want now ;) I think we want both correctness *and* performance, for both VHE and nVHE. Things like EL0 registers should be able to be moved to load/put on all implementations, and the correct switching be done at the right spot only when required. Thanks, M. -- Jazz is not dead. It just smells funny...
Re: [PATCH v7 1/3] arm64: kvm: Save/restore MTE registers
On 02/02/2021 15:36, Marc Zyngier wrote: On 2021-01-15 15:28, Steven Price wrote: Define the new system registers that MTE introduces and context switch them. The MTE feature is still hidden from the ID register as it isn't supported in a VM yet. Signed-off-by: Steven Price --- arch/arm64/include/asm/kvm_host.h | 4 ++ arch/arm64/include/asm/kvm_mte.h | 74 ++ arch/arm64/include/asm/sysreg.h | 3 +- arch/arm64/kernel/asm-offsets.c | 3 + arch/arm64/kvm/hyp/entry.S | 7 ++ arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 4 ++ arch/arm64/kvm/sys_regs.c | 14 ++-- 7 files changed, 104 insertions(+), 5 deletions(-) create mode 100644 arch/arm64/include/asm/kvm_mte.h diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 11beda85ee7e..51590a397e4b 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -148,6 +148,8 @@ enum vcpu_sysreg { SCTLR_EL1, /* System Control Register */ ACTLR_EL1, /* Auxiliary Control Register */ CPACR_EL1, /* Coprocessor Access Control */ + RGSR_EL1, /* Random Allocation Tag Seed Register */ + GCR_EL1, /* Tag Control Register */ ZCR_EL1, /* SVE Control */ TTBR0_EL1, /* Translation Table Base Register 0 */ TTBR1_EL1, /* Translation Table Base Register 1 */ @@ -164,6 +166,8 @@ enum vcpu_sysreg { TPIDR_EL1, /* Thread ID, Privileged */ AMAIR_EL1, /* Aux Memory Attribute Indirection Register */ CNTKCTL_EL1, /* Timer Control Register (EL1) */ + TFSRE0_EL1, /* Tag Fault Status Register (EL0) */ + TFSR_EL1, /* Tag Fault Stauts Register (EL1) */ s/Stauts/Status/ Is there any reason why the MTE registers aren't grouped together? I has been under the impression this list is sorted by the encoding of the system registers, although double checking I've screwed up the order of TFSRE0_EL1/TFSR_EL1, and not all the other fields are sorted that way. I'll move them together in their own section. PAR_EL1, /* Physical Address Register */ MDSCR_EL1, /* Monitor Debug System Control Register */ MDCCINT_EL1, /* Monitor Debug Comms Channel Interrupt Enable Reg */ diff --git a/arch/arm64/include/asm/kvm_mte.h b/arch/arm64/include/asm/kvm_mte.h new file mode 100644 index ..62bbfae77f33 --- /dev/null +++ b/arch/arm64/include/asm/kvm_mte.h @@ -0,0 +1,74 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2020 ARM Ltd. + */ +#ifndef __ASM_KVM_MTE_H +#define __ASM_KVM_MTE_H + +#ifdef __ASSEMBLY__ + +#include + +#ifdef CONFIG_ARM64_MTE + +.macro mte_switch_to_guest g_ctxt, h_ctxt, reg1 +alternative_if_not ARM64_MTE + b .L__skip_switch\@ +alternative_else_nop_endif + mrs \reg1, hcr_el2 + and \reg1, \reg1, #(HCR_ATA) + cbz \reg1, .L__skip_switch\@ + + mrs_s \reg1, SYS_RGSR_EL1 + str \reg1, [\h_ctxt, #CPU_RGSR_EL1] + mrs_s \reg1, SYS_GCR_EL1 + str \reg1, [\h_ctxt, #CPU_GCR_EL1] + mrs_s \reg1, SYS_TFSRE0_EL1 + str \reg1, [\h_ctxt, #CPU_TFSRE0_EL1] + + ldr \reg1, [\g_ctxt, #CPU_RGSR_EL1] + msr_s SYS_RGSR_EL1, \reg1 + ldr \reg1, [\g_ctxt, #CPU_GCR_EL1] + msr_s SYS_GCR_EL1, \reg1 + ldr \reg1, [\g_ctxt, #CPU_TFSRE0_EL1] + msr_s SYS_TFSRE0_EL1, \reg1 + +.L__skip_switch\@: +.endm + +.macro mte_switch_to_hyp g_ctxt, h_ctxt, reg1 +alternative_if_not ARM64_MTE + b .L__skip_switch\@ +alternative_else_nop_endif + mrs \reg1, hcr_el2 + and \reg1, \reg1, #(HCR_ATA) + cbz \reg1, .L__skip_switch\@ + + mrs_s \reg1, SYS_RGSR_EL1 + str \reg1, [\g_ctxt, #CPU_RGSR_EL1] + mrs_s \reg1, SYS_GCR_EL1 + str \reg1, [\g_ctxt, #CPU_GCR_EL1] + mrs_s \reg1, SYS_TFSRE0_EL1 + str \reg1, [\g_ctxt, #CPU_TFSRE0_EL1] Can't the EL0 state save/restore be moved to the C code? True, that should be safe. I'm not sure how I missed that. + + ldr \reg1, [\h_ctxt, #CPU_RGSR_EL1] + msr_s SYS_RGSR_EL1, \reg1 + ldr \reg1, [\h_ctxt, #CPU_GCR_EL1] + msr_s SYS_GCR_EL1, \reg1 + ldr \reg1, [\h_ctxt, #CPU_TFSRE0_EL1] + msr_s SYS_TFSRE0_EL1, \reg1 + +.L__skip_switch\@: +.endm + +#else /* CONFIG_ARM64_MTE */ + +.macro mte_switch_to_guest g_ctxt, h_ctxt, reg1 +.endm + +.macro mte_switch_to_hyp g_ctxt, h_ctxt, reg1 +.endm + +#endif /* CONFIG_ARM64_MTE */ +#endif /* __ASSEMBLY__ */ +#endif /* __ASM_KVM_MTE_H */ diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index 8b5e7e5c3cc8..0a01975d331d 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -574,7 +574,8 @@ #define SCTLR_ELx_M (BIT(0)) #define SCTLR_ELx_FLAGS (SCTLR_ELx_M | SCTLR_ELx_A | SCTLR_ELx_C | \ - SCTLR_ELx_SA | SCTLR_ELx_I | SCTLR_ELx_IESB) + SCTLR_ELx_SA | SCTLR_ELx_I | SCTLR_ELx_IESB | \ +
Re: [PATCH v7 1/3] arm64: kvm: Save/restore MTE registers
On 2021-01-15 15:28, Steven Price wrote: Define the new system registers that MTE introduces and context switch them. The MTE feature is still hidden from the ID register as it isn't supported in a VM yet. Signed-off-by: Steven Price --- arch/arm64/include/asm/kvm_host.h | 4 ++ arch/arm64/include/asm/kvm_mte.h | 74 ++ arch/arm64/include/asm/sysreg.h| 3 +- arch/arm64/kernel/asm-offsets.c| 3 + arch/arm64/kvm/hyp/entry.S | 7 ++ arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 4 ++ arch/arm64/kvm/sys_regs.c | 14 ++-- 7 files changed, 104 insertions(+), 5 deletions(-) create mode 100644 arch/arm64/include/asm/kvm_mte.h diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 11beda85ee7e..51590a397e4b 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -148,6 +148,8 @@ enum vcpu_sysreg { SCTLR_EL1, /* System Control Register */ ACTLR_EL1, /* Auxiliary Control Register */ CPACR_EL1, /* Coprocessor Access Control */ + RGSR_EL1, /* Random Allocation Tag Seed Register */ + GCR_EL1,/* Tag Control Register */ ZCR_EL1,/* SVE Control */ TTBR0_EL1, /* Translation Table Base Register 0 */ TTBR1_EL1, /* Translation Table Base Register 1 */ @@ -164,6 +166,8 @@ enum vcpu_sysreg { TPIDR_EL1, /* Thread ID, Privileged */ AMAIR_EL1, /* Aux Memory Attribute Indirection Register */ CNTKCTL_EL1,/* Timer Control Register (EL1) */ + TFSRE0_EL1, /* Tag Fault Status Register (EL0) */ + TFSR_EL1, /* Tag Fault Stauts Register (EL1) */ s/Stauts/Status/ Is there any reason why the MTE registers aren't grouped together? PAR_EL1,/* Physical Address Register */ MDSCR_EL1, /* Monitor Debug System Control Register */ MDCCINT_EL1,/* Monitor Debug Comms Channel Interrupt Enable Reg */ diff --git a/arch/arm64/include/asm/kvm_mte.h b/arch/arm64/include/asm/kvm_mte.h new file mode 100644 index ..62bbfae77f33 --- /dev/null +++ b/arch/arm64/include/asm/kvm_mte.h @@ -0,0 +1,74 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2020 ARM Ltd. + */ +#ifndef __ASM_KVM_MTE_H +#define __ASM_KVM_MTE_H + +#ifdef __ASSEMBLY__ + +#include + +#ifdef CONFIG_ARM64_MTE + +.macro mte_switch_to_guest g_ctxt, h_ctxt, reg1 +alternative_if_not ARM64_MTE + b .L__skip_switch\@ +alternative_else_nop_endif + mrs \reg1, hcr_el2 + and \reg1, \reg1, #(HCR_ATA) + cbz \reg1, .L__skip_switch\@ + + mrs_s \reg1, SYS_RGSR_EL1 + str \reg1, [\h_ctxt, #CPU_RGSR_EL1] + mrs_s \reg1, SYS_GCR_EL1 + str \reg1, [\h_ctxt, #CPU_GCR_EL1] + mrs_s \reg1, SYS_TFSRE0_EL1 + str \reg1, [\h_ctxt, #CPU_TFSRE0_EL1] + + ldr \reg1, [\g_ctxt, #CPU_RGSR_EL1] + msr_s SYS_RGSR_EL1, \reg1 + ldr \reg1, [\g_ctxt, #CPU_GCR_EL1] + msr_s SYS_GCR_EL1, \reg1 + ldr \reg1, [\g_ctxt, #CPU_TFSRE0_EL1] + msr_s SYS_TFSRE0_EL1, \reg1 + +.L__skip_switch\@: +.endm + +.macro mte_switch_to_hyp g_ctxt, h_ctxt, reg1 +alternative_if_not ARM64_MTE + b .L__skip_switch\@ +alternative_else_nop_endif + mrs \reg1, hcr_el2 + and \reg1, \reg1, #(HCR_ATA) + cbz \reg1, .L__skip_switch\@ + + mrs_s \reg1, SYS_RGSR_EL1 + str \reg1, [\g_ctxt, #CPU_RGSR_EL1] + mrs_s \reg1, SYS_GCR_EL1 + str \reg1, [\g_ctxt, #CPU_GCR_EL1] + mrs_s \reg1, SYS_TFSRE0_EL1 + str \reg1, [\g_ctxt, #CPU_TFSRE0_EL1] Can't the EL0 state save/restore be moved to the C code? + + ldr \reg1, [\h_ctxt, #CPU_RGSR_EL1] + msr_s SYS_RGSR_EL1, \reg1 + ldr \reg1, [\h_ctxt, #CPU_GCR_EL1] + msr_s SYS_GCR_EL1, \reg1 + ldr \reg1, [\h_ctxt, #CPU_TFSRE0_EL1] + msr_s SYS_TFSRE0_EL1, \reg1 + +.L__skip_switch\@: +.endm + +#else /* CONFIG_ARM64_MTE */ + +.macro mte_switch_to_guest g_ctxt, h_ctxt, reg1 +.endm + +.macro mte_switch_to_hyp g_ctxt, h_ctxt, reg1 +.endm + +#endif /* CONFIG_ARM64_MTE */ +#endif /* __ASSEMBLY__ */ +#endif /* __ASM_KVM_MTE_H */ diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h index 8b5e7e5c3cc8..0a01975d331d 100644 --- a/arch/arm64/include/asm/sysreg.h +++ b/arch/arm64/include/asm/sysreg.h @@ -574,7 +574,8 @@ #define SCTLR_ELx_M(BIT(0)) #define SCTLR_ELx_FLAGS(SCTLR_ELx_M | SCTLR_ELx_A | SCTLR_ELx_C | \ -SCTLR_ELx_SA | SCTLR_ELx_I | SCTLR_ELx_IESB) +SCTLR_ELx_SA | SCTLR_ELx_I | SCTLR_ELx_IESB | \ +SCTLR_ELx_ITFSB) /* SCTLR_EL2 specific flags. */ #define SCTLR_EL2_RES1 ((BIT(4)) | (BIT(5)) | (BIT(11)) | (BIT(16)) | \ diff --git