Re: [PATCH v3 4/7] arm64/fpsimd: Stop using TIF_SVE to manage register saving in KVM
On Tue, Sep 20, 2022 at 07:04:24PM +0100, Marc Zyngier wrote:
> Mark Brown wrote:
> > - switch (last->to_save) {
> > - case FP_STATE_TASK:
> > - break;
> > - case FP_STATE_FPSIMD:
> > - WARN_ON_ONCE(save_sve_regs);
> > - break;
> > - case FP_STATE_SVE:
> > - WARN_ON_ONCE(!save_sve_regs);
> > - break;
> > - }
> Given how short-lived this code is, consider dropping it altogether.
> Actually, the previous patch would make a lot more sense if it was
> merged with this one.
My thinking here is to introduce the state tracking and the
behaviour change separately to make it easier to unpick things if
anything goes wrong, it means that the behaviour change is in
clearly isolated patches separate to the more wide spread changes
to behaviour. The early patches make it more explicit what we
are currently doing, the later ones do new things.
signature.asc
Description: PGP signature
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v3 4/7] arm64/fpsimd: Stop using TIF_SVE to manage register saving in KVM
On Mon, 15 Aug 2022 23:55:26 +0100,
Mark Brown wrote:
>
> Now that we are explicitly telling the host FP code which register state
> it needs to save we can remove the manipulation of TIF_SVE from the KVM
> code, simplifying it and allowing us to optimise our handling of normal
> tasks. Remove the manipulation of TIF_SVE from KVM and instead rely on
> to_save to ensure we save the correct data for it.
>
> Signed-off-by: Mark Brown
> ---
> arch/arm64/kernel/fpsimd.c | 22 --
> arch/arm64/kvm/fpsimd.c| 3 ---
> 2 files changed, 4 insertions(+), 21 deletions(-)
>
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 7be20ced2c45..aaea2dc02cbd 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -436,8 +436,8 @@ static void task_fpsimd_load(void)
> * last, if KVM is involved this may be the guest VM context rather
> * than the host thread for the VM pointed to by current. This means
> * that we must always reference the state storage via last rather
> - * than via current, other than the TIF_ flags which KVM will
> - * carefully maintain for us.
> + * than via current, if we are saving KVM state then it will have
> + * ensured that the type of registers to save is set in last->to_save.
> */
> static void fpsimd_save(void)
> {
> @@ -454,27 +454,13 @@ static void fpsimd_save(void)
> if (test_thread_flag(TIF_FOREIGN_FPSTATE))
> return;
>
> - if (test_thread_flag(TIF_SVE)) {
> + if ((last->to_save == FP_STATE_TASK && test_thread_flag(TIF_SVE)) ||
> + last->to_save == FP_STATE_SVE) {
> save_sve_regs = true;
> save_ffr = true;
> vl = last->sve_vl;
> }
>
> - /*
> - * For now we're just validating that the requested state is
> - * consistent with what we'd otherwise work out.
> - */
> - switch (last->to_save) {
> - case FP_STATE_TASK:
> - break;
> - case FP_STATE_FPSIMD:
> - WARN_ON_ONCE(save_sve_regs);
> - break;
> - case FP_STATE_SVE:
> - WARN_ON_ONCE(!save_sve_regs);
> - break;
> - }
> -
Given how short-lived this code is, consider dropping it altogether.
Actually, the previous patch would make a lot more sense if it was
merged with this one.
> if (system_supports_sme()) {
> u64 *svcr = last->svcr;
>
> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> index db0b2bacaeb8..8a79823fce68 100644
> --- a/arch/arm64/kvm/fpsimd.c
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -151,7 +151,6 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
>>arch.fp_type, fp_type);
>
> clear_thread_flag(TIF_FOREIGN_FPSTATE);
> - update_thread_flag(TIF_SVE, vcpu_has_sve(vcpu));
> }
> }
>
> @@ -208,7 +207,5 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> sysreg_clear_set(CPACR_EL1, CPACR_EL1_ZEN_EL0EN, 0);
> }
>
> - update_thread_flag(TIF_SVE, 0);
> -
> local_irq_restore(flags);
> }
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH v3 4/7] arm64/fpsimd: Stop using TIF_SVE to manage register saving in KVM
Now that we are explicitly telling the host FP code which register state
it needs to save we can remove the manipulation of TIF_SVE from the KVM
code, simplifying it and allowing us to optimise our handling of normal
tasks. Remove the manipulation of TIF_SVE from KVM and instead rely on
to_save to ensure we save the correct data for it.
Signed-off-by: Mark Brown
---
arch/arm64/kernel/fpsimd.c | 22 --
arch/arm64/kvm/fpsimd.c| 3 ---
2 files changed, 4 insertions(+), 21 deletions(-)
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 7be20ced2c45..aaea2dc02cbd 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -436,8 +436,8 @@ static void task_fpsimd_load(void)
* last, if KVM is involved this may be the guest VM context rather
* than the host thread for the VM pointed to by current. This means
* that we must always reference the state storage via last rather
- * than via current, other than the TIF_ flags which KVM will
- * carefully maintain for us.
+ * than via current, if we are saving KVM state then it will have
+ * ensured that the type of registers to save is set in last->to_save.
*/
static void fpsimd_save(void)
{
@@ -454,27 +454,13 @@ static void fpsimd_save(void)
if (test_thread_flag(TIF_FOREIGN_FPSTATE))
return;
- if (test_thread_flag(TIF_SVE)) {
+ if ((last->to_save == FP_STATE_TASK && test_thread_flag(TIF_SVE)) ||
+ last->to_save == FP_STATE_SVE) {
save_sve_regs = true;
save_ffr = true;
vl = last->sve_vl;
}
- /*
-* For now we're just validating that the requested state is
-* consistent with what we'd otherwise work out.
-*/
- switch (last->to_save) {
- case FP_STATE_TASK:
- break;
- case FP_STATE_FPSIMD:
- WARN_ON_ONCE(save_sve_regs);
- break;
- case FP_STATE_SVE:
- WARN_ON_ONCE(!save_sve_regs);
- break;
- }
-
if (system_supports_sme()) {
u64 *svcr = last->svcr;
diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index db0b2bacaeb8..8a79823fce68 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -151,7 +151,6 @@ void kvm_arch_vcpu_ctxsync_fp(struct kvm_vcpu *vcpu)
>arch.fp_type, fp_type);
clear_thread_flag(TIF_FOREIGN_FPSTATE);
- update_thread_flag(TIF_SVE, vcpu_has_sve(vcpu));
}
}
@@ -208,7 +207,5 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
sysreg_clear_set(CPACR_EL1, CPACR_EL1_ZEN_EL0EN, 0);
}
- update_thread_flag(TIF_SVE, 0);
-
local_irq_restore(flags);
}
--
2.30.2
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
