diff mbox series

[v2,7/7] arm64: nofpsmid: Handle TIF_FOREIGN_FPSTATE flag cleanly

Message ID 20191217183402.2259904-8-suzuki.poulose@arm.com
State New
Headers show
Series arm64: Fix support for no FP/SIMD | expand

Commit Message

Suzuki K Poulose Dec. 17, 2019, 6:34 p.m. UTC 
We detect the absence of FP/SIMD after an incapable CPU is brought up,
and by then we have kernel threads running already with TIF_FOREIGN_FPSTATE set
which could be set for early userspace applications (e.g, modprobe triggered
from initramfs) and init. This could cause the applications to loop forever in
do_nofity_resume() as we never clear the TIF flag, once we now know that
we don't support FP.

Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag
for tasks which may have them set, as we would have done in the normal
case, but avoiding touching the hardware state (since we don't support any).

Also to make sure we handle the cases seemlessly we categorise the
helper functions to two :
 1) Helpers for common core code, which calls into take appropriate
    actions without knowing the current FPSIMD state of the CPU/task.

    e.g fpsimd_restore_current_state(), fpsimd_flush_task_state(),
        fpsimd_save_and_flush_cpu_state().

    We bail out early for these functions, taking any appropriate actions
    (e.g, clearing the TIF flag) where necessary to hide the handling
    from core code.

 2) Helpers used when the presence of FP/SIMD is apparent.
    i.e, save/restore the FP/SIMD register state, modify the CPU/task
    FP/SIMD state.
    e.g,

    fpsimd_save(), task_fpsimd_load() - save/restore task FP/SIMD registers

    fpsimd_bind_task_to_cpu()  \
                                - Update the "state" metadata for CPU/task.
    fpsimd_bind_state_to_cpu() /

    fpsimd_update_current_state() - Update the fp/simd state for the current
                                    task from memory.

    These must not be called in the absence of FP/SIMD. Put in a WARNING
    to make sure they are not invoked in the absence of FP/SIMD.

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.

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>
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(-)

-- 
2.23.0

Comments

Marc Zyngier Dec. 17, 2019, 7:05 p.m. UTC | #1 
Hi Suzuki,

On 2019-12-17 18:34, Suzuki K Poulose wrote:
> We detect the absence of FP/SIMD after an incapable CPU is brought 

> up,

> and by then we have kernel threads running already with

> TIF_FOREIGN_FPSTATE set

> which could be set for early userspace applications (e.g, modprobe 

> triggered

> from initramfs) and init. This could cause the applications to loop

> forever in

> do_nofity_resume() as we never clear the TIF flag, once we now know 

> that

> we don't support FP.

>

> Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag

> for tasks which may have them set, as we would have done in the 

> normal

> case, but avoiding touching the hardware state (since we don't 

> support any).

>

> Also to make sure we handle the cases seemlessly we categorise the

> helper functions to two :

>  1) Helpers for common core code, which calls into take appropriate

>     actions without knowing the current FPSIMD state of the CPU/task.

>

>     e.g fpsimd_restore_current_state(), fpsimd_flush_task_state(),

>         fpsimd_save_and_flush_cpu_state().

>

>     We bail out early for these functions, taking any appropriate 

> actions

>     (e.g, clearing the TIF flag) where necessary to hide the handling

>     from core code.

>

>  2) Helpers used when the presence of FP/SIMD is apparent.

>     i.e, save/restore the FP/SIMD register state, modify the CPU/task

>     FP/SIMD state.

>     e.g,

>

>     fpsimd_save(), task_fpsimd_load() - save/restore task FP/SIMD 

> registers

>

>     fpsimd_bind_task_to_cpu()  \

>                                 - Update the "state" metadata for 

> CPU/task.

>     fpsimd_bind_state_to_cpu() /

>

>     fpsimd_update_current_state() - Update the fp/simd state for the 

> current

>                                     task from memory.

>

>     These must not be called in the absence of FP/SIMD. Put in a 

> WARNING

>     to make sure they are not invoked in the absence of FP/SIMD.

>

> 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! ;-)

> 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).

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
Suzuki K Poulose Dec. 18, 2019, 11:42 a.m. UTC | #2 
Hi Marc,

On 17/12/2019 19:05, Marc Zyngier wrote:
> Hi Suzuki,

> 

> On 2019-12-17 18:34, Suzuki K Poulose wrote:

>> We detect the absence of FP/SIMD after an incapable CPU is brought up,

>> and by then we have kernel threads running already with

>> TIF_FOREIGN_FPSTATE set

>> which could be set for early userspace applications (e.g, modprobe 

>> triggered

>> from initramfs) and init. This could cause the applications to loop

>> forever in

>> do_nofity_resume() as we never clear the TIF flag, once we now know that

>> we don't support FP.

>>

>> Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag

>> for tasks which may have them set, as we would have done in the normal

>> case, but avoiding touching the hardware state (since we don't support 

>> any).

>>

>> Also to make sure we handle the cases seemlessly we categorise the

>> helper functions to two :

>>  1) Helpers for common core code, which calls into take appropriate

>>     actions without knowing the current FPSIMD state of the CPU/task.

>>

>>     e.g fpsimd_restore_current_state(), fpsimd_flush_task_state(),

>>         fpsimd_save_and_flush_cpu_state().

>>

>>     We bail out early for these functions, taking any appropriate actions

>>     (e.g, clearing the TIF flag) where necessary to hide the handling

>>     from core code.

>>

>>  2) Helpers used when the presence of FP/SIMD is apparent.

>>     i.e, save/restore the FP/SIMD register state, modify the CPU/task

>>     FP/SIMD state.

>>     e.g,

>>

>>     fpsimd_save(), task_fpsimd_load() - save/restore task FP/SIMD 

>> registers

>>

>>     fpsimd_bind_task_to_cpu()  \

>>                                 - Update the "state" metadata for 

>> CPU/task.

>>     fpsimd_bind_state_to_cpu() /

>>

>>     fpsimd_update_current_state() - Update the fp/simd state for the 

>> current

>>                                     task from memory.

>>

>>     These must not be called in the absence of FP/SIMD. Put in a WARNING

>>     to make sure they are not invoked in the absence of FP/SIMD.

>>

>> 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 ?

Cheers
Suzuki
Marc Zyngier Dec. 18, 2019, 11:56 a.m. UTC | #3 
On 2019-12-18 11:42, Suzuki Kuruppassery Poulose wrote:
> Hi Marc,

>

> On 17/12/2019 19:05, Marc Zyngier wrote:

>> Hi Suzuki,

>> On 2019-12-17 18:34, Suzuki K Poulose wrote:

>>> We detect the absence of FP/SIMD after an incapable CPU is brought 

>>> up,

>>> and by then we have kernel threads running already with

>>> TIF_FOREIGN_FPSTATE set

>>> which could be set for early userspace applications (e.g, modprobe 

>>> triggered

>>> from initramfs) and init. This could cause the applications to loop

>>> forever in

>>> do_nofity_resume() as we never clear the TIF flag, once we now know 

>>> that

>>> we don't support FP.

>>>

>>> Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag

>>> for tasks which may have them set, as we would have done in the 

>>> normal

>>> case, but avoiding touching the hardware state (since we don't 

>>> support any).

>>>

>>> Also to make sure we handle the cases seemlessly we categorise the

>>> helper functions to two :

>>>  1) Helpers for common core code, which calls into take appropriate

>>>     actions without knowing the current FPSIMD state of the 

>>> CPU/task.

>>>

>>>     e.g fpsimd_restore_current_state(), fpsimd_flush_task_state(),

>>>         fpsimd_save_and_flush_cpu_state().

>>>

>>>     We bail out early for these functions, taking any appropriate 

>>> actions

>>>     (e.g, clearing the TIF flag) where necessary to hide the 

>>> handling

>>>     from core code.

>>>

>>>  2) Helpers used when the presence of FP/SIMD is apparent.

>>>     i.e, save/restore the FP/SIMD register state, modify the 

>>> CPU/task

>>>     FP/SIMD state.

>>>     e.g,

>>>

>>>     fpsimd_save(), task_fpsimd_load() - save/restore task FP/SIMD 

>>> registers

>>>

>>>     fpsimd_bind_task_to_cpu()  \

>>>                                 - Update the "state" metadata for 

>>> CPU/task.

>>>     fpsimd_bind_state_to_cpu() /

>>>

>>>     fpsimd_update_current_state() - Update the fp/simd state for 

>>> the current

>>>                                     task from memory.

>>>

>>>     These must not be called in the absence of FP/SIMD. Put in a 

>>> WARNING

>>>     to make sure they are not invoked in the absence of FP/SIMD.

>>>

>>> 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
would get set though. But it probably doesn't matter (WTF is going
to run KVM with such broken HW?), and better safe than sorry.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
Suzuki K Poulose Dec. 18, 2019, noon UTC | #4 
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:

>>> Hi Suzuki,

>>> On 2019-12-17 18:34, Suzuki K Poulose wrote:

>>>> We detect the absence of FP/SIMD after an incapable CPU is brought up,

>>>> and by then we have kernel threads running already with

>>>> TIF_FOREIGN_FPSTATE set

>>>> which could be set for early userspace applications (e.g, modprobe 

>>>> triggered

>>>> from initramfs) and init. This could cause the applications to loop

>>>> forever in

>>>> do_nofity_resume() as we never clear the TIF flag, once we now know 

>>>> that

>>>> we don't support FP.

>>>>

>>>> Fix this by making sure that we clear the TIF_FOREIGN_FPSTATE flag

>>>> for tasks which may have them set, as we would have done in the normal

>>>> case, but avoiding touching the hardware state (since we don't 

>>>> support any).

>>>>

>>>> Also to make sure we handle the cases seemlessly we categorise the

>>>> helper functions to two :

>>>>  1) Helpers for common core code, which calls into take appropriate

>>>>     actions without knowing the current FPSIMD state of the CPU/task.

>>>>

>>>>     e.g fpsimd_restore_current_state(), fpsimd_flush_task_state(),

>>>>         fpsimd_save_and_flush_cpu_state().

>>>>

>>>>     We bail out early for these functions, taking any appropriate 

>>>> actions

>>>>     (e.g, clearing the TIF flag) where necessary to hide the handling

>>>>     from core code.

>>>>

>>>>  2) Helpers used when the presence of FP/SIMD is apparent.

>>>>     i.e, save/restore the FP/SIMD register state, modify the CPU/task

>>>>     FP/SIMD state.

>>>>     e.g,

>>>>

>>>>     fpsimd_save(), task_fpsimd_load() - save/restore task FP/SIMD 

>>>> registers

>>>>

>>>>     fpsimd_bind_task_to_cpu()  \

>>>>                                 - Update the "state" metadata for 

>>>> CPU/task.

>>>>     fpsimd_bind_state_to_cpu() /

>>>>

>>>>     fpsimd_update_current_state() - Update the fp/simd state for the 

>>>> current

>>>>                                     task from memory.

>>>>

>>>>     These must not be called in the absence of FP/SIMD. Put in a 

>>>> WARNING

>>>>     to make sure they are not invoked in the absence of FP/SIMD.

>>>>

>>>> 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.

Cheers
Suzuki
diff mbox series

Patch

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 3eb338f14386..240c52b71cda 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -269,7 +269,7 @@  static void sve_free(struct task_struct *task)
  */
 static void task_fpsimd_load(void)
 {
-	WARN_ON(!have_cpu_fpsimd_context());
+	WARN_ON(!system_supports_fpsimd() || !have_cpu_fpsimd_context());
 
 	if (system_supports_sve() && test_thread_flag(TIF_SVE))
 		sve_load_state(sve_pffr(&current->thread),
@@ -289,6 +289,7 @@  static void fpsimd_save(void)
 		this_cpu_ptr(&fpsimd_last_state);
 	/* set by fpsimd_bind_task_to_cpu() or fpsimd_bind_state_to_cpu() */
 
+	WARN_ON(!system_supports_fpsimd());
 	WARN_ON(!have_cpu_fpsimd_context());
 
 	if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
@@ -1092,6 +1093,7 @@  void fpsimd_bind_task_to_cpu(void)
 	struct fpsimd_last_state_struct *last =
 		this_cpu_ptr(&fpsimd_last_state);
 
+	WARN_ON(!system_supports_fpsimd());
 	last->st = &current->thread.uw.fpsimd_state;
 	last->sve_state = current->thread.sve_state;
 	last->sve_vl = current->thread.sve_vl;
@@ -1114,6 +1116,7 @@  void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st, void *sve_state,
 	struct fpsimd_last_state_struct *last =
 		this_cpu_ptr(&fpsimd_last_state);
 
+	WARN_ON(!system_supports_fpsimd());
 	WARN_ON(!in_softirq() && !irqs_disabled());
 
 	last->st = st;
@@ -1128,8 +1131,19 @@  void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *st, void *sve_state,
  */
 void fpsimd_restore_current_state(void)
 {
-	if (!system_supports_fpsimd())
+	/*
+	 * For the tasks that were created before we detected the absence of
+	 * FP/SIMD, the TIF_FOREIGN_FPSTATE could be set via fpsimd_thread_switch(),
+	 * e.g, init. This could be then inherited by the children processes.
+	 * If we later detect that the system doesn't support FP/SIMD,
+	 * we must clear the flag for  all the tasks to indicate that the
+	 * FPSTATE is clean (as we can't have one) to avoid looping for ever in
+	 * do_notify_resume().
+	 */
+	if (!system_supports_fpsimd()) {
+		clear_thread_flag(TIF_FOREIGN_FPSTATE);
 		return;
+	}
 
 	get_cpu_fpsimd_context();
 
@@ -1148,7 +1162,7 @@  void fpsimd_restore_current_state(void)
  */
 void fpsimd_update_current_state(struct user_fpsimd_state const *state)
 {
-	if (!system_supports_fpsimd())
+	if (WARN_ON(!system_supports_fpsimd()))
 		return;
 
 	get_cpu_fpsimd_context();
@@ -1179,7 +1193,13 @@  void fpsimd_update_current_state(struct user_fpsimd_state const *state)
 void fpsimd_flush_task_state(struct task_struct *t)
 {
 	t->thread.fpsimd_cpu = NR_CPUS;
-
+	/*
+	 * If we don't support fpsimd, bail out after we have
+	 * reset the fpsimd_cpu for this task and clear the
+	 * FPSTATE.
+	 */
+	if (!system_supports_fpsimd())
+		return;
 	barrier();
 	set_tsk_thread_flag(t, TIF_FOREIGN_FPSTATE);
 
@@ -1193,6 +1213,7 @@  void fpsimd_flush_task_state(struct task_struct *t)
  */
 static void fpsimd_flush_cpu_state(void)
 {
+	WARN_ON(!system_supports_fpsimd());
 	__this_cpu_write(fpsimd_last_state.st, NULL);
 	set_thread_flag(TIF_FOREIGN_FPSTATE);
 }
@@ -1203,6 +1224,8 @@  static void fpsimd_flush_cpu_state(void)
  */
 void fpsimd_save_and_flush_cpu_state(void)
 {
+	if (!system_supports_fpsimd())
+		return;
 	WARN_ON(preemptible());
 	__get_cpu_fpsimd_context();
 	fpsimd_save();
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));
 	return !!(vcpu->arch.flags & KVM_ARM64_FP_ENABLED);
 }