diff for duplicates of <20171017115024.GS1845@lvm> diff --git a/a/1.txt b/N1/1.txt index 8ea7579..459d9eb 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -12,218 +12,3 @@ On Tue, Oct 10, 2017 at 07:38:39PM +0100, Dave Martin wrote: > Other instances are ported appropriately. I don't understand this paragraph, beginning from ", so this...". - - -From reading the code, what I think is the reason for having to flush -the SVE state (and mark the host state invalid) is that even though we -disallow SVE usage in the guest, the guest can use the normal FP state, -and while we always fully preserve the host state, this could still -corrupt some additional SVE state not properly preserved for the host. -Is that correct? - -> -> As a side effect of this refactoring, a this_cpu_write() in -> fpsimd_cpu_pm_notifier() is changed to __this_cpu_write(). This -> should be fine, since cpu_pm_enter() is supposed to be called only -> with interrupts disabled. - -Otherwise the patch itself looks good to me. - -Thanks, --Christoffer - -> -> Signed-off-by: Dave Martin <Dave.Martin@arm.com> -> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> -> Cc: Marc Zyngier <marc.zyngier@arm.com> -> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> -> --- -> arch/arm/include/asm/kvm_host.h | 3 +++ -> arch/arm64/include/asm/fpsimd.h | 1 + -> arch/arm64/include/asm/kvm_arm.h | 4 +++- -> arch/arm64/include/asm/kvm_host.h | 11 +++++++++++ -> arch/arm64/kernel/fpsimd.c | 31 +++++++++++++++++++++++++++++-- -> arch/arm64/kvm/hyp/switch.c | 6 +++--- -> virt/kvm/arm/arm.c | 3 +++ -> 7 files changed, 53 insertions(+), 6 deletions(-) -> -> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h -> index 4a879f6..242151e 100644 -> --- a/arch/arm/include/asm/kvm_host.h -> +++ b/arch/arm/include/asm/kvm_host.h -> @@ -293,4 +293,7 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, -> int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, -> struct kvm_device_attr *attr); -> -> +/* All host FP/SIMD state is restored on guest exit, so nothing to save: */ -> +static inline void kvm_fpsimd_flush_cpu_state(void) {} -> + -> #endif /* __ARM_KVM_HOST_H__ */ -> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h -> index 3cfdfbe..10b2824 100644 -> --- a/arch/arm64/include/asm/fpsimd.h -> +++ b/arch/arm64/include/asm/fpsimd.h -> @@ -75,6 +75,7 @@ extern void fpsimd_restore_current_state(void); -> extern void fpsimd_update_current_state(struct fpsimd_state *state); -> -> extern void fpsimd_flush_task_state(struct task_struct *target); -> +extern void sve_flush_cpu_state(void); -> -> /* Maximum VL that SVE VL-agnostic software can transparently support */ -> #define SVE_VL_ARCH_MAX 0x100 -> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h -> index dbf0537..7f069ff 100644 -> --- a/arch/arm64/include/asm/kvm_arm.h -> +++ b/arch/arm64/include/asm/kvm_arm.h -> @@ -186,7 +186,8 @@ -> #define CPTR_EL2_TTA (1 << 20) -> #define CPTR_EL2_TFP (1 << CPTR_EL2_TFP_SHIFT) -> #define CPTR_EL2_TZ (1 << 8) -> -#define CPTR_EL2_DEFAULT 0x000033ff -> +#define CPTR_EL2_RES1 0x000032ff /* known RES1 bits in CPTR_EL2 */ -> +#define CPTR_EL2_DEFAULT CPTR_EL2_RES1 -> -> /* Hyp Debug Configuration Register bits */ -> #define MDCR_EL2_TPMS (1 << 14) -> @@ -237,5 +238,6 @@ -> -> #define CPACR_EL1_FPEN (3 << 20) -> #define CPACR_EL1_TTA (1 << 28) -> +#define CPACR_EL1_DEFAULT (CPACR_EL1_FPEN | CPACR_EL1_ZEN_EL1EN) -> -> #endif /* __ARM64_KVM_ARM_H__ */ -> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h -> index e923b58..674912d 100644 -> --- a/arch/arm64/include/asm/kvm_host.h -> +++ b/arch/arm64/include/asm/kvm_host.h -> @@ -25,6 +25,7 @@ -> #include <linux/types.h> -> #include <linux/kvm_types.h> -> #include <asm/cpufeature.h> -> +#include <asm/fpsimd.h> -> #include <asm/kvm.h> -> #include <asm/kvm_asm.h> -> #include <asm/kvm_mmio.h> -> @@ -384,4 +385,14 @@ static inline void __cpu_init_stage2(void) -> "PARange is %d bits, unsupported configuration!", parange); -> } -> -> +/* -> + * All host FP/SIMD state is restored on guest exit, so nothing needs -> + * doing here except in the SVE case: -> +*/ -> +static inline void kvm_fpsimd_flush_cpu_state(void) -> +{ -> + if (system_supports_sve()) -> + sve_flush_cpu_state(); -> +} -> + -> #endif /* __ARM64_KVM_HOST_H__ */ -> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c -> index a9cb794..6ae3703 100644 -> --- a/arch/arm64/kernel/fpsimd.c -> +++ b/arch/arm64/kernel/fpsimd.c -> @@ -1073,6 +1073,33 @@ void fpsimd_flush_task_state(struct task_struct *t) -> t->thread.fpsimd_state.cpu = NR_CPUS; -> } -> -> +static inline void fpsimd_flush_cpu_state(void) -> +{ -> + __this_cpu_write(fpsimd_last_state, NULL); -> +} -> + -> +/* -> + * Invalidate any task SVE state currently held in this CPU's regs. -> + * -> + * This is used to prevent the kernel from trying to reuse SVE register data -> + * that is detroyed by KVM guest enter/exit. This function should go away when -> + * KVM SVE support is implemented. Don't use it for anything else. -> + */ -> +#ifdef CONFIG_ARM64_SVE -> +void sve_flush_cpu_state(void) -> +{ -> + struct fpsimd_state *const fpstate = __this_cpu_read(fpsimd_last_state); -> + struct task_struct *tsk; -> + -> + if (!fpstate) -> + return; -> + -> + tsk = container_of(fpstate, struct task_struct, thread.fpsimd_state); -> + if (test_tsk_thread_flag(tsk, TIF_SVE)) -> + fpsimd_flush_cpu_state(); -> +} -> +#endif /* CONFIG_ARM64_SVE */ -> + -> #ifdef CONFIG_KERNEL_MODE_NEON -> -> DEFINE_PER_CPU(bool, kernel_neon_busy); -> @@ -1113,7 +1140,7 @@ void kernel_neon_begin(void) -> } -> -> /* Invalidate any task state remaining in the fpsimd regs: */ -> - __this_cpu_write(fpsimd_last_state, NULL); -> + fpsimd_flush_cpu_state(); -> -> preempt_disable(); -> -> @@ -1234,7 +1261,7 @@ static int fpsimd_cpu_pm_notifier(struct notifier_block *self, -> case CPU_PM_ENTER: -> if (current->mm) -> task_fpsimd_save(); -> - this_cpu_write(fpsimd_last_state, NULL); -> + fpsimd_flush_cpu_state(); -> break; -> case CPU_PM_EXIT: -> if (current->mm) -> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c -> index 35a90b8..951f3eb 100644 -> --- a/arch/arm64/kvm/hyp/switch.c -> +++ b/arch/arm64/kvm/hyp/switch.c -> @@ -48,7 +48,7 @@ static void __hyp_text __activate_traps_vhe(void) -> -> val = read_sysreg(cpacr_el1); -> val |= CPACR_EL1_TTA; -> - val &= ~CPACR_EL1_FPEN; -> + val &= ~(CPACR_EL1_FPEN | CPACR_EL1_ZEN); -> write_sysreg(val, cpacr_el1); -> -> write_sysreg(__kvm_hyp_vector, vbar_el1); -> @@ -59,7 +59,7 @@ static void __hyp_text __activate_traps_nvhe(void) -> u64 val; -> -> val = CPTR_EL2_DEFAULT; -> - val |= CPTR_EL2_TTA | CPTR_EL2_TFP; -> + val |= CPTR_EL2_TTA | CPTR_EL2_TFP | CPTR_EL2_TZ; -> write_sysreg(val, cptr_el2); -> } -> -> @@ -117,7 +117,7 @@ static void __hyp_text __deactivate_traps_vhe(void) -> -> write_sysreg(mdcr_el2, mdcr_el2); -> write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2); -> - write_sysreg(CPACR_EL1_FPEN, cpacr_el1); -> + write_sysreg(CPACR_EL1_DEFAULT, cpacr_el1); -> write_sysreg(vectors, vbar_el1); -> } -> -> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c -> index b9f68e4..4d3cf9c 100644 -> --- a/virt/kvm/arm/arm.c -> +++ b/virt/kvm/arm/arm.c -> @@ -652,6 +652,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) -> */ -> preempt_disable(); -> -> + /* Flush FP/SIMD state that can't survive guest entry/exit */ -> + kvm_fpsimd_flush_cpu_state(); -> + -> kvm_pmu_flush_hwstate(vcpu); -> -> kvm_timer_flush_hwstate(vcpu); -> -- -> 2.1.4 -> -> _______________________________________________ -> kvmarm mailing list -> kvmarm@lists.cs.columbia.edu -> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm diff --git a/a/content_digest b/N1/content_digest index 2ec3f1e..7bc4a3e 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -30,221 +30,6 @@ "> is abstracted out as a separate helper fpsimd_flush_cpu_state().\n" "> Other instances are ported appropriately.\n" "\n" - "I don't understand this paragraph, beginning from \", so this...\".\n" - "\n" - "\n" - "From reading the code, what I think is the reason for having to flush\n" - "the SVE state (and mark the host state invalid) is that even though we\n" - "disallow SVE usage in the guest, the guest can use the normal FP state,\n" - "and while we always fully preserve the host state, this could still\n" - "corrupt some additional SVE state not properly preserved for the host.\n" - "Is that correct?\n" - "\n" - "> \n" - "> As a side effect of this refactoring, a this_cpu_write() in\n" - "> fpsimd_cpu_pm_notifier() is changed to __this_cpu_write(). This\n" - "> should be fine, since cpu_pm_enter() is supposed to be called only\n" - "> with interrupts disabled.\n" - "\n" - "Otherwise the patch itself looks good to me.\n" - "\n" - "Thanks,\n" - "-Christoffer\n" - "\n" - "> \n" - "> Signed-off-by: Dave Martin <Dave.Martin@arm.com>\n" - "> Reviewed-by: Alex Benn\303\251e <alex.bennee@linaro.org>\n" - "> Cc: Marc Zyngier <marc.zyngier@arm.com>\n" - "> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>\n" - "> ---\n" - "> arch/arm/include/asm/kvm_host.h | 3 +++\n" - "> arch/arm64/include/asm/fpsimd.h | 1 +\n" - "> arch/arm64/include/asm/kvm_arm.h | 4 +++-\n" - "> arch/arm64/include/asm/kvm_host.h | 11 +++++++++++\n" - "> arch/arm64/kernel/fpsimd.c | 31 +++++++++++++++++++++++++++++--\n" - "> arch/arm64/kvm/hyp/switch.c | 6 +++---\n" - "> virt/kvm/arm/arm.c | 3 +++\n" - "> 7 files changed, 53 insertions(+), 6 deletions(-)\n" - "> \n" - "> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h\n" - "> index 4a879f6..242151e 100644\n" - "> --- a/arch/arm/include/asm/kvm_host.h\n" - "> +++ b/arch/arm/include/asm/kvm_host.h\n" - "> @@ -293,4 +293,7 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu,\n" - "> int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu,\n" - "> \t\t\t struct kvm_device_attr *attr);\n" - "> \n" - "> +/* All host FP/SIMD state is restored on guest exit, so nothing to save: */\n" - "> +static inline void kvm_fpsimd_flush_cpu_state(void) {}\n" - "> +\n" - "> #endif /* __ARM_KVM_HOST_H__ */\n" - "> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h\n" - "> index 3cfdfbe..10b2824 100644\n" - "> --- a/arch/arm64/include/asm/fpsimd.h\n" - "> +++ b/arch/arm64/include/asm/fpsimd.h\n" - "> @@ -75,6 +75,7 @@ extern void fpsimd_restore_current_state(void);\n" - "> extern void fpsimd_update_current_state(struct fpsimd_state *state);\n" - "> \n" - "> extern void fpsimd_flush_task_state(struct task_struct *target);\n" - "> +extern void sve_flush_cpu_state(void);\n" - "> \n" - "> /* Maximum VL that SVE VL-agnostic software can transparently support */\n" - "> #define SVE_VL_ARCH_MAX 0x100\n" - "> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h\n" - "> index dbf0537..7f069ff 100644\n" - "> --- a/arch/arm64/include/asm/kvm_arm.h\n" - "> +++ b/arch/arm64/include/asm/kvm_arm.h\n" - "> @@ -186,7 +186,8 @@\n" - "> #define CPTR_EL2_TTA\t(1 << 20)\n" - "> #define CPTR_EL2_TFP\t(1 << CPTR_EL2_TFP_SHIFT)\n" - "> #define CPTR_EL2_TZ\t(1 << 8)\n" - "> -#define CPTR_EL2_DEFAULT\t0x000033ff\n" - "> +#define CPTR_EL2_RES1\t0x000032ff /* known RES1 bits in CPTR_EL2 */\n" - "> +#define CPTR_EL2_DEFAULT\tCPTR_EL2_RES1\n" - "> \n" - "> /* Hyp Debug Configuration Register bits */\n" - "> #define MDCR_EL2_TPMS\t\t(1 << 14)\n" - "> @@ -237,5 +238,6 @@\n" - "> \n" - "> #define CPACR_EL1_FPEN\t\t(3 << 20)\n" - "> #define CPACR_EL1_TTA\t\t(1 << 28)\n" - "> +#define CPACR_EL1_DEFAULT\t(CPACR_EL1_FPEN | CPACR_EL1_ZEN_EL1EN)\n" - "> \n" - "> #endif /* __ARM64_KVM_ARM_H__ */\n" - "> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h\n" - "> index e923b58..674912d 100644\n" - "> --- a/arch/arm64/include/asm/kvm_host.h\n" - "> +++ b/arch/arm64/include/asm/kvm_host.h\n" - "> @@ -25,6 +25,7 @@\n" - "> #include <linux/types.h>\n" - "> #include <linux/kvm_types.h>\n" - "> #include <asm/cpufeature.h>\n" - "> +#include <asm/fpsimd.h>\n" - "> #include <asm/kvm.h>\n" - "> #include <asm/kvm_asm.h>\n" - "> #include <asm/kvm_mmio.h>\n" - "> @@ -384,4 +385,14 @@ static inline void __cpu_init_stage2(void)\n" - "> \t\t \"PARange is %d bits, unsupported configuration!\", parange);\n" - "> }\n" - "> \n" - "> +/*\n" - "> + * All host FP/SIMD state is restored on guest exit, so nothing needs\n" - "> + * doing here except in the SVE case:\n" - "> +*/\n" - "> +static inline void kvm_fpsimd_flush_cpu_state(void)\n" - "> +{\n" - "> +\tif (system_supports_sve())\n" - "> +\t\tsve_flush_cpu_state();\n" - "> +}\n" - "> +\n" - "> #endif /* __ARM64_KVM_HOST_H__ */\n" - "> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c\n" - "> index a9cb794..6ae3703 100644\n" - "> --- a/arch/arm64/kernel/fpsimd.c\n" - "> +++ b/arch/arm64/kernel/fpsimd.c\n" - "> @@ -1073,6 +1073,33 @@ void fpsimd_flush_task_state(struct task_struct *t)\n" - "> \tt->thread.fpsimd_state.cpu = NR_CPUS;\n" - "> }\n" - "> \n" - "> +static inline void fpsimd_flush_cpu_state(void)\n" - "> +{\n" - "> +\t__this_cpu_write(fpsimd_last_state, NULL);\n" - "> +}\n" - "> +\n" - "> +/*\n" - "> + * Invalidate any task SVE state currently held in this CPU's regs.\n" - "> + *\n" - "> + * This is used to prevent the kernel from trying to reuse SVE register data\n" - "> + * that is detroyed by KVM guest enter/exit. This function should go away when\n" - "> + * KVM SVE support is implemented. Don't use it for anything else.\n" - "> + */\n" - "> +#ifdef CONFIG_ARM64_SVE\n" - "> +void sve_flush_cpu_state(void)\n" - "> +{\n" - "> +\tstruct fpsimd_state *const fpstate = __this_cpu_read(fpsimd_last_state);\n" - "> +\tstruct task_struct *tsk;\n" - "> +\n" - "> +\tif (!fpstate)\n" - "> +\t\treturn;\n" - "> +\n" - "> +\ttsk = container_of(fpstate, struct task_struct, thread.fpsimd_state);\n" - "> +\tif (test_tsk_thread_flag(tsk, TIF_SVE))\n" - "> +\t\tfpsimd_flush_cpu_state();\n" - "> +}\n" - "> +#endif /* CONFIG_ARM64_SVE */\n" - "> +\n" - "> #ifdef CONFIG_KERNEL_MODE_NEON\n" - "> \n" - "> DEFINE_PER_CPU(bool, kernel_neon_busy);\n" - "> @@ -1113,7 +1140,7 @@ void kernel_neon_begin(void)\n" - "> \t}\n" - "> \n" - "> \t/* Invalidate any task state remaining in the fpsimd regs: */\n" - "> -\t__this_cpu_write(fpsimd_last_state, NULL);\n" - "> +\tfpsimd_flush_cpu_state();\n" - "> \n" - "> \tpreempt_disable();\n" - "> \n" - "> @@ -1234,7 +1261,7 @@ static int fpsimd_cpu_pm_notifier(struct notifier_block *self,\n" - "> \tcase CPU_PM_ENTER:\n" - "> \t\tif (current->mm)\n" - "> \t\t\ttask_fpsimd_save();\n" - "> -\t\tthis_cpu_write(fpsimd_last_state, NULL);\n" - "> +\t\tfpsimd_flush_cpu_state();\n" - "> \t\tbreak;\n" - "> \tcase CPU_PM_EXIT:\n" - "> \t\tif (current->mm)\n" - "> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c\n" - "> index 35a90b8..951f3eb 100644\n" - "> --- a/arch/arm64/kvm/hyp/switch.c\n" - "> +++ b/arch/arm64/kvm/hyp/switch.c\n" - "> @@ -48,7 +48,7 @@ static void __hyp_text __activate_traps_vhe(void)\n" - "> \n" - "> \tval = read_sysreg(cpacr_el1);\n" - "> \tval |= CPACR_EL1_TTA;\n" - "> -\tval &= ~CPACR_EL1_FPEN;\n" - "> +\tval &= ~(CPACR_EL1_FPEN | CPACR_EL1_ZEN);\n" - "> \twrite_sysreg(val, cpacr_el1);\n" - "> \n" - "> \twrite_sysreg(__kvm_hyp_vector, vbar_el1);\n" - "> @@ -59,7 +59,7 @@ static void __hyp_text __activate_traps_nvhe(void)\n" - "> \tu64 val;\n" - "> \n" - "> \tval = CPTR_EL2_DEFAULT;\n" - "> -\tval |= CPTR_EL2_TTA | CPTR_EL2_TFP;\n" - "> +\tval |= CPTR_EL2_TTA | CPTR_EL2_TFP | CPTR_EL2_TZ;\n" - "> \twrite_sysreg(val, cptr_el2);\n" - "> }\n" - "> \n" - "> @@ -117,7 +117,7 @@ static void __hyp_text __deactivate_traps_vhe(void)\n" - "> \n" - "> \twrite_sysreg(mdcr_el2, mdcr_el2);\n" - "> \twrite_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2);\n" - "> -\twrite_sysreg(CPACR_EL1_FPEN, cpacr_el1);\n" - "> +\twrite_sysreg(CPACR_EL1_DEFAULT, cpacr_el1);\n" - "> \twrite_sysreg(vectors, vbar_el1);\n" - "> }\n" - "> \n" - "> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c\n" - "> index b9f68e4..4d3cf9c 100644\n" - "> --- a/virt/kvm/arm/arm.c\n" - "> +++ b/virt/kvm/arm/arm.c\n" - "> @@ -652,6 +652,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)\n" - "> \t\t */\n" - "> \t\tpreempt_disable();\n" - "> \n" - "> +\t\t/* Flush FP/SIMD state that can't survive guest entry/exit */\n" - "> +\t\tkvm_fpsimd_flush_cpu_state();\n" - "> +\n" - "> \t\tkvm_pmu_flush_hwstate(vcpu);\n" - "> \n" - "> \t\tkvm_timer_flush_hwstate(vcpu);\n" - "> -- \n" - "> 2.1.4\n" - "> \n" - "> _______________________________________________\n" - "> kvmarm mailing list\n" - "> kvmarm@lists.cs.columbia.edu\n" - > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm + "I don't understand this paragraph, beginning from \", so this...\"." -1602a0e245eaf091d0b8039c3b153e2026414dc55db0d09d166150a336f9b610 +bb2c565630fbf4b8ceacea549fd77facb1f401f18c7443072f2ed16bb05aa754
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox