Re: [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly
From: Suzuki Kuruppassery Poulose
Date: Mon Jan 13 2020 - 05:28:13 EST
On 10/01/2020 15:21, Marc Zyngier wrote:
On 2019-12-18 12:00, Suzuki Kuruppassery Poulose wrote:
On 18/12/2019 11:56, Marc Zyngier wrote:
On 2019-12-18 11:42, Suzuki Kuruppassery Poulose wrote:
Hi Marc,
On 17/12/2019 19:05, Marc Zyngier wrote:
KVM also uses the TIF_FOREIGN_FPSTATE flag to manage the FP/SIMD
state
on the CPU. However, without FP/SIMD support we trap all accesses and
inject undefined instruction. Thus we should never "load" guest
state.
Add a sanity check to make sure this is valid.
Yes, but no, see below.
Fixes: 82e0191a1aa11abf ("arm64: Support systems without FP/ASIMD")
Cc: Will Deacon <will@xxxxxxxxxx>
Cc: Mark Rutland <mark.rutland@xxxxxxx>
Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
Cc: Marc Zyngier <marc.zyngier@xxxxxxx>
No idea who that guy is. It's a fake! ;-)
Sorry about that, will fix it.
Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
---
Âarch/arm64/kernel/fpsimd.c | 31 +++++++++++++++++++++++++++----
Âarch/arm64/kvm/hyp/switch.c |Â 9 +++++++++
Â2 files changed, 36 insertions(+), 4 deletions(-)
[...]
diff --git a/arch/arm64/kvm/hyp/switch.c
b/arch/arm64/kvm/hyp/switch.c
index 72fbbd86eb5e..9696ebb5c13a 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -28,10 +28,19 @@
Â/* Check whether the FP regs were dirtied while in the host-side run
loop: */
Âstatic bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
Â{
+ÂÂÂ /*
+ÂÂÂÂ * When the system doesn't support FP/SIMD, we cannot rely on
+ÂÂÂÂ * the state of _TIF_FOREIGN_FPSTATE. However, we will never
+ÂÂÂÂ * set the KVM_ARM64_FP_ENABLED, as the FP/SIMD accesses always
+ÂÂÂÂ * inject an abort into the guest. Thus we always trap the
+ÂÂÂÂ * accesses.
+ÂÂÂÂ */
ÂÂÂÂ if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
ÂÂÂÂÂÂÂÂ vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ KVM_ARM64_FP_HOST);
+ÂÂÂ WARN_ON(!system_supports_fpsimd() &&
+ÂÂÂÂÂÂÂ (vcpu->arch.flags & KVM_ARM64_FP_ENABLED));
Careful, this will panic the host if it happens on a !VHE host
(calling non-inline stuff from a __hyp_text function is usually
not a good idea).
Ouch! Sorry about that WARN_ON()! I could drop the warning and
make this :
if (!system_supports_fpsimd() ||
ÂÂÂ (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE))
ÂÂÂÂvcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ KVM_ARM64_FP_HOST);
to make sure we never say fp is enabled.
What do you think ?
Sure, that would work. I can't really see how KVM_ARM64_FP_ENABLED
Thanks I have fixed this locally now.
would get set though. But it probably doesn't matter (WTF is going
Right. That cannot be set to begin with, as the first access to FP/SIMD
injects an abort back to the guest, which is why I added a WARN() to
begin with.
Just wanted to be extra safe.
to run KVM with such broken HW?), and better safe than sorry.
Right, with no COMPAT KVM support it is really hard to get this far.
So with the above fix:
Acked-by: Marc Zyngier <maz@xxxxxxxxxx>
ÂÂÂÂÂÂÂ M.
Thanks, I have changed the KVM hunk to :
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index 72fbbd86eb5e..e5816d885761 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -28,7 +28,15 @@
/* Check whether the FP regs were dirtied while in the host-side run
loop: */
static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
{
- if (vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
+ /*
+ * When the system doesn't support FP/SIMD, we cannot rely on
+ * the _TIF_FOREIGN_FPSTATE flag. However, we always inject an
+ * abort on the very first access to FP and thus we should never
+ * see KVM_ARM64_FP_ENABLED. For added safety, make sure we always
+ * trap the accesses.
+ */
+ if (!system_supports_fpsimd() ||
+ vcpu->arch.host_thread_info->flags & _TIF_FOREIGN_FPSTATE)
vcpu->arch.flags &= ~(KVM_ARM64_FP_ENABLED |
KVM_ARM64_FP_HOST);
Suzuki