From: Suzuki Kuruppassery Poulose <suzuki.poulose@arm.com> To: Marc Zyngier <maz@kernel.org> Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, will@kernel.org, mark.rutland@arm.com, dave.martin@arm.com, catalin.marinas@arm.com, ard.biesheuvel@linaro.org, christoffer.dall@arm.com, Marc Zyngier <marc.zyngier@arm.com> Subject: Re: [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly Date: Mon, 13 Jan 2020 10:28:08 +0000 [thread overview] Message-ID: <ea8f50f9-66fa-1cf5-1292-a205993258fa@arm.com> (raw) In-Reply-To: <e1ba712b42886594fe1095019f2c5813@kernel.org> 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@kernel.org> >>>>>> Cc: Mark Rutland <mark.rutland@arm.com> >>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>>>> No idea who that guy is. It's a fake! ;-) >>>> >>>> Sorry about that, will fix it. >>>> >>>>> >>>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >>>>>> --- >>>>>> 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@kernel.org> > > 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
WARNING: multiple messages have this Message-ID (diff)
From: Suzuki Kuruppassery Poulose <suzuki.poulose@arm.com> To: Marc Zyngier <maz@kernel.org> Cc: mark.rutland@arm.com, ard.biesheuvel@linaro.org, Marc Zyngier <marc.zyngier@arm.com>, catalin.marinas@arm.com, linux-kernel@vger.kernel.org, christoffer.dall@arm.com, will@kernel.org, dave.martin@arm.com, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly Date: Mon, 13 Jan 2020 10:28:08 +0000 [thread overview] Message-ID: <ea8f50f9-66fa-1cf5-1292-a205993258fa@arm.com> (raw) In-Reply-To: <e1ba712b42886594fe1095019f2c5813@kernel.org> 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@kernel.org> >>>>>> Cc: Mark Rutland <mark.rutland@arm.com> >>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com> >>>>>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>>>> No idea who that guy is. It's a fake! ;-) >>>> >>>> Sorry about that, will fix it. >>>> >>>>> >>>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com> >>>>>> --- >>>>>> 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@kernel.org> > > 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-01-13 10:28 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-12-17 18:33 [PATCH v2 0/7] arm64: Fix support for no FP/SIMD Suzuki K Poulose 2019-12-17 18:33 ` Suzuki K Poulose 2019-12-17 18:33 ` [PATCH v2 1/7] arm64: Introduce system_capabilities_finalized() marker Suzuki K Poulose 2019-12-17 18:33 ` Suzuki K Poulose 2020-01-10 14:50 ` Catalin Marinas 2020-01-10 14:50 ` Catalin Marinas 2019-12-17 18:33 ` [PATCH v2 2/7] arm64: fpsimd: Make sure SVE setup is complete before SIMD is used Suzuki K Poulose 2019-12-17 18:33 ` Suzuki K Poulose 2020-01-10 11:51 ` Catalin Marinas 2020-01-10 11:51 ` Catalin Marinas 2020-01-10 18:41 ` Suzuki Kuruppassery Poulose 2020-01-10 18:41 ` Suzuki Kuruppassery Poulose 2019-12-17 18:33 ` [PATCH v2 3/7] arm64: cpufeature: Fix the type of no FP/SIMD capability Suzuki K Poulose 2019-12-17 18:33 ` Suzuki K Poulose 2020-01-10 14:50 ` Catalin Marinas 2020-01-10 14:50 ` Catalin Marinas 2019-12-17 18:33 ` [PATCH v2 4/7] arm64: cpufeature: Set the FP/SIMD compat HWCAP bits properly Suzuki K Poulose 2019-12-17 18:33 ` Suzuki K Poulose 2020-01-10 14:51 ` Catalin Marinas 2020-01-10 14:51 ` Catalin Marinas 2019-12-17 18:34 ` [PATCH v2 5/7] arm64: ptrace: nofpsimd: Fail FP/SIMD regset operations Suzuki K Poulose 2019-12-17 18:34 ` Suzuki K Poulose 2020-01-10 15:12 ` Catalin Marinas 2020-01-10 15:12 ` Catalin Marinas 2020-01-10 18:42 ` Suzuki Kuruppassery Poulose 2020-01-10 18:42 ` Suzuki Kuruppassery Poulose 2019-12-17 18:34 ` [PATCH v2 6/7] arm64: signal: nofpsimd: Handle fp/simd context for signal frames Suzuki K Poulose 2019-12-17 18:34 ` Suzuki K Poulose 2020-01-10 12:34 ` Catalin Marinas 2020-01-10 12:34 ` Catalin Marinas 2019-12-17 18:34 ` [PATCH v2 7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly Suzuki K Poulose 2019-12-17 18:34 ` Suzuki K Poulose 2019-12-17 19:05 ` Marc Zyngier 2019-12-17 19:05 ` Marc Zyngier 2019-12-18 11:42 ` Suzuki Kuruppassery Poulose 2019-12-18 11:42 ` Suzuki Kuruppassery Poulose 2019-12-18 11:56 ` Marc Zyngier 2019-12-18 11:56 ` Marc Zyngier 2019-12-18 12:00 ` Suzuki Kuruppassery Poulose 2019-12-18 12:00 ` Suzuki Kuruppassery Poulose 2020-01-10 15:21 ` Marc Zyngier 2020-01-10 15:21 ` Marc Zyngier 2020-01-13 10:28 ` Suzuki Kuruppassery Poulose [this message] 2020-01-13 10:28 ` Suzuki Kuruppassery Poulose 2020-01-10 14:49 ` Catalin Marinas 2020-01-10 14:49 ` Catalin Marinas
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=ea8f50f9-66fa-1cf5-1292-a205993258fa@arm.com \ --to=suzuki.poulose@arm.com \ --cc=ard.biesheuvel@linaro.org \ --cc=catalin.marinas@arm.com \ --cc=christoffer.dall@arm.com \ --cc=dave.martin@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=marc.zyngier@arm.com \ --cc=mark.rutland@arm.com \ --cc=maz@kernel.org \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.