diff mbox series

arm64/fpsimd: Exit streaming mode when flushing tasks

Message ID 20230607-arm64-flush-svcr-v1-1-7821a6199e0a@kernel.org
State New
Headers
Series arm64/fpsimd: Exit streaming mode when flushing tasks |

Commit Message

Mark Brown June 7, 2023, 8:30 p.m. UTC 
  Ensure there is no path where we might attempt to save SME state after we
flush a task by updating the SVCR register state as well as updating our
in memory state. I haven't seen a specific case where this is happening or
seen a path where it might happen but for the cost of a single low overhead
instruction it seems sensible to close the potential gap.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/fpsimd.c | 1 +
 1 file changed, 1 insertion(+)


---
base-commit: 44c026a73be8038f03dbdeef028b642880cf1511
change-id: 20230607-arm64-flush-svcr-47cc76a8cbbc

Best regards,

Comments

Anders Roxell June 8, 2023, 3:28 p.m. UTC | #1 
On Wed, 7 Jun 2023 at 22:42, Mark Brown <broonie@kernel.org> wrote:
>
> Ensure there is no path where we might attempt to save SME state after we
> flush a task by updating the SVCR register state as well as updating our
> in memory state. I haven't seen a specific case where this is happening or
> seen a path where it might happen but for the cost of a single low overhead
> instruction it seems sensible to close the potential gap.
>
> Signed-off-by: Mark Brown <broonie@kernel.org>

Applied this onto todays next tag next-20230608 and ran
kselftest-arm64 on a FVP model.
I still see the "BUG: KFENCE: memory corruption in
fpsimd_release_task+0x1c/0x3c".

I'm trying to use the latest kselftest from today with older next tags
trying to find when
this issue started to happen.

Cheers,
Anders


> ---
>  arch/arm64/kernel/fpsimd.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 2fbafa5cc7ac..1627e0efe39a 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -1649,6 +1649,7 @@ void fpsimd_flush_thread(void)
>
>                 fpsimd_flush_thread_vl(ARM64_VEC_SME);
>                 current->thread.svcr = 0;
> +               sme_smstop_sm();
>         }
>
>         current->thread.fp_type = FP_STATE_FPSIMD;
>
> ---
> base-commit: 44c026a73be8038f03dbdeef028b642880cf1511
> change-id: 20230607-arm64-flush-svcr-47cc76a8cbbc
>
> Best regards,
> --
> Mark Brown <broonie@kernel.org>
>
Mark Rutland June 8, 2023, 3:51 p.m. UTC | #2 
On Wed, Jun 07, 2023 at 09:30:51PM +0100, Mark Brown wrote:
> Ensure there is no path where we might attempt to save SME state after we
> flush a task by updating the SVCR register state as well as updating our
> in memory state. I haven't seen a specific case where this is happening or
> seen a path where it might happen but for the cost of a single low overhead
> instruction it seems sensible to close the potential gap.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/kernel/fpsimd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 2fbafa5cc7ac..1627e0efe39a 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -1649,6 +1649,7 @@ void fpsimd_flush_thread(void)
>  
>  		fpsimd_flush_thread_vl(ARM64_VEC_SME);
>  		current->thread.svcr = 0;
> +		sme_smstop_sm();

I don't think we should blindly do this if we never expect to get here in that
state; this is just going to mask bugs and make them harder to debug going
forwards.

If we need this, it'd be better to have something like:

	if (WARN_ON_ONCE(sme_is_in_streaming_mode()))
		sme_smstop_sm();

... so that we can identify this case and fix it.

Thanks,
Mark.
Mark Brown June 8, 2023, 4:04 p.m. UTC | #3 
On Thu, Jun 08, 2023 at 04:51:26PM +0100, Mark Rutland wrote:
> On Wed, Jun 07, 2023 at 09:30:51PM +0100, Mark Brown wrote:

> >  		fpsimd_flush_thread_vl(ARM64_VEC_SME);
> >  		current->thread.svcr = 0;
> > +		sme_smstop_sm();

> I don't think we should blindly do this if we never expect to get here in that
> state; this is just going to mask bugs and make them harder to debug going
> forwards.

> If we need this, it'd be better to have something like:

> 	if (WARN_ON_ONCE(sme_is_in_streaming_mode()))
> 		sme_smstop_sm();

> ... so that we can identify this case and fix it.

No, being here in streaming mode is valid so that check would be wrong -
if there is an issue the issue would be that we're expecting that any
further use of the register state would involve reloading from memory
but there would be some path where we end up doing something that uses
the in register state again rather than reloading.  The change ensures
that the saved and register states are in sync so that can't go wrong,
meaning we don't need to go confirm if there's such a path.

Though now I look again we should do a full SMSTOP since a similar
concern applies to ZA.
diff mbox series

Patch

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 2fbafa5cc7ac..1627e0efe39a 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -1649,6 +1649,7 @@  void fpsimd_flush_thread(void)
 
 		fpsimd_flush_thread_vl(ARM64_VEC_SME);
 		current->thread.svcr = 0;
+		sme_smstop_sm();
 	}
 
 	current->thread.fp_type = FP_STATE_FPSIMD;