Message ID | 20230607-arm64-flush-svcr-v1-1-7821a6199e0a@kernel.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | arm64/fpsimd: Exit streaming mode when flushing tasks | expand |
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> >
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.
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 --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;
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,